Skip to content

Conversation

@janvarga
Copy link
Contributor

@janvarga janvarga commented Nov 26, 2025

The scripts_traits crate was the only crate depending on strum with the
derive feature. This accidentally allowed other crates to import strum
macros via strum:: without declaring their own dependency on strum_macros,
causing compilation issues when running ./mach test-unit -p net.

This PR makes the imports consistent across the code base by:

  • replacing all strum_macro:: imports with strum:: imports
  • removing strum_macro dependencies
  • adding derive feature to the strum workspace

Testing: Unit tests continue to pass

@janvarga janvarga marked this pull request as ready for review November 26, 2025 20:42
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 26, 2025
@sagudev
Copy link
Member

sagudev commented Nov 26, 2025

Wouldn't it make more sense to simply add derive feature to strum in workspace toml, as I think nearly all uses of strum uses the macros?

@mrobinson
Copy link
Member

Would it also be possible to move the derive feature to the root Cargo.toml to avoid this issue?

@janvarga
Copy link
Contributor Author

janvarga commented Nov 26, 2025

I usually prefer to be explicit about things like this (explicit deps, etc), but currently anything which fixes ./mach test-unit -p net works for me :)

@janvarga
Copy link
Contributor Author

Let me try it the other way around.

@janvarga
Copy link
Contributor Author

@janvarga janvarga marked this pull request as draft November 26, 2025 21:05
The `scripts_traits` crate was the only crate depending on `strum` with the
`derive` feature. This accidentally allowed other crates to import strum
macros via `strum::` without declaring their own dependency on `strum_macros`,
causing compilation issues when running `./mach test-unit -p net`.

This patch makes the imporys consistent across the codebase by:
- replacing all `strum_macro::` imports with `strum::` imports
- removing strum_macro dependencies
- adding derive feature to the strum workspace

Signed-off-by: Jan Varga <jvarga@igalia.com>
@janvarga janvarga changed the title Fix inconsistent strum macro imports and missing strum_macros dependencies Fix inconsistent strum dependencies and imports Nov 26, 2025
@janvarga
Copy link
Contributor Author

Ok, I reworked it as suggested.

@janvarga janvarga marked this pull request as ready for review November 26, 2025 21:19
@janvarga janvarga requested a review from gterzian as a code owner November 26, 2025 21:19
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Thanks. This is a nice cleanup.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 26, 2025
@mrobinson mrobinson added this pull request to the merge queue Nov 26, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 26, 2025
Merged via the queue into servo:main with commit 0899b87 Nov 26, 2025
31 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 26, 2025
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