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

fix(cli): get-target #270

Merged
merged 3 commits into from
Feb 23, 2024
Merged

fix(cli): get-target #270

merged 3 commits into from
Feb 23, 2024

Conversation

0xYYY
Copy link
Contributor

@0xYYY 0xYYY commented Feb 19, 2024

Issue

To build from source on platforms without pre-built toolchain (e.g. x86_64 macOS), the Book instruct to run the cargo prove build-toolchain command.

In the build_toolchain command, the function get_target() will be called.

let target = get_target();

But it only returns the target triplet for the platforms with pre-built binaries and panic otherwise.

sp1/cli/src/lib.rs

Lines 65 to 77 in 8ac9e54

#[allow(unreachable_code)]
pub fn get_target() -> &'static str {
#[cfg(all(target_arch = "x86_64", target_os = "linux"))]
return "x86_64-unknown-linux-gnu";
#[cfg(all(target_arch = "aarch64", target_os = "linux"))]
return "aarch64-unknown-linux-gnu";
#[cfg(all(target_arch = "aarch64", target_os = "macos"))]
return "aarch64-apple-darwin";
panic!("Unsupported architecture. Please build the toolchain from source.")
}

Meaning that building from source on other platforms will encounter the following error.

Build completed successfully in 0:07:59
info: no toolchain installed for 'succinct'
Succesfully removed existing toolchain.
Found built toolchain directory at ./rust/build/x86_64-apple-darwin/stage2.
Succesfully linked the toolchain to rustup.
thread 'main' panicked at cli/src/lib.rs:74:5:
Unsupported architecture. Please build the toolchain from source.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Solution

This PR uses the target_lexicon crate to get the target triplet of the host machine.

@tamirhemo
Copy link
Contributor

Seems like we may want different functions: one to notify users that the platform is not supported, and one for getting the target when building from source. Any thoughts @ctian1 , @jtguibas ?

@0xYYY
Copy link
Contributor Author

0xYYY commented Feb 22, 2024

one to notify users that the platform is not supported

sp1/cli/src/lib.rs

Lines 79 to 91 in 8ac9e54

#[allow(unreachable_code)]
pub fn get_toolchain_download_url() -> &'static str {
#[cfg(all(target_arch = "x86_64", target_os = "linux"))]
return "https://pub-e4d9616fb885415597ff4c4d2b476ffb.r2.dev/rust-toolchain-x86_64-unknown-linux-gnu.tar.gz";
#[cfg(all(target_arch = "aarch64", target_os = "linux"))]
return "https://pub-e4d9616fb885415597ff4c4d2b476ffb.r2.dev/rust-toolchain-aarch64-unknown-linux-gnu.tar.gz";
#[cfg(all(target_arch = "aarch64", target_os = "macos"))]
return "https://pub-e4d9616fb885415597ff4c4d2b476ffb.r2.dev/rust-toolchain-aarch64-apple-darwin.tar.gz";
panic!("Unsupported architecture. Please build the toolchain from source.")
}

Can this function fit the need?

For example, in the install_toolchain command, both get_target() and get_toolchain_download_url() are called. get_target() determine the host machine's architecture, while get_toolchain_download_url() determine whether the host machine's architecture is supported.

let target = get_target();
let toolchain_asset_name = format!("rust-toolchain-{}.tar.gz", target);
let toolchain_archive_path = root_dir.join(toolchain_asset_name.clone());
let toolchain_dir = root_dir.join(target);
let toolchain_download_url = get_toolchain_download_url();

Copy link
Member

@ctian1 ctian1 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM

@ctian1 ctian1 merged commit aa71d90 into succinctlabs:main Feb 23, 2024
4 checks passed
jtguibas pushed a commit that referenced this pull request Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants