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

Make loom dependency optional #666

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Make loom dependency optional #666

merged 1 commit into from
Feb 23, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Feb 23, 2021

The loom dependency will never compile unless users enable cfg, but the way cargo and crates.io handle cfg-ed dependencies is not very good and confuses users.

This issue seems to work around by the way proposed by @matklad in #487 (comment).
So this PR uses that workaround.

Closes #665

r? @jeehoonkang @jonhoo

@taiki-e
Copy link
Member Author

taiki-e commented Feb 23, 2021

bors d=jonhoo

@bors
Copy link
Contributor

bors bot commented Feb 23, 2021

✌️ jonhoo can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 23, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 23, 2021

Build succeeded:

@bors bors bot merged commit f8c9922 into master Feb 23, 2021
@bors bors bot deleted the loom branch February 23, 2021 18:56
@taiki-e
Copy link
Member Author

taiki-e commented Feb 25, 2021

@matklad @djc: Do you need me to do a new release?

@matklad
Copy link
Contributor

matklad commented Feb 25, 2021

I would appreciate one!

bors bot added a commit that referenced this pull request Feb 25, 2021
667: Prepare for the next release r=taiki-e a=taiki-e

Changes:

- crossbeam-epoch 0.9.2 -> 0.9.3
  - Make `loom` dependency optional. (#666)
- crossbeam-utils 0.8.2 -> 0.8.3
  - Make `loom` dependency optional. (#666)

Co-authored-by: Taiki Endo <[email protected]>
@taiki-e
Copy link
Member Author

taiki-e commented Feb 25, 2021

Published in crossbeam-utils 0.8.3 & crossbeam-epoch 0.9.3.

@djc
Copy link

djc commented Feb 25, 2021

@taiki-e thanks for getting this resolved so quickly!

@faern
Copy link

faern commented Jun 10, 2024

Do you people remember why you put loom both behind a cfg and made it optional? I see you still have this solution in place. I faced the same issue as this PR solves, but I found no downside of only making it an optional dependency: faern/oneshot#43. I'm trying to figure out if I missed some aspect of this. I have not published my crate yet since making the change, so would love to figure out any issues first :P

@taiki-e
Copy link
Member Author

taiki-e commented Jun 11, 2024

@faern See #487 (comment) (and subsequent 2 comments) and #638 for related discussion. (TL;DR that is not compliant with Cargo's recommendations)

@faern
Copy link

faern commented Jun 11, 2024

I have read those, and it did not shed any light on it for me. I'm not really asking why it's optional, rather why it's behind cfg(loom). I know the feature goes against the rule that features should be additive. Also internals for testing should not be exposed with features.

But changing from [dependencies] loom = ... to [target.'cfg(loom)'.dependencies] loom = ... does not help from what I can tell. The loom feature is still listed in cargo metadata. And my crate is built with the loom feature on if I do cargo build --all-features. The only difference is that if I guard loom both behind being optional and [target.'cfg(loom)'.dependencies] is that if a downstream user builds with the loom feature enabled, they still get standard functionality (not loom)... But that's ugly. Having them build with loom is also ugly, but yeah.

Is your motivation that by making it a cfg, even if it is exposed as a feature, enabling it does nothing for downstream?

EDIT: Cargo is indeed not really equipped to handle this well. But yeah, your solution is probably the least bad :) I move towards that also: faern/oneshot#45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Hide loom from dependency graph
5 participants