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: non absolute build paths are relative to the build manifest #1819

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

nhtyy
Copy link
Collaborator

@nhtyy nhtyy commented Nov 25, 2024

sp1 build searches for the programs from the current dir

this can have unexpected behaviour if calling from an importing crate in a differnt location

opt in, but deprecating, way to have more reliable program resolution

crates/build/src/build.rs Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 25, 2024

SP1 Performance Test Results

Branch: n/build
Commit: 13f21a7
Author: nhtyy

program cycles execute (mHz) core (kHZ) compress (KHz) time success
fibonacci 11291 0.17 2.72 0.46 25s
ssz-withdrawals 2757356 16.15 126.71 35.07 1m19s
tendermint 12593597 6.69 266.02 100.10 2m8s

Copy link
Contributor

@yuwen01 yuwen01 left a comment

Choose a reason for hiding this comment

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

Is there a way to avoid exposing build_program_with_maybe_args and the macro?

@@ -119,14 +126,56 @@ pub fn build_program(path: &str) {
///
/// # Arguments
///
/// * `path` - A string slice that holds the path to the program directory.
/// * `path` - A path to the guest program directory
Copy link
Contributor

Choose a reason for hiding this comment

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

nit punc

/// `args` - A [`BuildArgs`] struct that contains various build configuration options.
/// If not provided, the default options are used.
#[macro_export]
macro_rules! build_program_from_path {
Copy link
Contributor

Choose a reason for hiding this comment

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

wait why does this have to be a macro?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we need to inline the manifest dir at compile time so the path is always relative from the crate dir

from slack:
My contrived example of why this is bad is:
Crate A: lives in dir A exports a function build_A_program that uses a relative path to call sp1_build::build_program
Crate B: lives in dir B imports this function
Crate B: Calls build_A_program with working dir B
this call will then try to look for the path relative to B which is def incorrect

/// ### Note:
/// This function is only exposed to support the `build_program_from_path!` macro.
/// It is not recommended to use this function directly.
pub fn build_program_with_maybe_args(path: impl AsRef<Path>, args: Option<BuildArgs>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should avoid exposing this function if possible, though ngl i'm not really sure how we'd do it.

@nhtyy nhtyy added the next-release A PR or an issue that should resolved before the next major release label Dec 4, 2024
@nhtyy nhtyy removed the next-release A PR or an issue that should resolved before the next major release label Dec 16, 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.

2 participants