-
Notifications
You must be signed in to change notification settings - Fork 707
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
Append paths in global config to progdb in configureCompiler
#10790
base: master
Are you sure you want to change the base?
Conversation
45e2214
to
7ee0d49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Would it be possible to add a test for this?
Could you perhaps add a testcase? See the similar test I added in 2c19bf3 |
I think a test would be possible to add. I however don't have the time at the moment for doing cabal stuff in my free time. |
I feel like I can contribute the test case. I just want some clarification from Cabal devs beforehand though: The The modification I would make to the test case is to make it cover Windows as well, which should cover the specific case raised in #9800 and probably would close #6304. |
I think it probably isn't skipped on CI, it's probably one which you have to pass |
If I recall correctly it is indeed skipped on CI on Windows because "Windows CI has no pkg-config". So probably if you make sure pkg-config is installed before starting the testsuite you might be able to not skip it and call it a day. I do wonder why I chose to skip it always on Windows and not only on Windows+CI 🤔 |
If you look at this CI run, https://github.com/haskell/cabal/actions/runs/13251642775/job/36994558671#step:17:1 for MacOS in a PR separate from this one, and search for "ExtraProgPath/" you will see it is mentioned twice. The first time it is a Cabal-the-library testsuite so it is skipped, and the second time it is a cabal-the-tool (cabal-install) testsuite. The second time it is run and the test is successful. What error does it show in your Mac? (Note you can filter the tests run with (Also many thanks for offering to add the test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three primes muddy the waters a bit much here. Could you rename those progdb
bindings to something more meaningful?
You're right, the tests do actually pass already. I was incorrectly comparing my local version (3.12.0.0) with the post-PR build of cabal-install when I should've been comparing a build pre-PR and post-PR. I'll write up the tests for the global progdb soon, likely as a new test called ExtraProgPathGlobal. I can't seem to find another test that creates and uses a global cabal config though, so if you have any quick pointers on how to mock that, it would be great. Otherwise, I'll take a stab during the weekend. |
@jasagredo Could you please enable edit access on the PR? There should be a tickbox on the sidebar. I've written the test and fixed the triple primes but can't push. Unless it's better making another PR? |
I can't do it because the GitHub app doesn't have that option :( . I will have access to a computer on Wednesday, otherwise feel free to open another PR and close this one as superseded. |
Closes #9800
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significant
in the changelog file.