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

Remove absolute registry paths from panic messages in wasm #1594

Merged
merged 8 commits into from
Nov 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/soroban-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ ulid = { workspace = true, features = ["serde"] }
strum = "0.17.1"
strum_macros = "0.17.1"
async-compression = { version = "0.4.12", features = ["tokio", "gzip"] }

shell-escape = "0.1.5"
tempfile = "3.8.1"
toml_edit = "0.21.0"
rust-embed = { version = "8.2.0", features = ["debug-embed"] }
Expand Down
131 changes: 126 additions & 5 deletions cmd/soroban-cli/src/commands/contract/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use cargo_metadata::{Metadata, MetadataCommand, Package};
use clap::Parser;
use itertools::Itertools;
use std::{
borrow::Cow,
collections::HashSet,
env,
ffi::OsStr,
Expand All @@ -12,6 +13,8 @@ use std::{
};
use stellar_xdr::curr::{Limits, ScMetaEntry, ScMetaV0, StringM, WriteXdr};

use crate::{commands::global, print::Print};

/// Build a contract from source
///
/// Builds all crates that are referenced by the cargo manifest (Cargo.toml)
Expand Down Expand Up @@ -96,6 +99,8 @@ pub enum Error {
CopyingWasmFile(io::Error),
#[error("getting the current directory: {0}")]
GettingCurrentDir(io::Error),
#[error("retreiving CARGO_HOME: {0}")]
CargoHome(io::Error),
#[error("reading wasm file: {0}")]
ReadingWasmFile(io::Error),
#[error("writing wasm file: {0}")]
Expand All @@ -108,7 +113,9 @@ const WASM_TARGET: &str = "wasm32-unknown-unknown";
const META_CUSTOM_SECTION_NAME: &str = "contractmetav0";

impl Cmd {
pub fn run(&self) -> Result<(), Error> {
pub fn run(&self, global_args: &global::Args) -> Result<(), Error> {
let print = Print::new(global_args.quiet);

let working_dir = env::current_dir().map_err(Error::GettingCurrentDir)?;

let metadata = self.metadata()?;
Expand Down Expand Up @@ -154,15 +161,31 @@ impl Cmd {
cmd.arg(format!("--features={activate}"));
}
}
let cmd_str = format!(
"cargo {}",
cmd.get_args().map(OsStr::to_string_lossy).join(" ")

if let Some(rustflags) = make_rustflags_to_remap_absolute_paths(&print)? {
cmd.env("CARGO_BUILD_RUSTFLAGS", rustflags);
}

let mut cmd_str_parts = Vec::<String>::new();
cmd_str_parts.extend(cmd.get_envs().map(|(key, val)| {
format!(
"{}={}",
key.to_string_lossy(),
shell_escape::escape(val.unwrap_or_default().to_string_lossy())
)
}));
cmd_str_parts.push("cargo".to_string());
cmd_str_parts.extend(
cmd.get_args()
.map(OsStr::to_string_lossy)
.map(Cow::into_owned),
);
let cmd_str = cmd_str_parts.join(" ");

if self.print_commands_only {
println!("{cmd_str}");
} else {
eprintln!("{cmd_str}");
print.infoln(cmd_str);
let status = cmd.status().map_err(Error::CargoCmd)?;
if !status.success() {
return Err(Error::Exit(status));
Expand Down Expand Up @@ -282,3 +305,101 @@ impl Cmd {
fs::write(target_file_path, wasm_bytes).map_err(Error::WritingWasmFile)
}
}

/// Configure cargo/rustc to replace absolute paths in panic messages / debuginfo
/// with relative paths.
///
/// This is required for reproducible builds.
///
/// This works for paths to crates in the registry. The compiler already does
/// something similar for standard library paths and local paths. It may not
/// work for crates that come from other sources, including the standard library
/// compiled from source, though it may be possible to accomodate such cases in
/// the future.
///
/// This in theory breaks the ability of debuggers to find source code, but
/// since we are only targetting wasm, which is not typically run in a debugger,
/// and stellar-cli only compiles contracts in release mode, the impact is on
/// debugging is expected to be minimal.
///
/// This works by setting the `CARGO_BUILD_RUSTFLAGS` environment variable,
/// with appropriate `--remap-path-prefix` option. It preserves the values of an
/// existing `CARGO_BUILD_RUSTFLAGS` environment variable.
///
/// This must be done some via some variation of `RUSTFLAGS` and not as
/// arguments to `cargo rustc` because the latter only applies to the crate
/// directly being compiled, while `RUSTFLAGS` applies to all crates, including
/// dependencies.
///
/// `CARGO_BUILD_RUSTFLAGS` is an alias for the `build.rustflags` configuration
/// variable. Cargo automatically merges the contents of the environment variable
/// and the variables from config files; and `build.rustflags` has the lowest
/// priority of all the variations of rustflags that Cargo accepts. And because
/// we merge our values with an existing `CARGO_BUILD_RUSTFLAGS`,
/// our setting of this environment variable should not interfere with the
/// user's ability to set rustflags in any way they want, but it does mean
/// that if the user sets a higher-priority rustflags that our path remapping
/// will be ignored.
///
/// The major downside of using `CARGO_BUILD_RUSTFLAGS` is that it is whitespace
/// separated, which means we cannot support paths with spaces. If we encounter
/// such paths we will emit a warning. Spaces could be accomodated by using
/// `CARGO_ENCODED_RUSTFLAGS`, but that has high precedence over other rustflags,
/// so we could be interfering with the user's own use of rustflags. There is
/// no "encoded" variant of `CARGO_BUILD_RUSTFLAGS` at time of writing.
///
/// This assumes that paths are Unicode and that any existing `CARGO_BUILD_RUSTFLAGS`
/// variables are Unicode. Non-Unicode paths will fail to correctly perform the
/// the absolute path replacement. Non-Unicode `CARGO_BUILD_RUSTFLAGS` will result in the
/// existing rustflags being ignored, which is also the behavior of
/// Cargo itself.
fn make_rustflags_to_remap_absolute_paths(print: &Print) -> Result<Option<String>, Error> {
let cargo_home = home::cargo_home().map_err(Error::CargoHome)?;
let cargo_home = format!("{}", cargo_home.display());

if cargo_home.find(|c: char| c.is_whitespace()).is_some() {
print.warnln("Cargo home directory contains whitespace. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

if env::var("RUSTFLAGS").is_ok() {
print.warnln("`RUSTFLAGS` set. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

if env::var("CARGO_ENCODED_RUSTFLAGS").is_ok() {
print.warnln("`CARGO_ENCODED_RUSTFLAGS` set. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

if env::var("TARGET_wasm32-unknown-unknown_RUSTFLAGS").is_ok() {
print.warnln("`TARGET_wasm32-unknown-unknown_RUSTFLAGS` set. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

let registry_prefix = format!("{cargo_home}/registry/src/");
let new_rustflag = format!("--remap-path-prefix={registry_prefix}=");

let mut rustflags = get_rustflags().unwrap_or_default();
rustflags.push(new_rustflag);

let rustflags = rustflags.join(" ");

Ok(Some(rustflags))
}

/// Get any existing `CARGO_BUILD_RUSTFLAGS`, split on whitespace.
///
/// This conveniently ignores non-Unicode values, as does Cargo.
fn get_rustflags() -> Option<Vec<String>> {
if let Ok(a) = env::var("CARGO_BUILD_RUSTFLAGS") {
let args = a
.split_whitespace()
.map(str::trim)
.filter(|s| !s.is_empty())
.map(str::to_string);
return Some(args.collect());
}

None
}
2 changes: 1 addition & 1 deletion cmd/soroban-cli/src/commands/contract/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ impl Cmd {
match &self {
Cmd::Asset(asset) => asset.run().await?,
Cmd::Bindings(bindings) => bindings.run().await?,
Cmd::Build(build) => build.run()?,
Cmd::Build(build) => build.run(global_args)?,
Cmd::Extend(extend) => extend.run().await?,
Cmd::Alias(alias) => alias.run(global_args)?,
Cmd::Deploy(deploy) => deploy.run(global_args).await?,
Expand Down
Loading