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

Update toml dependency to latest version #2044

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgeisler
Copy link
Contributor

@mgeisler mgeisler commented Mar 21, 2024

I am in the process of vendoring UniFFI in AOSP, which means that I have to make it play nice with the existing crates found here:

https://cs.android.com/android/platform/superproject/+/main:external/rust/crates/

Android tries to only vendor each crate in a single version, so it would be helpful for me if the toml dependency could be updated.

This updates the “Requires” version to 1.70. I believe this is okay since this version is below 1.74 which is what mozilla-central currently requires.

@mgeisler mgeisler marked this pull request as ready for review March 21, 2024 11:46
@mgeisler mgeisler requested a review from a team as a code owner March 21, 2024 11:46
@mgeisler mgeisler requested review from badboy and removed request for a team March 21, 2024 11:46
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Unfortunately this causes problems for mozilla-central, so we need to hold off on this for at least the next release.

@mgeisler
Copy link
Contributor Author

Unfortunately this causes problems for mozilla-central, so we need to hold off on this for at least the next release.

Ah, thanks for looking! I tried to read rust-versions.md and it points to util.py as the definition of the version mozilla-central supports. Since this is 1.74, I was hoping it would be okay to bump up the version used in CI here?

There's probably something I'm missing?

I am in the process of vendoring UniFFI in AOSP, which means that I
have to make it play nice with the existing crates found here:

  https://cs.android.com/android/platform/superproject/+/main:external/rust/crates/

Android tries to only vendor each crate in a single version, so it
would be helpful for me if the toml dependency could be updated.

This updates the “Requires” version to 1.67. I believe this is okay
since this version is below 1.74 which is what mozilla-central
currently requires.
@mhammond
Copy link
Member

I think the problem is toml - ie, https://searchfox.org/mozilla-central/rev/f63ca2952da98e0817bdae0ddf1314281a497106/Cargo.lock#5755-5762 - bumping that version here would probably mean m-c ends up with 2 versions of that crate which we try hard to avoid.

@badboy
Copy link
Member

badboy commented Mar 21, 2024

Yup, sorry, should have been more precise. We also try to avoid dupe crates in tree.
I'm already trying to get current UniFFI in. I don't mind fixing up the other toml uses, but prefer doing that after we get in what we have now to keep the required changes I have to do smaller.

@mgeisler
Copy link
Contributor Author

Thanks both for explaining! So it seems we're in the same boat 😅 Just with different trees of crates to update.

On my side in Android, I'll just carry the small patch here to make uniffi_macros build with a recent toml dependency. If/when the dependency is updated here and in m-c, then I'll notice on the next import and drop the patch.

We can close the PR here if you like since you're obviously already on top of this.

@badboy
Copy link
Member

badboy commented Mar 22, 2024

Let's leave it open, I hope we can cut a uniffi release next week, and after that we can land this.

@badboy badboy self-assigned this Apr 2, 2024
@badboy
Copy link
Member

badboy commented Apr 8, 2024

With the release out we can now make progress on this.
It's relatively simple to pull the new version into mozilla-central, we can patch over the few uses of the old crate (and only UniFFI needs to change the API call really).
However this does pull in a couple new crates that we will need to audit (using cargo-vet) (uniffi does not [yet] use cargo-vet, but Glean and mozilla-central do).

5 unvetted dependencies:
  serde_spanned:0.6.5 missing ["safe-to-deploy"]
  toml:0.8.12 missing ["safe-to-deploy"]
  toml_datetime:0.6.5 missing ["safe-to-deploy"]
  toml_edit:0.22.9 missing ["safe-to-deploy"]
  winnow:0.6.5 missing ["safe-to-deploy"]

We pull in other trusted audits from Google (via https://github.com/google/rust-crate-audits).
Would you be able to do those audits? That would speed things along.

@mgeisler
Copy link
Contributor Author

mgeisler commented Apr 8, 2024

We pull in other trusted audits from Google (via https://github.com/google/rust-crate-audits).
Would you be able to do those audits? That would speed things along.

Yes, I like to help here! It would be useful for me to become more familiar with cargo vet. I'll put it on my TODO and try to come back to it in a few days.

@mgeisler
Copy link
Contributor Author

I looked a bit at the cargo vet reviews today, but didn't immediately find a place for me to put them so that they end up in https://github.com/google/rust-crate-audits. I'll have to look some more!

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