build.rs can be run from arbitrary directories

[illicitonion]

Currently it hard-codes an assumption that the build script is run from the $CARGO_MANIFEST_DIR. While this is true for cargo, and typically is true for other build systems, some toolchains may have other requirements.

1e1021d started encoding that this assumption may not always hold, this commit just extends that other places the assumption is currently made.

This was tested by adding a call to
std::env::set_current_dir("/tmp").unwrap() at the top of the build script's main function, and has been used to build both the pregenerated version and the non-pregenerated version of the build script.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.

[briansmith]

Thanks. The cargo docs at https://doc.rust-lang.org/cargo/reference/build-scripts.html do say "In addition to environment variables, the build script’s current directory is the source directory of the build script’s package." It seems to me that an alternate runner of build.rs (like Bazel) should change the current working directory accordingly before executing the build script.

[illicitonion]

Bazel's rust rules does run in the same directory as cargo would by default, but it offers an override in case a user's C++ toolchain strongly assumes that it's being run from the repository root (which is Bazel's standard mode of operation for other kind of builds, so something that can happen when people are using slightly weird set-ups). While I agree that this is a somewhat niche scenario, I've run into it in the wild, so it'd be handy support if it's not too much hassle to do so.

[briansmith]

I am already working on adapting build.rs so that it can be run in a 2nd mode where it will generate a BUILD.gn for GN, which is not too different from Bazel. I would very much like to extend that work so that it could be run on a 3rd mode to generate a Bazel Build file too. Then users of GN and Bazel would not need to run the build.rs at all, as some users of ring are not allowed to run build.rs scripts. Is this an alternative you'd be interested in helping with?

[illicitonion]

I am already working on adapting build.rs so that it can be run in a 2nd mode where it will generate a BUILD.gn for GN, which is not too different from Bazel. I would very much like to extend that work so that it could be run on a 3rd mode to generate a Bazel Build file too. Then users of GN and Bazel would not need to run the build.rs at all, as some users of ring are not allowed to run build.rs scripts.

That sounds great! I think for the Bazel side we'd generally one want cc_library that can be depended on (either having multiple generated per OS/arch which we can select between, or selecting the srcs differently per OS/arch, depending on how complex the customisation is).

@dtolnay recently(ish) contributed a feature to Bazel's rules_rust where this customisation can be done completely in the Cargo.toml file without needing any intervention from users of ring (see this example) which should hopefully make this really easy to consume, too.

Is this an alternative you'd be interested in helping with?

Very happy to talk! I'm not sure how much time I can dedicate to the actual implementation, but happy to discuss, review, and see what time I can make to help implement. (With a caveat that I'm also away for ~the month of March :))

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.30%. Comparing base (b873b0d) to head (5e28ed3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1962   +/-   ##
=======================================
  Coverage   96.29%   96.30%           
=======================================
  Files         135      135           
  Lines       20663    20663           
  Branches      226      226           
=======================================
+ Hits        19898    19899    +1     
  Misses        730      730           
+ Partials       35       34    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[briansmith]

There are still two relative paths here, in Command::new and the include/, so I think this probably won't work if the current working directory isn't what Cargo would set, unless you are building from a packaged release of ring that skips the generate_sources_and_preassemble step.

If you actually are running nasm in Bazel through build.rs, I am guessing you need a solution for the Command::new independently of the current working directory issue. Probably we should introduce a cargo cfg directive for providing the path to nasm.