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

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

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

juhp
Copy link
Collaborator

@juhp juhp commented Nov 3, 2023

Check that the numbers of versions output is equal to the number of pkgconf's

fixes #8923

Noting that the pkgconf behavior was since reverted upstream in 2.0.

(checking that equal pkgnames are output also doesn't hurt per se, but that should be trivially true and this should cover that case too anyway)


This PR modifies cabal behaviour

Include the following checklist in your PR:


QA Notes

A simple testcase is building libarchive with cabal build -f system-libarchive and pkgconf-1.9 on Linux (eg with Fedora Linux 39)* - though any package using pkgconfig-depends will do.

See also the original issue for more details.

(* which is how I ran into this again while trying to update ghcup version from source)

@juhp juhp mentioned this pull request Nov 3, 2023
4 tasks
@gbaz
Copy link
Collaborator

gbaz commented Nov 4, 2023

I'm confused. Isn't this the same thing fixed by #9134 which is already merged?

@grayjay
Copy link
Collaborator

grayjay commented Nov 6, 2023

I think this PR is fixing a bug in #9134.

@juhp
Copy link
Collaborator Author

juhp commented Nov 6, 2023

I'm confused. Isn't this the same thing fixed by #9134 which is already merged?

Please see my comment on the code there

@georgefst
Copy link

georgefst commented Nov 17, 2023

Could we consider just dropping the batch query completely? This was asked in #9134 (comment) and #9134 (comment). AFAICT all it gives us is a fairly small speedup on some systems, while doubling the time for anyone with any broken packages (which is common: #8930) and adding complexity.

(It would be even better if we cached results on a per-package basis, as in #9360.)

@grayjay
Copy link
Collaborator

grayjay commented Nov 17, 2023

I would rather merge this PR as is, as a straightforward bug fix. The change to pkg-config querying logic could go in a separate PR.

@juhp juhp added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Nov 19, 2023
@juhp
Copy link
Collaborator Author

juhp commented Nov 19, 2023

Does this require a changelog entry?

@Kleidukos
Copy link
Member

yes please! :)

@juhp juhp force-pushed the pkgconf-1.9 branch 2 times, most recently from b43fb62 to c17050b Compare November 19, 2023 13:13
@Kleidukos Kleidukos added the attention: needs-manual-qa PR is destined for manual QA label Nov 19, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 22, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Nov 23, 2023

This got stuck due to changes CI mandatory tests rule. Rebasing...

@Mikolaj
Copy link
Member

Mikolaj commented Nov 23, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 23, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Nov 23, 2023

And now some randomly cancelled jobs. Let me restart. [Edit: I see @Kleidukos already did, but it's still borked. One more time...]

Check that the numbers of *versions* output is equal to the number of pkgconf's

fixes haskell#8923

The pkgconf behavior was reverted upstream in 2.0

(this should cover the case too of checking that equal pkgList lines are output also)
@mergify mergify bot merged commit 6314590 into haskell:master Nov 23, 2023
46 checks passed
@juhp juhp deleted the pkgconf-1.9 branch November 24, 2023 07:51
@juhp
Copy link
Collaborator Author

juhp commented Nov 24, 2023

Thanks - I think it would be good to backport to 3.10 at least

@Mikolaj
Copy link
Member

Mikolaj commented Nov 24, 2023

If we backport it. do we want to backport anything related (e.g., depending or depended upon)?

@jasagredo
Copy link
Collaborator

Confirmed working on Windows, it queries for multiple packages. Somewhat related I found #9479. But this PR works

@Mikolaj
Copy link
Member

Mikolaj commented Dec 7, 2023

@Kleidukos: do we want to backport this for 3.10.3?

@juhp: do we need to backport anything first? any PR deps?

@Kleidukos
Copy link
Member

Yes we absolutely backport this one to 3.10, thank you @Mikolaj

@Mikolaj
Copy link
Member

Mikolaj commented Dec 14, 2023

@juhp: I assume your lack of response means we don't need to backport anything else in tandem, so let me just backport this alone.

@Mikolaj
Copy link
Member

Mikolaj commented Dec 14, 2023

@mergify backport 3.10

Copy link
Contributor

mergify bot commented Dec 14, 2023

backport 3.10

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-manual-qa PR is destined for manual QA cabal-install: solver merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge tested-on: windows QA has been successful on Windows
Projects
Status: To Test
Development

Successfully merging this pull request may close these issues.

pkgconf-provided pkg-config does --modversion differently; dependency resolution fails mysteriously
7 participants