Skip to content

Don't enable euclid's std/libm features#481

Closed
DJMcNab wants to merge 2 commits intolinebender:mainfrom
DJMcNab:euclid-features
Closed

Don't enable euclid's std/libm features#481
DJMcNab wants to merge 2 commits intolinebender:mainfrom
DJMcNab:euclid-features

Conversation

@DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Aug 11, 2025

The euclid feature is only intended for conversions. Therefore, if our users are using it, they should be enabling the correct features themselves.

Related to #463

This has the side-effect of working around rust-lang/cargo#10801 (but that is not the motivation for this change)

The `euclid` feature is only intended for conversions (see linebender#463)
Therefore, if our users are using it, they should be enabling the correct features themselves

Related to linebender#463

This has the side-effect of working around rust-lang/cargo#10801
(but that is not the motivation for this change)
jneem
jneem previously approved these changes Aug 11, 2025
@jneem jneem dismissed their stale review August 11, 2025 16:20

Actually, I'm not sure about this since both kurbo and euclid need either std or libm to compile. If we don't forward kurbo's choice to euclid, we force them to specify both...

@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 11, 2025

we force them to specify both...

That is exactly the point. We are not meaningfully providing access to euclid; we don't re-export it for example. The feature only exists for interop between Kurbo and Euclid.
If one of our end-users, as a client of euclid, does not specify either the std or libm feature on euclid, their dependency specification on euclid is wrong, and we should not be masking that.

Because as soon as they removed their dependency on Kurbo (or one of their users tried to use a feature combination where they didn't depend on Kurbo), their program would fail to compile.

Now separately, the impact this has on CI is extremely painful (because of rust-lang/cargo#11467). And so that would be a reason not to do this, unless @xStrom has some bright idea here (other than the __private_for_ci_unstable_euclid_std feature, which could be viable).

@sagudev
Copy link
Contributor

sagudev commented Aug 11, 2025

For the CI I think we should just ignore euclid and do separate step that checks euclid with proper features.

Because as soon as they removed their dependency on Kurbo (or one of their users tried to use a feature combination where they didn't depend on Kurbo), their program would fail to compile.

We have a lot of that in servo.

@xStrom
Copy link
Member

xStrom commented Aug 13, 2025

I understand and sympathize with the point that as Kurbo's use of euclid doesn't care whether std or libm is enabled, that decision should not be influenced by Kurbo. The leaner Cargo.lock is a nice side-effect too.

However, I think the practical cost of this change is too high. The CI impact hints at it, but isn't my main concern. Instead, it is the breakage of simply doing cargo clippy --all-features. I think that being able to use --all-features when developing Kurbo (or anything else for that matter) is a massive ergonomics win. In CI we can do all sorts of hacks and feature mixes, but we don't want to require using those hacks for development. This would be an especially major friction point for new contributors.

Also, I would assume that in practice Kurbo enabling std / libm for euclid doesn't cause any issues. The impacted scenario would be where someone depends on kurbo/std but does not want euclid/std. The libm case shouldn't be an issue, as std usually overrides libm when both are requested. I imagine that in almost every practical case the choice of std vs libm is made once at the top level and having that propagate is exactly what is desired.

Daniel, perhaps you have some additional practical motivations for doing this change? That would be very interesting!

However, if it is mostly about the conceptual question of Kurbo not influencing the choice, then I'd advocate for us living with that sin and enjoying the ergonomical benefits of --all-features just working.


PS. This whole mess really should be solved by Cargo so that we wouldn't need a libm feature at all. It's a source of several pains.

@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 13, 2025

Daniel, perhaps you have some additional practical motivations for doing this change? That would be very interesting!

I was planning on documenting the Cargo features which Kurbo has (because our current doc is incomplete), and so when I coincidentally ran into this from #kurbo > ✔ `euclid` in kurbo 0.11.3, I thought I'd give it a go.

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