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.

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

"Build system" #48

Closed
khyperia opened this issue Oct 10, 2020 · 10 comments
Closed

"Build system" #48

khyperia opened this issue Oct 10, 2020 · 10 comments
Assignees
Labels
t: enhancement A new feature or improvement to an existing one. t: tracking issue An issue tracking the progress of a specific feature or change.

Comments

@khyperia
Copy link
Contributor

khyperia commented Oct 10, 2020

This is a tracking issue for what I've been calling the "build system", but in reality the full task list is larger than just that.

In general, this is a group of related issues that would be great for someone other than me to own and drive forwards. These tasks are spread across various "functionality levels", see that issue for more information.

  • Create a crate to be used by users build.rs, that takes a path to a shader crate and spits back a spir-v binary that can be include!d (or perhaps a better designed version of this). I've created an extremely rough and minimally-featured prototype in the spirv-builder crate - we need to expand it quite a bit, in both robustness and features.
  • Integrate spirv-opt and spirv-val into statically linkable libraries that are included in rustc_codegen_spirv. This means the user will not have to install spirv-tools on their system. Tracking issue: Add bindings for spirv-opt #31
  • Generate host-side bindings, tracking issue: Ergonomic Rust CPU to Rust GPU / SPIRV bindings #10. I like the approach of "SYCL compiles down it's GPU side C++ code along with some side-cart metadata first, this metadata is then converted into regular host side C++". This means that rustc_codegen_spirv would spit out some form of metadata for what bindings are in the module, and then the build.rs helper crate would generate rust source files from that metadata. (Or perhaps rustc_codegen_spirv would spit out rust files directly and the builder would just include them, but, whatever, exact behavior to be determined later. Having multiple backends - e.g. ash and vulkano both - complicates this.)
  • Possibly adding any hooks/etc. to that generated code to enable the rendergraph/flamegraph support.
@repi repi added the t: enhancement A new feature or improvement to an existing one. label Oct 10, 2020
@khyperia
Copy link
Contributor Author

khyperia commented Oct 13, 2020

Technical requirements for a user-facing build system, and what abilities we have to solve that!

Reading through this section of the readme is likely helpful: https://github.com/EmbarkStudios/rust-gpu#getting-started-for-power-users-who-dont-want-to-use-spirv-builder

We need to do a couple things to build a spir-v crate:

  1. Build "the compiler". This is the rustc_codegen_spirv crate. This requires linking against rustc nightly internals (rustc-dev rustup component), and produces a dynamic library.
  2. That dynamic library isn't a whole compiler, it's merely a backend plugin for rustc, and is given to rustc via the flag -Z codegen-backend=path_to_library. So, we need to set that flag when compiling the "gpu crate". The rustc version must match, so, nightly again.
  3. We also need to pass a few flags to cargo (not rustc) when building the "gpu crate", namely -Z build-std=core --target spirv-unknown-unknown.
  4. We need to find the spir-v file the compilation of the "gpu crate" produced, and make it available to, say, the vulkan app using it as a shader.

This is all terribly complicated for users, so we want to simplify it.

  • One fancy hack is: If a regular crate references the rustc_codegen_spirv crate as a normal Cargo.toml reference, then cargo will build the dynamic library for us, and give us the path to it (via placing it on LD_LIBRARY_PATH). Then, we can find the dylib, std::process::Command::new("cargo"), and pass in all the above flags.
  • Cargo also has a "json output format" that gives us a list of all files that rustc produced. We can parse this json and figure out what files it built.

That "fancy hack" crate (currently called spirv-builder) can be consumed in a number of ways, current ideas we've thought of so far is:

  • Reference spirv-builder in a build.rs, call the function calling Command::new("cargo"), and then set an environment variable to the path of the spir-v file created. This allows the host vulkan program to use include_bytes!() on the file. The downside here is that the app now requires nightly, because there's no break in the chain of compilation.
  • Reference spirv-builder in a normal rust executable, called via script, and copy the spir-v file to a known location (that is possibly checked in). Upside is that the normal rust executable can be nightly, but the main app can be stable.

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Nov 3, 2020

