briansmith / ring

Safe, fast, small crypto using Rust
Other
3.68k stars 693 forks source link

build.rs can be run from arbitrary directories #1962

Closed illicitonion closed 5 months ago

illicitonion commented 5 months ago

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.

1e1021d05bdd41cb02bbc43615c9dd7ecd35f853 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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.