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.

Sign up for GitHub

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

Jump to bottom

Don't warn about 'prepositive-qualified-module' in Paths_pkgs #8336

Merged
merged 1 commit into from
Oct 4, 2022
Merged

Don't warn about 'prepositive-qualified-module' in Paths_pkgs #8336

merged 1 commit into from
Oct 4, 2022

Conversation

martijnbastiaan
Copy link
Collaborator

Enabling '-Wprepositive-qualified-module' in a project leads to warnings
when used in combination with Cabal's Paths_* feature. Given that this
is code outside of a user's control, this warning should be disabled
locally.

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@martijnbastiaan
Copy link
Collaborator Author

T3827 sporadically fails, but I don't think it has anything to do with this PR :)

Mikolaj
Mikolaj approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

BTW, that's an interesting failure. Let me rerun CI and see if it repeats.

@Mikolaj Mikolaj added attention: needs-review re: warnings Concerning warnings printed by cabal labels Aug 3, 2022
@martijnbastiaan
Copy link
Collaborator Author

martijnbastiaan commented Aug 3, 2022 •
edited
Loading

BTW, that's an interesting failure. Let me rerun CI and see if it repeats.

Yeah, I got it while testing on my own repo too.

It seems to happen ~50% of the time. Let me just scrap that comment, CI tests a mix of operation systems, and I can't seem to reproduce it locally anymore.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 3, 2022

Yeah, I got it while testing on my own repo too. It seems to happen ~50% of the time.

On Linux? Which GHC version?

@martijnbastiaan
Copy link
Collaborator Author

It failed on macos-latest ghc-9.2.3 (and locally on Ubuntu, but I can't seem to reproduce that now).

@martijnbastiaan
Copy link
Collaborator Author

I've only seen it on GHC 9.2.

@martijnbastiaan
Copy link
Collaborator Author

Last CI failure on github.com/martijnbastiaan: https://github.com/martijnbastiaan/cabal/runs/7646056444?check_suite_focus=true.

This was referenced Aug 3, 2022
Merged
Open
@Mikolaj
Copy link
Member

Mikolaj commented Aug 4, 2022

I think I've fixed CI, do let me rebase and check.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 4, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 4, 2022

rebase

✅ Branch has been successfully rebased

@andreabedini andreabedini force-pushed the account-for-prepositive-qualified-module branch from 5ceec75 to 6340661 Compare August 4, 2022 00:03
ulysses4ever
ulysses4ever approved these changes Aug 8, 2022
View reviewed changes
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @martijnbastiaan! do you want to apply the merge-me label?

@martijnbastiaan
Copy link
Collaborator Author

Yes please.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 8, 2022

@martijnbastiaan: I took the liberty of giving you the Triage privilege for the cabal repo. If this is OK, please verify by setting the merge_me label.

@martijnbastiaan martijnbastiaan added the merge me Tell Mergify Bot to merge label Aug 8, 2022
@martijnbastiaan
Copy link
Collaborator Author

Done! Let's see if it gets picked up.

@ulysses4ever
Copy link
Collaborator

@martijnbastiaan just in case: we have a Mergify setting that makes it wait for 2 days before merging (this is to ensure that everyone is happy have chance to chime in if not).

@andreabedini andreabedini force-pushed the account-for-prepositive-qualified-module branch from 6340661 to 921bf6b Compare August 10, 2022 14:42
@Mikolaj
Copy link
Member

Mikolaj commented Aug 11, 2022

Let me rebase to see if the recent CI workaround unblocks merging.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 11, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2022

rebase

✅ Branch has been successfully rebased

@andreabedini andreabedini force-pushed the account-for-prepositive-qualified-module branch 2 times, most recently from d44a102 to e271f60 Compare August 13, 2022 17:57
@Mikolaj
Copy link
Member

Mikolaj commented Aug 16, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 16, 2022

rebase

✅ Branch has been successfully rebased

@ulysses4ever ulysses4ever force-pushed the account-for-prepositive-qualified-module branch from e271f60 to 7465118 Compare August 16, 2022 22:08
andreasabel
andreasabel reviewed Aug 18, 2022
View reviewed changes
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I have some stylistic nit-picking...

Cabal/src/Distribution/Simple/Build/PathsModule/Z.hs Outdated Show resolved Hide resolved
cabal-testsuite/PackageTests/PathsModule/ImportQualifiedPost/my.cabal Outdated Show resolved Hide resolved
templates/Paths_pkg.template.hs Outdated Show resolved Hide resolved
@martijnbastiaan martijnbastiaan removed the merge me Tell Mergify Bot to merge label Aug 18, 2022
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
@martijnbastiaan martijnbastiaan force-pushed the account-for-prepositive-qualified-module branch from 7465118 to 5365335 Compare October 2, 2022 06:51
@martijnbastiaan
Don't warn about 'prepositive-qualified-module' in Paths_pkgs
cc8d5df 
Enabling '-Wprepositive-qualified-module' in a project leads to warnings
when used in combination with Cabal's Paths_* feature. Given that this
is code outside of a user's control, this warning should be disabled
locally.
@martijnbastiaan martijnbastiaan force-pushed the account-for-prepositive-qualified-module branch from 5365335 to cc8d5df Compare October 2, 2022 06:52
@martijnbastiaan
Copy link
Collaborator Author

I've applied the style suggestions. Given that this was approved before and that it hasn't attracted further comments, I've taken the liberty to re-apply the merge me label.

@martijnbastiaan martijnbastiaan added the merge me Tell Mergify Bot to merge label Oct 2, 2022
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Oct 4, 2022
@mergify mergify bot merged commit a49c1dd into haskell:master Oct 4, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Oct 24, 2022

@martijnbastiaan: thank you for the PR once again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasabel andreasabel andreasabel left review comments

@ulysses4ever ulysses4ever ulysses4ever approved these changes

@Mikolaj Mikolaj Mikolaj approved these changes

Assignees
No one assigned
Labels
attention: needs-backport 3.8 merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: warnings Concerning warnings printed by cabal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martijnbastiaan @Mikolaj @ulysses4ever @andreasabel