I've been thinking about this quite a bit in past couple of days, in relation to testing the crate, as right now we use spirv-builder for the tests and it complicates the testing process for a lot of the same reasons it's complicated for users. I think there's a few modifications to how we're building that would dramatically simplify the process. First I want to outline who we are targeting with the build system.

  • Users People or organisations who want to use rust-gpu in their projects but don't want to contribute or modify themselves. Builds should be easy for them to setup and use in their projects.
  • Contributors People or organisations who want to use and contribute to rust-gpu, they want to be able to build and test their changes. Builds should as close to CI as possible.
  • CI Continuous Integration wants to be able to easily build and test entirely from scratch. Builds should be fast and easy to re-produce for debugging.

I think the following changes together would drastically simplify the build process, and make rust-gpu both easier to test and use.

  1. Dropping cargo for building sysroot in favour of building it with rustc directly.
  2. Adding the option to use and install the spirv-unknown-unknown sysroot and rustc_codegen_spirv shared library to your system.
  3. Moving spirv-builder building rustc_codegen_spirv to be at runtime.
  4. Changing spirv-builder into our rust-gpu build and testing library/tool both for development and users. (Similar to x.py from rust)
  • Adding a CLI with the following commands
    • test command which runs the entire testsuite. This would replace the current shell scripts, and unify testing between CI and local development. It would also allow adding additional testing steps with non rust/cargo tools, such as using mdbook-linkcheck for checking that hyperlinks in the dev guide are all working.
    • build like test but only builds.
    • install command to build and install a release version of rust-gpu to your system.

I've laid out the motivation for each of the changes below.

  1. Right now the dependency on using cargo to build sysroot complicates testing and using spire-builder. Tests need to build serially to re-use libcore, and any user of spirv-builder needs to also be using cargo for their build system. Ideally the crate should be able to build without invoking cargo at all, and we should allow user's to integrate with their existing build systems.
  2. In tandem with 1 and managing our own sysroot, we could install it and the final shared library somewhere where they could be re-used. This would be great for users as they could just use -Z codegen-backend and have it work, and for tests since we could built and install ahead on CI and run the integrations tests on a prebuilt backend. We could even go slightly further for users and provide shell aliases (e.g. rustc-gpu=$(rustc +nightly-YYYY-MM-DD -Z codegen-backend=./path). )
    • We need to ensure we're running on the same nightly the installed version was compiled with it. But rustc_codegen_spirv could be able to handle checking that by embedding the compiled version hash and invoking rustc on entry for its version.
    • We'd still retain an option to build and run entirely within the build script like today.
  3. Currently we're compiling rustc_codegen_spirv with spirv-builder as it makes it the implementation simpler by exposing rustc_codegen_spirv through LD_LIBRARY_PATH. But this has also had a side-effect where the use-installed-tools and use-compiled-tools features have to be propagated into crates in order to provide that option. This makes it quite annoying to add new crates (such as examples or tests) in the repository, as they really shouldn't have to expose features to be tested with either of those tools.
    • Moving this dependency to runtime would it make easier to test both without propagating feature flags, and would allow us to add some ergonomics where we could detect if tools are installed first before compiling them.
  4. In relation to all of the above it's already becoming hard to "fully" test the project locally, as right now there isn't a single command that will perform the tests. Having this functionality in a single tool (and probably a single command) makes it a lot easier to test and be confident that your code is ready for review.

At the end of the day this is the expected flow for each of the use cases with the proposed changes.

User
Install workflow:

  • git clone <EmbarkStudios/rust-gpu>
  • cargo run -p spirv-builder install
  • (In the future those first two steps could be shipped as a cargo-install script, so something like cargo install rust-gpu invokes a build script that does those steps for you.)
  • Done, you should be able to use -Z codegen-backend with nightly rust directly.

In build scripts:

