Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix cabal install --program-suffix/prefix (fix #10290 and #10476) #10483

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Oct 28, 2024

#10290 shows: when checking for existing installations, cabal would not account for an affix (suffix or prefix). So, if you had a hello binary installed, installing a second one with a non-empty affix (a perfectly legal operation) would fail.

The reason seemed to be a typo in 09c04e9, which passed the arguments to the Symlink structure in a wrong order.

#10476 shows: when failing to install a binary because of an existing one, cabal would report suffix-less existing target even if a suffix was set.

This turned out to be a little issue in pretty printing.


I haven't gotten around to adding tests, sadly.


QA notes

Try to check that both #10290 and #10476 don't reproduce with this patch (they both have clear instructions for reproduction).


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual QA passes here also.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this looks reasonable (and very much like how I remember the patch to be), we should add a regression test for this. I can try to contribute one

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Oct 28, 2024

Thanks! It'd be awesome to get a test. I don't know when I could get to it, so please contribute!

@fendor
Copy link
Collaborator

fendor commented Oct 29, 2024

I wrote a test and I think I found another bug EDIT: I did not find a bug, I ran the wrong binary 🙃
Is everyone fine with me adding another 60s testcase? 😅

@geekosaur
Copy link
Collaborator

The more tests, the better!

@ulysses4ever
Copy link
Collaborator Author

Yes, the more the merrier! We can spare some seconds on this provided how tricky this is.

@ulysses4ever
Copy link
Collaborator Author

@fendor just to double-check: did you mean to add one more test or is it the currently posted one that takes those 60 seconds?

@fendor
Copy link
Collaborator

fendor commented Oct 30, 2024

@ulysses4ever The added test takes 60s on my machine, and I think it is pretty exhaustively testing the overwrite policy code path.

@ulysses4ever
Copy link
Collaborator Author

Awesome!

@ulysses4ever ulysses4ever added the squash+merge me Tell Mergify Bot to squash-merge label Oct 30, 2024
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Oct 30, 2024
@ulysses4ever
Copy link
Collaborator Author

mm,I wonder why this is not being merged...

@geekosaur
Copy link
Collaborator

@mergify refresh

Copy link
Contributor

mergify bot commented Nov 2, 2024

refresh

✅ Pull request refreshed

ulysses4ever and others added 2 commits November 2, 2024 18:18
When checking for existing installations, cabal would not account for
an affix (suffix or prefix). So, if you had a `hello` binary installed, installing a
second one with a non-empty affix (a perfectly legal operation) would
fail.

The reason seemed to be a typo in
09c04e9, which passed the arguments to
the Symlink structure in a wrong order.

When failing to install a binary because of an existing one, cabal would
report suffix-less existing target even if a suffix was set.
Add regression tests for the `program-prefix` and `program-suffix` flags
combined with the overwrite-policy.
In short, the overwrite-policy needs to take potential program affixes
into account when deciding whether it will need to overwrite a program
path during installation.
@geekosaur geekosaur force-pushed the program-suffix-broken-issue-10290 branch from fa72c05 to 85a4ead Compare November 2, 2024 22:18
@geekosaur
Copy link
Collaborator

Uh, there's a #commits-behind=0 in the wrong place; it's supposed to be on merge+no rebase.

@geekosaur
Copy link
Collaborator

It should go in after CI completes. Meanwhile I offer you #10505 which should probably go in quickly before this happens again.

@mergify mergify bot merged commit ee3c313 into master Nov 2, 2024
49 checks passed
@mergify mergify bot deleted the program-suffix-broken-issue-10290 branch November 2, 2024 23:41
@ulysses4ever
Copy link
Collaborator Author

@mergify backport 3.14

Copy link
Contributor

mergify bot commented Nov 3, 2024

backport 3.14

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 3, 2024
…10483)

