glam dependency is not optional in spirv-std

Question

glam dependency is not optional in spirv-std

marstaik opened this issue a year ago · 14 comments

The readme in spirv-std states:

Optionally, through the use of the "glam" feature, it includes some boilerplate trait implementations to make glam vector types compatible with these APIs.

However, there is no feature flag that actually controls the glam bindings. As such, on a clean dependency with no features specified, spirv-std requires glam as a dependency to build. Also, the workspace level default-features=false does not seem to get used properly, making std included with glam.

I would definitely like to see this optional, and then perhaps introduce an nalgebra helper as well in the future.

--- stderr
      Blocking waiting for file lock on build directory
     Compiling core v0.0.0 (/home/marios/.rustup/toolchains/nightly-2024-04-24-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core)
     Compiling compiler_builtins v0.1.109
     Compiling serde_json v1.0.132
     Compiling smallvec v1.13.2
     Compiling spirv-tools-sys v0.8.0
     Compiling libm v0.2.11
     Compiling num-traits v0.2.19
     Compiling typenum v1.17.0
     Compiling spirv-tools v0.10.0
     Compiling spirt v0.4.0
     Compiling rustc_codegen_spirv-types v0.9.0 (https://github.com/Rust-GPU/rust-gpu.git#0da80f8a)
     Compiling rustc_codegen_spirv v0.9.0 (https://github.com/Rust-GPU/rust-gpu.git#0da80f8a)
     Compiling rustc-std-workspace-core v1.99.0 (/home/marios/.rustup/toolchains/nightly-2024-04-24-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/rustc-std-workspace-core)
     Compiling bytemuck v1.19.0
     Compiling spirv-std-types v0.9.0 (https://github.com/Rust-GPU/rust-gpu.git#0da80f8a)
     Compiling bitflags v1.3.2
     Compiling glam v0.24.2
  error[E0463]: can't find crate for `std`
    --> /home/marios/.cargo/registry/src/index.crates.io-6f17d22bba15001f/num-traits-0.2.19/src/lib.rs:23:1
     |
  23 | extern crate std;
     | ^^^^^^^^^^^^^^^^^ can't find crate
     |
     = note: the `spirv-unknown-vulkan1.2` target may not support the standard library
     = help: consider building the standard library from source with `cargo build -Zbuild-std`

  error[E0463]: can't find crate for `std`
    |
    = note: the `spirv-unknown-vulkan1.2` target may not support the standard library
    = note: `std` is required by `glam` because it does not declare `#![no_std]`
    = help: consider building the standard library from source with `cargo build -Zbuild-std`

Answer 1 · 2024-11-18T10:25:25.000Z

glam used to be an optional dependency. Back then the sampling textures function would return a vector type of your choosing, specified in the Image! macro, that could convert from an array of 4 floats (or less if you specified the exact image type). But by specifying the returned vector type in the Image! macro, it would specify it via a generic on Image making it incompatible with any other type wanting a different vector type to be returned. TLDR: a bit of a mess.

Some time ago it was decided to remove this feature of being able to return whatever vector type and sample() will just return glam's Vec4 (or Vec3, Vec2, float ...). And the end user should just use mint to convert it to whatever math library they're using.

If we really wanted to make glam optional again, these functions would have to return [f32; 4] which would make libraries using glam and not using glam incompatible, be a breaking change and it just adds more complexity for saving compilation time on a relatively small library.

Answer 2 · 2024-11-18T10:40:03.000Z

About num-traits failing to compile:
It seems like you have some (transient) dependency on num-traits that requires the std feature. Could run cargo tree -i num-traits -e features and try to see if there's some other library depending on the std or default feature?
Unfortunately, you cannot specify --target spirv-unknown-vulkan1.2 due to default cargo not knowing of this target. So if you're running on current master, you will see that spirv-std depending on the default feature of num-traits on CPU targets. But when compiling for spirv* it should instead disable default features and not actually depend on default / std.

For example, nalgebra with default depends on nalgebra with std depends on num-traits with std.

Answer 3 · 2024-11-18T23:07:04.000Z

We could try something like returning T: impl From<[f32; 4]>, but that would have implications for type inference.

Answer 4 · 2024-11-19T04:15:50.000Z

What if the Sampler types definitions or inner members where config guarded entirely? Instead of [f32;4], make the features like glam and nalgebra mutually exclusive, and config guard the types depending on the feature used

Answer 5 · 2024-11-19T09:56:37.000Z

Let's say we were to make those features. Then if you'd made a library that implements some technique, you need to choose between the glam or nalgebra feature to be able to sample textures at all, and you will be incompatible with every library using the other. That's effectively splitting the ecosystem in two camps, not happening.
Then I'd rather force-feed glam to everyone and tell them to convert it to the vector library of their choice. (And we noticed most often they choose glam)

Answer 6 · 2024-11-19T16:44:40.000Z

What would be the effective difference if glam remains the default enabled feature though? Glam would still be force fed with an option to change vector libraries

Answer 7 · 2024-11-21T05:18:15.000Z

About num-traits failing to compile: It seems like you have some (transient) dependency on num-traits that requires the std feature. Could run cargo tree -i num-traits -e features and try to see if there's some other library depending on the std or default feature? Unfortunately, you cannot specify --target spirv-unknown-vulkan1.2 due to default cargo not knowing of this target. So if you're running on current master, you will see that spirv-std depending on the default feature of num-traits on CPU targets. But when compiling for spirv* it should instead disable default features and not actually depend on default / std.

For example, nalgebra with default depends on nalgebra with std depends on num-traits with std.

num-traits v0.2.19
├── approx v0.5.1
│   ├── nalgebra v0.33.2
│   │   ├── nalgebra feature "bytemuck"
│   │   │   ├── shaders v0.0.0 (/home/marios/proj/hestia_vk/packages/shaders)
│   │   │   │   └── shaders feature "default" (command-line)
│   │   │   └── vulkan v0.0.0 (/home/marios/proj/hestia_vk/packages/vulkan)
│   │   │       └── vulkan feature "default" (command-line)
│   │   │           └── hestia v0.0.0 (/home/marios/proj/hestia_vk/packages/hestia)
│   │   │               └── hestia feature "default" (command-line)
│   │   ├── nalgebra feature "macros"
│   │   │   ├── shaders v0.0.0 (/home/marios/proj/hestia_vk/packages/shaders) (*)
│   │   │   └── vulkan v0.0.0 (/home/marios/proj/hestia_vk/packages/vulkan) (*)
│   │   └── nalgebra feature "nalgebra-macros"
│   │       └── nalgebra feature "macros" (*)
│   └── simba v0.9.0
│       └── nalgebra v0.33.2 (*)
├── nalgebra v0.33.2 (*)
└── simba v0.9.0 (*)
├── num-traits feature "default"
│   ├── rustc_codegen_spirv v0.9.0 (https://github.com/Rust-GPU/rust-gpu.git#d5d347b0)
│   │   └── spirv-builder v0.9.0 (https://github.com/Rust-GPU/rust-gpu.git#d5d347b0)
│   │       ├── spirv-builder feature "default"
│   │       │   [build-dependencies]
│   │       │   └── shaders v0.0.0 (/home/marios/proj/hestia_vk/packages/shaders) (*)
│   │       └── spirv-builder feature "use-compiled-tools"
│   │           └── spirv-builder feature "default" (*)
│   │   └── rustc_codegen_spirv feature "use-compiled-tools"
│   │       └── spirv-builder feature "use-compiled-tools" (*)
│   └── spirv-std v0.9.0 (https://github.com/Rust-GPU/rust-gpu.git#d5d347b0)
│       └── spirv-std feature "default"
│           └── shaders v0.0.0 (/home/marios/proj/hestia_vk/packages/shaders) (*)
├── num-traits feature "i128"
│   ├── num-complex v0.4.6
│   │   ├── nalgebra v0.33.2 (*)
│   │   └── simba v0.9.0 (*)
│   ├── num-integer v0.1.46
│   │   └── num-integer feature "i128"
│   │       └── num-rational v0.4.2
│   │           └── nalgebra v0.33.2 (*)
│   └── num-rational v0.4.2 (*)
└── num-traits feature "std"
    └── num-traits feature "default" (*)

Seems like the only one requiring num-traits "default", and thus "std" is rust-gpu itself (if I am reading that correctly)

Answer 8 · 2024-11-21T05:55:57.000Z

I've uploaded a minimum reproduction here: https://github.com/marstaik/rust-gpu-repro

Answer 9 · 2024-11-21T11:31:02.000Z

Perhaps if we stick to the mint traits the compiler could transparently swap out the actual implementation at compile time? Similar to how when you open a file the compiler does different work on different platforms with different deps.

I could see a world where certain cards need certain specialized representations, especially TPUs and NPUs.

Answer 10 · 2024-11-21T16:55:12.000Z

Found your issue:

Your declared the resolver to be version 1 but must be version 2 for rust-gpu to work properly (in your root Cargo.toml). Version 1 likes to "merge" features a bit too aggressively and is causing num-traits to get the std feature, even though it shouldn't.
If you fix that, you'll notice that building the shader will call the build.rs script which will build the shader... recursively. Instead of fixing this, I'd rather recommend moving the build script to the crate that consumes the shader binaries, this way you aren't wasting compile time building and running the build script again for the spirv target. (Due to spirv target being build in another target dir it will not reuse artifacts.)

I've made a repo detailing exactly how to setup rust-gpu with vulkano, feel free to take a look, even if you're graphics API is not vulkano it should be similar enough. We've already discussed internally that we need such an example repo for each major graphics API (wgpu, ash) and hopefully we'll get them ready before the next release.

Answer 11 · 2024-11-22T01:47:46.000Z

Thanks @Firestar99 , in addition to those two steps, I had to look at the example to enable some things like

.multimodule(true)
.spirv_metadata(SpirvMetadata::NameVariables)
.print_metadata(MetadataPrintout::DependencyOnly)
.build()?;

despite not using vulkano, (I am using ash directly). But It does all seem to build now. I was getting errors from the spriv-tools it seems:

error: No OpEntryPoint instruction was found. This is only allowed if the Linkage capability is being used.
    |
    = note: module `/home/marios/proj/hestia_vk/target/spirv-builder/spirv-unknown-vulkan1.2/release/deps/shaders.spv`

  warning: an unknown error occurred
    |
    = note: spirv-opt failed, leaving as unoptimized
    = note: module `/home/marios/proj/hestia_vk/target/spirv-builder/spirv-unknown-vulkan1.2/release/deps/shaders.spv`

  error: error:0:0 - No OpEntryPoint instruction was found. This is only allowed if the Linkage capability is being used.
    |
    = note: spirv-val failed
    = note: module `/home/marios/proj/hestia_vk/target/spirv-builder/spirv-unknown-vulkan1.2/release/deps/shaders.spv`

Back to the original topic, @LegNeato , using the mint trait would allow you to template/generic the Samplers by your own Vector type, correct?

So in user code we would probably have to do a type declaration like so:

// nalegbra
pub type SamplerNa = SamplerImpl<Vector3<f32>>;

pub type SamplerGlam = SamplerImpl<Vec3>;

If that works, I don't mind putting some time into getting this working.

Answer 12 · 2024-11-22T12:33:13.000Z

I knew there was another issue I found, but forgot when I started writing the list. All shader entry points must be accessible as pub, aka. both the entry point as well as the entire mod path all need to be pub. In your example your mod example is not :D
(we should really add a lint for that)

Answer 13 · 2024-11-23T12:20:49.000Z

The pub stuff is EmbarkStudios/rust-gpu#1140

Answer 14 · 2024-11-23T12:22:39.000Z

@marstaik yeah, that's my thinking. I'm not experienced enough with this area to know if that's a good idea or not.