-
Notifications
You must be signed in to change notification settings - Fork 698
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
make cabal-install integration tests hermetic #10205
Conversation
Also, someone told me "nobody runs validate locally" — but I've needed to do so several times, and in particular I prefer to validate locally before bogging down CI when possible. |
717745c
to
544c8e1
Compare
Uh, weird validate error? I don't think that's me, it looks like it hadn't even reached IntegrationTests2 yet. |
bf6469b
to
8cbb41b
Compare
I came up with a change that makes it work locally, but may make some tests unhappy. Guess we'll see. (ETA: seems to be working.) I could argue the first version was more correct, since we should really not rely on |
Manual QA is simple: run
(I'm not sure if |
If it "needs-review", it should be un-drafted, presumably... |
I would like people to comment on it, specifically on my approach, before I commit to it. |
96b77ea
to
e46888d
Compare
Nice idea. I have noticed when running tests locally: cabal run cabal-testsuite:cabal-tests -- --hide-successes --with-cabal=./bin/cabal That I receive many failures due to the following lines in the output: +Warning: Both <ROOT>/cabal.dist/home/.cabal and /home/tommy/.config/cabal/config exist - ignoring the former.
+It is advisable to remove one of them. In that case, we will use the remaining one by default (unless '$CABAL_DIR' is explicitly set). While the right answer in this particular situation is to delete the extra dir (it appears to be created by nix in some circumstances), it would be nice if the tests were isolated. My issue may be out of scope for this particular PR (I tried it out and still received the errors), but I thought I'd comment since it seems related. |
I don't use Nix, so I don't think that's the cause. In any case, this patch sets |
Also, my guess is you're getting them from things other than the |
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.
Thanks again!
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.
Looks reasonable.
I was bitten by this with my:
ghc-options: -optc-Wno-pragma-pack -optc-Wno-macro-redefined -optc-Wno-missing-declarations
They were using the user's config, which is a problem if they don't have one or if it contains settings that interfere with tests (such as `documentation: True`). I'm not entirely certain of this, but it seems to work here. It's a bit of a hack, though.
e46888d
to
cd45432
Compare
@mergify backport 3.12 |
✅ Backports have been created
|
make cabal-install integration tests hermetic (backport #10205)
They were using the user's config, which is a problem if they don't have one or if it contains settings that interfere with tests (such as
documentation: True
).I'm not entirely certain of this, but it seems to work here. It's a bit of a hack, though.
Closes: #10187
Template B: This PR does not modify behaviour or interface
E.g. the PR only touches documentation or tests, does refactorings, etc.
Include the following checklist in your PR: