-
Notifications
You must be signed in to change notification settings - Fork 704
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
Avoid spurious warnings from -with-rtsopts (fix #4255) #8183
Conversation
cb4c34d
to
0dd5ee6
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.
Looks great. Any chance of a tiny test (one that breaks currently, works with this patch)?
e436535
to
1b4691a
Compare
@Mikolaj a test added. Not sure how to show that it fails without the patch :-) |
With hand gestures. Does it fail? |
-- Test that setup shows all the 'autogen-modules' warnings. | ||
main = setupAndCabalTest $ do | ||
setup' "configure" [] >>= | ||
assertOutputDoesNotContain "Warning: Instead of 'ghc-options: -I0' use 'include-dirs: 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.
That's the behaviour without your fix?
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.
Yes. This is documented in #4255 and reproduces today.
Ok, I moved back one commit, here's the test (not very user-friendly, but notice "unexpected: Warning:"): Console output
|
4fe9aaf
to
4389cc3
Compare
After wrangling with the test suite for a bit, I have CI green finally. |
Windows CI acted up, so I restarted failing jobs. |
import Distribution.Simple | ||
main = defaultMain |
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 think that file is no longer needed (with high enough cabal-version?).
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 can experiment and remove it. Would CI catch the problem if it exists?
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.
Most definitely yes. It even catches problems that don't exist (e.g., the Windows validated runs).
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.
Love it: the ultimate response of a sound type checker — "reject!"
Meanwhile, CI finished green without the file.
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.
LGTM
merge_me? |
Fix #4255.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!