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

Don't add extra-prog-path to ~/.config/cabal/config (#8951) #8952

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

sol
Copy link
Member

@sol sol commented May 18, 2023

(when initially creating it)

This partially addresses #8951.


Please include the following checklist in your PR:

Bonus points for added automated tests!

@Mikolaj
Copy link
Member

Mikolaj commented Jun 2, 2023

@sol: may we help you with the CI? I guess the last failure is expected (tests needs to be adjusted)? Is there anything else in the CI log that is concerning?

@sol
Copy link
Member Author

sol commented Jun 2, 2023

@Mikolaj I rebased and adjusted that test.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM given my limited understading.

@Mikolaj Mikolaj requested a review from gbaz June 3, 2023 08:37
@ulysses4ever
Copy link
Collaborator

There's another PR in flight that claims to resolve the same ticket? #8972

@sol
Copy link
Member Author

sol commented Jun 3, 2023

There's another PR in flight that claims to resolve the same ticket? #8972

@ulysses4ever "We could (and probably should) do both."

#8951 (comment)

@@ -0,0 +1,3 @@
synopsis: Don't add `extra-prog-path: ~/.local/bin` when initially creating `~/.config/cabal/config`
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking: I'd love a short description that would explain the benefit (which we all know, but that would save some pointer chasing for the casual reader in the changelog interface of the package index.)

Copy link
Member

Choose a reason for hiding this comment

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

@sol: how about this extra explanation?

@Mikolaj
Copy link
Member

Mikolaj commented Jun 17, 2023

It seems this one is not going to make it in time for 3.10.2.0 and we don't want to rush anything in a point release, unless a regression is super-terrible.

@sol
Copy link
Member Author

sol commented Jun 17, 2023

we don't want to rush anything in a point release, unless a regression is super-terrible.

I don't want to be overly dramatic, but this regression (== the regression introduced with #8506) can silently corrupt builds.

Why is that? Because a failing build due to the wrong version of a build tool being used is actually the best case scenario here. What might happen instead is that the build succeeds, possibly producing a different result.

It's not just hypothetical. I spent a whole day debugging this, and I didn't do so for no reason. I did it, because there was something fishy, because cabal did not do what I specified in the .cabal file.

from what I understand, when a feature introduces a regression, there are two ways to address this: Either revert the feature, or fix the regression.

So I would hope that either

For this PR, is the issue that we don't have the second approving review? If yes, @gbaz can you take a look?

@gbaz
Copy link
Collaborator

gbaz commented Jun 17, 2023

I think 8972 is the most important thing to merge. This PR only affects first-time cabal users, as otherwise configs tend to have settings inherited from prior cabals instead of being generated afresh. Its probably a worthwhile cleanup regardless, that said, in that when installing with e.g. ghcup, the cabal bin path gets added to the path anyway.

@sol
Copy link
Member Author

sol commented Jun 17, 2023

when installing with e.g. ghcup, the cabal bin path gets added to the path anyway

To put it more concretely for the casual reader:

  • You want the cabal bin dir on your PATH. That's completely fine and does not interact in anyway with this story / regression.
  • On the other hand, having the cabal bin dir as extra-prog-path is at best useless and as of now proliferates this regression .

@Mikolaj
Copy link
Member

Mikolaj commented Jun 17, 2023

In that case let's merge (perhaps speeding it up by setting merge_delay_passed manually) and then try to backport.

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Jun 17, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jun 19, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Jun 20, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Jun 20, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2023

rebase

✅ Branch has been successfully rebased

@Kleidukos
Copy link
Member

@Mergifyio backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2023

backport 3.10

✅ Backports have been created

@mergify mergify bot merged commit 24934dd into haskell:master Jun 20, 2023
Kleidukos added a commit that referenced this pull request Jul 12, 2023
…port #8952) (#9050)

Don't add `extra-prog-path` to `~/.config/cabal/config` (#8951)

(when initially creating it)

(cherry picked from commit ea55955)

# Conflicts:
#	cabal-install/src/Distribution/Client/Config.hs

* fix conflict

* Merge branch '3.10' into mergify/bp/3.10/pr-8952
mergify bot added a commit that referenced this pull request Jul 13, 2023
Don't add `extra-prog-path` to `~/.config/cabal/config` (#8951) (backport #8952)
@andreasabel andreasabel added the re: extra-prog-path Concerning the `extra-prog-path` configuration option label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: extra-prog-path Concerning the `extra-prog-path` configuration option regression in 3.10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants