-
Notifications
You must be signed in to change notification settings - Fork 697
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
Include extra-lib-dirs-static into PackageHash #7794
Conversation
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.
Given that there's no test and, I assume, the main PR will be tested, could you add a changelog and mention there the main PR and/or main issue?
We could add a test similar to the used one for check the fix for duplicate entries in package hash inputs: https://github.com/haskell/cabal/tree/master/cabal-testsuite/PackageTests/Regression/T6906 |
@alexbiehl: CI should run fine once you rebase --- I fixed the failure. |
c4544de
to
94f3113
Compare
CI is green, we just need a minimal changelog file and test. |
@alexbiehl: may I help you in getting this PR to the finish line? :) |
I am sorry I didn't get to this. Help would be very much appreciated. |
I also opened https://gitlab.haskell.org/ghc/ghc/-/issues/20759 upstream to track the GHC side of things. |
Given that this is a part of #6935, the changelog.d/pr-7794 could be
|
@alexbiehl: I think a copy-paste of this test would be an acceptably cheap way of testing this functionality. What do you think? Would you add that? |
@alexbiehl: ping? |
hmm #6935 is a pr, not an issue, and afaics #6935 does not have a clear linked issue, so maybe we should remove |
@Mergifyio rebase master |
✅ Branch has been successfully rebased |
94f3113
to
88d2372
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.
the pr looks good but needs a changelog entry and a test, thanks
Changelog entry + tests should be added - approved otherwise |
@mergify rebase |
✅ Branch has been successfully rebased |
88d2372
to
9139c5b
Compare
@alexbiehl: the PR is still good, CI passes! May we assist you in shipping it? Alternatively, would you like to post it for adoption? |
@alexbiehl: respectful ping? |
Hi! I am not sure how to test this TBH. Also this is a bug fix for something that is purely internal rn so I am not sure it needs an entry in the changelog. |
@alexbiehl: I hope we can meet in the middle. :D We require changelogs for anything that differs from last published version, regardless of how internal the things are (so only a fix of a bug introduced in devel version would not require a changelog, if it refrained from modifying any extra things). However, if the test is hard to do and the change is internal, I think we can risk it (more than usual) without a dedicated test. BTW, if you could also smuggle into the PR the changelog fixup @jneira mentions in #7794 (comment), that would be super-awesome. |
f470d46
to
298a0c6
Compare
298a0c6
to
d30e2be
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.
a test would be great, bur as you consider, thanks!
@alexbiehl: feel free to add the trigger label: merge_me or merge_me+squash. |
@alexbiehl: thank you once again. BTW. we now merge via the mergify labels: https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions, which prevents complicated merge histories such us the one you just created and give interested people 2 more days to look at the final code before it's (rebased and) merged. |
Continuing working on the matter and found we were missing the extra-libs-static in PackageHash.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!