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
Disable warnings when building stackage dependencies #1146
Disable warnings when building stackage dependencies #1146
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.
That's great, thanks for contributing this.
I've commented with a few questions.
This is also relevant to #1059.
def expand_make_variables(name, ctx, strings, extra_label_attrs): | ||
# All labels in all attributes should be location-expandable. | ||
label_attrs = [ | ||
ctx.attr.deps, |
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.
Why assume that all rules have a deps
attribute instead of taking label_attrs
as a parameter to expand_make_variables
?
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 was under the impression that these particular attributes are common to all build rules (https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes), so it felt redundant/error-prone to pass them all the time.
Can change it back if you'd rather this to be explicit!
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.
Fair enough, thanks for explaining. I have no strong preference either way.
499259d
to
a35431c
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.
Thank you!
def expand_make_variables(name, ctx, strings, extra_label_attrs): | ||
# All labels in all attributes should be location-expandable. | ||
label_attrs = [ | ||
ctx.attr.deps, |
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.
Fair enough, thanks for explaining. I have no strong preference either way.
So it can be used in other places
We then pass those options to `runghc Setup.hs configure`. This can be particularly useful for external cabal packages that may print warnings or even have `-Werror`.
They pollute the output (even when pulled from a remote cache) and are not actionable. In particular, packages that define `-Werror` can be hard to install when using a newer ghc.
a35431c
to
4ecc51c
Compare
Not sure I understand why mergify complains here. I thought it was missing a rebase but that didn't help. Do I need to fix something on our side? |
Tried a re-approve and re-run on the "checks" tab. Neither worked so I merged manually. |
Fixes #1026, following @mboes' suggestion there.