// In build.rs
fn main () {
   // Pseudo code (This could also be a single function that takes an enum.)
   SpirvBuilder::auto_detect(build_dir)?;
   SpirvBuilder::use_installed(build_dir)?;
   SpirvBuilder::from_source(build_dir)?;
	// For people who just want locations and don't want us to set flags.
   let (sysroot, codegen_backend_path) = builder.get_sources(build_dir)?;
}

Contributor

  • git clone <EmbarkStudios/rust-gpu>
  • cargo run -p spirv-builder install (optional)
  • Makes changes
  • cargo run -p spirv-builder test (default will auto-detect if you have tools installed and use those for building)

CI

  • git clone <EmbarkStudios/rust-gpu>
  • cargo run -p spirv-builder install
  • cargo run -p spirv-builder test --use-installed-tools
  • cargo run -p spirv-builder test --use-compiled-tools

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 3, 2020

Adding the option to use an install the spirv-unknown-unknown sysroot and rustc_codegen_spirv shared library to your system.

You can add it as $(rustc --print sysroot)/lib/rustlib/$HOST_TRIPLE/lib/codegen-backends/librustc_codegen_spirv-$RUST_RELEASE.so. Where $RUST_RELEASE is something like 1.49.0-nightly. This matches libLLVM-11-1.49.0-nightly.so. Then it will be available as rustc -Zcodegen-backend=spirv. I think it would make sense to change the required dir to $(rustc --print sysroot)/lib/codegen-backend or $(rustc --print sysroot)/lib/rustlib/codegen-backend though.

@khyperia
Copy link
Contributor Author

khyperia commented Nov 3, 2020

Then it will be available as rustc -Zcodegen-backend=spirv.

Wait what?!? I haven't seen or found code for that anywhere in rustc, could you point it out? I've only seen the codepaths for "here's a filename, dlopen it" and then a hardcoded "if it's the string llvm, or now cranelift, load the statically linked backend".

Edit: or do you mean submitting a PR to rustc to add support for this?

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 3, 2020

It turns out that it is also possible to add it to a sysroot passed using --sysroot, not just the sysroot that is part of rustc itself: https://github.com/rust-lang/rust/blob/8e8939b8045b7af6076fb718e2e298844aaf4650/compiler/rustc_interface/src/util.rs#L409

Edit: actually not, I misread the code. It has to be part of the default sysroot.

Wait what?!? I haven't seen or found code for that anywhere in rustc, could you point it out? I've only seen the codepaths for "here's a filename, dlopen it" and then a hardcoded "if it's the string llvm, or now cranelift, load the statically linked backend".

That is get_codegen_sysroot which I re-introduced in rust-lang/rust#77975 to load cg_clif when built as part of rustc.

@khyperia
Copy link
Contributor Author

khyperia commented Nov 3, 2020

oh! haha, apparently haven't git pull'd on rust-lang/rust in a while, so that's why my rg codegen-backends didn't show up with anything related to searching a path with that directory :P

@joonazan

This comment has been minimized.

@XAMPPRocky

This comment has been minimized.

@XAMPPRocky
Copy link
Member

A recent development that could change this, is the new "artifact dependencies" Cargo RFC (rust-lang/rfcs#3028), which will allow you to specify an artifact dependency in cargo (See example below). I think if this RFC is accepted and implemented, it will negate most if not all of the need for spirv-builder and #239. Though of course this RFC is still being discussed and I'm not sure what our urgency is to have a better build system in the near term.

sky-shader = { path = "./sky-shader", artifact = "cdylib", target = "spirv-unknown-unknown" }
const SHADER: &[u8] = include_bytes!(env!("CARGO_CDYLIB_DIR_SKY_SHADER"));

@khyperia
Copy link
Contributor Author

khyperia commented Apr 1, 2021

We've figured out the build system enough, and things work well enough, that we should probably make more focused issues on particular problems we're having, or feature desires (e.g. the cargo artifact dependency RFC)

@khyperia khyperia closed this as completed Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement A new feature or improvement to an existing one. t: tracking issue An issue tracking the progress of a specific feature or change.
Projects
None yet
Development

No branches or pull requests

5 participants