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 6 commits into
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. |
|
Thanks for working on this — the problem is real and worth solving. However, I think we can get a better result with a simpler approach that doesn't require maintaining a hardcoded target list. Instead of checking against a list upfront, we can improve the error message at the point where the download actually fails. Since many users will have Deno installed, we can get a good error message from the Deno downloader by checking the response status: const resp = await fetch(url);
if (resp.status === 404) {
console.error(`No prebuilt V8 archive found at ${url}`);
console.error(`If no prebuilt is available for your target, compile V8 from source with V8_FROM_SOURCE=1`);
Deno.exit(1);
}
if (!resp.ok) Deno.exit(1);That covers the majority of users. For the Python and curl fallback paths, we can replace the bare if !status.success() {
panic!(
"Failed to download V8 prebuilt archive from {url}\n\
If no prebuilt archive is available for your target, \
compile V8 from source by setting V8_FROM_SOURCE=1"
);
}This approach is self-maintaining (no list to keep in sync with releases), handles all edge cases automatically (features, profiles, new targets), and is a much smaller change. |
09aafa6 to
789b9e6
Compare
|
@bartlomieju Implemented your changes and rebased on main :) |
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