Try each pkg-config query separatedly #9134

jasagredo · 2023-07-17T11:51:31Z

MinGW's pkg-config returns only one version even if queried for multiple libraries. This PR makes it so that cabal will invoke pkg-config once for each requested library if the result is not of the expected length, therefore supporting these pkg-config implementations, in particular mingw-w64-x86_64-pkgconf (which is also installed by GHCup when bootstrapping MSYS2).

Javier@DESKTOP-JDT20GG MINGW64 ~
$ pkg-config --modversion libcrypto libffi
3.1.1

In the same spirit as rust-lang/pkg-config-rs#92, as we also are not GCC, we also set PKG_CONFIG_ALLOW_SYSTEM_(CFLAGS|LIBS) when calling pkg-config

QA Notes

Building a package that depends on more than one library provided by pkg-config on MinGW should now be able to find the required library.

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.

Kleidukos · 2023-07-17T17:14:06Z

@jasagredo Thank you for the initiative! Would you mind adding QA notes and check the boxes in the description? :)

michaelpj · 2023-07-18T13:52:26Z

A bit of git blame turns up some previous discussion on the PR that added the individual fallback. It would be good to get this reasoning into the code in comments! #8496 (comment)

andreabedini · 2023-07-18T14:33:52Z

Related #8930

jasagredo · 2023-07-19T11:24:09Z

I've modified the logic to fallback to individual query if the length of the returned list doesn't match the length of the query, which is precisely what happens on Windows. On any other flows, the behavior should be the same.

andreabedini · 2023-07-21T07:11:03Z

I'm good with this, @michaelpj?

andreabedini

👍

michaelpj · 2023-07-21T10:19:21Z

Personally I think the argument for doing the batch query didn't seem that strong, and it seems to regularly cause problems. So I'd be in favour of the original change in this PR, but perhaps it's best to be conservative. Unless @gbaz wants to weigh in?

changelog.d/pkgconfig-once

grayjay · 2023-07-27T19:26:14Z

cabal-install-solver/src/Distribution/Solver/Types/PkgConfigDb.hs

-             pkgConfigDbFromList . catMaybes <$> mapM (getIndividualVersion pkgConfig) pkgNames
+        if exitCode == ExitSuccess && length pkgNames == length pkgList
+          then (return . pkgConfigDbFromList . zip pkgNames) (lines pkgVersions)
+          else -- if there's a single broken pc file the above fails, so we fall back into calling it individually


It would help to add the second reason for the fallback to this comment.

jasagredo · 2023-07-31T09:34:48Z

I have not yet confirmed my suspicions, but I think this is due to faulty pkg-configs installed in MSYS2, see this.

In particular:

Strawberry perl's pkg-config (included by default in GH runners) does not work as expected at all
pkgconf can't interpret C:... paths.
mingw-w64-x86_64-pkgconf returns only one version when queried for multiple.
mingw-w64-x86_64-pkg-config will work OK if and when we provide PKG_CONFIG_ALLOW_SYSTEM_(CFLAGS|LIBS) env vars

Mikolaj · 2023-09-04T10:41:38Z

@jasagredo: how can we help to move this forward?

jasagredo · 2023-09-16T15:02:57Z

@jasagredo: how can we help to move this forward?

I think the right way forward is to:

Close this PR

2a. GHCup in Windows checks if pkg-config installed is the appropriate one (see my comment above) and if so, offers the user to add those env vars to .bashrc or equivalent.

2b. Cabal in Windows detects if pkg-config is the appropriate one and if so, sets the env vars when calling it.

