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

Test rustc flag and feature flags #153

Merged
merged 4 commits into from
Nov 9, 2018

Conversation

damienmg
Copy link
Collaborator

@damienmg damienmg commented Nov 9, 2018

PR #151 broke some usage of those flags that make cargo raze build file not work any more.

This PR adds those 2 flag to a library so we notice those breakage. It allow the change for "rustc_flags" and fix the "features" argument.

As a consequence passing ["--arg value"] to rustc_flags no longer works but that's fine (it's a reasonable breaking change). One should either do ["--arg", "value"] or ["--arg=value"].

In order to test those flags, PR bazelbuild#151 broke some usage of those flags that
make `cargo raze` build file not work any more.
@damienmg damienmg changed the title Add flags and features to the default library WIP: Add flags and features to the default library Nov 9, 2018
@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018

/cc @mfarrugi fyi

…hat's fine

I'll send a PR to cargo-raze to fix it on cargo raze side rather.
@mfarrugi
Copy link
Collaborator

mfarrugi commented Nov 9, 2018

Ah, didn't think of it as a breaking change. LGTM.

What's WIP about this?

@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018

For now I fixed the args for one and sent a PR on cargo-raze to fix it (it's a breaking change but one that make sense).

The crate_features is still broken, I am trying to figure out why.

Features needs to be quoted on the command line apparently.
@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018

Ok the fix is there now :)

@damienmg damienmg changed the title WIP: Add flags and features to the default library A Nov 9, 2018
@damienmg damienmg changed the title A Test flag and feature flags Nov 9, 2018
rust/private/rustc.bzl Outdated Show resolved Hide resolved
@damienmg damienmg changed the title Test flag and feature flags Test rustc flag and feature flags Nov 9, 2018
@acmcarther
Copy link
Collaborator

acmcarther commented Nov 9, 2018

I can't tell from a glance, but I suspect you already know:

What happens if you try to use the --arg value syntax with this change? Does it fail in a way that is obvious, or does it barf with some incomprehensible error?

(LGTM % answer to Q)

@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018

Side-note: this change does not introduce the incompatible change, it was introduced by #151. It just make it explicit that there is a breaking change.

The error I had was okish but could be better: error: Unrecognized option: 'cap-lints allow'

The only difficulty was that it was coming from cargo-raze generated BUILD files.

@acmcarther
Copy link
Collaborator

Yeah, I understand that the original issue came from #151. I was mostly just wondering if this was also a good time to make the user-facing error easier to understand. It doesn't seem to me that there will be another obvious opportunity unless someone files a bug.

Ultimate decision is up to you since you're the one that had to debug the original issue, so you're most familiar with how hard that is to do with the current output.

@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018

If I were setting the flag myself in the BUILD file the error would have been easy to understand, I had to figure things out because the error came from generated BUILD file.

I don't think it is worth the added complexity to output another error message (that would be probably 2-3 lines of code)

@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018

Can someone merge it? I don't have write access to this repository.

@acmcarther acmcarther merged commit a672892 into bazelbuild:master Nov 9, 2018
@acmcarther
Copy link
Collaborator

Done!

@damienmg
Copy link
Collaborator Author

damienmg commented Nov 9, 2018 via email

@damienmg damienmg deleted the bugfix-head branch November 10, 2018 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants