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

Roll back once_cell to 1.19.0. #6370

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Oct 4, 2024

Roll back wgpu's dependencies on once_cell from 1.20.1 to 1.19.0.

Version 1.20.1 of once_cell added a more complex conditional dependency on portable-atomic, which causes cargo metadata to incorrectly list portable-atomic as a dependency even though the given once_cell features are not enabled.

The Firefox source tree uses cargo vet to enforce supply-chain auditing. Since cargo vet depends on cargo metadata to tell it what crates are going to be included in the tree, the extraneous dependency above adds portable-atomic to the set of sources we must audit. Since portable-atomic is roughly [edit] 28kloc, we would like to avoid this.

Nothing in wgpu actually needs once_cell 1.20; it was upgraded by Dependabot. So the simplest workaround for the moment is to roll back the version.

Roll back `wgpu`'s dependencies on `once_cell` from 1.20.1 to 1.19.0.

Version 1.20.1 of `once_cell` added a more complex conditional
dependency on `portable-atomic`, which causes `cargo metadata` to
incorrectly list `portable-atomic` as a dependency even though the
given `once_cell` features are not enabled.

The Firefox source tree uses `cargo vet` to enforce supply-chain
auditing. Since `cargo vet` depends on `cargo metadata` to tell it
what crates are going to be included in the tree, the extraneous
dependency above adds `portable-atomic` to the set of sources we must
audit. Since `portable-atomic` is roughly 50kloc, we would like to
avoid this.

Nothing in `wgpu` actually needs `once_cell` 1.20; it was upgraded by
Dependabot. So the simplest workaround for the moment is to roll back
the version.
@jimblandy jimblandy requested a review from a team as a code owner October 4, 2024 17:09
@ErichDonGubler
Copy link
Member

The offending Cargo bug: rust-lang/cargo#10801

Recognition of this bug from once_cell upstream: matklad/once_cell#265 (comment)

Complaint from myself about the above: rust-lang/cargo#10801 (comment)

@ErichDonGubler ErichDonGubler enabled auto-merge (rebase) October 4, 2024 17:15
@ErichDonGubler ErichDonGubler merged commit ee0d170 into gfx-rs:trunk Oct 4, 2024
27 checks passed
@jimblandy jimblandy deleted the roll-back-once-cell-1.19.0 branch October 4, 2024 18:22
@taiki-e
Copy link

taiki-e commented Oct 4, 2024

I have sent a PR to once_cell to work around the cargo bug: matklad/once_cell#267

roughly 50kloc

Well, at least more than half of them are tests (precisely its helper, perhaps should be extracted to another repository EDIT: I moved it into its own repository).

$ tokei tests                               
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 Language              Files        Lines         Code     Comments       Blanks
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 LD Script                 1          102           71           14           17
 TOML                      7          204          158            8           38
─────────────────────────────────────────────────────────────────────────────────
 Rust                    240        31641        30151          977          513
 |- Markdown               1            9            0            7            2
 (Total)                            31650        30151          984          515
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 Total                   248        31956        30380         1006          570
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

$ tokei tests/helper 
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 Language              Files        Lines         Code     Comments       Blanks
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 TOML                      1           30           24            0            6
─────────────────────────────────────────────────────────────────────────────────
 Rust                    233        30257        28848          953          456
 |- Markdown               1            9            0            7            2
 (Total)                            30266        28848          960          458
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 Total                   234        30296        28872          960          464
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

@jimblandy
Copy link
Member Author

I have sent a PR to once_cell to work around the cargo bug: matklad/once_cell#267

@taiki-e Thank you very much for this - it's greatly appreciated!

As it turns out, we are using once_cell functions that are present in std but still experimental, so we need to keep the dependency on once_cell in some form or another.

I don't think there's anything wrong with portable-atomic spending lines to do what it needs to do. It's difficult for Firefox to take on new dependencies even when their engineering is good. But auditing 28kloc is still a substantial chunk of time.

@jimblandy
Copy link
Member Author

For the future: it seems like with matklad/once_cell#267, released as 1.20.2, we can probably just accept another once_cell upgrade if offered/needed.

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