build: Notify when no pre-built archive is available for the target#1945
build: Notify when no pre-built archive is available for the target#1945TheJanzap wants to merge 5 commits intodenoland:mainfrom
Conversation
bartlomieju
left a comment
There was a problem hiding this comment.
Review
The idea is good — failing early with a clear message is much better than a confusing 404. But the implementation has several issues:
Bug: The target list is wrong
The PR's PREBUILT_TARGETS list has "aarch64-unknown-linux", but the actual Rust target triple (and the release asset name) is "aarch64-unknown-linux-gnu". This means is_target_prebuilt would return false for aarch64 Linux, incorrectly panicking on one of the most common targets.
Bug: Windows targets lack debug prebuilts
The Windows targets (aarch64-pc-windows-msvc, x86_64-pc-windows-msvc) only have release prebuilts in the actual GitHub releases. A debug build targeting Windows would pass the is_target_prebuilt check but still 404. The check doesn't account for profile.
Design concern: Hardcoded list is fragile
The list will drift as targets are added/removed. An alternative approach: catch the 404 from the download and provide the helpful error message with the V8_FROM_SOURCE=1 suggestion at that point. This would be self-maintaining and always accurate.
Minor: RUSTY_V8_MIRROR detection
The check base == default_base is an indirect proxy for "RUSTY_V8_MIRROR is unset." A cleaner approach would be env::var("RUSTY_V8_MIRROR").is_err().
Summary
The aarch64-unknown-linux typo is a blocker — it would break aarch64 Linux builds. The missing debug/release distinction for Windows is a secondary concern. I'd suggest either:
- Fix the list and add a
profilecheck, or - (Better) Replace the hardcoded list with a 404-triggered error message in the download logic
|
I also briefly considered checking for the 404, but IMO the current approach is simpler. The download is triggered by Deno, Python and curl. The clean way would be to check each one for the 404, but since they are launched as external processes, we only get the exit codes. Depending on the program, there may or may not be a specific exit code for a 404 we can check for. I wager the code for this could get exhaustive and messy. I've fixed the other problems you mentioned in your review, but if you really want, I can try implement exit-code checks. |
|
How about fallback to bulld from source unconditionally when prebuilt download fails? |
Not sure how I feel about this. Currently, we explicitly tell the user that they have to build from source, and by setting the env var they acknowledge this. If we simply fall back to a source build, the user might have no idea that this is happening and wonder why their build is taking so long (especially if v8 is a (in)direct dependency). Yes, there will probably be a line in the log telling the user this, but I think hardly anyone will spot the message, as it'll probably be surrounded by a ton of other log messages. |
This PR adds a panic into
build.rswhen a v8 build is started with a target that v8 does not provide a pre-built archive for andRUSTY_V8_MIRRORis unset (i.e. the script downloads the archive from this GitHub repo). In this case, the panic message advises to rebuild v8 withV8_FROM_SOURCE=1.I currently use a hardcoded list of targets for this, but if you have a better solution, please suggest so :)
I am also a bit unsure if the placement of the code is appropriate, as the build script is quite expansive, so feel free to also suggest improvements there.
Implements the feature discussed in #1877