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

Remove "portable-atomic?/require-cas" from race feature #267

Merged
merged 1 commit into from
Oct 5, 2024

Conversation

taiki-e
Copy link
Contributor

@taiki-e taiki-e commented Oct 4, 2024

This removes "portable-atomic?/require-cas" from race feature to work around Cargo bug (rust-lang/cargo#10801).

Instead, this increases the minimal version of portable-atomic to 1.8 (which includes diagnostic improvements by taiki-e/portable-atomic#181) so that it can keep good diagnostics with compilers at least Rust 1.78 (2024-05) or later.

More context: #265 (comment)
This is definitely a Cargo bug, but the fix does not seem to be easy: rust-lang/cargo#11938
In my understanding, the only users who would be affected by this workaround are users of older compilers, so I think it is acceptable.

@@ -21,7 +21,7 @@ members = ["xtask"]

[dependencies]
parking_lot_core = { version = "0.9.10", optional = true, default-features = false }
portable-atomic = { version = "1.7", optional = true, default-features = false }
portable-atomic = { version = "1.8", optional = true, default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

Let’s maybe keep it at 1.7? The users are going to get 1.8 or later by default anyway, but it’s nice to keep an option for users with older compilers to use the older version.

(note: as this is an optional dep, it is out of scope for the declared MSRV policy, so not actually against bumping msrv with portable atomic on. But if we can avoid that, that would be a kind thing to do for some subset of the users)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

portable-atomic's MSRV has always been Rust 1.34, whether 1.7, 1.8 or the current latest version (1.9).

The use of new compiler features to improve diagnostics has been done in a way that does not break compatibility with older compilers.

@jimblandy
Copy link

jimblandy commented Oct 5, 2024

A bit of context from a user, about why I'd like to see this PR merged:

The cargo bug has been there since 2022, with a lot of interest from affected people and not much progress. So I'm not optimistic it'll get fixed soon.

At Mozilla, the cargo bug has caused us ongoing trouble over the years: Firefox uses cargo vet and cargo vendor, which means that every dependency reported by cargo metadata needs to be hand-audited, and then its code is copied into our tree. This means that taking on an unneeded dependency is something we really try to avoid. I don't want to audit ~50kloc of portable-atomic if we're not even going to compile it.

Of course, we have workarounds for silly bureaucratic cases like this. We are already patching several spurious dependencies (due to targets we don't need) to empty crates, for example. In other cases we have use local fork of a crate. But it all adds to the load of kludges we have to carry forward.

@matklad matklad merged commit 0aef2f8 into matklad:master Oct 5, 2024
1 check passed
@matklad
Copy link
Owner

matklad commented Oct 5, 2024

Released as https://crates.io/crates/once_cell/1.20.2

@taiki-e taiki-e deleted the portable-atomic branch October 5, 2024 16:22
@matklad
Copy link
Owner

matklad commented Oct 5, 2024

@jimblandy FWIW, if you are not using portable atomic, then probably you don't actually need once_cell crate. Most of the API is now available in std (but not in core), so I am thinking that the primary role of once_cell for the ecosystem right now is exactly to serve as a poly-fill for no-std use-cases.

Though I'd imagine removing once_cell from the entire dependency tree is going to be quite a mission at this point!

@jimblandy
Copy link

That's a good point. Yes, we largely have to take the ecosystem as we find it. But I'm going to go audit the code I'm actually responsible for's use of once_cell right now...

@taiki-e
Copy link
Contributor Author

taiki-e commented Oct 5, 2024

@jimblandy

which means that every dependency reported by cargo metadata needs to be hand-audited

We are already patching several spurious dependencies (due to targets we don't need) to empty crates, for example.

FWIW, as for unnecessary target-specific deps from cargo metadata, it could be avoided by passing --filter-platform option (like cargo-nextest/cargo-hack did).

I don't want to audit ~50kloc of portable-atomic if we're not even going to compile it.

As I mentioned in the wgpu PR, 50kloc is the amount of code in the entire repository and crate itself is less than half of that, so I feel that the process of trying to have the entire repository audited instead of the code published on crates.io is also problematic.

@jimblandy
Copy link

Just to avoid misunderstandings: I have no complaint about portable-atomic being 50kloc, or 500kloc, or 5kloc. Whatever it takes to get the job done usefully and responsibly is completely fine. I have no idea what's involved.

As it turns out, cargo vet does only consider the code published on crates.io. (I saw a line count of around 50k in cargo vet's estimate of the review burden, but that included code from other crates; then later I ran tokei on a checkout of portable-atomic and again saw 50k. So, I made two different mistakes that gave the same answer.) But the exact amount isn't really relevant to what I was trying to say. I would be just as reluctant to audit 25kloc of code we're not using.

FWIW, as for unnecessary target-specific deps from cargo metadata, it could be avoided by passing --filter-platform option (like cargo-nextest/cargo-hack did).

Thanks for the suggestion - this sounds like something we should try.

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