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

Allow multiple uses of extra_rustc_flags #1407

Closed
wants to merge 4 commits into from

Conversation

mypmc
Copy link
Contributor

@mypmc mypmc commented Jun 12, 2022

I would like to support the following use cases.

In .bazelrc.

build:dev --@rules_rust//:extra_rustc_flags="-C..."
build:test --config=dev
build:test --@rules_rust//:extra_rustc_flags="-C..." # add additional flags
build:fuzz --config=test
build:fuzz --@rules_rust//:extra_rustc_flags="-Cpasses=sancov,-Cllvm-args=-sanitizer-coverage-level=3,-Cllvm-args=-sanitizer-coverage-inline-8bit-counters,-Znew-llvm-pass-manager=no"

@mypmc
Copy link
Contributor Author

mypmc commented Jun 12, 2022

out of luck :(

Bazel 4.0.0's baseline was cut on Nov 18. The allow_multiple parameter to config.string() was added on Dec 10. You can use a version of Bazel closer to head if you want to experiment with this feature.

@mypmc
Copy link
Contributor Author

mypmc commented Jun 13, 2022

Whenever there is a new Bazel LTS release, all releases of rules_rust will maintain support for the older LTS version for at least 3 months unless Bazel doesn't allow this.

Bumped the min bazel version but I have no idea this met the rules_rust policy.

@hlopko
Copy link
Member

hlopko commented Jun 14, 2022

Could you move the min supported bazel version bump to a separate PR please? It's a big-ish deal so it's good to not have it hidden in this PR.

@hlopko hlopko requested a review from krasimirgg June 14, 2022 06:35
@hlopko
Copy link
Member

hlopko commented Jun 14, 2022

Otherwise looks good to me, but I'll kindly ask @krasimirgg to double check.

@krasimirgg
Copy link
Collaborator

I'm a bit concerned with the switch from a string list to a string and the "always-on" splitting-by-comma that this change introduces. IMO we need to have a good way to escape commas. An example is something like this where we want to pass arguments with commas in them though rustc all the way down to the linker, e.g., pass -Clink-arg=-Wl,-Bstatic as a single argument in an invocation like rustc ... -Clink-arg=-Wl,-Bstatic.

Is there a supported mechanism to allow multiple string_list flags in Bazel?

@mypmc
Copy link
Contributor Author

mypmc commented Jun 14, 2022

-Clink-arg=-Wl,-Bstatic as a single argument

mm, I didn't take that case into consideration because:

But it is helpful to be able to pass arguments including commas.

@UebelAndre
Copy link
Collaborator

Perhaps this should be an upstream issue for Bazel to allow multiple string_list flags?

@mypmc
Copy link
Contributor Author

mypmc commented Jun 14, 2022

related??


I have an another idea. How about introducing new build setting e.g., extra_rustc_flag?

build --@rules_rust//:extra_rustc_flags="-C...,-C...,-Z..."

build:dev --@rules_rust//:extra_rustc_flag="-C..."
build:dev --@rules_rust//:extra_rustc_flag="-C..."

build:test --config=dev
build:test --@rules_rust//:extra_rustc_flag="-C..."

build:fuzz --config=test
build:fuzz --@rules_rust//:extra_rustc_flag="-C..."
build:fuzz --@rules_rust//:extra_rustc_flag="-C..."
build:fuzz --@rules_rust//:extra_rustc_flag="-Z..."

Looks verbose but straightforward to extend flags. What do you think?

@krasimirgg
Copy link
Collaborator

I have an another idea. How about introducing new build setting e.g., extra_rustc_flag?

I like this, to clarify a bit the semantics I read from your proposal: we keep the extra_rustc_flags as-is (string_list), and extra_rustc_flag is string + allow_multiple=true, and each extra_rustc_flag is singular (not split by comma), and the extra_rustc_flag-s are appended after the extra_rustc_flags.

@mypmc
Copy link
Contributor Author

mypmc commented Jun 15, 2022

Created #1413, so I will close this PR. Thanks for your review.

@mypmc mypmc closed this Jun 15, 2022
@mypmc mypmc deleted the mypmc/rustc-flags branch June 15, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants