From ffb3b5e14420ec3cf0f19fc713160f79c4b3bfb8 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 25 Oct 2023 10:32:24 +0800 Subject: [PATCH 1/6] build workers on demand --- polkadot/node/core/pvf/src/testing.rs | 68 +++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/polkadot/node/core/pvf/src/testing.rs b/polkadot/node/core/pvf/src/testing.rs index 31bcfe7fadb6..6e971dcc1d58 100644 --- a/polkadot/node/core/pvf/src/testing.rs +++ b/polkadot/node/core/pvf/src/testing.rs @@ -62,6 +62,39 @@ pub fn validate_candidate( pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { // Only needs to be called once for the current process. static WORKER_PATHS: OnceLock> = OnceLock::new(); + + fn maybe_build_workers(build_prep: bool, build_exec: bool) { + let build_args = match (build_prep, build_exec) { + (true, true) => "build --bin polkadot-prepare-worker --bin polkadot-execute-worker", + (true, false) => "build --bin polkadot-prepare-worker", + (false, true) => "build --bin polkadot-execute-worker", + (false, false) => "", + }; + + if build_args.len() > 0 { + eprintln!("Building workers..."); + + let mut wd = std::env::current_exe().unwrap(); + while !wd.ends_with("polkadot-sdk") { + wd.pop(); + } + + let child = match std::process::Command::new("cargo") + .args(build_args.split(' ')) + .stdout(std::process::Stdio::piped()) + .current_dir(wd) + .spawn() + { + Ok(child) => child, + Err(err) => panic!("Failed to start cargo build: {}", err), + }; + + if let Err(err) = child.wait_with_output() { + panic!("Failed to build workers: {}", err); + } + } + } + let mutex = WORKER_PATHS.get_or_init(|| { let mut workers_path = std::env::current_exe().unwrap(); workers_path.pop(); @@ -71,22 +104,37 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { let mut execute_worker_path = workers_path.clone(); execute_worker_path.push(EXECUTE_BINARY_NAME); + let mut build_prep = false; + let mut build_exec = false; + // Check that the workers are valid. - if !prepare_worker_path.is_executable() || !execute_worker_path.is_executable() { - panic!("ERROR: Workers do not exist or are not executable. Workers directory: {:?}", workers_path); + + if !prepare_worker_path.is_executable() { + eprintln!("WARNING: Prepare worker does not exist or is not executable. Workers directory: {:?}", workers_path); + build_prep = true; } - let worker_version = - get_worker_version(&prepare_worker_path).expect("checked for worker existence"); - if worker_version != NODE_VERSION { - panic!("ERROR: Prepare worker version {worker_version} does not match node version {NODE_VERSION}; worker path: {prepare_worker_path:?}"); + if !execute_worker_path.is_executable() { + eprintln!("WARNING: Execute worker does not exist or is not executable. Workers directory: {:?}", workers_path); + build_exec = true; } - let worker_version = - get_worker_version(&execute_worker_path).expect("checked for worker existence"); - if worker_version != NODE_VERSION { - panic!("ERROR: Execute worker version {worker_version} does not match node version {NODE_VERSION}; worker path: {execute_worker_path:?}"); + + if let Ok(ver) = get_worker_version(&prepare_worker_path) { + if ver != NODE_VERSION { + eprintln!("WARNING: Prepare worker version {ver} does not match node version {NODE_VERSION}; worker path: {prepare_worker_path:?}"); + build_prep = true; + } + } + + if let Ok(ver) = get_worker_version(&execute_worker_path) { + if ver != NODE_VERSION { + eprintln!("WARNING: Execute worker version {ver} does not match node version {NODE_VERSION}; worker path: {execute_worker_path:?}"); + build_exec = true; + } } + maybe_build_workers(build_prep, build_exec); + // We don't want to check against the commit hash because we'd have to always rebuild // the calling crate on every commit. eprintln!("WARNING: Workers match the node version, but may have changed in recent commits. Please rebuild them if anything funny happens. Workers path: {workers_path:?}"); From 2086a4dad00cc4eab1e9f743a05b4c5e6e658823 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 25 Oct 2023 23:44:14 +0800 Subject: [PATCH 2/6] as per advices --- polkadot/node/core/pvf/src/testing.rs | 48 ++++++++++++--------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/polkadot/node/core/pvf/src/testing.rs b/polkadot/node/core/pvf/src/testing.rs index 6e971dcc1d58..a85ea2d92057 100644 --- a/polkadot/node/core/pvf/src/testing.rs +++ b/polkadot/node/core/pvf/src/testing.rs @@ -64,34 +64,28 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { static WORKER_PATHS: OnceLock> = OnceLock::new(); fn maybe_build_workers(build_prep: bool, build_exec: bool) { - let build_args = match (build_prep, build_exec) { - (true, true) => "build --bin polkadot-prepare-worker --bin polkadot-execute-worker", - (true, false) => "build --bin polkadot-prepare-worker", - (false, true) => "build --bin polkadot-execute-worker", - (false, false) => "", - }; - - if build_args.len() > 0 { + let mut build_args = vec!["build"]; + if build_prep { + build_args.push("--bin=polkadot-prepare-worker"); + } + if build_exec { + build_args.push("--bin=polkadot-execute-worker"); + } + + if build_prep || build_exec { eprintln!("Building workers..."); - let mut wd = std::env::current_exe().unwrap(); - while !wd.ends_with("polkadot-sdk") { - wd.pop(); + let mut dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + if !dir.ends_with("polkadot-sdk") { + dir.pop(); } - let child = match std::process::Command::new("cargo") - .args(build_args.split(' ')) + std::process::Command::new("cargo") + .args(build_args) .stdout(std::process::Stdio::piped()) - .current_dir(wd) - .spawn() - { - Ok(child) => child, - Err(err) => panic!("Failed to start cargo build: {}", err), - }; - - if let Err(err) = child.wait_with_output() { - panic!("Failed to build workers: {}", err); - } + .current_dir(dir) + .status() + .expect("Failed to run the build program"); } } @@ -110,25 +104,25 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { // Check that the workers are valid. if !prepare_worker_path.is_executable() { - eprintln!("WARNING: Prepare worker does not exist or is not executable. Workers directory: {:?}", workers_path); + eprintln!("Prepare worker does not exist or is not executable. Workers directory: {:?}", workers_path); build_prep = true; } if !execute_worker_path.is_executable() { - eprintln!("WARNING: Execute worker does not exist or is not executable. Workers directory: {:?}", workers_path); + eprintln!("Execute worker does not exist or is not executable. Workers directory: {:?}", workers_path); build_exec = true; } if let Ok(ver) = get_worker_version(&prepare_worker_path) { if ver != NODE_VERSION { - eprintln!("WARNING: Prepare worker version {ver} does not match node version {NODE_VERSION}; worker path: {prepare_worker_path:?}"); + eprintln!("Prepare worker version {ver} does not match node version {NODE_VERSION}; worker path: {prepare_worker_path:?}"); build_prep = true; } } if let Ok(ver) = get_worker_version(&execute_worker_path) { if ver != NODE_VERSION { - eprintln!("WARNING: Execute worker version {ver} does not match node version {NODE_VERSION}; worker path: {execute_worker_path:?}"); + eprintln!("Execute worker version {ver} does not match node version {NODE_VERSION}; worker path: {execute_worker_path:?}"); build_exec = true; } } From 1f36e3afc0d1e03c8e1aa92f7a62e40d21fdbc7b Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Thu, 26 Oct 2023 08:33:36 +0800 Subject: [PATCH 3/6] clarify --- polkadot/node/core/pvf/src/testing.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/polkadot/node/core/pvf/src/testing.rs b/polkadot/node/core/pvf/src/testing.rs index a85ea2d92057..d2332d761f12 100644 --- a/polkadot/node/core/pvf/src/testing.rs +++ b/polkadot/node/core/pvf/src/testing.rs @@ -75,6 +75,7 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { if build_prep || build_exec { eprintln!("Building workers..."); + // ensure the build targets are available let mut dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); if !dir.ends_with("polkadot-sdk") { dir.pop(); From fc7509832e6cf3858dca752f5251e44654b514dc Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Sat, 28 Oct 2023 03:46:27 +0800 Subject: [PATCH 4/6] specify package for cargo build --- polkadot/node/core/pvf/src/testing.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/polkadot/node/core/pvf/src/testing.rs b/polkadot/node/core/pvf/src/testing.rs index d2332d761f12..b066d5bc12e0 100644 --- a/polkadot/node/core/pvf/src/testing.rs +++ b/polkadot/node/core/pvf/src/testing.rs @@ -64,7 +64,7 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { static WORKER_PATHS: OnceLock> = OnceLock::new(); fn maybe_build_workers(build_prep: bool, build_exec: bool) { - let mut build_args = vec!["build"]; + let mut build_args = vec!["build", "--package=polkadot"]; if build_prep { build_args.push("--bin=polkadot-prepare-worker"); } @@ -75,16 +75,11 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { if build_prep || build_exec { eprintln!("Building workers..."); - // ensure the build targets are available - let mut dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - if !dir.ends_with("polkadot-sdk") { - dir.pop(); - } - + let dir = env!("CARGO_MANIFEST_DIR"); std::process::Command::new("cargo") .args(build_args) .stdout(std::process::Stdio::piped()) - .current_dir(dir) + .current_dir() .status() .expect("Failed to run the build program"); } From f99c2f4d3964792603ecb11a40c001ec1e7f4a4a Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Sat, 28 Oct 2023 03:51:21 +0800 Subject: [PATCH 5/6] glitch --- polkadot/node/core/pvf/src/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/node/core/pvf/src/testing.rs b/polkadot/node/core/pvf/src/testing.rs index b066d5bc12e0..8fbcc226684d 100644 --- a/polkadot/node/core/pvf/src/testing.rs +++ b/polkadot/node/core/pvf/src/testing.rs @@ -79,7 +79,7 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { std::process::Command::new("cargo") .args(build_args) .stdout(std::process::Stdio::piped()) - .current_dir() + .current_dir(dir) .status() .expect("Failed to run the build program"); } From d0b421f987b1863db34fbd2795bdb137e1513fb3 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Tue, 31 Oct 2023 16:25:34 +0800 Subject: [PATCH 6/6] let cargo decide whether to build --- polkadot/node/core/pvf/src/testing.rs | 58 ++++++++++----------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/polkadot/node/core/pvf/src/testing.rs b/polkadot/node/core/pvf/src/testing.rs index 8fbcc226684d..169b55d34b56 100644 --- a/polkadot/node/core/pvf/src/testing.rs +++ b/polkadot/node/core/pvf/src/testing.rs @@ -55,7 +55,7 @@ pub fn validate_candidate( Ok(result) } -/// Retrieves the worker paths, checks that they exist and does a version check. +/// Retrieves the worker paths and builds workers as needed. /// /// NOTE: This should only be called in dev code (tests, benchmarks) as it relies on the relative /// paths of the built workers. @@ -63,25 +63,24 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { // Only needs to be called once for the current process. static WORKER_PATHS: OnceLock> = OnceLock::new(); - fn maybe_build_workers(build_prep: bool, build_exec: bool) { - let mut build_args = vec!["build", "--package=polkadot"]; - if build_prep { - build_args.push("--bin=polkadot-prepare-worker"); - } - if build_exec { - build_args.push("--bin=polkadot-execute-worker"); - } - - if build_prep || build_exec { - eprintln!("Building workers..."); - - let dir = env!("CARGO_MANIFEST_DIR"); - std::process::Command::new("cargo") - .args(build_args) - .stdout(std::process::Stdio::piped()) - .current_dir(dir) - .status() - .expect("Failed to run the build program"); + fn build_workers() { + let build_args = vec![ + "build", + "--package=polkadot", + "--bin=polkadot-prepare-worker", + "--bin=polkadot-execute-worker", + ]; + let exit_status = std::process::Command::new("cargo") + // wasm runtime not needed + .env("SKIP_WASM_BUILD", "1") + .args(build_args) + .stdout(std::process::Stdio::piped()) + .status() + .expect("Failed to run the build program"); + + if !exit_status.success() { + eprintln!("Failed to build workers: {}", exit_status.code().unwrap()); + std::process::exit(1); } } @@ -94,40 +93,25 @@ pub fn get_and_check_worker_paths() -> (PathBuf, PathBuf) { let mut execute_worker_path = workers_path.clone(); execute_worker_path.push(EXECUTE_BINARY_NAME); - let mut build_prep = false; - let mut build_exec = false; - - // Check that the workers are valid. - + // explain why a build happens if !prepare_worker_path.is_executable() { eprintln!("Prepare worker does not exist or is not executable. Workers directory: {:?}", workers_path); - build_prep = true; } - if !execute_worker_path.is_executable() { eprintln!("Execute worker does not exist or is not executable. Workers directory: {:?}", workers_path); - build_exec = true; } - if let Ok(ver) = get_worker_version(&prepare_worker_path) { if ver != NODE_VERSION { eprintln!("Prepare worker version {ver} does not match node version {NODE_VERSION}; worker path: {prepare_worker_path:?}"); - build_prep = true; } } - if let Ok(ver) = get_worker_version(&execute_worker_path) { if ver != NODE_VERSION { eprintln!("Execute worker version {ver} does not match node version {NODE_VERSION}; worker path: {execute_worker_path:?}"); - build_exec = true; } } - maybe_build_workers(build_prep, build_exec); - - // We don't want to check against the commit hash because we'd have to always rebuild - // the calling crate on every commit. - eprintln!("WARNING: Workers match the node version, but may have changed in recent commits. Please rebuild them if anything funny happens. Workers path: {workers_path:?}"); + build_workers(); Mutex::new((prepare_worker_path, execute_worker_path)) });