Remove remaining dependencies using download_rust_deps #19300

bridiver · 2023-07-16T18:46:48Z

Resolves brave/brave-browser#31658

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:

tools/crates/cargo_audit/v0_17/crate/src/lockfile.rs

bsclifton

++ for CODEOWNERS

BUILD.gn

goodov · 2023-07-17T08:06:15Z

build/commands/lib/config.js

@@ -1065,6 +1065,7 @@ Object.defineProperty(Config.prototype, 'defaultOptions', {
    let env = Object.assign({}, process.env)
    env = this.addPathToEnv(env, path.join(this.depotToolsDir, 'python-bin'), true)
    env = this.addPathToEnv(env, path.join(this.depotToolsDir, 'python2-bin'), true)
+    env = this.addPathToEnv(env, path.join(this.srcDir, 'third_party', 'rust-toolchain', 'bin'), true)


do you really need this? We should not add new things into PATH if possible.

Can't you use the full path in places where you need to call something from this dir?

Yes, otherwise cargo can't find rustc or it will use local rustc which could be the wrong version. This ensures that we're always using the upstream rust toolchain. I do use the full path to cargo, but cargo uses PATH to find rustc and I didn't see an option to pass the path for it

we should remove audit_deps from gn files. It doesn't make sense to keep it there, because this target is only called from an npm command and it doesn't require any build dependency.

a follow up to cleanup this would be nice.

cargo can't find rustc or it will use local rustc which could be the wrong version

cargo is supposed to respect a RUSTC env variable. Maybe setting that in the build context would be less invasive than altering the path?

@rillian well I think either one has the same fundamental issue, the problem is that @goodov doesn't want to add things here in general because it makes it harder to run gn without using npm. I think the fix here is just to move audit deps out of gn like he suggested. Unless there's some way to specific RUSTC on the command line other than RUSTC=.... cargo ... because I don't believe you can do that with a gn action. It may be good for the script only version though.

.github/CODEOWNERS

rillian · 2023-07-17T16:28:08Z

BUILD.gn

+  args = [
+    rebase_path(cargo_exe),
+    "build",
+    "--release",


How are we feeling about --locked/--frozen this week?

Good question, I just used the same command that upstream recommended for gnrt. I guess this could have the same issues as we had with the old download_rust_deps script so maybe we should use locked here?

Seems like each have their own failure mode. I'm fine with it either way.

thypon

Is it failing with an error or failing silently on auditor fail?
The change seems sound to me, for the rest

fmarier

script/audit_deps.py changes look good.

Remove remaining dependencies using download_rust_deps

….57.x). (#19447) * Merge pull request #18955 from brave/cargo-rustc-cr115 Convert to rust_static_lib * fix patch to exclude compiler builtins for linux arm64 (#19118) don't use compiler_builtins for linux arm64 because the prebuilt library doesn't exist yet in upstream * Merge pull request #19125 from brave/issues/31415 Use Brave forked version of futures-retry * Merge pull request #19132 from brave/win-prebuilt-fix add win arm64 back in temporarily until we get the llvm update with t… * Merge pull request #19232 from brave/rust-dep-fixup Rust dep fixup * Merge pull request #19300 from brave/rust-dep-cleanup-2 Remove remaining dependencies using download_rust_deps * Merge pull request #19320 from brave/rust-dep-cleanup-3 * Merge pull request #19339 from brave/win-rust-asan-fix upstream patch https://chromium-review.googlesource.com/c/chromium/sr… * Fix __rust_dealloc on Windows. (#19116) * Merge pull request #19322 from brave/audit-branding-fix audit_deps needs branding for some reason * Merge pull request #19426 from brave/rust-unsafe-fn * Merge pull request #18808 from brave/cr116 Upgrade from Chromium 115 to Chromium 116 * Merge pull request #19453 from brave/android_text_view_id_conflict_fix [C116] [Android] Fix for the `text_view` id conflict * Fix loading.desktop.brave after cr116 (#19450) * Fix loading.desktop.brave after cr116 * Fix lint errors * Merge pull request #19455 from brave/maxk-fix-narrow-settings-menu [cr116 follow-up] Fixes narrow settings menu button. * Merge pull request #19457 from brave/maxk-disable-kOptimizationGuideFetchingForSRP [cr116 follow-up] Disables kOptimizationGuideFetchingForSRP feature. * Merge pull request #19458 from brave/maxk-fix-brave-toolbar [cr116 follow-up] Fixes toolbar buttons misalignment. * Update padding after brave toolbar is initialized (#19484) As we're not updating padding during in the middle of initialization, we should do it after it finishes. * Fix broken on-demand updates on Windows (#19469) --------- Co-authored-by: Brian Johnson <[email protected]> Co-authored-by: Aleksey Khoroshilov <[email protected]> Co-authored-by: samartnik <[email protected]> Co-authored-by: Mikhail <[email protected]> Co-authored-by: Sangwoo Ko <[email protected]> Co-authored-by: Michael Herrmann <[email protected]>

bridiver added 2 commits July 14, 2023 14:41

general rust dep cleanup

92e8a72

rust dep cleanup

3b94a5e

bridiver requested a review from a team as a code owner July 16, 2023 18:46

github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Jul 16, 2023

bridiver self-assigned this Jul 16, 2023

build audit_deps with upstream toolchain

f0f3ae9

bridiver requested review from a team as code owners July 16, 2023 21:34

github-actions bot reviewed Jul 16, 2023

View reviewed changes

tools/crates/cargo_audit/v0_17/crate/src/lockfile.rs Show resolved Hide resolved

github-actions bot added the needs-security-review label Jul 16, 2023

github-actions bot assigned fmarier and thypon Jul 16, 2023

bridiver changed the title ~~Rust dep cleanup 2~~ Remove remaining dependencies using download_rust_deps Jul 16, 2023

thypon removed the needs-security-review label Jul 16, 2023

thypon unassigned fmarier and thypon Jul 16, 2023

mihaiplesa approved these changes Jul 16, 2023

View reviewed changes

bridiver added 3 commits July 16, 2023 16:22

presubmit fix

de2bdff

add codeowner for third_party/rust

f193f8d

ignore cargo_audit for semgrep

47b9fc2

bridiver requested review from thypon, a team and bsclifton as code owners July 16, 2023 23:25

fix rustc path for running cargo

d9a2c92

bsclifton approved these changes Jul 17, 2023

View reviewed changes

goodov reviewed Jul 17, 2023

View reviewed changes

.github/CODEOWNERS Outdated Show resolved Hide resolved

bridiver added 2 commits July 17, 2023 07:18

is_win -> host_os == "win"

8e93397

fix codeowners so all third_party/rust changes require bridiver approval

95f6caf

goodov approved these changes Jul 17, 2023

View reviewed changes

rillian approved these changes Jul 17, 2023

View reviewed changes

mihaiplesa approved these changes Jul 17, 2023

View reviewed changes

thypon reviewed Jul 17, 2023

View reviewed changes

fmarier approved these changes Jul 17, 2023

View reviewed changes

thypon approved these changes Jul 17, 2023

View reviewed changes

bridiver merged commit 0d8d769 into master Jul 17, 2023

bridiver deleted the rust-dep-cleanup-2 branch July 17, 2023 18:19

github-actions bot added this to the 1.58.x - Nightly milestone Jul 17, 2023

emerick mentioned this pull request Jul 18, 2023

CI noplatform job is failing in audit deps due to missing branding files brave/brave-browser#31692

Closed

mkarolin pushed a commit that referenced this pull request Jul 20, 2023

Merge pull request #19300 from brave/rust-dep-cleanup-2

cec6df5

Remove remaining dependencies using download_rust_deps

mkarolin mentioned this pull request Jul 20, 2023

[DO NOT MERGE!] Uplift rust changes from master (1.58.x) to 1.57.x #19370

Closed

25 tasks

mkarolin mentioned this pull request Jul 27, 2023

Upgrade from Chromium 115 to Chromium 116 + Rust changes (uplift to 1.57.x). #19447

Merged

25 tasks

mkarolin pushed a commit that referenced this pull request Aug 3, 2023

Merge pull request #19300 from brave/rust-dep-cleanup-2

857d8c7

Remove remaining dependencies using download_rust_deps

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove remaining dependencies using download_rust_deps #19300

Remove remaining dependencies using download_rust_deps #19300

bridiver commented Jul 16, 2023 •

edited

Loading

bsclifton left a comment

goodov Jul 17, 2023

bridiver Jul 17, 2023 •

edited

Loading

goodov Jul 17, 2023

rillian Jul 17, 2023

bridiver Jul 17, 2023

rillian Jul 17, 2023

bridiver Jul 17, 2023

rillian Jul 17, 2023

thypon left a comment

fmarier left a comment

Remove remaining dependencies using download_rust_deps #19300

Remove remaining dependencies using download_rust_deps #19300

Conversation

bridiver commented Jul 16, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

bsclifton left a comment

Choose a reason for hiding this comment

goodov Jul 17, 2023

Choose a reason for hiding this comment

bridiver Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

goodov Jul 17, 2023

Choose a reason for hiding this comment

rillian Jul 17, 2023

Choose a reason for hiding this comment

bridiver Jul 17, 2023

Choose a reason for hiding this comment

rillian Jul 17, 2023

Choose a reason for hiding this comment

bridiver Jul 17, 2023

Choose a reason for hiding this comment

rillian Jul 17, 2023

Choose a reason for hiding this comment

thypon left a comment

Choose a reason for hiding this comment

fmarier left a comment

Choose a reason for hiding this comment

bridiver commented Jul 16, 2023 •

edited

Loading

bridiver Jul 17, 2023 •

edited

Loading