Add stderr and termcolor features to Naga

[bushrat011899]

Connections

• Contributes to Support no_std where possible (specifically for wasm32v1-none) #6826

Description
To add eventual no_std support, termcolor must be an optional dependency. It is currently only required by codespan-reporting when its termcolor feature is enabled. This PR adds a similar termcolor feature to naga, which is enabled in naga-cli, wgpu, and wgpu-hal. Additionally, a stderr feature was added to allow enabling the std crate within naga for the purposes of using std::io::stderr directly when termcolor cannot be used. This is currently only enabled in naga-cli since the public API gated by this feature is unused in the rest of WGPU.

Testing

Compiled with and without termcolor feature enabled.

Squash or Rebase?

Squash

Checklist

• Run cargo fmt.
• Run taplo format.
• Run cargo clippy --tests. If applicable, add:
  • --target wasm32-unknown-unknown
• Run cargo xtask test to run tests.
• If this contains user-facing changes, add a CHANGELOG.md entry.

[ErichDonGubler]

Sorry for taking so long on this one! I'd put it off because I strongly suspected there was an easier way to do this without duplicating logic. Finally found time to investigate a bit, and it looks like I'm right. I added 1f0c355 to put the tip of this PR in the state I think it should be. Whaddaya think?

If that looks good to you, then I'm happy to rope in another maintainer for a quick rubber stamp. 🙂

[bushrat011899]

Sorry for taking so long on this one! I'd put it off because I strongly suspected there was an easier way to do this without duplicating logic.

No need to apologize! Thanks for taking your time on this!

Finally found time to investigate a bit, and it looks like I'm right. I added 1f0c355 to put the tip of this PR in the state I think it should be. Whaddaya think?

I like it! My only concern is something I regret in the upstream change I made to codespan-reporting where if naga's termcolor/stderr features get out of sync with codespan-reporting's termcolor/std features, then a compile failure could occur. It's easily solved by end users by just enabling the appropriate features, but it's something I'd like to address upstream to make using the crate easier.

If that looks good to you, then I'm happy to rope in another maintainer for a quick rubber stamp. 🙂

Just making a slight change to use impl T instead of a mixture of dyn T and impl T but otherwise all good from me!

[ErichDonGubler]

@gfx-rs/wgpu: Would appreciate a second set of eyes for the overall diff to make sure somebody else is happy with the work we're doing here. Otherwise, I intend to merge in the next couple of days. 😀

[cwfitzgerald]

Small notes

[bushrat011899]

I believe the test failure is spurious.

[ErichDonGubler]

issue(non-blocking): I'm concerned that the code being changed here, though historically changed only infrequently, might become brittle in an unpleasant way for future maintenance; the amount of cfg we're doing here suggests we'll need to be careful around it.

suggestion(if-minor): Could we add some CI for testing this particular 2x2 matrix of termcolor and stderr being enabled? I suppose this could be follow-up work, given the amount of the work we've already done here.

Sorry for the belated feedback here; I know we're late in this process.

CC @cwfitzgerald.

[bushrat011899]

I agree feature coverage is a tricky problem to manage. Looking at the current CI:

• wgpu requires termcolor, so termcolor && !stderr is covered
• no_std wont be compatible with either feature, so in the near-term there will be a no_std test that will cover !termcolor && !stderr
• CI already checks for all features, so that covers termcolor && stderr

That leaves !termcolor && stderr as the untested configuration that would need to be explicitly added to CI.

I would suggest a follow-up PR which adds a CI task specifically to test stagnation-risk feature configurations. I imagine it would use a matrix of package and features, and run those against Clippy.

# check-stagnation.yml
jobs:
  check-stagnation:
    strategy:
      fail-fast: false
      matrix:
        include:
          - name: Windows x86_64
            package: naga
            features: "stderr"
    steps:
      # ...
      - name: Clippy
        shell: bash
        run: |
          set -e
          cargo clippy -p ${{ matrix.package}} --features ${{ matrix.features }} --no-default-features

This would allow for a single place for all "stagnation-risk" combinations to live in CI, making it clear these are supported combinations, but might be worth refactoring in the future since they aren't actively used internally.

[cwfitzgerald]

I think having a platform independent CI check is a good idea as it could be pretty quick. Yeah I think that would be fine, but I worry that this is a patch over the bigger problem of how many features we have and testing that more generically

[cwfitzgerald]

Lets definitely not block this on it though

[bushrat011899]

[...] I worry that this is a patch over the bigger problem of how many features we have and testing that more generically

Yeah we're tackling that very issue in Bevy right now too. You might be interested in looking at bevyengine/bevy#18822. I'm hoping to remove a lot of feature complexity from Bevy using it, and it gives us cfg_aliases without a build.rs, which is a nice bonus.

[cwfitzgerald]

Interesting! I worry about how well RA will tolerate working inside a macro

[cwfitzgerald]

Nice!