I do think that GHCup should fail in Windows if a non-appropriate pkg-config is found, the same way it might fail if gcc is non existent. (I might be wrong, I don't recall if it installs gcc for you if it is not present)

Probably @hasufell has some thoughts on this.

hasufell · 2023-09-17T01:27:50Z

I do think that GHCup should fail in Windows if a non-appropriate pkg-config is found

GHCup doesn't track your system tools.

These things are installed by the powershell bootstrap script and only on windows, because of how special the situation is with msys2.

But in general, this is not GHCup's business to take care of your non-Haskell stuff.

I might be wrong, I don't recall if it installs gcc for you if it is not present

It does not.

jasagredo · 2023-09-18T14:49:38Z

Sorry @hasufell it seems I was confused with the scope of the responsibilities for GHCup. However, in the case of GHCup bootstrapping MSYS2, would it make sense to install an appropriate pkg-config there?

In any case, if we cannot rely on an appropriate pkg-config being installed, we can use this PR to query for each pkg individually when the list of results is not the expected length, but also we could make cabal set PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 and PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 when calling pkg-config. This should be innocuous on systems without this problem.

TL;DR: I propose setting those 2 env vars when cabal calls pkg-config as a defensive measure, on top of what this PR already does.

What do you think? @Mikolaj

hasufell · 2023-09-18T14:59:04Z

However, in the case of GHCup bootstrapping MSYS2, would it make sense to install an appropriate pkg-config there?

We currently install mingw-w64-x86_64-pkgconf: https://github.com/haskell/ghcup-hs/blob/371eda962f713fe483616310291004a82e5b029d/scripts/bootstrap/bootstrap-haskell.ps1#L471

The end user might however uninstall it or install arbitrary other ones. This is just a convenience feature in the bootstrapper. We can't control what pacman has installed. Cabal should be maximally compatible here.

jasagredo · 2023-09-18T15:03:06Z

@hasufell alright, so this PR will make it work in presence of mingw-w64-x86_64-pkgconf by calling multiple times, one for each needed package. So that is OK. Thanks!!

Mikolaj · 2023-09-19T09:18:05Z

Makes sense. Thank you all for the effective discussion.

@jasagredo, before you set the merge label, does the description of the PR still match the new code? Do you think it makes sense to ping all four reviewers and tell them what changed since they've reviewed last time?

…query length MinGW's pkg-config returns only one version even if queried for multiple libraries.

juhp · 2023-11-03T03:42:08Z

Though honestly I think we should only query individual pkgconf's separately going forward.

Does cabal really needs all the pkgconf's even? I also wonder what the use-case is?

gbaz · 2023-11-03T04:02:34Z

The solver needs to know the installed packages on the system so it can make use of them in creating a build-plan. They are needed together rather than individually so that the solver can be pure rather than performing IO queries over the course of its execution.

juhp · 2023-11-03T06:00:45Z

At least pkgconf-2.0 seems to revert to the old pkgconf-1.8 behavior

But quite a few distros are currently on pkgconf-1.9 so we really need to fix this properly

jasagredo · 2023-11-03T09:33:52Z

This PR targeted solving the inability of Mingw's pkg-conf to return multiple results.

I think it is very reasonable to start discussions about what you propose @juhp but maybe in a new issue/PR? I think #8930 is in the same line you are proposing.

juhp · 2023-11-03T10:17:07Z

Yep, thanks I did that in #9391- I think I am addressing #8923 and I hope it also covers this case.

The mingw issue is different to Linux? That part is still unclear to me - does pkgconf-2.0 help?

jasagredo · 2023-11-03T11:02:14Z

Perhaps it does help? I have not tried (I'm not developing on windows nowadays). However the environment variables flags should be mandatory in Windows and harmless in Linux.

Mingw has this weird inconvenience where there are 3 pkgconf implementations and each one produces different outputs.

Mikolaj · 2023-12-21T11:34:34Z

@jasagredo (and presumably @grayjay) suggest to backport this to 3.10 branch for the 3.10.3.0 release. Let me mark it provisionally with a label, create a backport PR to see the conflicts and let's discuss.

Mikolaj · 2023-12-21T11:35:15Z

@mergify backport

mergify · 2023-12-21T11:35:30Z

backport

❌ No backport have been created

No destination branches found

Mikolaj · 2023-12-21T11:35:42Z

@mergify backport 3.10

mergify · 2023-12-21T11:35:46Z

backport 3.10

✅ Backports have been created

#9550 Try each pkg-config query separatedly (backport #9134) has been created for branch 3.10 but encountered conflicts

Mikolaj · 2023-12-21T11:37:06Z

The conflicts don't seem terrible.

Mikolaj · 2023-12-21T11:58:10Z

Here's the relevant comment by @grayjay: #9520 (comment)

Try each pkg-config query separatedly (backport #9134)

andreabedini added the re: pkg-config Concerning pkg-config and pkgconfig-depends constraints label Jul 18, 2023

jasagredo force-pushed the jasagredo/each-pkgconfig-once branch 2 times, most recently from bf9de80 to 4b87276 Compare July 19, 2023 11:23

andreabedini approved these changes Jul 21, 2023

View reviewed changes

gbaz approved these changes Jul 21, 2023

View reviewed changes

fgaz reviewed Jul 23, 2023

View reviewed changes

changelog.d/pkgconfig-once Show resolved Hide resolved

fgaz added the squash+merge me Tell Mergify Bot to squash-merge label Jul 23, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jul 25, 2023

grayjay reviewed Jul 27, 2023

View reviewed changes

Mikolaj removed squash+merge me Tell Mergify Bot to squash-merge merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Sep 4, 2023

jasagredo force-pushed the jasagredo/each-pkgconfig-once branch 5 times, most recently from 15e8fc8 to 2d6dcd9 Compare September 19, 2023 07:55

jasagredo added 2 commits October 7, 2023 11:15

Try each pkg-config query separatedly if returned list doesn't match …

3afd3a3

…query length MinGW's pkg-config returns only one version even if queried for multiple libraries.

Set PKG_CONFIG_ALLOW_SYSTEM_(CFLAGS|LIBS) by default

9f15045

Mikolaj force-pushed the jasagredo/each-pkgconfig-once branch from 2d6dcd9 to 9f15045 Compare October 7, 2023 11:15

mergify bot merged commit a0538d9 into haskell:master Oct 7, 2023
44 checks passed

georgefst mentioned this pull request Oct 22, 2023

Cache the solver's pkg-config calls #9360

Draft

4 tasks

juhp mentioned this pull request Nov 3, 2023

Release Cabal-3.10.2.1 #9350

Merged

gbaz mentioned this pull request Nov 4, 2023

cabal-install-solver: fix pkgconf 1.9 --modversion regression #9391

Merged

4 tasks

juhp added a commit to fedora-haskell/cabal-install that referenced this pull request Nov 6, 2023

add pkgconf fix from haskell/cabal#9134

1f3a2fd

georgefst mentioned this pull request Nov 24, 2023

Simplify pkg-config logic by removing the batch query #9478

Open

jasagredo deleted the jasagredo/each-pkgconfig-once branch November 27, 2023 22:15

carbolymer mentioned this pull request Dec 4, 2023

[BUG] - Can't build cardano-node on Fedora - conflict: pkg-config package libsodium-any, not found in the pkg-config database IntersectMBO/cardano-node#5561

Closed

grayjay mentioned this pull request Dec 21, 2023

cabal-install-solver: fix pkgconf 1.9 --modversion regression (backport #9391) #9520

Merged

Mikolaj added the attention: needs-backport 3.10 label Dec 21, 2023

mergify bot mentioned this pull request Dec 21, 2023

Try each pkg-config query separatedly (backport #9134) #9550

Merged

mergify bot added a commit that referenced this pull request Dec 22, 2023

Merge pull request #9550 from haskell/mergify/bp/3.10/pr-9134

d09cd57

Try each pkg-config query separatedly (backport #9134)

Mikolaj removed the attention: needs-backport 3.10 label Dec 28, 2023

andreabedini mentioned this pull request Jun 21, 2024

Make clear when pkg-config is not present #10122

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try each pkg-config query separatedly #9134

Try each pkg-config query separatedly #9134

jasagredo commented Jul 17, 2023 •

edited

Loading

Kleidukos commented Jul 17, 2023

michaelpj commented Jul 18, 2023

andreabedini commented Jul 18, 2023

jasagredo commented Jul 19, 2023

andreabedini commented Jul 21, 2023

andreabedini left a comment

michaelpj commented Jul 21, 2023

grayjay Jul 27, 2023

jasagredo commented Jul 31, 2023 •

edited

Loading

Mikolaj commented Sep 4, 2023

jasagredo commented Sep 16, 2023

hasufell commented Sep 17, 2023

jasagredo commented Sep 18, 2023

hasufell commented Sep 18, 2023

jasagredo commented Sep 18, 2023

Mikolaj commented Sep 19, 2023

juhp commented Nov 3, 2023

gbaz commented Nov 3, 2023

juhp commented Nov 3, 2023

jasagredo commented Nov 3, 2023

juhp commented Nov 3, 2023 •

edited

Loading

jasagredo commented Nov 3, 2023 •

edited

Loading

Mikolaj commented Dec 21, 2023 •

edited

Loading

Mikolaj commented Dec 21, 2023

mergify bot commented Dec 21, 2023

Mikolaj commented Dec 21, 2023

mergify bot commented Dec 21, 2023 •

edited

Loading

Mikolaj commented Dec 21, 2023

Mikolaj commented Dec 21, 2023

Try each pkg-config query separatedly #9134

Try each pkg-config query separatedly #9134

Conversation

jasagredo commented Jul 17, 2023 • edited Loading

QA Notes

Kleidukos commented Jul 17, 2023

michaelpj commented Jul 18, 2023

andreabedini commented Jul 18, 2023

jasagredo commented Jul 19, 2023

andreabedini commented Jul 21, 2023

andreabedini left a comment

Choose a reason for hiding this comment

michaelpj commented Jul 21, 2023

grayjay Jul 27, 2023

Choose a reason for hiding this comment

jasagredo commented Jul 31, 2023 • edited Loading

Mikolaj commented Sep 4, 2023

jasagredo commented Sep 16, 2023

hasufell commented Sep 17, 2023

jasagredo commented Sep 18, 2023

hasufell commented Sep 18, 2023

jasagredo commented Sep 18, 2023

Mikolaj commented Sep 19, 2023

juhp commented Nov 3, 2023

gbaz commented Nov 3, 2023

juhp commented Nov 3, 2023

jasagredo commented Nov 3, 2023

juhp commented Nov 3, 2023 • edited Loading

jasagredo commented Nov 3, 2023 • edited Loading

Mikolaj commented Dec 21, 2023 • edited Loading

Mikolaj commented Dec 21, 2023

mergify bot commented Dec 21, 2023

❌ No backport have been created

Mikolaj commented Dec 21, 2023

mergify bot commented Dec 21, 2023 • edited Loading

✅ Backports have been created

Mikolaj commented Dec 21, 2023

Mikolaj commented Dec 21, 2023

jasagredo commented Jul 17, 2023 •

edited

Loading

jasagredo commented Jul 31, 2023 •

edited

Loading

juhp commented Nov 3, 2023 •

edited

Loading

jasagredo commented Nov 3, 2023 •

edited

Loading

Mikolaj commented Dec 21, 2023 •

edited

Loading

mergify bot commented Dec 21, 2023 •

edited

Loading