Add a crate_name attribute to Rust rules #645
Conversation
|
@hlopko Can you review? |
|
I'm new to writing Bazel rules, so I have a few questions on the test failures.
|
There was a problem hiding this comment.
Thanks for the contribution!
This allows specifying a different crate name than the one derived by default from the Bazel target name.
It'd be nice to avoid this feature if possible by changing names, but if that's not possible, this change generally seems reasonable :)
Finally, there is also a use of name_to_crate_name in
cargo_build_script.bzlto replace hyphens in feature names with
underscores. This has nothing to do with crate names; it's just using an existing function that conveniently happens to be doing the thing the code needs. I think this use of name_to_crate_name should be replaced with a local function that does the same thing, but again, I'll do so in a separate commit.
I have #643 out which overhauls this - would appreciate your thoughts (and it may change how you're thinking about that separate commit :))
Thanks for pointing that out -- I've added comments there (rather many of them in fact...). I do still feel that using the |
I'm getting test failures on Windows for bazelbuild#645 where the linker is complaining that it can't write to the output file. The only reason I can imagine for this is that the test environment is forbidding writes to anything other than the declared output files. As I can't test on Windows directly myself, I regard this as an experimental change that I'm pushing to a draft PR solely for the purpose of checking whether this fixes the failing test.
This fixes test failures on Windows that I was getting for bazelbuild#645 where the linker was complaining that it couldn't write to the output file. I assume that the test environment is preventing writes to anything other than the declared output files.
This fixes test failures on Windows that I was getting for bazelbuild#645 where the linker was complaining that it couldn't write to the output file. I assume that the test environment is preventing writes to anything other than the declared output files.
martinboehme
force-pushed
the
crate-name
branch
from
b56298a
to
8275882
Compare
|
Rebasing to HEAD now. |
This allows specifying a different crate name than the one derived by default from the Bazel target name. This is useful, for example, if you want to use a target name that would not be legal as a crate name, e.g. a target name containing slashes. This commit introduces a function _crate_name() and uses it where we were formerly determining the crate name using name_to_crate_name(ctx.label.name). There were some additional uses of name_to_crate_name in the test rule where we weren't actually determining a crate name but an output file name for the test binary or the test launcher. It's not clear to me why these needed the hyphen-to-underscore conversion that name_to_crate_name performs, as hyphens are legal in file names. I think these places should be doing one of the following: - Create a file name based on the target name, without hyphen-to-underscore substitutions. - Create a file name based on the (possibly non-default) crate name. I think one of these two changes should be made, but I don't want to do so in the commit because either of these changes would create a change in behavior for existing uses of the rust_test rule. Also, the issue of test binary and test lancher file names is largely orthogonal to the purpose of this commit. Finally, there is also a use of name_to_crate_name in cargo_build_script.bzl to replace hyphens in feature names with underscores. This has nothing to do with crate names; it's just using an existing function that conveniently happens to be doing the thing the code needs. I think this use of name_to_crate_name should be replaced with a local function that does the same thing, but again, I'll do so in a separate commit.
This fixes test failures on Windows that I was getting for bazelbuild#645 where the linker was complaining that it couldn't write to the output file. I assume that the test environment is preventing writes to anything other than the declared output files.
The label isn't required, because we can also get the name from the attributes.
martinboehme
force-pushed
the
crate-name
branch
from
cdcdec0
to
87bb473
Compare
|
I've added a TODO that the I originally planned to do this in this PR, but then I stumbled across a bug in the Clippy aspect implementation where it's accessing |
Normal `rust_binary` targets don't do this, so for consistency, don't do it in tests either. The motivation for this change is that I'm trying to whittle down the number of uses of the `name_to_crate_name` function, as it doesn't take a possible `crate_name` attribute into account. For context, see also the changes and discussion in bazelbuild#645
Normal `rust_binary` targets don't do this, so for consistency, don't do it in tests either. An additional motivation for this change is that I'm trying to whittle down the number of uses of the `name_to_crate_name` function, as it doesn't take a possible `crate_name` attribute into account. For context, see also the changes and discussion in bazelbuild#645
Normal `rust_binary` targets don't do this, so for consistency, don't do it in tests either. An additional motivation for this change is that I'm trying to whittle down the number of uses of the `name_to_crate_name` function, as it doesn't take a possible `crate_name` attribute into account. For context, see also the changes and discussion in #645
* Move assert_argv_* helpers to a common helper library. * Add a crate_name attribute to Rust rules. This allows specifying a different crate name than the one derived by default from the Bazel target name. This is useful, for example, if you want to use a target name that would not be legal as a crate name, e.g. a target name containing slashes. This commit introduces a function _crate_name() and uses it where we were formerly determining the crate name using name_to_crate_name(ctx.label.name). There were some additional uses of name_to_crate_name in the test rule where we weren't actually determining a crate name but an output file name for the test binary or the test launcher. It's not clear to me why these needed the hyphen-to-underscore conversion that name_to_crate_name performs, as hyphens are legal in file names. I think these places should be doing one of the following: - Create a file name based on the target name, without hyphen-to-underscore substitutions. - Create a file name based on the (possibly non-default) crate name. I think one of these two changes should be made, but I don't want to do so in the commit because either of these changes would create a change in behavior for existing uses of the rust_test rule. Also, the issue of test binary and test lancher file names is largely orthogonal to the purpose of this commit. Finally, there is also a use of name_to_crate_name in cargo_build_script.bzl to replace hyphens in feature names with underscores. This has nothing to do with crate names; it's just using an existing function that conveniently happens to be doing the thing the code needs. I think this use of name_to_crate_name should be replaced with a local function that does the same thing, but again, I'll do so in a separate commit. * Use single-argument version of fail() for backwards compatibility. * Emit binary directly with desired name instead of copying. This fixes test failures on Windows that I was getting for bazelbuild#645 where the linker was complaining that it couldn't write to the output file. I assume that the test environment is preventing writes to anything other than the declared output files. * In crate_name test, test that invalid characters are reported correctly. * Simplify _crate_name() function so it only takes the attributes. The label isn't required, because we can also get the name from the attributes. * Add a TODO to respect crate_name attr in CARGO_CRATE_NAME env var.
Normal `rust_binary` targets don't do this, so for consistency, don't do it in tests either. An additional motivation for this change is that I'm trying to whittle down the number of uses of the `name_to_crate_name` function, as it doesn't take a possible `crate_name` attribute into account. For context, see also the changes and discussion in bazelbuild#645
This allows specifying a different crate name than the one derived by default from the Bazel target name.
This is useful, for example, if you want to use a target name that would not be legal as a crate name, e.g. a target name containing slashes.
This commit introduces a function
_crate_name()and uses it where we were formerly determining the crate name usingname_to_crate_name(ctx.label.name).There were some additional uses of
name_to_crate_namein the test rule where we weren't actually determining a crate name but an output file name for the test binary or the test launcher. It's not clear to me why these needed the hyphen-to-underscore conversion thatname_to_crate_nameperforms, as hyphens are legal in file names. I think these places should be doing one of the following:I think one of these two changes should be made, but I don't want to do so in the commit because either of these changes would create a change in behavior for existing uses of the
rust_testrule. Also, the issue of test binary and test launcher file names is largely orthogonal to the purpose of this commit.Finally, there is also a use of name_to_crate_name in
cargo_build_script.bzlto replace hyphens in feature names withunderscores. This has nothing to do with crate names; it's just using an existing function that conveniently happens to be doing the thing the code needs. I think this use of name_to_crate_name should be replaced with a local function that does the same thing, but again, I'll do so in a separate commit.