Skip to content

Conversation

@schell
Copy link
Collaborator

@schell schell commented Dec 20, 2024

NOT TO MERGE

This PR is just so I can thrash against CI for windows builds while making additions to

tombh and others added 9 commits December 17, 2024 13:11
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`.
@schell schell closed this Dec 20, 2024
@schell schell reopened this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants