Skip to content
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

Merged
merged 1 commit into from
May 2, 2024

Conversation

sheaf
Copy link
Collaborator

@sheaf sheaf commented Apr 29, 2024

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

@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 29, 2024

PR template is missing @sheaf , is this a user-visible (cabal/API) change or not?

@andreasabel
Copy link
Member

Testcase?

@alt-romes
Copy link
Collaborator

@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 Cabal. There should be no visible change. This is fixing a bug (#9940) where cabal-install was passing too-recent flags to an older Cabal version.

@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 30, 2024

@ffaf1 This is a change to how cabal-install invokes Cabal. There should be no visible change. This is fixing a bug (#9940) where cabal-install was passing too-recent flags to an older Cabal version.

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.

@alt-romes
Copy link
Collaborator

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?

@sheaf
Copy link
Collaborator Author

sheaf commented Apr 30, 2024

PR template is missing @sheaf , is this a user-visible (cabal/API) change or not?

This fixes a bug in the master version of the cabal-install executable. As reported in #9940, this bug currently only impacts people who build cabal-install from master and try to use it to compile a package with a custom setup with Cabal == 3.12.*. If this makes it into the next cabal-install release then no released cabal-install version will be affected by this bug.

@sheaf
Copy link
Collaborator Author

sheaf commented Apr 30, 2024

Testcase?

I am adding a test now.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

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?

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.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 30, 2024

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.

@sheaf sheaf force-pushed the workingDirVersionCheck branch from 397dcbb to 50cf162 Compare April 30, 2024 11:21
@sheaf
Copy link
Collaborator Author

sheaf commented Apr 30, 2024

I've added a test (PackageTests/WorkingDir) and confirmed that it fails before the patch and succeeds after.

@sheaf sheaf force-pushed the workingDirVersionCheck branch from 50cf162 to ef633b3 Compare April 30, 2024 12:27
@sheaf sheaf force-pushed the workingDirVersionCheck branch from ef633b3 to 8029f06 Compare April 30, 2024 13:51
| otherwise = error "the impossible just happened" -- see first guard
where
flags_latest = flags
flags_3_11_0 =
flags_3_13_0 =
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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?

@sheaf sheaf added the merge me Tell Mergify Bot to merge label May 2, 2024
@andreasabel andreasabel added merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days regression on master Regression that is unreleased and needs to be fixed before release labels May 2, 2024
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
@Mikolaj Mikolaj force-pushed the workingDirVersionCheck branch from 8029f06 to abd4ead Compare May 2, 2024 12:55
@mergify mergify bot merged commit bfd9bfb into master May 2, 2024
50 checks passed
@mergify mergify bot deleted the workingDirVersionCheck branch May 2, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge regression on master Regression that is unreleased and needs to be fixed before release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong condition for passing --working-dir flag
5 participants