Sync hook for downloading `rust-toolchain-aux`. #27749

szilardszaloki · 2025-02-20T19:23:13Z

Resolves brave/brave-browser#44132.

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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

brave-builds · 2025-02-20T21:32:38Z

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

goodov · 2025-02-25T04:36:27Z

build/rust/download_rust_toolchain_aux.py

+
+    with tempfile.TemporaryFile() as temp_file:
+        try:
+            deps.DownloadUrl(


npm run sync should not download this if the required version is already unpacked.
Will this be a no-op if the required version is already downloaded and unpacked?

Currently, we don't store the version of these artifacts — they are always in sync with whatever Rust and Clang revisions Chromium has in tools/rust/update_rust.py and tools/clang/scripts/update.py, respectively. We can also bundle src/third_party/rust-toolchain/VERSION with them, which will make it possible to only perform the download when necessary.

I'm okay with any approach, just make sure this is no-op if the required file is already downloaded. You can use sha256 or git commit or something else. You can look into existing deps and how they handle this.

build/commands/lib/config.js

goodov · 2025-02-27T06:47:22Z

build/rust/download_rust_toolchain_aux.py

+                      "to build the required Rust aux package.")
+            raise
+
+        with temporary_directory() as temp_dir:


this can be replaced with with tempfile.TemporaryDirectory() as temp_dir:

make sure to unpack the content into the source tree, NOT into TMP as TMP most likely will have AV protection enabled (on Windows at least), which may trigger it randomly.

but you should just use deps.DownloadAndUnpack.

The AV thing is a good point, but the same applies to the tempfile.NamedTemporaryFile() call in deps.DownloadAndUnpack(), no?

The AV thing is a good point, but the same applies to the tempfile.NamedTemporaryFile() call in deps.DownloadAndUnpack(), no?

that is true, but for some reason Chromium is fine with that (in their deps downloader impl they download to tmp).

My guess is the risk of AV triggering on an archive access is much lower rather than writing a real executable on disk when unpacking.

build/rust/download_rust_toolchain_aux.py

goodov · 2025-02-27T06:54:36Z

build/rust/download_rust_toolchain_aux.py

+            raise
+
+        with temporary_directory() as temp_dir:
+            file.seek(0)


pls consider handling *.tar.xz in deps.DownloadAndUnpack instead of this file juggling here.

The main concern with DownloadAndUnpack() is not that it doesn't handle XZ files, but that it gets rid of the temp file it downloads the archive into after extracting it (which we want to keep). It's an outdated version of Chromium's DownloadAndUnpack() in tools/clang/scripts/update.py by the way.

Also, a minimal file juggling is necessary, as the files in the package go to different places.

goodov · 2025-02-28T07:33:17Z

build/rust/download_rust_toolchain_aux.py

+            deps.DownloadAndUnpack(
+                f'{deps_config.DEPS_PACKAGES_URL}/rust-toolchain-aux/{build_rust_toolchain_aux.package_name()}',
+                temp_dir,
+                output_file=PACKAGE)


sure, you can keep the whole archive and make some changes to DownloadAndUnpack, but consider storing a simple text file like ${RUST_TOOLCHAIN}/.brave_toolchain_aux_package with the downloaded/unpacked package and read/write it from you script to detect if you need the update.

goodov · 2025-02-28T07:34:25Z

script/deps.py

            else:
-                t = tarfile.open(tmp_file.name, mode='r:gz')
+                t = tarfile.open(file.name, mode='r:*')


this is cool, I didn't know about this mode.

goodov · 2025-03-03T07:05:06Z

build/rust/download_rust_toolchain_aux.py

+        print(f'Skipping {Path(__file__).stem}')
+        return
+
+    with tempfile.TemporaryDirectory(dir=RUST_TOOLCHAIN) as temp_dir:


nit: this may left temp directories in a tree if the script was aborted via Ctrl+C.

but it's fine for now.

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build labels Feb 20, 2025

github-actions bot assigned szilardszaloki Feb 20, 2025

szilardszaloki added 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/run-macos-x64 Run CI builds for macOS x64 labels Feb 20, 2025

Base automatically changed from szilard/44131-build-rust-toolchain-aux-windows to master February 20, 2025 21:47

szilardszaloki changed the base branch from master to szilard/44140-fix-arcname February 21, 2025 00:27

szilardszaloki changed the base branch from szilard/44140-fix-arcname to master February 21, 2025 00:31

szilardszaloki changed the base branch from master to szilard/44140-fix-arcname February 21, 2025 00:37

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch from 372c951 to 3ddbebf Compare February 21, 2025 00:38

szilardszaloki changed the base branch from szilard/44140-fix-arcname to master February 21, 2025 07:01

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch from 3ddbebf to 37daed8 Compare February 21, 2025 15:48

szilardszaloki removed 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/run-macos-x64 Run CI builds for macOS x64 labels Feb 21, 2025

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch from 37daed8 to 868486d Compare February 21, 2025 19:07

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch 3 times, most recently from f25ddea to 7ca0bae Compare February 24, 2025 22:22

szilardszaloki marked this pull request as ready for review February 24, 2025 22:22

szilardszaloki requested review from a team as code owners February 24, 2025 22:22

szilardszaloki removed 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/run-macos-x64 Run CI builds for macOS x64 labels Feb 24, 2025

goodov reviewed Feb 25, 2025

View reviewed changes

goodov reviewed Feb 27, 2025

View reviewed changes

szilardszaloki added 2 commits February 27, 2025 10:50

Downloads rust-toolchain-aux.

68ef37a

fix-up

fdf2bf6

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch from 1041cb1 to 7f7159c Compare February 27, 2025 17:59

fix-up 2

c0d6cac

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch from 7f7159c to c0d6cac Compare February 27, 2025 19:45

goodov reviewed Feb 28, 2025

View reviewed changes

fix-up 3

d6a2653

szilardszaloki force-pushed the szilard/44132-sync-hook-for-downloading-rust-toolchain-aux branch from a85b4ae to d6a2653 Compare February 28, 2025 22:42

goodov approved these changes Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync hook for downloading `rust-toolchain-aux`. #27749

Sync hook for downloading `rust-toolchain-aux`. #27749

szilardszaloki commented Feb 20, 2025 •

edited

Loading

brave-builds commented Feb 20, 2025

goodov Feb 25, 2025

szilardszaloki Feb 25, 2025

goodov Feb 25, 2025

goodov Feb 27, 2025

szilardszaloki Feb 27, 2025

goodov Feb 28, 2025

goodov Feb 27, 2025

szilardszaloki Feb 27, 2025

goodov Feb 28, 2025

goodov Feb 28, 2025

goodov Mar 3, 2025

Sync hook for downloading rust-toolchain-aux. #27749

Are you sure you want to change the base?

Sync hook for downloading rust-toolchain-aux. #27749

Conversation

szilardszaloki commented Feb 20, 2025 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sync hook for downloading `rust-toolchain-aux`. #27749

Sync hook for downloading `rust-toolchain-aux`. #27749

szilardszaloki commented Feb 20, 2025 •

edited

Loading