Skip to content

Support passing arbitrary extra flags to rustc#566

Merged
UebelAndre merged 27 commits intobazelbuild:mainfrom
djmarcin:extra-codegen
Sep 2, 2021
Merged

Support passing arbitrary extra flags to rustc#566
UebelAndre merged 27 commits intobazelbuild:mainfrom
djmarcin:extra-codegen

Conversation

@djmarcin
Copy link
Copy Markdown
Contributor

@djmarcin djmarcin commented Feb 1, 2021

Adds a bazel command line flag for passing arbitrary extra arguments to all (non-exec configuration) rustc invocations. This is necessary beyond using rustc_flags if you want to enable something that needs to be enabled globally, such as lto=thin.

Usage:

bazel build --@rules_rust//:extra_rustc_flags=--codegen=lto=thin //some:target

Comment thread rust/private/rustc.bzl Outdated
@djmarcin djmarcin force-pushed the extra-codegen branch 3 times, most recently from 5e48949 to 531310f Compare February 16, 2021 18:35
Copy link
Copy Markdown
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, this is a very useful PR!!

Comment thread BUILD Outdated
Comment thread BUILD Outdated
Comment thread docs/BUILD Outdated
Comment thread rust/private/clippy.bzl Outdated
Comment thread rust/private/rust.bzl Outdated
Comment thread rust/private/rustc.bzl Outdated
@UebelAndre
Copy link
Copy Markdown
Collaborator

Howdy, any updates here? This seems like an awesome feature 😄

@hlopko
Copy link
Copy Markdown
Member

hlopko commented May 20, 2021

Friendly ping, I'm happy to take over if you're too busy, just let me know.

@djmarcin
Copy link
Copy Markdown
Contributor Author

This is updated on latest HEAD and tests are passing.

Comment thread rust/private/rustc.bzl Outdated
@djmarcin
Copy link
Copy Markdown
Contributor Author

Added some docs, wasn't completely sure where to put them so I added them to the defs section.

Copy link
Copy Markdown
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit but otherwise looks good to me! 😄 Thanks for reviving this PR!!

Comment thread rust/private/rustc.bzl Outdated
@sayrer
Copy link
Copy Markdown
Contributor

sayrer commented Jul 20, 2021

Could we land this? :)

@djmarcin
Copy link
Copy Markdown
Contributor Author

Could we land this? :)

I'll resolve the conflicts. I think there was some bikeshedding over the flag name. I don't have a strong opinion.

@djmarcin
Copy link
Copy Markdown
Contributor Author

djmarcin commented Jul 20, 2021

I retested LTO and it turns out that it's no longer broken with proc-macros (probably something I did with toolchain resolution in my repo lately if I had to guess). In any case, excluding these flags from the exec configuration still seems like the right thing to do to me. I've changed to detecting this based off of the genfiles_dir which appears to always contain the string -exec- when running in the exec configuration. It's a hack, but I don't see where else to get this information.

Comment thread rust/private/rustc.bzl Outdated
@hlopko
Copy link
Copy Markdown
Member

hlopko commented Aug 10, 2021

I asked about the host/exec detection in bazelbuild/bazel#13308 (comment), let's see what they say.

Could you add a unittest showing that a flag does appear on the command line for a target in target configuration, and that it doesn't appear for a target in exec configuration? Should be fairly simple given https://github.com/bazelbuild/bazel-skylib/blob/main/lib/unittest.bzl#L188. Thank you so much!

@hlopko
Copy link
Copy Markdown
Member

hlopko commented Aug 10, 2021

Ok, it seems the genfiles_dir path is the only way to detect exec configuration. From me this PR looks good, modulo the unit test (if you don't have capacity to add the test please let us know, maybe somebody will be able to help out).

@djmarcin djmarcin force-pushed the extra-codegen branch 4 times, most recently from dcac704 to 98d7be5 Compare August 31, 2021 08:26
@djmarcin
Copy link
Copy Markdown
Contributor Author

@hlopko merged latest main and added tests

@UebelAndre
Copy link
Copy Markdown
Collaborator

Is the PR description up to date? How do we set flags?

@djmarcin
Copy link
Copy Markdown
Contributor Author

djmarcin commented Aug 31, 2021 via email

Copy link
Copy Markdown
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems some stuff got removed from docs. If you fix this I can merge give that @hlopko gave a thumbs up pending a test

Comment thread docs/symbols.bzl
_rust_bindgen_repositories = "rust_bindgen_repositories",
)
load(
"@rules_rust//cargo:defs.bzl",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bad merge conflict resolution. Can you restore this and the other deleted things in this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redid the merge, looks better now, I think. Please take another look.

Copy link
Copy Markdown
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @djmarcin!!

@UebelAndre UebelAndre merged commit 52e3ba4 into bazelbuild:main Sep 2, 2021
@djmarcin djmarcin deleted the extra-codegen branch September 2, 2021 01:20
@UebelAndre UebelAndre mentioned this pull request Sep 11, 2021
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.

4 participants