-
Notifications
You must be signed in to change notification settings - Fork 701
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
Filter Setup flags: filter working dir on < 3.13 #9951
Conversation
PR template is missing @sheaf , is this a user-visible (cabal/API) change or not? |
Testcase? |
@andreasabel At the moment the testing infrastructure does not yet allow a recent cabal install version to be tested against multiple Cabal versions (although @mpickering has recently laid out the foundations for this). So, just like for the remaining filtered flags, there's no easy way to add a test for this one. @ffaf1 This is a change to how cabal-install invokes |
I appreciate the explanation. The more we use the two PR templates, the easier it is not to have hot messes during release time like #9952. |
The PR templates would have achieved nothing for #9952 in particular, even if they are useful in other situations. #9952 was caused by a change which would have filled any of the templates trivially. It happened to break GHC 8.2 which wasn't tested in CI. Rather, when CI was dropped for GHC 8.2 the bounds (of the base dependency) for Cabal at the time should have been updated to disallow the versions we stopped testing. Right? |
This fixes a bug in the master version of the |
I am adding a test now. |
Yes, but one part of #9952 is that dropping GHC 8.2 has not been recorded in release notes. With proper PR template and the new clearer definition of a user-visible change, the PR that tweaked CI would have to include a changelog file so the release notes would be fine. With some luck, and the recent extra comments in CI scripts, following the process carefully would also make omitting the base bound harder. |
Also, why are we arguing about the process here? If you don't have the time to follow the process, apologize and move on. If you don't like the process, file a ticket with proposed changes. I don't think arguing with a person that points out the process has not been followed is productive. |
397dcbb
to
50cf162
Compare
I've added a test ( |
50cf162
to
ef633b3
Compare
ef633b3
to
8029f06
Compare
| otherwise = error "the impossible just happened" -- see first guard | ||
where | ||
flags_latest = flags | ||
flags_3_11_0 = | ||
flags_3_13_0 = |
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.
I wonder how we are going to backport this. Probably this one stays, but the 3.11 above is not changed to 3.13? Or is it about which version is the last know version of flags, not last known version of cabal at all, so no problem if the flags version is in the future?
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.
Oh right... I think we might not even need to backport anything, because cabal-install-3.12.x
will not attempt to pass any --working-dir
arguments because it doesn't have the working directory patch.
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.
Oh, great! Regardless, let's merge this one. @sheaf: could you kindly set the merge_me or squash+merge_me label?
The --working-dir flag is only available for Cabal >= 3.13. This commit fixes an incorrect conditional: we only filtered out this flag for Cabal < 3.11, instead of < 3.13. Test: PackageTests/WorkingDir Fixes #9940
8029f06
to
abd4ead
Compare
The --working-dir flag is only available for Cabal >= 3.13. This commit fixes an incorrect conditional: we only filtered out this flag for Cabal < 3.11, instead of < 3.13.
Fixes #9940