Make clear when pkg-config is not present - #10122
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.
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! |
|
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.
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 👍 |
There was a problem hiding this comment.
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
b5b972d to
00f4734
Compare
sebright
left a comment
There was a problem hiding this comment.
Thanks for simplifying PkgConfigDb, too!
|
@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!)