-
Notifications
You must be signed in to change notification settings - Fork 723
Make clear when pkg-config is not present #10122
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
Conversation
3fd17ee to
5fd4617
Compare
|
Oh god I had this exact problem happen recently. Thank you so much! |
Kleidukos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could you please add some QA notes? :)
|
Sure! QA and changelog will come as soon as I have some time to write them. Thanks! |
cabal-install-solver/src/Distribution/Solver/Modular/Message.hs
Outdated
Show resolved
Hide resolved
cabal-install-solver/src/Distribution/Solver/Modular/Message.hs
Outdated
Show resolved
Hide resolved
cabal-install-solver/src/Distribution/Solver/Modular/Validate.hs
Outdated
Show resolved
Hide resolved
|
It looks like the messaging is inconsistent. Apparently the solver used to ignore pkg-config dependencies when pkg-config is missing (which is what this message says) but in 68e2737 the behaviour was changed to backtrack. Do we need to reconsider this? Is backtracking (and likely failing) without pkg-config the right thing to do? Maybe we could instead make sure Cabal has the same behaviour? (that is, to ignore pkg-config dependencies if pkg-config is missing) |
andreabedini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of implementation, I'd prefer to change the call site (validateTree).
The value I see in the pkgConfigPkgIsPresent function is to merge the NoPkgConfig case into False, but if we want to keep them separate, we should propagate the change up:
We turn functions producing PkgConfigDb into producing Maybe PkgConfigDb where
newtype PkgConfigDb = PkgConfigDb (M.Map PkgconfigName (Maybe
Other than changing few types, only code changes I found are in two places:
Distribution.Solver.Modular.Validate.validateTreewhich is perfect because it's were you want to deal with the absence of pkg-config.Distribution.Client.ProjectPlanning.elaborateInstallPlanwhich is a bit surprising but good that we found it! 😂
WDYT?
Perhaps that is something to consider. The very least I want to do with this PR is to clarify the error message on the current logic, a follow-up PR could deal with whether or not the current behaviour is the wanted one. |
Yes, backtracking is a right thing to do and it is a crucial ingredient in multiple packages (e. g., |
I had no intention to change that behaviour. You make a good argument. In this case we should make sure the behaviour (and its reason) is documented and that the code is consistent with it. |
5fd4617 to
1bca648
Compare
|
@andreabedini I unified your branch and mine into this one, and fixed some compilation errors plus some rewording on the error message. Let me know how it looks 👍 |
cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that, in readPkgConfigDb, we use ioErrorHandler to call noPkgConfig in case something fails.
So if pkg-config is available but calling it fails for some reason, we will still return Nothing and backtrack on all pkg-config constraints. This possibiliy is mentioned in the comment right above.
Do we need to worry about this? Recent experience showed that querying pkg-config can be unreliable (see #8930, #9134, #9360, #9478, and my own #9422, ping @georgefst).
Ping @Mikolaj @Kleidukos @ulysses4ever @Bodigrim @georgefst
@jasagredo beside updating that message above, the implementation looks good to me now 👍.
Ah, indeed the loss of granularity is problematic. That being said I am not knowledgeable enough to assert that calling pkg-config twice will produce a reliable result (reliably failed or unreliably succeeded on the second time). |
e3ee938 to
727acd8
Compare
727acd8 to
b5b972d
Compare
andreabedini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
b5b972d to
00f4734
Compare
sebright
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for simplifying PkgConfigDb, too!
cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs
Outdated
Show resolved
Hide resolved
|
@grayjay I think both of your comments are very valid points. I will incorporate them, then merge. Thanks! |
a03daba to
1dfae21
Compare
|
Rebased on latest |
When no pkg-config program was found, cabal would claim that the package is not in the db, instead of telling clearly that no pkg-config was found at all.
1dfae21 to
345a8d2
Compare
When no pkg-config program was found, cabal would claim that the package is not in the db, instead of telling clearly that no pkg-config was found at all.
QA Notes
On a system which has no pkg-config, with this package (for example):
The following message will be shown:
In verbose mode, the following warning will also be printed:
To emulate this one can do the following invocation that temporarily renames
pkg-configexecutable: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!)