Skip to content
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

Jump to bottom

buildrs_additional_environment_variables is no longer respected after #166 #179

Closed
dayfine opened this issue Jun 29, 2020 · 7 comments · Fixed by #180
Closed

buildrs_additional_environment_variables is no longer respected after #166 #179

dayfine opened this issue Jun 29, 2020 · 7 comments · Fixed by #180

Comments

@dayfine
Copy link

dayfine commented Jun 29, 2020

After #166, buildrs_additional_environment_variables is no longer respected. An example breakage is that rust-protobuf can no longer be built, as its build.rs expected RUSTC and CARGO_PKG_VERSION. I temporarily unblocked myself by downgrading from 0.3.5 to 0.3.3, but I think the buildrs_additional_environment_variables should keep being supported, or it should be removed with its replacement documented.
@dayfine dayfine changed the title buildrs_additional_environment_variables is no only respected after #166 buildrs_additional_environment_variables is no longer respected after #166 Jun 29, 2020
This was referenced Jun 29, 2020
Closed
Merged
@damienmg
Copy link
Member

#166 should pass along RUSTC and CARGO_PKG_VERSION, can we have a repro?

Also I ran in a couple of different problems with latest version of cargo raze that could be related, cf: #177

@dayfine
Copy link
Author

dayfine commented Jun 29, 2020 •
edited
Loading

It passes RUSTC along (by means other than buildrs_additional_environment_variables), but not CARGO_PKG_VERSION.

I am building rust-protobuf in this repo by relying on buildrs_additional_environment_variables and some other magic: https://github.com/dayfine/xlab/blob/master/third_party/cargo/Cargo.toml. IIRC:

  • If I use 0.3.4 and above, it will first complain the rustc magic I put in is not a rust_library or cc_library
  • Then after removing the rustc magic, CARGO_PKG_VERSION is still not properly passed, i.e. the buildrs_additional_environment_variables is not in the generated BUILD file. I've tried to use additonal_flags or additional_envs, which will show up in the generated BUILD file, but with gen_buildrs = true, the build invocation does not actually pass CARGO_PKG_VERSION and the package still complain about missing CARGO_PKG_VERSION

@damienmg
Copy link
Member

Indeed the CARGO_PKG_VERSION is not added, let me try to send a PR to fix that on rules_rust.

damienmg added a commit to damienmg/rules_rust that referenced this issue Jun 30, 2020
@damienmg
Add the ability to pass extraneous environment variables to build scr…
cc88181 
…ipts

cargo-raze as the `buildrs_additional_environment_variables` that allow
extraneous environment variable to be passed to the build script. In addition
this should enable for everybody to add the environment variable not added
at this time.

That's the rules_rust side of the fix for google/cargo-raze#179.
damienmg added a commit to damienmg/rules_rust that referenced this issue Jun 30, 2020
@damienmg
Pass extraneous environment variables to build scripts
f02e4b8 
cargo-raze has `buildrs_additional_environment_variables` which allows
extraneous environment variable to be passed to the build script. In addition
this should enable for everybody to add the environment variables that are
not added by the `cargo_build_script` rule itself.

That's the rules_rust side of the fix for google/cargo-raze#179.
@damienmg damienmg mentioned this issue Jun 30, 2020
Merged
damienmg added a commit to damienmg/rules_rust that referenced this issue Jun 30, 2020
@damienmg
Pass extraneous environment variables to build scripts
51d382e 
cargo-raze has `buildrs_additional_environment_variables` which allows
extraneous environment variable to be passed to the build script. In addition
this should enable for everybody to add the environment variables that are
not added by the `cargo_build_script` rule itself.

That's the rules_rust side of the fix for google/cargo-raze#179.
damienmg added a commit to bazelbuild/rules_rust that referenced this issue Jun 30, 2020
@damienmg
Pass extraneous environment variables to build scripts (#362)
149400c 
cargo-raze has `buildrs_additional_environment_variables` which allows
extraneous environment variable to be passed to the build script. In addition
this should enable for everybody to add the environment variables that are
not added by the `cargo_build_script` rule itself.

That's the rules_rust side of the fix for google/cargo-raze#179.
@dayfine
Copy link
Author

dayfine commented Jul 1, 2020

Thanks, and it will also need to be passed in the template: https://github.com/google/cargo-raze/blob/master/impl/src/templates/partials/build_script.template?

@damienmg damienmg mentioned this issue Jul 1, 2020
Merged
@damienmg
Copy link
Member

damienmg commented Jul 1, 2020

Yes :) #180 was already on my computer yesterday but now it is sent for review :)

damienmg added a commit that referenced this issue Jul 1, 2020
@damienmg
Support additional enviroment variable for build script (#180)
8ee049e 
Since the switch to the cargo_build_script rule, the
buildrs_additional_environment_variables was no longer working, this
change add this support back.

Fixes #179
@dayfine
Copy link
Author

dayfine commented Jul 5, 2020

Hi Damien, could a new release be cut so I can install the new version and verify this change? Thanks!

@acmcarther
Copy link
Member

Published 0.3.7 containing this fix.

@dayfine dayfine mentioned this issue Jul 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support additional enviroment variable for build script damienmg/cargo-raze
3 participants
@acmcarther @damienmg @dayfine