* fix cabal install --program-suffix/prefix (fix #10290 and #10476)

When checking for existing installations, cabal would not account for
an affix (suffix or prefix). So, if you had a `hello` binary installed, installing a
second one with a non-empty affix (a perfectly legal operation) would
fail.

The reason seemed to be a typo in
09c04e9, which passed the arguments to
the Symlink structure in a wrong order.

When failing to install a binary because of an existing one, cabal would
report suffix-less existing target even if a suffix was set.

* Add regression tests for overwrite policies and porgram-affixes

Add regression tests for the `program-prefix` and `program-suffix` flags
combined with the overwrite-policy.
In short, the overwrite-policy needs to take potential program affixes
into account when deciding whether it will need to overwrite a program
path during installation.

---------

Co-authored-by: Fendor <fendor@posteo.de>
(cherry picked from commit ee3c313)
@geekosaur
Copy link
Collaborator

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Nov 3, 2024

backport 3.12

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 3, 2024
…10483)

* fix cabal install --program-suffix/prefix (fix #10290 and #10476)

When checking for existing installations, cabal would not account for
an affix (suffix or prefix). So, if you had a `hello` binary installed, installing a
second one with a non-empty affix (a perfectly legal operation) would
fail.

The reason seemed to be a typo in
09c04e9, which passed the arguments to
the Symlink structure in a wrong order.

When failing to install a binary because of an existing one, cabal would
report suffix-less existing target even if a suffix was set.

* Add regression tests for overwrite policies and porgram-affixes

Add regression tests for the `program-prefix` and `program-suffix` flags
combined with the overwrite-policy.
In short, the overwrite-policy needs to take potential program affixes
into account when deciding whether it will need to overwrite a program
path during installation.

---------

Co-authored-by: Fendor <fendor@posteo.de>
(cherry picked from commit ee3c313)
geekosaur pushed a commit that referenced this pull request Nov 3, 2024
…10483)

* fix cabal install --program-suffix/prefix (fix #10290 and #10476)

When checking for existing installations, cabal would not account for
an affix (suffix or prefix). So, if you had a `hello` binary installed, installing a
second one with a non-empty affix (a perfectly legal operation) would
fail.

The reason seemed to be a typo in
09c04e9, which passed the arguments to
the Symlink structure in a wrong order.

When failing to install a binary because of an existing one, cabal would
report suffix-less existing target even if a suffix was set.

* Add regression tests for overwrite policies and porgram-affixes

Add regression tests for the `program-prefix` and `program-suffix` flags
combined with the overwrite-policy.
In short, the overwrite-policy needs to take potential program affixes
into account when deciding whether it will need to overwrite a program
path during installation.

---------

Co-authored-by: Fendor <fendor@posteo.de>
(cherry picked from commit ee3c313)
geekosaur pushed a commit that referenced this pull request Nov 3, 2024
…10483)

* fix cabal install --program-suffix/prefix (fix #10290 and #10476)

When checking for existing installations, cabal would not account for
an affix (suffix or prefix). So, if you had a `hello` binary installed, installing a
second one with a non-empty affix (a perfectly legal operation) would
fail.

The reason seemed to be a typo in
09c04e9, which passed the arguments to
the Symlink structure in a wrong order.

When failing to install a binary because of an existing one, cabal would
report suffix-less existing target even if a suffix was set.

* Add regression tests for overwrite policies and porgram-affixes

Add regression tests for the `program-prefix` and `program-suffix` flags
combined with the overwrite-policy.
In short, the overwrite-policy needs to take potential program affixes
into account when deciding whether it will need to overwrite a program
path during installation.

---------

Co-authored-by: Fendor <fendor@posteo.de>
(cherry picked from commit ee3c313)
geekosaur pushed a commit that referenced this pull request Nov 3, 2024
…10483)

