Skip to content

Conversation

@gbaz
Copy link
Collaborator

@gbaz gbaz commented Sep 5, 2021

Potentially resolves #7448

This is a one lexeme change that I would argue "fixes" pkgconfig-depends handling in the solver. Before, if there was no pkg-config that could be found, the solver would optimistically succeed. But since the builder then requires using pkg-config to get the linking flags, this would necessarily fail on build anyway. Now, we fail. This lets a user set up an auto flag that toggles between using a pkgconfig-depends and explicit extra-libraries etc stanza.

I think this gives a nice solution that is strictly better than the prior situation.

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.

Another one-liner that solves world peace? Sounds legit. :D

But seriously, this could work. Any chance for a test and changelog?

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 6, 2021

Changelog, sure. I'm not quite sure how one might add a test for something like this, since it explicitly involves pkg-config not being on the path, etc. Advice in that direction is appreciated.

@sebright
Copy link
Collaborator

sebright commented Sep 7, 2021

I'm not quite sure how one might add a test for something like this, since it explicitly involves pkg-config not being on the path, etc.

You could add unit tests for solving without pkg-config, similar to the existing pkg-config tests:

, testGroup "Pkg-config dependencies" [
runTest $ mkTestPCDepends [] dbPC1 "noPkgs" ["A"] anySolverFailure
, runTest $ mkTestPCDepends [("pkgA", "0")] dbPC1 "tooOld" ["A"] anySolverFailure
, runTest $ mkTestPCDepends [("pkgA", "1.0.0"), ("pkgB", "1.0.0")] dbPC1 "pruneNotFound" ["C"] (solverSuccess [("A", 1), ("B", 1), ("C", 1)])
, runTest $ mkTestPCDepends [("pkgA", "1.0.0"), ("pkgB", "2.0.0")] dbPC1 "chooseNewest" ["C"] (solverSuccess [("A", 1), ("B", 2), ("C", 1)])
]

It looks like all of the tests create a pkg-config DB by calling pkgConfigDbFromList , so you would need to add an option for no DB:

, testPkgConfigDb = pkgConfigDbFromList pkgConfigDb

@gbaz
Copy link
Collaborator Author

gbaz commented Sep 9, 2021

Thanks! I implemented tests as suggested

, runTest $ mkTest dbBuildable2 "choose version that sets buildable to false" ["A"] (solverSuccess [("A", 1), ("B", 2)])
]
, testGroup "Pkg-config dependencies" [
runTest $ mkTestPCDepends [] dbPC1 "noPkgs" ["A"] anySolverFailure
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be useful to also keep the empty pkg-config test case, using Just [].

@gbaz gbaz added the squash+merge me Tell Mergify Bot to squash-merge label Sep 9, 2021
@mergify mergify bot merged commit 68e2737 into master Sep 10, 2021
@gbaz gbaz mentioned this pull request Dec 2, 2021
@fgaz
Copy link
Member

fgaz commented Feb 4, 2022

  • The documentation has been updated, if necessary.

👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

attention: needs-review cabal-install: solver merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days re: pkg-config Concerning pkg-config and pkgconfig-depends constraints squash+merge me Tell Mergify Bot to squash-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make pkgconfig-depends usable

6 participants