Append paths in global config to progdb in configureCompiler v2 #10826
Conversation
We add a new PackageTest for cabal-install, which makes sure that the extra-prog-path in the global config is used when resolving pkg-config instead of anything already on PATH. We do this by specifying a "bad" pkg-config to extra-prog-path that returns exit code 1. On Windows, Cabal logs are identical in the following two cases: - the override didn't work, and pkg-config was not found on the system - the override did work, and querying it failed (expectedly) To work around this, we add a "good" pkg-config script to PATH before- hand, so that cabal succeeds in the first case and the logs differ. This test is a stripped down reproduction of what happens when a user on Windows adds MSYS2 paths to extra-prog-path but not the system PATH. This is a reasonable setup (often, adding MSYS2 to system PATH is not good practice), and is even created automatically by GHCup.
yutotakano
changed the title
yutotakano
force-pushed
the
yt/fix-paths-windows
branch
from
49b8f22 to
2e768df
Compare
yutotakano
force-pushed
the
yt/fix-paths-windows
branch
from
2e768df to
d20122d
Compare
|
Hi! Thank you for the fix. Does "UNEXPECTED FAIL: PackageTests/ExtraProgPathGlobal/setup.test.hs" sound reasonable or is this suspect of being an unrelated CI failure? |
|
cHi! That's unexpected, and I've been meaning to fix it, but I haven't got my hands on a Linux/Mac machine yet to debug (the test passes on Windows but it's also supposed to pass on other OSes). Please wait a bit! Sorry, I should've marked this PR as a draft or something. EDIT: in retrospect, I realised that I need this as a non-draft PR to run the CI checks on it. I'll write a message here once it's ready! |
|
@Mikolaj This is ready for review! The failure reason was simple: the script I added in the repo didn't have +x permissions, which is why it failed to run on Mac and Linux. |
|
@yutotakano: thank you! Let me add a label to get the reviews started. |
There was a problem hiding this comment.
Note to reviewers, this shim pkg-config was taken as-is from cabal-testsuite\PackageTests\PkgConfigParse\pkg-config, and its sole purpose is to provide something that looks and acts like pkg-config for the purposes of the test. (Importantly, it runs on Windows where pkg-config doesn't exist using the .exe file as a proxy that calls this script)
|
This is subtle stuff. It'd be great to add QA notes https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#qa-notes. |
|
@ulysses4ever I've added some notes on what my intended functionality was and how QA can test it. I hope I'm understanding correctly what you meant by QA notes? I also realised that the changelog entry file has the superceded PR number, is that fine? I suspect changing it would invalidate existing approval reviews, which is fine by me but additional effort for cabal maintainers. |
|
Fixing the changelog file shouldn't invalidate anything, so, please, go ahead and fix it. QA notes look great, thank you! Next step is to find someone with access to a Windows machine :-) |
|
Done re:changelog file! I'll await the reviews :) |
Supersedes #10790 by adding tests and fixing triple-prime variable naming.
Closes #9800.
QA Notes
Cabal should be able to resolve system packages using pkg-config when pkg-config doesn't exist in the system PATH but is provided in the path specified in
~/.cabal/configin theextra-prog-path.On Mac/Unix and other systems where pkg-config exists in PATH, there should theoretically be no regressions due to this PR. Package dependency resolution and command line interface should be the same before and after.
On Windows where pkg-config does not exist on PATH by default, specifying one on
extra-prog-path(like which is done by recent GHCup automatically) should now work for resolving dependencies, when previously it would not work. You should be able to QA this by:pkg-configinto the Mingw environment:ghcup run -m -- pacman -S mingw64/mingw-w64-x86_64-pkg-configpkgconfig-dependson somethingTemplate Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.