Use Precompiled Sources to Link Against about shaderc-rs HOT 18 CLOSED

Hey knappador@, thanks for the interest! I'm happy to facilitate that.

Right now shaderc-rs is statically linking against shaderc_combined.a (https://github.com/google/shaderc-rs/blob/master/build/build.rs#L90). But that can certainly be changed to link dynamically against shaderc_shared.so under some new cargo feature (https://github.com/google/shaderc-rs/blob/master/Cargo.toml#L18).

Broadly, I think maybe it's time to create a proper *-sys crate (by splitting out the ffi.rs) to handle the native library discovery/building and linking procedure. shaderc-rs will then become a nice Rust API wrapper on top of shaderc-sys.

I'm not necessarily have time for this recently; so PRs definitely very welcome! :)

from shaderc-rs.

Hey @knappador, thanks for the contribution! But I don't quite understand your rationale to spin off a completely new repo. IMHO, we can just turn this repo to host both using Cargo workspace for the following reasons:

• The sys and lib crates needs to be developed together tightly. We want to expose all features added in sys via lib; and all features in lib needs support from sys. Having everything in one Git repo makes things much easier since you can just submit them in one commit. If we have two separate repos, it requires double the work.
• It's easier to rollback on failures and bump version containing the rollback if everything is in one repo. With separate repos, it suddenly requires coordination. And GitHub does not have good support for coordinating multi-repo submission.
• For history. Having everything in one repo naturally preserves the history in the past and also keeps the history in sync for the future. (As said in the first point, these two crates are highly related.) With separate repos, things (esp. doc, README, etc.) can go out of sync silently. And keep them in sync again requires double the work.
• For licensing. This repo is copyrighted to Google and we have bots for checking CLAs here. Having a new repo not inside google/ organization loses that. That can likely cause licensing issues. (I'm not a lawyer though! :)
• ...

In summary, I think these two crates are highly related, it's better for them to stay together.

Would you like to explain your rationale? Thanks! :)

from shaderc-rs.

The path I'm taking is to split both of these on version 0.4 and create my own shaderc-sys crate. I'll give you co-ownership as soon as it's up and add a volunteer to aid future maintenance.

I have begun work by naively splitting the project with shared git history up until 0.4.

Currently shaderc-rs is building off of my local copy of shaderc-sys, so should be able to PR this for 0.4 soon.

from shaderc-rs.

We will lose all CI visibility on the sys crate. I can re-add this in using a Circle CI solution based on e-nguyen's config, which uses an Arch Linux image and can test the pkg-config setup.

from shaderc-rs.

@antiagainst I haven't used workspaces yet, so the thought didn't occur to me while figuring out what they would even look like as a sys crate. I'll go punch the revision history.

from shaderc-rs.

I didn't find a delete button to go beyond yanking a crate off of crates.io, so it's an appropriate time to consider the goal state:

1. Two crates, version 0.4.1 or so, unpublished until merge is green lighted
2. Yanked shaderc-sys 0.4.0 from crates
3. Workspace in shaderc-rs repo created with the contents of shaderc-sys that I split out. Version history will start back at the main shaderc-rs master.

from shaderc-rs.

Regarding the actual changes intended for the sys crate build options, I've so far come up with the following plan:

• Add basic /usr/lib/ and pkg-config detection of shaderc system library to use it as a preferred source of libshaderc
• Unset default for build-from-source and leave it as a fallback / override in case no system library is detected. This is consistent with recommendations to avoid downloading by default and the recommendation to not configure default crate features since it makes life hard on downstream configurations.
• Add support for env basedLIB_SHADERC_DIR as an override / explicit location
  https://github.com/kornelski/rust-lcms2-sys/blob/bed8356d67ff21fd0476ce2045ad410272ab553a/src/build.rs#L16-L26
• Preserve but warn about the use of the build directory for placing shaderc. When I tested this option, it requested I place libshaderc_combined.a an unpredictable directory that is resistant to automation.

These changes will be in a PR based on the shaderc-sys branch but independently mergable

from shaderc-rs.

Agreed to your above comments. Thanks @knappador! :)

