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

Don't enable serde's std feature #108

Merged

Conversation

waywardmonkeys
Copy link
Collaborator

When we use the serde?/std syntax for the feature, we end up with a public dependency on serde even when it isn't enabled or used. It isn't compiled, but it feels wrong. We don't need the std feature from serde, so no need to enable it and someone that does will already have it enabled.

When we use the `serde?/std` syntax for the feature, we end up
with a public dependency on `serde` even when it isn't enabled
or used. It isn't compiled, but it feels wrong. We don't need
the `std` feature from `serde`, so no need to enable it and
someone that does will already have it enabled.
@waywardmonkeys waywardmonkeys force-pushed the remove-serde-std-enablement branch from 1eaddb7 to 02c7fdd Compare December 21, 2024 17:57
@tomcur
Copy link
Member

tomcur commented Dec 21, 2024

Interesting. In the case we did need Serde's std feature if Color's std was enabled, how would that be specified while bringing in Serde only with the serde feature enabled? I thought that was the intention behind this syntax.

@waywardmonkeys
Copy link
Collaborator Author

Not sure, actually! I wish we didn't have this feature at all, but we need it for Peniko for now (optional there too).

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Dec 21, 2024
Merged via the queue into linebender:main with commit e203342 Dec 21, 2024
16 checks passed
@waywardmonkeys waywardmonkeys deleted the remove-serde-std-enablement branch December 21, 2024 19:57
@waywardmonkeys waywardmonkeys added this to the 0.2.1 milestone Dec 21, 2024
@xStrom
Copy link
Member

xStrom commented Dec 22, 2024

we end up with a public dependency on serde

What do you mean by this?

I don't see serde with cargo tree.

# cargo tree -p color -F std
color v0.2.0

The serde dependency is also clearly marked as optional on crates.io.

Where are you seeing serde show up without specifying the serde feature?

@waywardmonkeys
Copy link
Collaborator Author

It shows up in the Cargo.lock and here:

svgtypes on  main [$] is 󰏗 v0.15.2 via  v1.83.0
❯ cargo add color
    Updating crates.io index
      Adding color v0.2.0 to dependencies
             Features:
             + std
             - bytemuck
             - libm
             - serde
    Updating crates.io index
     Locking 7 packages to latest compatible versions
      Adding color v0.2.0
      Adding proc-macro2 v1.0.92
      Adding quote v1.0.37
      Adding serde v1.0.216
      Adding serde_derive v1.0.216
      Adding syn v2.0.91
      Adding unicode-ident v1.0.14

It doesn't build, but is a bad look for when we're trying to make sure that we aren't using that stuff at all.

@xStrom
Copy link
Member

xStrom commented Dec 22, 2024

That's interesting and frankly seems like a bug in Cargo. I might try looking more into it later, but for Color I guess it's fine to just not specify the std of serde for now.

@xStrom
Copy link
Member

xStrom commented Dec 22, 2024

It is indeed Cargo's fault, and the issue is tracked in cargo#10801.

The issue comes from the fact that apparently Cargo has two different resolvers. One old dependency resolver, which is used to generate Cargo.lock and download dependencies. That resolver has no idea about features or anything like that. Then there is a second resolver, the feature resolver, which will use the versions from Cargo.lock and determine which actual dependencies to use and which features to enable.

The dependency resolver will generate a union of dependencies that could be used under the assumption that every platform is targeted and all features are enabled. So we had serde pop up, but others are seeing way more extreme situations, e.g. getrandom seeing 33 packages being added to the list.

The good news is that there are plenty of people who find this situation annoying and are complaining. The bad news is that a proposed potential solution includes a complete redesign of the Cargo dependency resolver and as of right now there isn't anyone doing that.

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