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
8 changes: 4 additions & 4 deletions crates/build/src/build.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::path::PathBuf;
use std::path::{Path, PathBuf};

use anyhow::Result;
use cargo_metadata::camino::Utf8PathBuf;
Expand Down Expand Up @@ -54,9 +54,9 @@ pub fn execute_build_program(
}

/// Internal helper function to build the program with or without arguments.
pub(crate) fn build_program_internal(path: &str, args: Option<BuildArgs>) {
pub(crate) fn build_program_internal(path: impl AsRef<Path>, args: Option<BuildArgs>) {
// Get the root package name and metadata.
let program_dir = std::path::Path::new(path);
let program_dir = path.as_ref().to_path_buf();
let metadata_file = program_dir.join("Cargo.toml");
let mut metadata_cmd = cargo_metadata::MetadataCommand::new();
let metadata = metadata_cmd.manifest_path(metadata_file).exec().unwrap();
Expand All @@ -83,7 +83,7 @@ pub(crate) fn build_program_internal(path: &str, args: Option<BuildArgs>) {
}

// Activate the build command if the dependencies change.
cargo_rerun_if_changed(&metadata, program_dir);
cargo_rerun_if_changed(&metadata, &program_dir);

// Check if RUSTC_WORKSPACE_WRAPPER is set to clippy-driver (i.e. if `cargo clippy` is the
// current compiler). If so, don't execute `cargo prove build` because it breaks
Expand Down
57 changes: 53 additions & 4 deletions crates/build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ mod utils;
use build::build_program_internal;
pub use build::{execute_build_program, generate_elf_paths};

use std::path::Path;

use clap::Parser;

const BUILD_TARGET: &str = "riscv32im-succinct-zkvm-elf";
Expand Down Expand Up @@ -104,13 +106,18 @@ impl Default for BuildArgs {
///
/// # Arguments
///
/// * `path` - A string slice that holds the path to the program directory.
/// * `path` - A path to the guest program directory, if not absolute, assumed to be relative to
/// the caller manifest directory.
///
/// This function is useful for automatically rebuilding the program during development
/// when changes are made to the source code or its dependencies.
///
/// Set the `SP1_SKIP_PROGRAM_BUILD` environment variable to `true` to skip building the program.
pub fn build_program(path: &str) {
///
///
/// ## Note: Using this function without an absolute path is not recommended.
/// Try using the `build_program_from_path!` macro instead.
pub fn build_program(path: impl AsRef<Path>) {
build_program_internal(path, None)
}

Expand All @@ -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.
///
/// Set the `SP1_SKIP_PROGRAM_BUILD` environment variable to `true` to skip building the program.
pub fn build_program_with_args(path: &str, args: BuildArgs) {
///
/// ## Note: Using this function without an absolute path is not recommended.
/// Try using the `build_program_from_path!` macro instead.
pub fn build_program_with_args(path: impl AsRef<Path>, args: BuildArgs) {
build_program_internal(path, Some(args))
}

/// Builds the program with the given arguments if the program at path, or one of its dependencies,
///
/// ### 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.

build_program_internal(path, args)
}

/// Build a program at the given path.
///
/// # Arguments
/// * `path` - A path to the guest program directory, if not absolute, assumed to be relative to
/// the callers manifest directory.
///
/// `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

($path:expr, $args:expr) => {
const MANIFEST: &str = std::env!("CARGO_MANIFEST_DIR");

fn ___adjust_path(p: impl AsRef<::std::path::Path>) -> ::std::path::PathBuf {
let p = p.as_ref();
if p.is_absolute() {
p.to_path_buf()
} else {
::std::path::Path::new(MANIFEST).join(p)
}
}

::sp1_build::build_program_with_maybe_args(___adjust_path($path), $args)
};
($path:expr) => {
::sp1_build::build_program_from_path!($path, None)
};
}

/// Returns the raw ELF bytes by the zkVM program target name.
///
/// Note that this only works when using `sp1_build::build_program` or
Expand Down
Loading