* fix cabal install --program-suffix/prefix (fix #10290 and #10476)

When checking for existing installations, cabal would not account for
an affix (suffix or prefix). So, if you had a `hello` binary installed, installing a
second one with a non-empty affix (a perfectly legal operation) would
fail.

The reason seemed to be a typo in
09c04e9, which passed the arguments to
the Symlink structure in a wrong order.

When failing to install a binary because of an existing one, cabal would
report suffix-less existing target even if a suffix was set.

* Add regression tests for overwrite policies and porgram-affixes

Add regression tests for the `program-prefix` and `program-suffix` flags
combined with the overwrite-policy.
In short, the overwrite-policy needs to take potential program affixes
into account when deciding whether it will need to overwrite a program
path during installation.

---------

Co-authored-by: Fendor <fendor@posteo.de>
(cherry picked from commit ee3c313)
@Kleidukos Kleidukos self-assigned this Nov 3, 2024
@Kleidukos
Copy link
Member

Kleidukos commented Nov 3, 2024

cabal-3.10.3.0


❯ cabal-3.10.3.0 install hello --overwrite-policy=never --install-method=symlink                      
Warning: /home/hecate/.cabal/config: Unrecognized field token on line 118
Warning: /home/hecate/.cabal/config: Unrecognized field token on line 118
Resolving dependencies...
Build profile: -w ghc-9.6.6 -O1
In order, the following will be built (use -v for more details):
 - hello-1.0.0.2 (exe:hello) (requires build)
Starting     hello-1.0.0.2 (all, legacy fallback)
Building     hello-1.0.0.2 (all, legacy fallback)
Installing   hello-1.0.0.2 (all, legacy fallback)
Completed    hello-1.0.0.2 (all, legacy fallback)
Symlinking 'hello' to '/home/hecate/.cabal/bin/hello'


❯ cabal-3.10.3.0 install --program-suffix=-new --overwrite-policy=never --install-method=symlink hello
Warning: /home/hecate/.cabal/config: Unrecognized field token on line 118
Warning: /home/hecate/.cabal/config: Unrecognized field token on line 118
Resolving dependencies...
Symlinking 'hello' to '/home/hecate/.cabal/bin/hello-new'


❯ file /home/hecate/.cabal/bin/hello-new
/home/hecate/.cabal/bin/hello-new: broken symbolic link to ../store/ghc-9.6.6/hello-1.0.0.2-5aff93f5d5a6eb8064efe52d47134888c59c06746f361b6b851fcc60f37cef30/bin/hello


❯ file .cabal/store/ghc-9.6.6/hello-1.0.0.2-5aff93f5d5a6eb8064efe52d47134888c59c06746f361b6b851fcc60f37cef30/bin/hello-new
.cabal/store/ghc-9.6.6/hello-1.0.0.2-5aff93f5d5a6eb8064efe52d47134888c59c06746f361b6b851fcc60f37cef30/bin/hello-new: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, BuildID[sha1]=e4ecc7f21d79228ab55971c98920692e145b5a86, not stripped

Verdict: cabal tries to link hello-new to the path of the package in the store, but does not take into account the program suffix and mistakenly creates a link to bin/hello instead of bin/hello-new

cabal-3.12.1.0

❯ cabal-3.12.1.0 install hello --overwrite-policy=never --install-method=symlink 
Resolving dependencies...
Build profile: -w ghc-9.6.6 -O1
In order, the following will be built (use -v for more details):
 - hello-1.0.0.2 (exe:hello) (requires build)
Starting     hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Building     hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Installing   hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Completed    hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Symlinking 'hello' to '/home/hecate/.cabal/bin/hello'


❯ file /home/hecate/.cabal/bin/hello                                                                                      
/home/hecate/.cabal/bin/hello: symbolic link to ../store/ghc-9.6.6/hello-1.0.0.2-324cc9d163477527beb3b1762a148ce1b1e097f545de95778fa0323d77272764/bin/hello


❯ cabal-3.12.1.0 install hello --overwrite-policy=never --install-method=symlink --program-suffix=-new
Resolving dependencies...
Error: [Cabal-7149]
Path '/home/hecate/.cabal/bin/hello' already exists. Use --overwrite-policy=always to overwrite.

Verdict: cabal does not take the program suffix into account at all, and tries to create a symbolic link under the un-suffixed name. Should it manage to do this at all, it will create a link to the un-suffixed binary name, which will have been installed this time:

❯ file /home/hecate/.cabal/bin/hello-new
/home/hecate/.cabal/bin/hello-new: symbolic link to ../store/ghc-9.6.6/hello-1.0.0.2-324cc9d163477527beb3b1762a148ce1b1e097f545de95778fa0323d77272764/bin/hello

cabal-head from November 4th ❌

❯ ls -lah =cabal        
lrwxrwxrwx. 1 hecate hecate 10  4 nov.  15:09 /home/hecate/.ghcup/bin/cabal -> cabal-head*

❯ cabal install hello --overwrite-policy=never --install-method=symlink                      
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Build profile: -w ghc-9.6.6 -O1
In order, the following will be built (use -v for more details):
 - hello-1.0.0.2 (exe:hello) (requires build)
Starting     hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Building     hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Installing   hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Completed    hello-1.0.0.2 (all, legacy fallback: cabal-version is less than 1.8)
Symlinking 'hello' to '/home/hecate/.cabal/bin/hello'

❯ cabal install hello --overwrite-policy=never --install-method=symlink --program-suffix=-new
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Error: [Cabal-7149]
Path '/home/hecate/.cabal/bin/hello' already exists. Use --overwrite-policy=always to overwrite.

Verdict: Can still reproduce the bug.

@geekosaur
Copy link
Collaborator

Right, that's part of what's being fixed (and for 3.12 was the source of the regression tickets, I believe). I have a backport in progress for whenever we put out 3.12.2.0.

@Kleidukos
Copy link
Member

I'm just waiting for the next nightly to test this PR. :)

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Nov 4, 2024

@Kleidukos cabal-head is busted, it looks: https://github.com/haskell/cabal/releases/tag/cabal-head says October 3rd is the last time it's updated. Filed #10515

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Nov 5, 2024

@Kleidukos can you try again with the current cabal-head?

@geekosaur
Copy link
Collaborator

There still isn't a current one; since my fix today was based on an up-to-date repo and nobody else pushed anything while it was in CI, Mergify pushed it straight in without running the workflow. That's why part of my major CI shakeup is moving prereleases to nightly jobs; there's no way to reliably do a prerelease on merge (if we switched to GitHub's merge queues there would be a way to do it as part of the merge, but we are at this point agreed that we need Mergify AIUI).

