Provide CARGO_PKG_NAME and CARGO_PKG_VERSION* for build scripts

[a1ph]

This is used e.g. by the protobuf crate build script.

[damienmg]

Hi,

Although I think it make senses to have CARGO_PKG_NAME since it does not extend the API surface, build_script_env should be used to provide CARGO_PKG_VERSION. There is plenty of env that does not matter for us, unless they make sense in the rust_binary I would prefer not to add a dedicated support for it.

[a1ph]

@damienmg thanks for looking into it.

My goal is to make all the crates compilable without having a need to put anything into the raze.crates section. Finding the right set of settings is a pain for developers, see e.g. google/cargo-raze#41 thread.

Besides that it is hard to maintain the settings in this section, as it requires putting exact crate versions that in fact may change on every cargo-raze run and developer needs to manually keep the versions up to date there.

[illicitonion]

@a1ph I think what @damienmg is suggesting is that instead of teaching cargo-raze to generate:

cargo_build_script(
    ...
    version = "2.1.0",
    ...
)

and having cargo_build_script turn that into an env var, we should teach cargo-raze to generate:

cargo_build_script(
    ...
    build_script_env = {
        "CARGO_PKG_VERSION": "2.1.0",
        "CARGO_PKG_VERSION_MAJOR": "2",
        ...
    },
    ...
)

which will Just Work with no modification to the rules.

I guess there's a philosophical question as to whether cargo-raze should be the thing translating cargo concepts to bazel ones, or cargo_build_script should be. I'm inclined to agree that for this one, setting version = "2.1.0" in a BUILD file is much nicer, because of how it expands out to multiple redundant environment variables.

I have no real preference for whether the version -> 4 env vars happens in the bzl for the rule, or the runner binary, though.

[damienmg]

Yes @illicitonion explained well what I was thinking. I agree that version is nicer but however this is a duplicate feature, since it is always easier to add feature than to remove them, I would err on the side of not adding a feature.

So I would prefer if this PR would just add the CARGO_PKG_NAME here and inject the CARGO_PKG_VERSION from cargo-raze. There is one case where I see that adding a version parameter make sense if we were to fetch it, but that's not the responsibility of cargo_build_script.

[a1ph]

I'm ok with moving the logic to cargo-raze. However please note it already generates version in cargo_build_script invocation, that I believe is required by rust_binary.

[damienmg]

Oh indeed I forgot that. I need to double check what the version in rust_binary do but in that case that make sense to keep your pr as-is.

…

On Sat, Jul 4, 2020, 4:30 AM Alexei Filippov ***@***.***> wrote: I'm ok with moving the logic to cargo-raze. However please note it already generates version in cargo_build_script invocation, that I believe is required by rust_binary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#366 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4MO7YLZXKUS5VU5I2D62TRZ2H43ANCNFSM4OPRRAEQ> .

[damienmg]

Sorry for all the push back, I totally missed this was a feature of rust_binary. This does not change any API in fact so LGTM.

[damienmg]

What about when there is a label (e.g. 1.2.3-blah or 2.3.4+bleh)?

Suggested change

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

[a1ph]

done. slightly different though.

[damienmg]

Thanks!