Use shader's Cargo.toml to get spirv-builder version and toolchain

[tombh]

We can still override the version and toolchain with CLI args. This
commit just changes the defaults.

I feel like there could be a lot of edge cases for this implementation.
So I consider it more of a first draft.

Notable changes:

• Spirv renamed to SpirvCli.
• New impl SpirvSource that does all the shader crate querying.
• CLI args change: --spirv-builder is now split into
  --spirv-builder-source and --spirv-builder-version.
• --shader-crate now lives on the install subcommand, which
  because it is inherited by build (where it was originally), means it
  doesn't make much of a difference.
• The full build test now has a teardown function making it idempotent.
• Cache folder structure now has sub folders for spirv-builder-cli's
  and rust-gpu repos.
• spirv-crate-template is updated to Git with revision 82a0f69
  because "0.9" doesn't seem to compile on Windows.

Things to consider:

• Can we remove TARGET_SPECS now, because we have a copy of the
  rust-gpu repo, we can get them from there instead?
• What's the UX expectations for changing spirv-std versions or
  overriding rust-gpu versions and toolchain? I think in most cases
  spirv-builder-cli rebuilds should occur because the changes will
  cause cache directory changes. But I think it'd be good to at least
  manually test this.
• Should the main build test be moved out into an integrations tests
  folder?
• I'd still like to test each spirv-builder-cli feature, ie:
  spirv-builder-pre-cli and spirv-builder-0_10. We're currently only
  testing spirv-builder-pre-cli.

[tombh]

So the CI tests have highlighted the curious case of not being able to compile a shader that exists in workspace whose root Cargo.lock has a different version from the shader's. It's curious because I don't have any idea whey spirv-builder-cli is even looking outside the shader crate that it's building?

I hadn't realised that you were manually compiling the shader-crate-template in the Github Action workflow. I had to disable that to get CI to pass on Linux and OSX. The Rust tests are actually testing the same build anyway, and they achieve that by copying shader-crate-template outside of the workspace to avoid the error I mention.

The windows error though is still a mystery, maybe it's because of parentheses in the path. I'll keep looking into it.

[schell]

I think these changes are great. I had some nits, some typos and some questions.

A few things I'd like to get clarified that weren't immediately obvious from the code (forgive my skimming):

• after everything is cached and built, will future invocations be possible without network activity?
  • Was that ever the case?
• How much more data are we caching?
• Does using cargo tree, or any of the other changes here, cause any significant increase in compile times? I'm guessing it's negligible compared to running codegen.

It seems you're still adding commits, so I'll wait until you say you're ready to do one last pass and approval. Awesome work! 👍

[tombh]

I've fixed everything from your review comments but the Windows build is still failing. The only clue I can see from the logs is this:

C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\spirv-tools-sys-0.8.0\spirv-tools\source\opt\fold_spec_constant_op_and_composite_pass.cpp : fatal error C1083: Cannot open compiler generated file: '': Invalid argument

So it's failing to build spirv-tools-sys which I can't think why would have changed in this PR.

But let me answer some of your questions anyway.

• At least there shouldn't be any significant network activity after the first build and the cache is warm. I wouldn't be surprise if at least some network activity happens, but like you suggest, that would have probably always been the case. I seem to remember you have to set things up with cargo vendor to have true network-less builds.
• The only extra thing we're caching is the rust-gpu repo.
• cargo tree runs in milliseconds. So by far the biggest difference is the clone of the rust-gpu repo.

[schell]

We just need to figure out this Windows error and then I think this is g2g 🚀 .

@tombh don't hesitate to thrash against CI on this ;)

[tombh]

Hehe sure, thanks, I won't hesitate. I'll get my thinking cap on 🤔

[schell]

I have some changes incoming, but I can't seem to push them. I'm not sure if my workflow is borked or if there's a permission error or what.

[tombh]

You want to push them onto this branch? I'm not sure that's a good idea, I do a lot of force pushing, so I could well overwrite any changes you push. I think a better idea is to make a PR against this PR. There's a nice feature in Github where you can change the underlying base branch for a PR, see here:

[schell]

Well I figured it out!

[tombh]

Those are some nice changes!

Though I'd prefer they were in their own branch. It's not so easy to pull in your changes into my local branch, I have to git stash, git checkout to a shared commit etc. And I generally try to strive for linear history, so I often git push --force my commits without thinking about it. I could easily overwrite your changes. And I see that Clippy is complaining, so maybe you'll have some more commits to push, so I don't want to be pushing whilst you still might be pushing. It all gets a bit complicated.

It's easy enough to make a PR targeting another PR, and then I can merge your changes into my PR as and when they're ready, without us stepping on each other's toes ❤️

[LegNeato]

FWIW jj is git-compatible and really helps with handling linear histories and juggling changes. Might be worth looking at.

[tombh]

Yeah jj is great, even Steve Klabnik is into it, he wrote a nice intro,

[schell]

I'll open up another PR into yours, @tombh 👍

[schell]

It's easy enough to make a PR targeting another PR

@tombh - is the method you're suggesting for me to fork your fork and then create a PR into your branch at your fork? Because I can't select your branch as the base for my changes in this repo.

[schell]

See my additional changes above @tombh ^

[schell]

Looks like currently on my branch (#28), spirv-tools-sys is failing to build on Windows.

[tombh]

I figured it out! Windows only fails when built inside cargo test. Why? I've no idea 😅 I'd guess it would be to do with some extra ENV that's set somewhere. But for now let's not worry about it and just go back to your method of building the shader-crate-template "manually" in the CI run.

To formalise it a bit I've added that and the lints to a justfile.

Thanks for the PR, I did include it. But I think I may still not have got my point across. Perhaps it's just enough to say that I think it's a Golden Rule to never add commits to another person's PR.

I had unpushed local commits when you pushed that commit. So I could neither push nor pull without overwriting code. I had to git reset ..., git stash, git pull, git stash pop, git commit, etc. It would have been possible to move your commit out of this PR, by creating a new branch and then git reset --hard the original branch to the last known safe public commit and then git push --force to get the PR back to normal. I mention all this just to point out that either way, one of us had to do some Git dances to workaround your commit. Hence the Golden Rule. It's just easier to always stick to your own branch/PR.

Another benefit of the Golden Rule is that it gives each developer the freedom to choose their own history management. For example I am perhaps an overly zealous proponent of linear history. Therefore that each commit should strive to be part of the beats of a narrative, that they should tell a coherent story such that they provide extra context and documentation. This is particularly useful for bisecting bugs (like the Windows one I was just debugging), or merely peeking a git blame inside my editor to help better understand certain unintuitive parts of the code. So linear history strives to make sure every commit has passed the tests, that similar commits are rebased into each other, and that purely cosmetic commits (like lint fixing) are amended into their parent commits.

Now whilst I may value such a practice, that doesn't mean that everybody should. We should have the freedom to follow our own approaches in our own PRs.

BTW I think another approach to bear in mind could be using some sort of staging branch, named staging, dev, next or some such. I could have targeted this PR at that branch rather than main. Which would have given us the freedom to share the code without having to pollute main with broken code.

[schell]

Don't worry, you got your point across, and I agree. I was recently contributing to a project and in that project, committing to the PR branch directly was their practice and I was surprised it worked. I decided I'd give it a try. Really I just wish I could make a PR into your PR without having to fork your fork. It would be nice to keep all the discussion here instead of another related PR 🤷 .

[schell]

Great work! Thank you :)