Two crates, version 0.4.1 or so, unpublished until merge is green lighted

We can have a commit to split crates and do some testing to make sure it works (esp. for your needs). Then have another commit to bump version and publish crates. We can also go to version 0.5 directly.

Workspace in shaderc-rs repo created with the contents of shaderc-sys that I split out. Version history will start back at the main shaderc-rs master.

That's nice! Thanks! :)

Add basic /usr/lib/ and pkg-config detection of shaderc system library to use it as a preferred source of libshaderc

IIUC, this should happen on the C++ shaderc project?

Unset default for build-from-source and leave it as a fallback / override in case no system library is detected. This is consistent with recommendations to avoid downloading by default and the recommendation to not configure default crate features since it makes life hard on downstream configurations.

Then the default would be to use system libshaderc dynamic/static lib if available, otherwise download and build? SGTM.

Add support for env basedLIB_SHADERC_DIR as an override / explicit location
https://github.com/kornelski/rust-lcms2-sys/blob/bed8356d67ff21fd0476ce2045ad410272ab553a/src/build.rs#L16-L26

This is nice! It makes sense to let this one have the highest priority because I think if an environment variable is explicitly given by the developer, then it means the developer wants to use it.

Preserve but warn about the use of the build directory for placing shaderc. When I tested this option, it requested I place libshaderc_combined.a an unpredictable directory that is resistant to automation.

Yes. This is actually used. So it's better to preserve for now.

from shaderc-rs.

IIUC, this should happen on the C++ shaderc project?

In our current form, if CMake is being used, the source has been downloaded and I don't think it's consistent to download the source and then search for libshaderc_combined.a. AFAIK the build for shaderc only produces libshaderc* so the behavior should decide one of either pre-installed or from source. Is that what you meant?

The Arch Linux pre-built package has a missing .pc file that pkg-config uses, which is not fatal, just annoying. I'm working on that upstream.

It makes sense to let this one have the highest priority

Totally. The overall preference order is to be: explicit > system detected > from source

from shaderc-rs.

I've uploaded the rebase. I am not sure yet how to avoid breaking the --no-default-features except to possibly inherit it into shaderc-rs and instead pipe that to a feature on to the new shaderc-sys

Transitive features. I'll take a look when I get back from eating.

from shaderc-rs.

e-nguyen#1

This PR adds the --no-defaults behavior back into shaderc-rs by causing shaderc-sys to implement the behavior when --no-defaults is passed to shaderc-rs.

I learned a lot about cargo, but not a way using common (durable) cargo syntax to invert --no-defaults into a feature enabler instead. I wound up combining two features to simulate one feature without tying shaderc-sys exclusively to shaderc-rs.

I feel like it's pretty hacky. It might be easier to read the code 😿 but I'll describe the scheme:

• check-passthrough must be enabled on the sys dependency to even allow setting the old --no-defaults behavior
• dont-use-deprecated feature of the sys package allows suppression of old --no-defaults to be set as a default in the rs package
• dont-use-deprecated must be disabled by passing --no-defaults to rs package to enable the old --no-defaults behavior in sys

We disable the feature suppression flag while enabling the overall behavior in order to obtain the old style --no-defaults behavior.

I hate something, but it does work for both shaderc-rs and shaderc-sys

from shaderc-rs.

Interestingly I went back and realized I cannot, against master, build libshaderc_combined.a and use it to complete the tests. Works fine from source, but against cargo clean && cargo test --no-default-features even after I copy the library in, I get a pile of errors. I use the same libshaderc_combined.a that is built for a working run of the tests. Is using the --no-default-features a supported workflow for the tests?

from shaderc-rs.

I'm just about done establishing a test baseline to be able to assess regressions in the module split.

Successful Baseline Tests on --no-default-features

On master branch, was able to build using only a pre-compiled copy of libshaderc_combined.a as long as it was built from source and then saved to clean and copy into a --no-default-features build.

To successfully run tests on master's --no-defaults path, did the following setup:

