-
Notifications
You must be signed in to change notification settings - Fork 98
feat: add verbose and extra-args to dfx build #347
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ use console::Style; | |
| use ic_http_agent::CanisterId; | ||
| use indicatif::{ProgressBar, ProgressDrawTarget, ProgressStyle}; | ||
| use rand::{thread_rng, Rng}; | ||
| use std::cell::RefCell; | ||
| use std::collections::{HashMap, HashSet}; | ||
| use std::ffi::OsStr; | ||
| use std::io::Read; | ||
|
|
@@ -31,8 +32,22 @@ pub fn construct() -> App<'static, 'static> { | |
| .takes_value(false) | ||
| .help(UserMessage::SkipFrontend.to_str()), | ||
| ) | ||
| .arg( | ||
| Arg::with_name("extra-args") | ||
| .long("extra-args") | ||
| .takes_value(true) | ||
| .help(UserMessage::BuildArgs.to_str()), | ||
| ) | ||
| .arg( | ||
| Arg::with_name("verbose") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verbosity should be a global flag. Also, this code is most likely to be wiped out when we start using slog, so why not do a PR now to do that instead?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. Do you prefer to put the flag to the global scope with only build supporting it, or promote it to global after we have verbose mode for most commands?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This again seems to be a global change, which should be done in a dedicated PR. I don't see an urgency to switch to slog, but I do see the urgency to enable verbose mode on build.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can start using slog here honestly. It doesn't have to be global. @chenyan-dfinity let's sync on this. |
||
| .long("verbose") | ||
| .takes_value(false) | ||
| .help(UserMessage::BuildVerbose.to_str()), | ||
| ) | ||
| } | ||
|
|
||
| thread_local!(static VERBOSE: RefCell<bool> = RefCell::new(false)); | ||
|
|
||
| fn get_asset_fn(assets: &AssetMap) -> String { | ||
| // Create the if/else series. | ||
| let mut cases = String::new(); | ||
|
|
@@ -79,9 +94,9 @@ struct MotokoParams<'a> { | |
| // The following fields will not be used by self.to_args() | ||
| // TODO move input into self.to_args once inject_code is deprecated. | ||
| input: &'a Path, | ||
| verbose: bool, | ||
| surpress_warning: bool, | ||
| inject_code: bool, | ||
| extra_args: &'a str, | ||
| } | ||
|
|
||
| impl MotokoParams<'_> { | ||
|
|
@@ -98,6 +113,10 @@ impl MotokoParams<'_> { | |
| cmd.args(&["--actor-alias", name, canister_id]); | ||
| } | ||
| }; | ||
| if !self.extra_args.is_empty() { | ||
| let args: Vec<_> = self.extra_args.split(' ').collect(); | ||
| cmd.args(args); | ||
| }; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -137,7 +156,7 @@ fn motoko_compile(cache: &dyn Cache, params: &MotokoParams<'_>, assets: &AssetMa | |
| .arg("--package") | ||
| .arg("stdlib") | ||
| .arg(&stdlib_path.as_path()); | ||
| run_command(cmd, params.verbose, params.surpress_warning)?; | ||
| run_command(cmd, params.surpress_warning)?; | ||
|
|
||
| if params.inject_code { | ||
| std::fs::remove_file(input_path)?; | ||
|
|
@@ -201,7 +220,7 @@ fn parse_moc_deps(line: &str) -> DfxResult<MotokoImport> { | |
| } | ||
| None => match fullpath { | ||
| Some(fullpath) => { | ||
| let path = PathBuf::from(fullpath); | ||
| let path = PathBuf::from(fullpath).canonicalize()?; | ||
| if !path.is_file() { | ||
| return Err(DfxError::BuildError(BuildErrorKind::DependencyError( | ||
| format!("Cannot find import file {}", path.display()), | ||
|
|
@@ -228,7 +247,7 @@ fn find_deps(cache: &dyn Cache, input_path: &Path, deps: &mut MotokoImports) -> | |
|
|
||
| let mut cmd = cache.get_binary_command("moc")?; | ||
| let cmd = cmd.arg("--print-deps").arg(&input_path); | ||
| let output = run_command(cmd, false, false)?; | ||
| let output = run_command(cmd, false)?; | ||
|
|
||
| let output = String::from_utf8_lossy(&output.stdout); | ||
| for dep in output.lines() { | ||
|
|
@@ -248,18 +267,16 @@ fn find_deps(cache: &dyn Cache, input_path: &Path, deps: &mut MotokoImports) -> | |
| fn build_did_js(cache: &dyn Cache, input_path: &Path, output_path: &Path) -> DfxResult { | ||
| let mut cmd = cache.get_binary_command("didc")?; | ||
| let cmd = cmd.arg("--js").arg(&input_path).arg("-o").arg(&output_path); | ||
| run_command(cmd, false, false)?; | ||
| run_command(cmd, false)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn run_command( | ||
| cmd: &mut std::process::Command, | ||
| verbose: bool, | ||
| surpress_warning: bool, | ||
| ) -> DfxResult<Output> { | ||
| if verbose { | ||
| println!("{:?}", cmd); | ||
| } | ||
| fn run_command(cmd: &mut std::process::Command, surpress_warning: bool) -> DfxResult<Output> { | ||
| VERBOSE.with(|v| { | ||
| if *v.borrow() { | ||
| println!("{:?}", cmd); | ||
| } | ||
| }); | ||
| let output = cmd.output()?; | ||
| if !output.status.success() { | ||
| Err(DfxError::BuildError(BuildErrorKind::CompilerError( | ||
|
|
@@ -306,6 +323,7 @@ fn build_file( | |
| name: &str, | ||
| id_map: &CanisterIdMap, | ||
| assets: &AssetMap, | ||
| extra_args: &str, | ||
| ) -> DfxResult { | ||
| let canister_info = CanisterInfo::load(config, name).map_err(|_| { | ||
| BuildError(BuildErrorKind::CanisterNameIsNotInConfigError( | ||
|
|
@@ -345,11 +363,11 @@ fn build_file( | |
| }, | ||
| surpress_warning: false, | ||
| inject_code: true, | ||
| verbose: false, | ||
| input: &input_path, | ||
| output: &output_wasm_path, | ||
| idl_path: &idl_dir_path, | ||
| idl_map: &id_map, | ||
| extra_args, | ||
| }; | ||
| motoko_compile(cache.as_ref(), ¶ms, assets)?; | ||
| // Generate IDL | ||
|
|
@@ -362,11 +380,11 @@ fn build_file( | |
| // Surpress the warnings the second time we call moc | ||
| surpress_warning: true, | ||
| inject_code: false, | ||
| verbose: false, | ||
| input: &input_path, | ||
| output: &output_idl_path, | ||
| idl_path: &idl_dir_path, | ||
| idl_map: &id_map, | ||
| extra_args, | ||
| }; | ||
| motoko_compile(cache.as_ref(), ¶ms, &HashMap::new())?; | ||
| std::fs::copy(&output_idl_path, &idl_file_path)?; | ||
|
|
@@ -462,7 +480,6 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| env.get_cache().install()?; | ||
|
|
||
| let green = Style::new().green().bold(); | ||
|
|
||
| let status_bar = ProgressBar::new_spinner(); | ||
| status_bar.set_draw_target(ProgressDrawTarget::stderr()); | ||
| status_bar.set_message("Building canisters..."); | ||
|
|
@@ -475,6 +492,13 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| } | ||
| let canisters = maybe_canisters.as_ref().unwrap(); | ||
|
|
||
| if args.is_present("verbose") { | ||
| VERBOSE.with(|v| { | ||
| *v.borrow_mut() = true; | ||
| }); | ||
| status_bar.set_draw_target(ProgressDrawTarget::hidden()); | ||
| } | ||
|
|
||
| // Build canister id map and dependency graph | ||
| let mut id_map = HashMap::new(); | ||
| let mut deps = HashMap::new(); | ||
|
|
@@ -518,10 +542,27 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| ); | ||
| build_stage_bar.enable_steady_tick(80); | ||
|
|
||
| if args.is_present("verbose") { | ||
| build_stage_bar.set_draw_target(ProgressDrawTarget::hidden()); | ||
| } | ||
|
|
||
| let extra_args: &str = args.value_of("extra-args").unwrap_or("").trim(); | ||
|
|
||
| // Build canister | ||
| for name in &seq.canisters { | ||
| build_stage_bar.println(&format!("{} canister {}", green.apply_to("Building"), name)); | ||
| match build_file(env, &config, name, &seq.get_ids(name), &HashMap::new()) { | ||
| build_stage_bar.println(&format!( | ||
| "{} canister {}", | ||
| green.apply_to("Compiling"), | ||
| name | ||
| )); | ||
| match build_file( | ||
| env, | ||
| &config, | ||
| name, | ||
| &seq.get_ids(name), | ||
| &HashMap::new(), | ||
| extra_args, | ||
| ) { | ||
| Ok(()) => {} | ||
| Err(e) => { | ||
| build_stage_bar.abandon(); | ||
|
|
@@ -547,7 +588,7 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| .current_dir(config.get_project_root()) | ||
| .stdout(std::process::Stdio::piped()) | ||
| .stderr(std::process::Stdio::piped()); | ||
| run_command(&mut cmd, false, false)?; | ||
| run_command(&mut cmd, false)?; | ||
|
|
||
| build_stage_bar.inc(1); | ||
| build_stage_bar.println(&format!( | ||
|
|
@@ -587,7 +628,14 @@ pub fn exec(env: &dyn Environment, args: &ArgMatches<'_>) -> DfxResult { | |
| } | ||
| } | ||
|
|
||
| match build_file(env, &config, &name, &seq.get_ids(&name), &assets) { | ||
| match build_file( | ||
| env, | ||
| &config, | ||
| &name, | ||
| &seq.get_ids(&name), | ||
| &assets, | ||
| extra_args, | ||
| ) { | ||
| Ok(()) => {} | ||
| Err(e) => { | ||
| build_stage_bar.abandon(); | ||
|
|
@@ -658,23 +706,23 @@ mod tests { | |
| build_target: BuildTarget::Debug, | ||
| surpress_warning: false, | ||
| inject_code: true, | ||
| verbose: false, | ||
| input: &input_path, | ||
| output: Path::new("/out/file.wasm"), | ||
| idl_path: Path::new("."), | ||
| idl_map: &HashMap::new(), | ||
| extra_args: "", | ||
| }; | ||
| motoko_compile(&cache, ¶ms, &HashMap::new()).expect("Function failed."); | ||
|
|
||
| let params = MotokoParams { | ||
| build_target: BuildTarget::IDL, | ||
| surpress_warning: false, | ||
| inject_code: false, | ||
| verbose: false, | ||
| input: Path::new("/in/file.mo"), | ||
| output: Path::new("/out/file.did"), | ||
| idl_path: Path::new("."), | ||
| idl_map: &HashMap::new(), | ||
| extra_args: "", | ||
| }; | ||
| motoko_compile(&cache, ¶ms, &HashMap::new()).expect("Function failed (didl_compile)"); | ||
| build_did_js( | ||
|
|
@@ -738,7 +786,7 @@ mod tests { | |
| ) | ||
| .unwrap(); | ||
|
|
||
| build_file(&env, &config, "name", &HashMap::new(), &HashMap::new()) | ||
| build_file(&env, &config, "name", &HashMap::new(), &HashMap::new(), "") | ||
| .expect("Function failed - build_file"); | ||
| assert!(output_path.exists()); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should separate those between where they're used.
extra-argshas no semantic meaning (moc-args?compiler-args?). We're also building more and more of a Motoko-specific build system here, and I don't agree with that direction. We can chat offline.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on slack, the proper way is to add flag into a config file like
dfx.json. Before we have the design, it would be nice to have an CLI flag to pass in extra flags. I'm fine rename it tomoc-args, but the intension is to provide a way to pass in extra arguments to any compilers. We can also add a comment in the user message warning that this flag is experimental and will go away.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on this? Let's place discussions/more details online.
I agree with the compiler arguments flag. I don't like the verbosity change -- or maybe I am missing something. I think we can use slog just in build instead, and then fully integrate it piece by piece.