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

move rust crates to chromium_crate_io #21759

Merged
merged 2 commits into from
Mar 12, 2024
Merged

Conversation

bridiver
Copy link
Collaborator

@bridiver bridiver commented Jan 26, 2024

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 lint, 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:

@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Jan 26, 2024
@rillian
Copy link
Contributor

rillian commented Jan 29, 2024

As usual, it would help future archaeologists if you included an overview of the change and its motivation in the commit message and/or PR desecription. From a brief look at the code it seems like:

  • Upstream chromium has changed the way they're vendoring third-party crates to use more of cargo's capabilities.
  • Instead of third_party/rust/crate_name/vX_Y/crate the third party source is in third_party/rust/chromium_crates_io/vendor/crate-X.Y.Z. First party metadata and build description for gn remain in the old location with the third party source referenced from there.

The indent change on line 161 of script/brave_license_helper.py seems spurious (.vscode comment).

I noticed the adblock version numbers don't all match, but that's true of the main branch as well. Looks like things have been moved into //brave/third_party/rust/chromium_crates_io/vendor/ rather than these being fresh copies from crates.io?

Splitting the source changes and file moves into separate commits might help the github review interface not fall over. :(

@bridiver
Copy link
Collaborator Author

bridiver commented Feb 1, 2024

The indent change on line 161 of script/brave_license_helper.py seems spurious (.vscode comment).

clang format applies to the whole file

@bridiver
Copy link
Collaborator Author

bridiver commented Feb 1, 2024

I noticed the adblock version numbers don't all match, but that's true of the main branch as well. Looks like things have been moved into //brave/third_party/rust/chromium_crates_io/vendor/ rather than these being fresh copies from crates.io?

This is definitely a fresh download from crates.io, although 3 days ago that may not have been the case

@bridiver
Copy link
Collaborator Author

bridiver commented Feb 1, 2024

As usual, it would help future archaeologists if you included an overview of the change and its motivation in the commit message and/or PR desecription. From a brief look at the code it seems like:

I intend to squash the commits

@bridiver bridiver force-pushed the rust-chromium-crates-io branch from fce9a28 to cde3a6e Compare March 6, 2024 22:35
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core labels Mar 6, 2024
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Copy link

socket-security bot commented Mar 7, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

# These have auto-generated license files and
# GetThirdPartyDepsFromGNDepsOutput causes strange license errors
# unless the this entire directory is excluded.
os.path.join('brave', 'third_party', 'rust'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can just exclude //brave/third_party/rust/chromium_crates_io/vendor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

please see the comment explaining why the entire directory is excluded

Copy link
Contributor

Choose a reason for hiding this comment

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

I read the comment, but I couldn't reproduce the errors. I suppose it doesn't matter much, but the narrower scope seemed a safer hack.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can try it again, but it failed with a very strange error before

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@bridiver bridiver force-pushed the rust-chromium-crates-io branch from a8e5906 to 95d2ece Compare March 7, 2024 21:23
@bridiver bridiver added the CI/run-windows-x86 Run CI builds for Windows x86 label Mar 7, 2024
@rillian
Copy link
Contributor

rillian commented Mar 7, 2024

NB you can address the audit failure by applying this patch to lol_html-0.3.1.

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@bridiver bridiver added CI/run-linux-arm64 Run CI builds for Linux arm64 CI/run-windows-arm64 Run CI builds for Windows arm64 labels Mar 7, 2024
@bridiver bridiver force-pushed the rust-chromium-crates-io branch from 95d2ece to b3a7fc6 Compare March 7, 2024 23:09
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@bridiver bridiver force-pushed the rust-chromium-crates-io branch from b3a7fc6 to fdcdcdf Compare March 8, 2024 17:23
@bridiver bridiver changed the base branch from master to cr123 March 8, 2024 17:24
@bridiver bridiver force-pushed the rust-chromium-crates-io branch from fdcdcdf to 83a570d Compare March 8, 2024 17:26
Base automatically changed from cr123 to master March 9, 2024 00:18
@bridiver bridiver marked this pull request as ready for review March 9, 2024 14:24
@bridiver bridiver requested review from a team and fmarier as code owners March 9, 2024 14:24
@bridiver bridiver force-pushed the rust-chromium-crates-io branch from c240962 to 9a0ea5a Compare March 9, 2024 21:35
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@bridiver bridiver force-pushed the rust-chromium-crates-io branch from 9a0ea5a to a3d081a Compare March 9, 2024 22:42
@bridiver bridiver requested a review from a team as a code owner March 9, 2024 22:42
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

]
features = [
"alloc",
"serde",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

double-checked this removal and it appears to be correct. This appears to only be enabled on v4

build_native_rust_unit_tests = false
edition = "2021"
cargo_pkg_version = "0.8.8"
cargo_pkg_authors =
"Andrius Aucinas <aaucinas@brave.com>, Anton Lazarev <alazarev@brave.com>"
cargo_pkg_authors = "Anton Lazarev <alazarev@brave.com>, Andrius Aucinas"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is removing Andrius' email here a bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems like maybe a gnrt bug? This was auto-generated

Copy link
Contributor

@rillian rillian left a comment

Choose a reason for hiding this comment

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

Works for me. We have some nits, and should figure out how to manage the cargo vet config four our own purposes, but I'd prefer to see that in a followup, since this will unblock other contributions involving rust code.

@fmarier are you ok with the license helper exemption until we can figure out how to tell it where the actual license files are?

Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

Licensing exception is okay, but it needs a follow-up issue to be filed so that we don't forget to figure out how to do this properly once the dust has settled.

@bridiver
Copy link
Collaborator Author

linux arm64 failure is unrelated to this PR. See #21443

Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

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

wallet++

@@ -0,0 +1,12 @@
diff --git a/tools/crates/gnrt/gen.rs b/tools/crates/gnrt/gen.rs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at least some of these patches can go to upstream, we'll figure out the details in a follow-up

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

patches and DEPS ++

@bridiver
Copy link
Collaborator Author

There are no substantive changes here for p3a so I'm going to go ahead and merge

@bridiver bridiver merged commit afb81b3 into master Mar 12, 2024
18 of 19 checks passed
@bridiver bridiver deleted the rust-chromium-crates-io branch March 12, 2024 18:16
@github-actions github-actions bot added this to the 1.65.x - Nightly milestone Mar 12, 2024
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) CI/run-linux-arm64 Run CI builds for Linux arm64 CI/run-windows-arm64 Run CI builds for Windows arm64 CI/run-windows-x86 Run CI builds for Windows x86 CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo.toml out of date
6 participants