@ulysses4ever
Copy link
Collaborator Author

@geekosaur do you know why it says "7 hours ago" by the binaries on the pre-release page although it shows the commit from October 3?
Screenshot_20241104-230122

@ulysses4ever
Copy link
Collaborator Author

I assume at this point @Kleidukos could be better off with building master locally ☺️

@geekosaur
Copy link
Collaborator

@geekosaur do you know why it says "7 hours ago" by the binaries on the pre-release page although it shows the commit from October 3?

I have no idea. Certainly the commit matches the date.

@geekosaur
Copy link
Collaborator

The binary inside the artifact has a date that agrees with the display above, but I note the source code links are dated October 3rd. No idea what's going on here since it shouldn't be able to access the older artifacts at this point.

@geekosaur
Copy link
Collaborator

I wonder if cabal --version on a non-release build should include a Git commit.

@Kleidukos
Copy link
Member

@geekosaur This has been bugging me too, as a daily user of cabal-head, this would be greatly appreciated

@Kleidukos
Copy link
Member

cabal-head from November 5th ✔️

❯ cabal install hello --overwrite-policy=never --install-method=symlink                      
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Symlinking 'hello' to '/home/hecate/.cabal/bin/hello'

❯ file =hello    
/home/hecate/.cabal/bin/hello: symbolic link to ../store/ghc-9.6.6/hello-1.0.0.2-324cc9d163477527beb3b1762a148ce1b1e097f545de95778fa0323d77272764/bin/hello

❯ cabal install hello --overwrite-policy=never --install-method=symlink --program-suffix=-new
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Symlinking 'hello' to '/home/hecate/.cabal/bin/hello-new'

❯ file =hello-new                                                                                                             
/home/hecate/.cabal/bin/hello-new: symbolic link to ../store/ghc-9.6.6/hello-1.0.0.2-324cc9d163477527beb3b1762a148ce1b1e097f545de95778fa0323d77272764/bin/hello

It's good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport 3.12 attention: needs-backport 3.14 cabal-install: cmd/install merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days ready and waiting Mergify is waiting out the cooldown period squash+merge me Tell Mergify Bot to squash-merge
Projects
Status: Testing Approved
Development

Successfully merging this pull request may close these issues.

4 participants