Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing binding in wgpu-core/src/instance.rs when profiling has a backend enabled #6422

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

cybersoulK
Copy link
Contributor

No description provided.

@cybersoulK cybersoulK requested a review from a team as a code owner October 17, 2024 22:28
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Oct 17, 2024

To my consternation, I can't reproduce this on my machine, and I suspect that that's why this was missed when @teoxoy introduced this in de97e54#diff-1b40237cc7b42c2140c93d8910af3cb74461441d611f1db3f69e81617229eccbR148 (see #6391) and @Wumpf reviewed.

The function's body from before seems obviously wrong; there's no backend variable actually bound at the point of the profiling::scope!(…) call using that identifier. The PR here seems like it should fix a compilation error by actually binding it, but…CI is failing (link) after linting the backend binding as unused, to top it all off.

We need clarity on why this compiles before, and doesn't seem to be used when supposedly fixed (and fails our CI), before we can proceed with merging this.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Whoops, forgot to do this. 😅

wgpu master doesn't compile, because of this typo
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Oct 17, 2024

Diagnosed the problem. When no features are enabled for profiling, its macros emit no code, which changes how unused code lints fire based entirely on what features get resolved for profiling. 😱 One can provoke the issue the OP encountered by enabling a profiling backend. A simple way to do this is to uncomment one of the features in benches/Cargo.toml:

--- a/benches/Cargo.toml
+++ b/benches/Cargo.toml
@@ -18,7 +18,7 @@
 
 [features]
 # Uncomment these features to enable tracy and superluminal profiling.
-# tracy = ["dep:tracy-client", "profiling/profile-with-tracy"]
+tracy = ["dep:tracy-client", "profiling/profile-with-tracy"]
 # superluminal = ["profiling/profile-with-superluminal"]
 
 [dependencies]

…and try to compile it:

$ cargo check --workspace --all-features

This is definitely an upstream bug that we need to work around for now. I've updated this patch to do just that.

This is also a CI problem, I think; we should be catching problems like this in our CI pipeline, and that we did not is unfortunate. CC @gfx-rs/wgpu for thoughts on this.

Going to merge the amended fix with haste. If others feel that this should be solved differently, I'm open to following up. I think it's worth keeping trunk compiling for everyone.

@ErichDonGubler ErichDonGubler changed the title wgpu trunk doesn't compile because of this typo in wgpu-core/src/instance.rs Fix missing binding in wgpu-core/src/instance.rs when profiling has a backend enabled Oct 18, 2024
@ErichDonGubler ErichDonGubler enabled auto-merge (squash) October 18, 2024 00:04
@ErichDonGubler ErichDonGubler merged commit a8214b6 into gfx-rs:trunk Oct 18, 2024
27 checks passed
@ErichDonGubler
Copy link
Member

ErichDonGubler commented Oct 18, 2024

Filed aclysma/profiling#87 with a fix for upstream. This does not fully address the CI concern I raised; we should be testing configurations of code that could break. I think the risk with profiling will be mitigated once my fix is upstreamed, though, and I'm not aware of other risks with builds. For now, I'll forgo filing an issue to add more build checks in CI.

@aclysma
Copy link

aclysma commented Oct 18, 2024

I recommend adding a build to CI that compiles with the type-check profiling backend enabled. Using this backend is basically equivalent to the PR being suggested. I was under the impression that this project was already using this backend in CI since it was actually suggested by kvark in 2021, presumably for this project. I added more detailed thoughts on the PR itself aclysma/profiling#87

@ErichDonGubler
Copy link
Member

Filed a follow-up issue for more systemically solving this via CI: #6462

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