Skip to content

Allow opting-out of the 0.0.0 crate version fallback #3659

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow opting-out of the 0.0.0 crate version fallback #3659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions cargo/private/cargo_build_script.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ load("//rust:rust_common.bzl", "BuildInfo", "CrateGroupInfo", "DepInfo")
# buildifier: disable=bzl-visibility
load(
"//rust/private:rustc.bzl",
"env_vars_from_version",
"get_compilation_mode_opts",
"get_linker_and_args",
)
Expand Down Expand Up @@ -407,13 +408,7 @@ def _cargo_build_script_impl(ctx):
})

if ctx.attr.version:
version = ctx.attr.version.split("+")[0].split(".")
patch = version[2].split("-") if len(version) > 2 else [""]
env["CARGO_PKG_VERSION_MAJOR"] = version[0]
env["CARGO_PKG_VERSION_MINOR"] = version[1] if len(version) > 1 else ""
env["CARGO_PKG_VERSION_PATCH"] = patch[0]
env["CARGO_PKG_VERSION_PRE"] = patch[1] if len(patch) > 1 else ""
env["CARGO_PKG_VERSION"] = ctx.attr.version
env.update(env_vars_from_version(ctx.attr.version))

# Pull in env vars which may be required for the cc_toolchain to work (e.g. on OSX, the SDK version).
# We hope that the linker env is sufficient for the whole cc_toolchain.
Expand Down
39 changes: 28 additions & 11 deletions rust/private/rustc.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,24 +129,40 @@ is_proc_macro_dep_enabled = rule(
build_setting = config.bool(flag = True),
)

def _get_rustc_env(attr, toolchain, crate_name):
"""Gathers rustc environment variables
def env_vars_from_version(version):
"""Gathers rustc environment variables corresponding to the version

Args:
attr (struct): The current target's attributes
toolchain (rust_toolchain): The current target's rust toolchain context
crate_name (str): The name of the crate to be compiled
version (str): The version of the crate to be compiled

Returns:
dict: Rustc environment variables
"""
version = attr.version if hasattr(attr, "version") else "0.0.0"
major, minor, patch = version.split(".", 2)
if "-" in patch:
patch, pre = patch.split("-", 1)
else:
pre = ""

return {
"CARGO_PKG_VERSION": version,
"CARGO_PKG_VERSION_MAJOR": major,
"CARGO_PKG_VERSION_MINOR": minor,
"CARGO_PKG_VERSION_PATCH": patch,
"CARGO_PKG_VERSION_PRE": pre,
}

def _get_rustc_env(attr, toolchain, crate_name):
"""Gathers rustc environment variables

Args:
attr (struct): The current target's attributes
toolchain (rust_toolchain): The current target's rust toolchain context
crate_name (str): The name of the crate to be compiled

Returns:
dict: Rustc environment variables
"""
result = {
"CARGO_CFG_TARGET_ARCH": "" if toolchain.target_arch == None else toolchain.target_arch,
"CARGO_CFG_TARGET_OS": "" if toolchain.target_os == None else toolchain.target_os,
Expand All @@ -155,12 +171,13 @@ def _get_rustc_env(attr, toolchain, crate_name):
"CARGO_PKG_DESCRIPTION": "",
"CARGO_PKG_HOMEPAGE": "",
"CARGO_PKG_NAME": attr.name,
"CARGO_PKG_VERSION": version,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this be unset for normal rust_* targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

See also https://bazelbuild.slack.com/archives/CSV56UT0F/p1759051609743919?thread_ts=1758229364.645099&cid=CSV56UT0F

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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...

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

"CARGO_PKG_VERSION_MAJOR": major,
"CARGO_PKG_VERSION_MINOR": minor,
"CARGO_PKG_VERSION_PATCH": patch,
"CARGO_PKG_VERSION_PRE": pre,
}

version_default = None if toolchain._incompatible_do_not_inject_degenerate_version_to_rustc_env else "0.0.0"
version = getattr(attr, "version", version_default)
if version:
result.update(env_vars_from_version(version))

if hasattr(attr, "_is_proc_macro_dep_enabled") and attr._is_proc_macro_dep_enabled[IsProcMacroDepEnabledInfo].enabled:
is_proc_macro_dep = "0"
if hasattr(attr, "_is_proc_macro_dep") and attr._is_proc_macro_dep[IsProcMacroDepInfo].is_proc_macro_dep:
Expand Down
3 changes: 3 additions & 0 deletions rust/settings/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ load(
"incompatible_change_clippy_error_format",
"incompatible_change_rust_test_compilation_output_directory",
"incompatible_do_not_include_data_in_compile_data",
"incompatible_do_not_inject_degenerate_version_to_rustc_env",
"lto",
"no_std",
"pipelined_compilation",
Expand Down Expand Up @@ -111,6 +112,8 @@ incompatible_change_rust_test_compilation_output_directory()

incompatible_do_not_include_data_in_compile_data()

incompatible_do_not_inject_degenerate_version_to_rustc_env()

lto()

no_std()
Expand Down
10 changes: 10 additions & 0 deletions rust/settings/settings.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,16 @@ def incompatible_do_not_include_data_in_compile_data():
issue = "https://github.com/bazelbuild/rules_rust/issues/2977",
)

# buildifier: disable=unnamed-macro
def incompatible_do_not_inject_degenerate_version_to_rustc_env():
"""A flag to control whether we inject env vars corresponding to version 0.0.0 if version is omitted.
"""
incompatible_flag(
name = "incompatible_do_not_inject_degenerate_version_to_rustc_env",
build_setting_default = False,
issue = "https://github.com/bazelbuild/rules_rust/issues/3674",
)

def codegen_units():
"""The default value for `--codegen-units` which also affects resource allocation for rustc actions.

Expand Down
5 changes: 5 additions & 0 deletions rust/toolchain.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ def _rust_toolchain_impl(ctx):
_incompatible_change_rust_test_compilation_output_directory = ctx.attr._incompatible_change_rust_test_compilation_output_directory[IncompatibleFlagInfo].enabled,
_toolchain_generated_sysroot = ctx.attr._toolchain_generated_sysroot[BuildSettingInfo].value,
_incompatible_do_not_include_data_in_compile_data = ctx.attr._incompatible_do_not_include_data_in_compile_data[IncompatibleFlagInfo].enabled,
_incompatible_do_not_inject_degenerate_version_to_rustc_env = ctx.attr._incompatible_do_not_inject_degenerate_version_to_rustc_env[IncompatibleFlagInfo].enabled,
_no_std = no_std,
_codegen_units = ctx.attr._codegen_units[BuildSettingInfo].value,
_experimental_use_allocator_libraries_with_mangled_symbols = ctx.attr.experimental_use_allocator_libraries_with_mangled_symbols,
Expand Down Expand Up @@ -986,6 +987,10 @@ rust_toolchain = rule(
default = Label("//rust/settings:incompatible_do_not_include_data_in_compile_data"),
doc = "Label to a boolean build setting that controls whether to include data files in compile_data.",
),
"_incompatible_do_not_inject_degenerate_version_to_rustc_env": attr.label(
default = Label("//rust/settings:incompatible_do_not_inject_degenerate_version_to_rustc_env"),
doc = "Label to a boolean build setting that controls whether to set rustc env vars based on a crate version of '0.0.0' if one is not explicitly provided. ",
),
"_no_std": attr.label(
default = Label("//rust/settings:no_std"),
),
Expand Down