Allow opting-out of the 0.0.0 crate version fallback #3659
Conversation
| "CARGO_PKG_DESCRIPTION": "", | ||
| "CARGO_PKG_HOMEPAGE": "", | ||
| "CARGO_PKG_NAME": attr.name, | ||
| "CARGO_PKG_VERSION": version, |
There was a problem hiding this comment.
Won't this be unset for normal rust_* targets?
There was a problem hiding this comment.
It will, but on the off chance that they actually need this they can either set env = {"CARGO_PKG_VERSION": version} explicitly or use cargo_toml_env_vars for proper support, like we do for cargo-bazel in this PR. I think it's confusing to have multiple ways to provide this functionality, and this one is more incomplete compared to cargo_toml_env_vars
There was a problem hiding this comment.
@UebelAndre another way of looking at it - previously, not setting the 'version' attribute on manually-written rust_library rules would mean they get compiled with CARGO_PKG_VERSION_MAJOR=0 and similar. That's also kinda weird and probably not the desired behavior...
There was a problem hiding this comment.
it's useful for things like clap which basically embeds the binary version for free in your crate due to these environment variables. While I can see the argument for separating out cargo environment variables I think the few that are there do offer some convenience folks have already taken advantage of. If this were to change I think it would need an incompatibility flag.
There was a problem hiding this comment.
+1 to keep the existing defaults CARGO_PKG_* derived from the rust_* rules in skylark. Many random crates in the ecosystem examine these and we have setup that imports them, generating BUILD files, deriving the crate name and version of the target from the Cargo.toml file in the process, and discarding the Cargo.toml files after that. While the defaults are not always correct, these defaults cover a good chunk of crates in our experience.
I agree it's super confusing in the presence of cargo_toml_env_vars, and there I think we shouldn't derive the defaults. Instead of a new incompatibility flag, could we make it that the implementation detects the presence of cargo_toml_env_vars dependency and disables the defaults derivation if that's present?
There was a problem hiding this comment.
How would you guys feel if we just stopped defaulting the version to 0.0.0 and left the vars unset in case the version wasn't provided? That would match the handling in the build_script rule and would give users an easy way to opt out of this behavior for all the version vars. It would also allow to share the duplicated parsing implementations that are inconsistent.
That would leave only CARGO_PKG_NAME which is sadly used by clap. (Fun fact, that usage is incorrect and it should use CARGO_BIN_NAME to get the right names, but that requires a clap fix...). We can tackle that one another day
There was a problem hiding this comment.
How would you guys feel if we just stopped defaulting the version to 0.0.0 and left the vars unset in case the version wasn't provided?
I like this. I think for us internally this will be flexible enough to handle the mix of third party dependencies with-and-without set version.
From broader point of view, still might make sense to put this feature behind an incompatible flag (strictly it feels like it is). Also curious what @UebelAndre thinks.
Sharing another thought I had last week: we could define a custom provider, say NoDefaultEnvVarsInfo(), and have cargo_toml_env_vars() return it as well as the DefaultInfo, and in rustc.bzl gate the setting of the defaults on any of the rustc_env_files providing it.
There was a problem hiding this comment.
I think adding a custom provider is a decent idea but since I'm not using it on my end (Creating the .env file in the repo phase) I would then need to write a new rule wrapping those files just to create it, not sure it's worth it? Lets start with the not-defaulting-version suggestion above.
The only change in behavior really is that previously it would manufacture the 0.0.0 version which was probably not correct, and if anyone really wants that behavior they can just set version = "0.0.0" themselves? I don't mind flagging the new behavior if ya'll think there's a risk of this breaking users, but I do think incompatible flags add a tiny bit of cognitive burden so I'd consider avoiding it for something like this which is arguably fixing an inconsistent/weird behavior?
Anyway, perhaps @UebelAndre can make the final call here and we can land the change? :)
There was a problem hiding this comment.
Changing the default seems fine but this should definitely follow https://github.com/bazelbuild/rules_rust/blob/a14b74125e142dbbf968ee47381a1d370bfd2ba6/COMPATIBILITY.md and add an incompatibility flag that gates the change in the default.
But in terms of de-duplication, crate_universe explicitly sets version so won't you still run into duplication there from the cargo extracted variables?
There was a problem hiding this comment.
OK, I will rework this. I'm ok adding the flag as long as we will be able to followup to flip the default after 3 months, which I'm also happy to do.
It's true that crate_universe will still set version (which we could change in a followup), but my crate_universe replacement won't :)
dzbarsky
changed the title
3 participants
Most users are configuring these (indirectly) via .env files anyway, so for them this is a source of duplicative or conflicting env vars.
This also unifies the 2 inconsistent implementations of the version parsing.