• cargo clean
• remove all submodule contents
• copy the following into the --no-defaults path of build.rs

        let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap();
        let target_env = env::var("CARGO_CFG_TARGET_ENV").unwrap();

        match (target_os.as_str(), target_env.as_str()) {
            ("linux", _) | ("windows", "gnu") => println!("cargo:rustc-link-lib=dylib=stdc++"),
            ("macos", _) => println!("cargo:rustc-link-lib=dylib=c++"),
            _ => {}
        }

• copy a pre-built libshaderc_combined.a into the searched directory
• cargo build --no-default-features
• cargo test --no-default features

Prebuild Package Link Errors When Building Tests

When switching to the libshaderc_combined.a from my system, I some link errors such as:

undefined reference to `spvtools::Optimizer::RegisterPerformancePasses()'

This only seems to occur when cargo is building the tests. I'm not sure how to proceed yet.

from shaderc-rs.

🎉 🌮 Tests passing for statically linked system library builds 🌮 🎉

Almost ready to wrap this up. Compiling the tests requires a handful of extra links (and likely these are needed by any dependent application as well) To test the resulting branch against a dependent application, I'll use E-Nguyen.

The static-linking block in master grows to:

    if env::var("CARGO_FEATURE_BUILD_NATIVE_SHADERC").is_err() {
        let out_dir = env::var("OUT_DIR").unwrap();
        println!("cargo:warning=Requested to skip building native C++ shaderc.");
        println!(
            "cargo:warning=Searching {} for shaderc_combined static lib...",
            out_dir
        );
        // Additional links that were required to run tests
        println!("cargo:rustc-link-search=native=/usr/lib/");
        println!("cargo:rustc-link-lib=static=SPIRV");
        println!("cargo:rustc-link-lib=static=SPIRV-Tools-opt");
        println!("cargo:rustc-link-lib=static=SPIRV-Tools");
        println!("cargo:rustc-link-lib=glslang");

        println!("cargo:rustc-link-search=native={}", out_dir);
        println!("cargo:rustc-link-lib=static=shaderc_combined");

        let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap();
        let target_env = env::var("CARGO_CFG_TARGET_ENV").unwrap();

        match (target_os.as_str(), target_env.as_str()) {
            ("linux", _) | ("windows", "gnu") => println!("cargo:rustc-link-lib=dylib=stdc++"),
            ("macos", _) => println!("cargo:rustc-link-lib=dylib=c++"),
            _ => {}
        }

        return;
    }

from shaderc-rs.

@antiagainst

System-installed libshaderc_combined.a will not be as "combined" as the one built from source in this project. SPIRV-tools etc are separate packages.

Where this can break is that shaderc-rs will build, but the dependent project might fail to build and complain about link errors caused by missing glslang and SPIRV.

I don't have a great solution. My proposal is to add the extra link-lib lines for SPIRV and glslang if I find them (on Linux) but assume a good libshaderc_combined.a otherwise. Documents would be updated to the effect that building from this project and saving a copy is a good workaround if one's platform doesn't have good SPRIV & glslang packages pre-built.

I should note that while I don't like this solution, the problem exists in the current --no-default-features situation.

If this course is sufficient, I will get it all packaged up for review. Blocking for feedback.

from shaderc-rs.

Oh, wow. That's great progress, thanks @knappador! And sorry about the delay. Was busy with some other work. :)

AFAICT, currently Shaderc's lib and package story needs to be further developed. The libshaderc_combined.a was initially to by-pass the involved linking procedure when using Glslang. I chose to use it because it provides a seemingly cleaner dependency by bundling everything in one lib. But I understand that it kind of defies the purpose of packages on Linux distros: you want one copy of each lib and mixing different copies can result in bad things. So I'm not quite happy with it either. What you said in #44 (comment) SGTM as a workaround. It is an incremental improvement over existing implementation also. So please go ahead. :)

With that said, do you still want me to review #46 and get it landed first? Or you'd prefer me to wait for further changes? Thanks!

from shaderc-rs.

I'm not blocked on this. Going to get a PR on top of #46 and then will shout when #46 + changes can go into master.

from shaderc-rs.

SG!

from shaderc-rs.