Skip to content

Conversation

@Firestar99
Copy link
Member

@Firestar99 Firestar99 commented Oct 17, 2025

@Firestar99 Firestar99 requested review from schell and tombh October 17, 2025 12:59
selecting the correct glam version manually when testing various spirv-std versions is hard
otherwise I can't build it
@Firestar99
Copy link
Member Author

Firestar99 commented Oct 17, 2025

The template's spirv-std update included Rust-GPU/rust-gpu#380, which forces glam to 0.30.8+. But specifying that glam version breaks all older rustc_codegen_spirv versions, see 0.8.0 and 0.9.0 failing. So I switched to using the reexported glam from spirv-std.

@Firestar99 Firestar99 changed the title Nextest support Nextest support and update template Oct 17, 2025
Copy link
Collaborator

@tombh tombh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our main::cache_dir() function already has a clause for tests:

    Ok(if cfg!(test) {
        let thread_id = std::thread::current().id();
        let id = format!("{thread_id:?}").replace('(', "-").replace(')', "");
        dir.join("tests").join(id)
    } else {
        dir
    })

So either we need to delete that or, what about if we pass the function the tempdirpath, like cache_dir(Option<PathBuf>)? I think it'd be nice to still be getting test coverage for that function.

But now I'm wondering about tempdir's default use of Drop to automate the directory removal. What if shader_crate_test_path() returned tempfile::TempDir? Would that mean we don't need the thread-local TEMPDIR nor even the tests_teardown?

@tuguzT
Copy link
Contributor

tuguzT commented Oct 29, 2025

Our main::cache_dir() function already has a clause for tests:

    Ok(if cfg!(test) {
        let thread_id = std::thread::current().id();
        let id = format!("{thread_id:?}").replace('(', "-").replace(')', "");
        dir.join("tests").join(id)
    } else {
        dir
    })

The main reason behind removing this condition and replacing it with tempfile::Tempdir inside of #117 is to be able to move this function out of CLI crate, because cfg!(test) is not portable across crates.

This is because of the following neat reason: when the parent crate has this cfg set in its own tests, child crate (which is dependent on the parent) does not have this cfg set, and so cache dir is always the same for all its tests (which is just dir).

So either we need to delete that or, what about if we pass the function the tempdirpath, like cache_dir(Option)? I think it'd be nice to still be getting test coverage for that function.

I really hope this hack (due to the lack of tempfile at the moment of writing this?) would be removed.
If this is really needed to create child directory inside of cache one, one could use TempDir::with_prefix_in.

Also we could provide an optional path argument for cargo-gpu command (or some builder as in my PR) to specify where to cache codegen backend files.

But now I'm wondering about tempdir's default use of Drop to automate the directory removal. What if shader_crate_test_path() returned tempfile::TempDir? Would that mean we don't need the thread-local TEMPDIR nor even the tests_teardown?

I haven't thought about that in my PR: I've just tried to preserve testing logic as much as possible (at that moment).
I think this could be beneficial to just return tempfile::TempDir (and maybe close it manually).

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.

4 participants