Shaderc-rs & Shaderc-sys split #46

knappador · 2019-03-30T04:42:21Z

--snip-- #44 has almost all of the discussion leading up to the merge commit notes

fixes #44

googlebot · 2019-03-30T04:42:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-30T04:42:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

knappador · 2019-03-30T04:45:05Z

I signed it!
@googlebot

googlebot · 2019-03-30T04:45:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Version 0.4.0 has been skipped and tag will not be used. History for a prospective split at 0.4.0 into two repos has been obliterated. Internal renaming in shaderc-rs/src/lib.rs ffi -> scs The crates were split as follows: Sys crate files - Build dependencies - build.rs - ffi.rs Rs crate files - lib.rs Top level - contrib agreement - .gitmodules - .gitignore - workspace Cargo.toml - CI scripts - license - readme

- `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 The old use of --no-default-features applies to shaderc-rs, but now it needs to be passed through to shaderc-sys. This is done by passing an inverted flag to suppress the behavior by default from shaderc-rs. When the suppression is removed with --no-default-features, the old behavior is re-enabled and shaderc-sys looks for a libshaderc_combined.a in the out directory When building shaderc-sys directly, only check for the passthrough flag if it could be enabled. It's a package opt-in deprecated behavior. It definitely needs deprecation.

knappador · 2019-04-16T21:03:19Z

@antiagainst I'm not sure what's up on the Appveyor results. I have little visibility outside of Arch Linux so far.

This PR should read fine as a series of independent commits.

Re-implemented the --no-defaults behavior
Added more robust build options & behavior

I primarily used this heavily referenced guide to writing sys crates to select what traditions to try to follow. The one area I'm not yet 100% clear on is overriding the static library location using a downstream project's Cargo.toml

To quickly run through a thorough test of the Linux build paths on Arch linux:

cargo test
cargo clean
cargo test --features build-from-source
cp ../target/debug/../libshaderc_combined.a .
cargo clean
cargo test SHADERC_STATIC=./libshaderc_combined.a
cargo clean
cargo test --features check-passthrough  # will cause trigger old --no-defaults behavior on sys crate
# cp the library into the specified location
# re-run with --features check-passthrough
cargo clean
mkdir libs
cp /usr/lib/libshaderc_combined.a libs/
cp /usr/lib/libglslang.a libs/
cp /usr/lib/libSPIRV.a libs/
cp /usr/lib/libSPIRV-Tools.a libs/
cargo test SHADERC_STATIC=./libs/

Happy to punch some commits into working on Appveyor but I'll need some information.

knappador · 2019-04-16T22:23:43Z

Handling the merge issue. Tests for shaderc-rs fail due to linking. I'm incorporating 367a4dd as well, which won't apply at all.

Might punch some more history later to convert the delete to a move if possible.

If you are here because your windows or macos builds are breaking, I apologize. The changes were not developed on these platforms. You will have to research older versions of the crate to establish whether they were in a "just works" state and if not, research your relevant platform options and make it work. This commit implements several new build behaviors: - Building libshaderc from source is now fallback behavior on Linux - Linux will check "/usr/lib/" for libraries by default - SPIRV and glslang libraries _may_ be picked up correctly if they exist in the same location as libshaderc, but funky combinations could break - Specification of a static library via SHADERC_STATIC, SHADERC_LIB_DIR, or SHADERC_LIB_PATH instead of the hacky --no-default-options - tests will build correctly when using hacky --no-default-options paths (stdc was not being included)

knappador · 2019-04-16T23:55:54Z

Full test sequence fine on my machine, on Linux. There was a regression in build.rs. glslang being included statically broke compiling the tests. I have no idea how I found that when I wrote down the critical hunk in #44

…fault-to-system-on-linux No actual conflicts.

Original PR: google#47 To merge master

knappador · 2019-04-17T00:38:35Z

@antiagainst looks like all three platforms have some level of support based on tests? Time to party yet? 📦 🎉

Let's remember to toss in a version bump right before merge. I think "0.5" is what we settled on for Use Precompiled Sources to Link Against #44

Feel free to keep pinging me on build issues moving forward.

I'm nominating this for merge and not blocked on anything.

knappador · 2019-04-17T01:27:32Z

I'm still thinking about necessary ways to pass through the new options to shaderc-sys. The command line settings "just work." I'm going to add in a patch to pass through the build-from-source feature to shaderc-rs since it's just easier than figuring out a nested dependency configuration for most people.

shaderc-sys/src/lib.rs

antiagainst

This is awesome progress! :) I've a few comments inlined. Thanks for the work again!

shaderc-rs/src/lib.rs

README.md

shaderc-rs/Cargo.toml

shaderc-sys/build/build.rs

shaderc-sys/src/lib.rs

shaderc-sys/build/build.rs

README.md

knappador · 2019-04-20T02:26:54Z

I didn't have a lot of time to review changes in f03ba63

Many of the comments were addressed. I'll punch the revision history after changes are more final.

Worth considering how the new structure might affect the docs. I have little experience yet working with Rust docs.
https://docs.rs/shaderc/0.3.16/shaderc/

antiagainst · 2019-04-20T12:45:52Z

shaderc-sys/Cargo.toml

 build-from-source = []
+check-inverted-no-defaults = []
+inverted-no-defaults = []


How do you think about shaderc-rs-use-sys-libs and use-sys-libs here then? Given the tight relationship between shaderc-rs and shaderc-sys, it is reasonable to have an option dedicated to shaderc-rs here. (I'm just trying to make the feature more self-documenting. :) )

This crazy inversion thing is only necessary because of how --no-default-features operates on a crate's config. I would support a rename to shaderc specific if you keep the backwards compatibility. If you axe the backwards compatibility, these features go away. shaderc-rs-inverted-no-defaults etc would be more appropriate. The features have no control over reading specific directories, only the old behavior of reading the ../out/ directory.

If you decide to axe no defaults, it's as simple as removing these two features in both crates and axing the block in build.rs that reads them, then removing the documents.

The reason I implemented the --no-default-features backwards compatibility is because you believed people were using it (possibly in CI?). It's wise to maintain at least one release of deprecation in any case.

SG. This will be removed soon, so I won't bother that much anymore. :)

antiagainst · 2019-04-20T12:48:45Z

shaderc-sys/Cargo.toml

+name = "shaderc_sys"
+
+[features]
+check-passthrough = []


I think another way is just to eliminate the --no-defaults-behavior support for shaderc-rs entirely. It was not a satisfying solution; and now we have SHADERC_LIB_DIR now and it is better. WDYT?

antiagainst

This is nice, thanks again for doing this! :)

Feel free to do whatever clean up you'd like to and then let me know. I will apply a few cosmetic fixes and then merge this.

…imited and should not be interpreted as a larger coherent change.

…ative to the dependency, not the location of where cargo was invoked. Warn the user if a relative path is relative, not a directory, or fails to resolve (usually file not found becuase of a bad relative path)

knappador · 2019-04-21T05:57:56Z

I added some cleanups after doing some testing and sitting back to leisurely read the code. One feature I actually changed was to try to canonicalize relative paths so that they would work... only to realize that the CWD changes during each recursion cargo makes to a dependency. Specifying a relative path must be done relative to shaderc-sys crate.

--rerun-after-changed and it's env variable counterpart deserve a look. I think we have to combine both for our case, one to detect lib file changes and the other to detect env changes. They have some funky behavior though. I didn't try them.

AFAIK the only outstanding question is to completely axe --no-default-features behavior or leave it deprecated for a few release cycles.

Tests

cargo test
cargo clean
cargo test --features build-from-source
cp ../target/debug/../libshaderc_combined.a ../
cargo clean
cargo test SHADERC_LIB_DIR=../
cargo clean
cargo test --no-default-features
# cp the library into the specified location
# re-run  test --no-default-features
cargo clean
mkdir ../libs
cp /usr/lib/libshaderc_combined.a ../libs/
cp /usr/lib/libglslang.a ../libs/
cp /usr/lib/libSPIRV.a ../libs/
cp /usr/lib/libSPIRV-Tools.a ../libs/
cargo test SHADERC_LIB_DIR=../libs/

…nion here since this is a combined crate's README that contains quite a bit of relevant build information for the crate in spite of a lot of shaderc-rs specific information. Two readmes might be a bad idea for maintenance.

antiagainst · 2019-04-21T13:38:06Z

Looks good! I just did some final touch over documentation. Will merge soon.

Good point about --no-default-features support. I think with semver, it should be fine to axe it entirely. (Sorry I didn't anticipate it as being this complicated, so I suggested we should support it earlier. We have users of it, see #36) But you already implemented it, let's be nice to keep it for another release. Thanks! :)

knappador · 2019-04-21T13:47:16Z

👍 Changes look good. Thanks for the extensive review, feedback, and refinements

vulkano-rs/vulkano#1183 crosslinking

antiagainst · 2019-04-22T03:16:54Z

Merged, thanks! @knappador, it seems I cannot publish shaderc-sys because I don't have the right access. Would you mind add me? Or If you want to do it, please feel free (use the top of the tree). :)

knappador · 2019-04-22T06:14:25Z

I believe I invited you already, I will resend when I get to my credentials

…

On Sun, Apr 21, 2019, 10:16 PM Lei Zhang ***@***.***> wrote: Merged, thanks! @knappador <https://github.com/knappador>, it seems I cannot publish shaderc-sys because I don't have the right access. Would you mind add me? Or If you want to do it, please feel free (use the top of the tree). :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE4Q4YGOROWGG2IAKKNQWLPRUU2PANCNFSM4HCOEWHQ> .

antiagainst · 2019-04-22T11:21:20Z

https://crates.io/me/pending-invites shows I've no pending owner invites.

knappador · 2019-04-22T14:34:04Z

@antiagainst

Invite sent. If there's no crate, the UI presents no option to manage owners 🙊

cargo publish from within the shaderc-sys crate failed first time. I changed the README path back to relative, success. I can PR this if you're unsure of what I did to get cargo to succeed.

Also it means I burned another semver to give you an invite 🙊 0.4.2 I guess 🐱

antiagainst · 2019-04-22T15:19:45Z

Nice, I've access now and published 0.5.0! \o/ Thanks!

Could you yank 0.4.1 now? :)

knappador · 2019-04-22T16:40:09Z

Yanked. Going to merge this downstream for CI justice.

knappador added 2 commits March 31, 2019 00:06

- rustfmt

71bfb64

knappador force-pushed the shaderc-sys-split branch from 1a0e7c2 to 71bfb64 Compare March 30, 2019 15:18

antiagainst mentioned this pull request Apr 2, 2019

Use Precompiled Sources to Link Against #44

Closed

knappador mentioned this pull request Apr 16, 2019

Replace glsl-to-spirv with shaderc gfx-rs/gfx#2731

Closed

4 tasks

knappador mentioned this pull request Apr 16, 2019

Update docs: Ninja now required to build with MSVC #48

Merged

knappador force-pushed the shaderc-sys-split branch from 962d2f2 to 7f8b5ac Compare April 16, 2019 23:54

knappador added 2 commits April 17, 2019 08:58

Merge branch 'master' of https://github.com/google/shaderc-rs into de…

6c036f1

…fault-to-system-on-linux No actual conflicts.

Manual reapplication of dfaefb0

e5a403e

Original PR: google#47 To merge master

build-from-source pass-through feature for shaderc-sys

03a06a7

knappador mentioned this pull request Apr 17, 2019

Tracking Issue for Shaderc Pre-Built Package Linking e-nguyen/e-nguyen#15

Open

knappador commented Apr 18, 2019

View reviewed changes

shaderc-sys/src/lib.rs Show resolved Hide resolved

antiagainst reviewed Apr 19, 2019

View reviewed changes

antiagainst reviewed Apr 20, 2019

View reviewed changes

antiagainst approved these changes Apr 20, 2019

View reviewed changes

knappador added 2 commits April 21, 2019 14:34

Assorted cleanups to names and documents. These changes are locally l…

ba3e43c

…imited and should not be interpreted as a larger coherent change.

Relative paths are kind of a pain in build scripts since they are rel…

1f49296

…ative to the dependency, not the location of where cargo was invoked. Warn the user if a relative path is relative, not a directory, or fails to resolve (usually file not found becuase of a bad relative path)

knappador force-pushed the shaderc-sys-split branch from f03ba63 to 1f49296 Compare April 21, 2019 05:40

knappador and others added 3 commits April 21, 2019 15:01

fix README path used to make docs & crates.io page

30b0657

Refine documentation

a24a62b

This was referenced Apr 21, 2019

Provide binary libraries, please google/shaderc#414

Closed

ShaderC Compile & CI vulkano-rs/vulkano#1183

Closed

antiagainst merged commit 675231a into google:master Apr 22, 2019

knappador deleted the shaderc-sys-split branch April 22, 2019 16:39

knappador mentioned this pull request Apr 23, 2019

Shaderc version bump vulkano-rs/vulkano#1197

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shaderc-rs & Shaderc-sys split #46

Shaderc-rs & Shaderc-sys split #46

knappador commented Mar 30, 2019 •

edited

Loading

googlebot commented Mar 30, 2019

googlebot commented Mar 30, 2019

knappador commented Mar 30, 2019

googlebot commented Mar 30, 2019

knappador commented Apr 16, 2019

knappador commented Apr 16, 2019

knappador commented Apr 16, 2019

knappador commented Apr 17, 2019

knappador commented Apr 17, 2019

antiagainst left a comment

knappador commented Apr 20, 2019

antiagainst Apr 20, 2019

knappador Apr 20, 2019

antiagainst Apr 21, 2019

antiagainst Apr 20, 2019

antiagainst left a comment

knappador commented Apr 21, 2019 •

edited

Loading

antiagainst commented Apr 21, 2019

knappador commented Apr 21, 2019 •

edited

Loading

antiagainst commented Apr 22, 2019

knappador commented Apr 22, 2019 via email

antiagainst commented Apr 22, 2019

knappador commented Apr 22, 2019

antiagainst commented Apr 22, 2019

knappador commented Apr 22, 2019

Shaderc-rs & Shaderc-sys split #46

Shaderc-rs & Shaderc-sys split #46

Conversation

knappador commented Mar 30, 2019 • edited Loading

googlebot commented Mar 30, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Mar 30, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

knappador commented Mar 30, 2019

googlebot commented Mar 30, 2019

knappador commented Apr 16, 2019

knappador commented Apr 16, 2019

knappador commented Apr 16, 2019

knappador commented Apr 17, 2019

knappador commented Apr 17, 2019

antiagainst left a comment

Choose a reason for hiding this comment

knappador commented Apr 20, 2019

antiagainst Apr 20, 2019

Choose a reason for hiding this comment

knappador Apr 20, 2019

Choose a reason for hiding this comment

antiagainst Apr 21, 2019

Choose a reason for hiding this comment

antiagainst Apr 20, 2019

Choose a reason for hiding this comment

antiagainst left a comment

Choose a reason for hiding this comment

knappador commented Apr 21, 2019 • edited Loading

antiagainst commented Apr 21, 2019

knappador commented Apr 21, 2019 • edited Loading

antiagainst commented Apr 22, 2019

knappador commented Apr 22, 2019 via email

antiagainst commented Apr 22, 2019

knappador commented Apr 22, 2019

antiagainst commented Apr 22, 2019

knappador commented Apr 22, 2019

knappador commented Mar 30, 2019 •

edited

Loading

knappador commented Apr 21, 2019 •

edited

Loading

knappador commented Apr 21, 2019 •

edited

Loading