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

Clean up Cargo.toml #22075

Closed
wants to merge 15 commits into from
Closed

Clean up Cargo.toml #22075

wants to merge 15 commits into from

Conversation

rillian
Copy link
Contributor

@rillian rillian commented Feb 15, 2024

Resolve various discrepancies with rust crates vendored in third_party/rust

  • Correct references to crates we're using from chromium
  • Remove crate source and references not used in the browser build (mostly from cargo-audit)
  • Update ppoprf source to match the declared version.
  • Run cargo to update Cargo.lock from the new Cargo.toml

Basically this aligns the Cargo descriptions more with what we actually have in the gn files, which will hopefully reduce diff noise the next time we use gnrt.

Resolves brave/brave-browser#36271

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@rillian rillian self-assigned this Feb 15, 2024
@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Feb 15, 2024
@rillian rillian marked this pull request as ready for review February 21, 2024 17:09
@rillian rillian requested review from a team and bridiver as code owners February 21, 2024 17:09
@rillian
Copy link
Contributor Author

rillian commented Feb 21, 2024

There's more cleanup to do, but landing this would be helpful reducing diff noise and checking other changes.

@@ -77,9 +73,57 @@ dependencies = [
"winapi",
]

[[package]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are all these new dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anstyle is used by clap_builder which is in upstream chromium. So is anstream but it's optional, so I'm not sure why we're pulling it in. Maybe the thing where cargo locks versions for optional dependencies even if they're not built?

I'll see if we really need the direct dependency on clap.

bitflags is a dep of adblock and is in upstream chromium.
bech32 was added recently for zcash support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in f4b6739. anstyle still replaces terminfo for cxxbridge, but I was able to remove a few other unnecessary entries.

@rillian rillian requested a review from bridiver February 21, 2024 23:13
@rillian rillian force-pushed the rillian/cargo-cleanup branch from 1b8bbe2 to 7a0bda0 Compare March 1, 2024 21:11
@rillian
Copy link
Contributor Author

rillian commented Mar 1, 2024

Rebased past #22349 to address android ci timeouts.

@rillian rillian force-pushed the rillian/cargo-cleanup branch from 7a0bda0 to d93fa0f Compare March 4, 2024 18:27
@rillian
Copy link
Contributor Author

rillian commented Mar 4, 2024

Rebased again to see if it resolved ios ci issue.

@kylehickinson
Copy link
Collaborator

Please rebase to fix iOS CI failure

rillian added 15 commits March 6, 2024 08:05
I assume this was copied from chromium's Cargo.toml, but it isn't
relevant here and prevents `cargo` from working with the file.
These crates are vendored in the chromium third_party/rust tree.
Update the paths to point there so `cargo` can usefully operate
on the description here.

NB: I'm unclear if `gnrt` or other automation actually reads these
paths, so it may be we can remove them entirely since they're not
required by components of brave-core. Leaving them in for now since
the full build will certainly use them.
Change patch entries to point to the moved crate sources in
chromium and remove some obsolete or conflicting entries.
Propagate Cargo.toml changes to the lockfile by running `cargo check`
with rustc 1.76.0 stable from the upstream rust project. At this
point cargo is satisfied with the depenencies, but a stand-alone
build doesn't complete because of missing `u128` config.
This is a transitive dependency of `cargo-audit` through `miniz_oxide`
and is not needed for the browser target.
This is a dependency of `cargo-audit` and is not needed for the
browser target.
These are dependencies of `cargo-audit` and is not needed for the
browser target.
This is a dependency of `cargo-audit` and is not needed for the
browser target. It's a dev-dependency of `ppoprf` but that's
not required to incorporate the library code.
Align the vendored source code with the v0.3.1 release specified
by `star_constellation`. We were already calling this version
that in `BUILD.gn` but the source code corresponded to the v0.3.0
release instead.

The only source change is to error handling in the `ppoprf::eval`
function which is not used client-side, so there is no behavior
change to the browser. This just removes a discrepancy so future
vendor diffs will be clean.

Addresses an 'patch...was not used' warning when running `cargo`
against third_party/rust/Cargo.toml.
Remove the explicit reference the `clap` rust crate from our
Cargo.toml. This is used by `cxxbridge` and some test-only
code, but isn't needed for the browser target and we can
use the upstream chromium version for the rest.

Addresses some non-vendored transitive dependencies appearing
in Cargo.lock.
This isn't part of the browser target. Removing it cleans unncessary
dependencies from Cargo.lock.
Tell a local cargo build to use the copy from chromium.
These were generated running `gnrt` on a branch where that
somewhat works, and then manually edited to remove the new
'License File' field. Upstream is using this because their
README annotations is in a separate part of the source tree
from the actual crate source, but our current style for
vendored crates doesn't have that issue.

core-foundation-sys is marked as not security-critical.
This is used to access the time zone database on macOS,
so the input isn't coming from network sources.
For the `patch` diversion in Cargo.toml to work, we need to use
an explicit reference to the prelease suffixed version which
contains the speedreader-specific code, since pre-release versions
aren't considered compatible without it. This lets cargo find
and record the correct metadata for the crate version.
This new crate was added after the initial pass, so it was
missing from the list of local paths telling cargo where
to find the source.
@rillian rillian force-pushed the rillian/cargo-cleanup branch from 03441cb to c583d00 Compare March 6, 2024 16:05
@rillian
Copy link
Contributor Author

rillian commented May 15, 2024

This is no longer relevant.

@rillian rillian closed this May 15, 2024
@rillian rillian deleted the rillian/cargo-cleanup branch May 15, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo.toml out of date
3 participants