-
Notifications
You must be signed in to change notification settings - Fork 413
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
Adds per_crate_rustc_flag build setting. #1827
Conversation
@UebelAndre any feedback here? |
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 this sounds reasonable.
It's worth thinking about future changes to this API if there is strong intent to later support regex matching instead of this more limited API. A few options:
1. We can just be comfortable making a breaking change later on. 2. We can name the current implementation something like `experimental_per_crate_rustc_flag` to flag that the API is unstable. When regexes are supported in Starlark, the implementation can be updated, and the `experimental_` prefix can be dropped. 3. We can force prefixes to start with `^` and end with `.*`. Any patterns that included unescaped regex characters or do not start with `^` and end with `.*` could be rejected with a `fail`. This would allow us to support more patterns in the future with proper regex matching, but would make that a backwards compatible change.
I think the way to make breaking changes in the future is with experimental or incompatibility flags similar to those found here: https://github.com/bazelbuild/rules_rust/blob/0.18.0/rust/settings/BUILD.bazel
As long as the parsing behavior is documented and tested we can make the appropriate changes as new functionality is made available.
Do you know if there's a feature request somewhere for regex support in Starlark?
I went ahead and added the
I saw some chatter about it in an issue in the Bazel issue tracker, but am having trouble finding it now unfortunately. |
@UebelAndre any thoughts on the latest changes? |
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.
Looking good! Just one question
I fixed the buildifier issues. |
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.
Thanks!
* Adds per_crate_rustc_flag build setting. * Regenerate documentation
Requested in #1711.
I'm putting up this initial PR to get early feedback, and to gauge the appetite for adding a feature of this sort, before investing further.
In my view, this feature would behave identically to
--per-file-copt
, but mapped to Rust. A good description of--per-file-copt
is available here: https://bazel.build/docs/user-manual#per-file-copt.A short TL;DR is that
--per-file-copt
takes values of the format[+-]regex[,[+-]regex]...@option[,option]...
. The regex(es) is matched against both the label of the target the source belongs to, and the execution path of the source file. If either of these match (the default or if prefixed with+
), or do not match (if prefixed with-
), the option(s) is added to the set of compilation flags.I'm proposing the following behavior changes to map this behavioral model to Rust:
There are additional factors that make conformance with
--per-file-copt
difficult. Namely, Starlark doesn't support regex matching today. As such, my proposal is to have--per_crate_rustc_flag
support prefix matching instead, until the date that regex matching comes to Starlark. Regex matching is likely to come at some point, as the C++ rules are being migrated to Starlark, and so--per-file-copt
will need to be implemented in Starlark.The current implementation does not support negative matching (
-
above), though that can be added.The current implementation also only supports a single prefix and a single option, though that bit is addressable (and
--per_crate_rustc_flag
can also be used multiple times, so this isn't a real limitation; IMO support for multiple regexes / flags in a single value is more motivated by the desire to mirror--per-file-copt
than anything else).A motivating use case for this change is to enable compilation of external crates in fastbuild configurations. With the current implementation, that would look like:
In the world where regex is supported, this would instead look like:
It's worth thinking about future changes to this API if there is strong intent to later support regex matching instead of this more limited API. A few options:
We can just be comfortable making a breaking change later on.
We can name the current implementation something like
experimental_per_crate_rustc_flag
to flag that the API is unstable. When regexes are supported in Starlark, the implementation can be updated, and theexperimental_
prefix can be dropped.We can force prefixes to start with
^
and end with.*
. Any patterns that included unescaped regex characters or do not start with^
and end with.*
could be rejected with afail
. This would allow us to support more patterns in the future with proper regex matching, but would make that a backwards compatible change.Anyway, please let me know what the appetite is to land a feature of this sort, and if there is appetite, what path forward you'd be most comfortable with.
Cheers.