From 4e6989c201f286ef7709f6c71651675808fbf62c Mon Sep 17 00:00:00 2001 From: Jaap Frolich Date: Thu, 13 Mar 2025 09:34:05 +0100 Subject: [PATCH 1/5] fix --- src/build/compile.rs | 44 +++++++++++++++++++++++++++++++++++--------- src/build/parse.rs | 4 ++-- src/helpers.rs | 9 ++++++++- 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/src/build/compile.rs b/src/build/compile.rs index b780908..1f05af0 100644 --- a/src/build/compile.rs +++ b/src/build/compile.rs @@ -148,7 +148,7 @@ pub fn compile( "cmi", ); - let cmi_digest = helpers::compute_file_hash(&cmi_path); + let cmi_digest = helpers::compute_file_hash(&Path::new(&cmi_path)); let package = build_state .get_package(&module.package_name) @@ -189,7 +189,7 @@ pub fn compile( &build_state.workspace_root, build_dev_deps, ); - let cmi_digest_after = helpers::compute_file_hash(&cmi_path); + let cmi_digest_after = helpers::compute_file_hash(&Path::new(&cmi_path)); // we want to compare both the hash of interface and the implementation // compile assets to verify that nothing changed. We also need to checke the interface @@ -466,7 +466,11 @@ pub fn compiler_args( format!( "{}:{}:{}", root_config.get_module(), - Path::new(file_path).parent().unwrap().to_str().unwrap(), + // compile into lib/bs and later install in the right place + Path::new("lib/bs") + .join(Path::new(file_path).parent().unwrap()) + .to_str() + .unwrap(), root_config.get_suffix() ), ] @@ -610,15 +614,37 @@ fn compile_file( // we need to copy the source file to the build directory. // editor tools expects the source file in lib/bs for finding the current package // and in lib/ocaml when referencing modules in other packages - let _ = std::fs::copy( - std::path::Path::new(&package.path).join(path), - std::path::Path::new(&package.get_build_path()).join(path), - ) - .expect("copying source file failed"); + + // update: we now generate the file in lib/bs/... and then install it in the right + // in-source location if the hash is different + + // the in-source file. This is the currently "installed" file + let in_source_hash = + helpers::compute_file_hash(&std::path::Path::new(&package.path).join(path)); + + // this is the file that we just generated + let generated_hash = helpers::compute_file_hash( + &std::path::Path::new(&package.get_build_path()).join(path), + ); + + match (in_source_hash, generated_hash) { + (Some(in_source_hash), Some(generated_hash)) if in_source_hash == generated_hash => { + // do nothing, the hashes are the same! + () + } + _ => { + // copy the file to the in-source location + let _ = std::fs::copy( + std::path::Path::new(&package.get_bs_build_path()).join(path), + std::path::Path::new(&package.path).join(path), + ) + .expect("copying source file failed"); + } + } let _ = std::fs::copy( std::path::Path::new(&package.path).join(path), - std::path::Path::new(&package.get_build_path()) + std::path::Path::new(&package.get_ocaml_build_path()) .join(std::path::Path::new(path).file_name().unwrap()), ) .expect("copying source file failed"); diff --git a/src/build/parse.rs b/src/build/parse.rs index 92935ce..023a329 100644 --- a/src/build/parse.rs +++ b/src/build/parse.rs @@ -205,9 +205,9 @@ pub fn generate_asts( // probably better to do this in a different function // specific to compiling mlmaps let compile_path = package.get_mlmap_compile_path(); - let mlmap_hash = helpers::compute_file_hash(&compile_path); + let mlmap_hash = helpers::compute_file_hash(&Path::new(&compile_path)); namespaces::compile_mlmap(package, module_name, &build_state.bsc_path); - let mlmap_hash_after = helpers::compute_file_hash(&compile_path); + let mlmap_hash_after = helpers::compute_file_hash(&Path::new(&compile_path)); let suffix = package .namespace diff --git a/src/helpers.rs b/src/helpers.rs index a41c609..6a759c8 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -308,13 +308,20 @@ pub fn format_namespaced_module_name(module_name: &str) -> String { } } -pub fn compute_file_hash(path: &str) -> Option { +pub fn compute_file_hash(path: &Path) -> Option { match fs::read(path) { Ok(str) => Some(blake3::hash(&str)), Err(_) => None, } } +pub fn get_source_file_from_rescript_file(path: &Path, suffix: &str) -> PathBuf { + path.with_extension( + // suffix.to_string includes the ., so we need to remove it + &suffix.to_string()[1..], + ) +} + fn has_rescript_config(path: &Path) -> bool { path.join("bsconfig.json").exists() || path.join("rescript.json").exists() } From d1fa395152873b34d2e57a381ae3e7444ad66134 Mon Sep 17 00:00:00 2001 From: Jaap Frolich Date: Fri, 14 Mar 2025 10:07:38 +0100 Subject: [PATCH 2/5] fix rebase issues --- src/build/compile.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/build/compile.rs b/src/build/compile.rs index 1f05af0..3865504 100644 --- a/src/build/compile.rs +++ b/src/build/compile.rs @@ -611,10 +611,6 @@ fn compile_file( implementation: Implementation { path, .. }, .. }) => { - // we need to copy the source file to the build directory. - // editor tools expects the source file in lib/bs for finding the current package - // and in lib/ocaml when referencing modules in other packages - // update: we now generate the file in lib/bs/... and then install it in the right // in-source location if the hash is different @@ -635,19 +631,12 @@ fn compile_file( _ => { // copy the file to the in-source location let _ = std::fs::copy( - std::path::Path::new(&package.get_bs_build_path()).join(path), + std::path::Path::new(&package.get_build_path()).join(path), std::path::Path::new(&package.path).join(path), ) .expect("copying source file failed"); } } - - let _ = std::fs::copy( - std::path::Path::new(&package.path).join(path), - std::path::Path::new(&package.get_ocaml_build_path()) - .join(std::path::Path::new(path).file_name().unwrap()), - ) - .expect("copying source file failed"); } _ => (), } From 916ba65915352ea0a4a673515bc296e8f9e91f03 Mon Sep 17 00:00:00 2001 From: Jaap Frolich Date: Tue, 18 Mar 2025 15:50:57 +0100 Subject: [PATCH 3/5] Fix local warnings --- src/build.rs | 13 +++++---- src/build/compile.rs | 67 ++++++++++++++++++++++++------------------- src/build/packages.rs | 2 ++ 3 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/build.rs b/src/build.rs index 9b707f5..7112589 100644 --- a/src/build.rs +++ b/src/build.rs @@ -405,9 +405,6 @@ pub fn incremental_build( } pb.finish(); if !compile_errors.is_empty() { - if helpers::contains_ascii_characters(&compile_warnings) { - println!("{}", &compile_warnings); - } if show_progress { println!( "{}{} {}Compiled {} modules in {:.2}s", @@ -418,7 +415,12 @@ pub fn incremental_build( default_timing.unwrap_or(compile_duration).as_secs_f64() ); } - println!("{}", &compile_errors); + if helpers::contains_ascii_characters(&compile_warnings) { + println!("{}", &compile_warnings); + } + if helpers::contains_ascii_characters(&compile_errors) { + println!("{}", &compile_errors); + } // mark the original files as dirty again, because we didn't complete a full build for (module_name, module) in build_state.modules.iter_mut() { if tracked_dirty_modules.contains(module_name) { @@ -437,8 +439,9 @@ pub fn incremental_build( default_timing.unwrap_or(compile_duration).as_secs_f64() ); } + if helpers::contains_ascii_characters(&compile_warnings) { - log::warn!("{}", &compile_warnings); + println!("{}", &compile_warnings); } Ok(()) } diff --git a/src/build/compile.rs b/src/build/compile.rs index 3865504..2d866fc 100644 --- a/src/build/compile.rs +++ b/src/build/compile.rs @@ -8,7 +8,7 @@ use super::packages; use crate::config; use crate::helpers; use ahash::{AHashMap, AHashSet}; -use anyhow::{anyhow, Result}; +use anyhow::anyhow; use console::style; use log::{debug, trace}; use rayon::prelude::*; @@ -22,7 +22,7 @@ pub fn compile( inc: impl Fn() + std::marker::Sync, set_length: impl Fn(u64), build_dev_deps: bool, -) -> Result<(String, String, usize)> { +) -> anyhow::Result<(String, String, usize)> { let mut compiled_modules = AHashSet::::new(); let dirty_modules = build_state .modules @@ -513,15 +513,14 @@ fn compile_file( project_root: &str, workspace_root: &Option, build_dev_deps: bool, -) -> Result> { +) -> Result, String> { let ocaml_build_path_abs = package.get_ocaml_build_path(); let build_path_abs = package.get_build_path(); let implementation_file_path = match &module.source_type { SourceType::SourceFile(ref source_file) => Ok(&source_file.implementation.path), - sourcetype => Err(anyhow!( + sourcetype => Err(format!( "Tried to compile a file that is not a source file ({}). Path to AST: {}. ", - sourcetype, - ast_path + sourcetype, ast_path )), }?; let module_name = helpers::file_path_to_module_name(implementation_file_path, &package.namespace); @@ -548,12 +547,11 @@ fn compile_file( Ok(x) if !x.status.success() => { let stderr = String::from_utf8_lossy(&x.stderr); let stdout = String::from_utf8_lossy(&x.stdout); - Err(anyhow!(stderr.to_string() + &stdout)) + Err(stderr.to_string() + &stdout) } - Err(e) => Err(anyhow!( + Err(e) => Err(format!( "Could not compile file. Error: {}. Path to AST: {:?}", - e, - ast_path + e, ast_path )), Ok(x) => { let err = std::str::from_utf8(&x.stderr) @@ -602,26 +600,30 @@ fn compile_file( ocaml_build_path_abs.to_string() + "/" + &module_name + ".cmi", ); } - match &module.source_type { - SourceType::SourceFile(SourceFile { - interface: Some(Interface { path, .. }), - .. - }) - | SourceType::SourceFile(SourceFile { - implementation: Implementation { path, .. }, - .. - }) => { + match (&module.source_type, is_interface) { + ( + SourceType::SourceFile(SourceFile { + implementation: Implementation { path, .. }, + .. + }), + false, + ) => { // update: we now generate the file in lib/bs/... and then install it in the right // in-source location if the hash is different // the in-source file. This is the currently "installed" file let in_source_hash = - helpers::compute_file_hash(&std::path::Path::new(&package.path).join(path)); + helpers::compute_file_hash(&helpers::get_source_file_from_rescript_file( + &std::path::Path::new(&package.path).join(path), + &root_package.config.get_suffix(), + )); // this is the file that we just generated - let generated_hash = helpers::compute_file_hash( - &std::path::Path::new(&package.get_build_path()).join(path), - ); + let generated_hash = + helpers::compute_file_hash(&helpers::get_source_file_from_rescript_file( + &std::path::Path::new(&package.get_build_path()).join(path), + &root_package.config.get_suffix(), + )); match (in_source_hash, generated_hash) { (Some(in_source_hash), Some(generated_hash)) if in_source_hash == generated_hash => { @@ -629,12 +631,19 @@ fn compile_file( () } _ => { + let source = helpers::get_source_file_from_rescript_file( + &std::path::Path::new(&package.get_build_path()).join(path), + &root_package.config.get_suffix(), + ); + let destination = helpers::get_source_file_from_rescript_file( + &std::path::Path::new(&package.path).join(path), + &root_package.config.get_suffix(), + ); + // println!("copying source file to in-source location"); + // println!("{}", source.display()); + // println!("{}", destination.display()); // copy the file to the in-source location - let _ = std::fs::copy( - std::path::Path::new(&package.get_build_path()).join(path), - std::path::Path::new(&package.path).join(path), - ) - .expect("copying source file failed"); + let _ = std::fs::copy(source, destination).expect("copying source file failed"); } } } @@ -642,7 +651,7 @@ fn compile_file( } if helpers::contains_ascii_characters(&err) { - if package.is_pinned_dep { + if package.is_pinned_dep || package.is_local_dep { // supress warnings of external deps Ok(Some(err)) } else { diff --git a/src/build/packages.rs b/src/build/packages.rs index cc3dae2..f65c7e0 100644 --- a/src/build/packages.rs +++ b/src/build/packages.rs @@ -59,6 +59,7 @@ pub struct Package { pub path: String, pub dirs: Option>, pub is_pinned_dep: bool, + pub is_local_dep: bool, pub is_root: bool, } @@ -410,6 +411,7 @@ fn make_package(config: config::Config, package_path: &str, is_pinned_dep: bool, .to_string(), dirs: None, is_pinned_dep, + is_local_dep: !package_path.contains("node_modules"), is_root, } } From dfb083eba9c525ff76d35b952e18ebb11ec92dd6 Mon Sep 17 00:00:00 2001 From: Jaap Frolich Date: Fri, 21 Mar 2025 09:32:09 +0100 Subject: [PATCH 4/5] remove copying js file --- src/build/compile.rs | 55 +------------------------------------------- 1 file changed, 1 insertion(+), 54 deletions(-) diff --git a/src/build/compile.rs b/src/build/compile.rs index 2d866fc..86c8443 100644 --- a/src/build/compile.rs +++ b/src/build/compile.rs @@ -466,11 +466,7 @@ pub fn compiler_args( format!( "{}:{}:{}", root_config.get_module(), - // compile into lib/bs and later install in the right place - Path::new("lib/bs") - .join(Path::new(file_path).parent().unwrap()) - .to_str() - .unwrap(), + Path::new(file_path).parent().unwrap().to_str().unwrap(), root_config.get_suffix() ), ] @@ -600,55 +596,6 @@ fn compile_file( ocaml_build_path_abs.to_string() + "/" + &module_name + ".cmi", ); } - match (&module.source_type, is_interface) { - ( - SourceType::SourceFile(SourceFile { - implementation: Implementation { path, .. }, - .. - }), - false, - ) => { - // update: we now generate the file in lib/bs/... and then install it in the right - // in-source location if the hash is different - - // the in-source file. This is the currently "installed" file - let in_source_hash = - helpers::compute_file_hash(&helpers::get_source_file_from_rescript_file( - &std::path::Path::new(&package.path).join(path), - &root_package.config.get_suffix(), - )); - - // this is the file that we just generated - let generated_hash = - helpers::compute_file_hash(&helpers::get_source_file_from_rescript_file( - &std::path::Path::new(&package.get_build_path()).join(path), - &root_package.config.get_suffix(), - )); - - match (in_source_hash, generated_hash) { - (Some(in_source_hash), Some(generated_hash)) if in_source_hash == generated_hash => { - // do nothing, the hashes are the same! - () - } - _ => { - let source = helpers::get_source_file_from_rescript_file( - &std::path::Path::new(&package.get_build_path()).join(path), - &root_package.config.get_suffix(), - ); - let destination = helpers::get_source_file_from_rescript_file( - &std::path::Path::new(&package.path).join(path), - &root_package.config.get_suffix(), - ); - // println!("copying source file to in-source location"); - // println!("{}", source.display()); - // println!("{}", destination.display()); - // copy the file to the in-source location - let _ = std::fs::copy(source, destination).expect("copying source file failed"); - } - } - } - _ => (), - } if helpers::contains_ascii_characters(&err) { if package.is_pinned_dep || package.is_local_dep { From 8f10888d3118024af708bd302b47a56b1d73db4a Mon Sep 17 00:00:00 2001 From: Jaap Frolich Date: Mon, 7 Apr 2025 16:40:54 +0200 Subject: [PATCH 5/5] fix tests --- src/build/packages.rs | 1 + src/helpers.rs | 7 ------- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/build/packages.rs b/src/build/packages.rs index f65c7e0..444815e 100644 --- a/src/build/packages.rs +++ b/src/build/packages.rs @@ -914,6 +914,7 @@ mod test { dirs: None, is_pinned_dep: false, is_root: false, + is_local_dep: false, } } #[test] diff --git a/src/helpers.rs b/src/helpers.rs index 6a759c8..0a091b1 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -315,13 +315,6 @@ pub fn compute_file_hash(path: &Path) -> Option { } } -pub fn get_source_file_from_rescript_file(path: &Path, suffix: &str) -> PathBuf { - path.with_extension( - // suffix.to_string includes the ., so we need to remove it - &suffix.to_string()[1..], - ) -} - fn has_rescript_config(path: &Path) -> bool { path.join("bsconfig.json").exists() || path.join("rescript.json").exists() }