Skip to content

Rebuild source dependencies when package output settings change #8540

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.

Sign up for GitHub

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

Open
cknitt wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rebuild source dependencies when package output settings change #8540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#### :bug: Bug fix

- Rewatch: rebuild source dependencies when the consuming project's package output settings change. https://github.com/rescript-lang/rescript/pull/8540
- Preserve multibyte characters when wrapping long source lines in compiler code frames. https://github.com/rescript-lang/rescript/pull/8520

#### :memo: Documentation
Expand Down
7 changes: 5 additions & 2 deletions rewatch/src/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ pub mod read_compile_state;

use self::parse::parser_args;
use crate::build::compile::{mark_modules_with_deleted_deps_dirty, mark_modules_with_expired_deps_dirty};
use crate::build::compiler_info::{CompilerCheckResult, verify_compiler_info, write_compiler_info};
use crate::build::compiler_info::{
CompilerCheckResult, get_package_output_specs, verify_compiler_info, write_compiler_info,
};
use crate::config::SourceMapCommand;
use crate::helpers::emojis::*;
use crate::helpers::{self};
Expand Down Expand Up @@ -191,7 +193,8 @@ pub fn initialize_build(
let source_map_args = project_context
.get_root_config()
.get_source_map_args(source_map_command);
let compiler_check = verify_compiler_info(&packages, &compiler, &source_map_args);
let package_output_specs = get_package_output_specs(project_context.get_root_config());
let compiler_check = verify_compiler_info(&packages, &compiler, &source_map_args, &package_output_specs);

if !packages::validate_packages_dependencies(&packages) {
return Err(anyhow!("Failed to validate package dependencies"));
Expand Down
6 changes: 3 additions & 3 deletions rewatch/src/build/clean.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn remove_iast(package: &packages::Package, source_file: &Path) {
));
}

fn remove_mjs_file(source_file: &Path, suffix: &str) {
pub(crate) fn remove_js_file(source_file: &Path, suffix: &str) {
let js_file = source_file.with_extension(
// suffix.to_string includes the ., so we need to remove it
&suffix[1..],
Expand Down Expand Up @@ -102,7 +102,7 @@ fn clean_source_files(build_state: &BuildState, root_config: &Config) {

rescript_file_locations
.par_iter()
.for_each(|(rescript_file_location, suffix)| remove_mjs_file(rescript_file_location, suffix));
.for_each(|(rescript_file_location, suffix)| remove_js_file(rescript_file_location, suffix));
}

// TODO: change to scan_previous_build => CompileAssetsState
Expand Down Expand Up @@ -145,7 +145,7 @@ pub fn cleanup_previous_build(
.get(package_name)
.expect("Could not find package");
remove_compile_assets(package, res_file_location);
remove_mjs_file(res_file_location, suffix);
remove_js_file(res_file_location, suffix);
remove_iast(package, res_file_location);
remove_ast(package, res_file_location);
match helpers::get_extension(ast_file_path).as_str() {
Expand Down
182 changes: 165 additions & 17 deletions rewatch/src/build/compiler_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,25 @@ use serde::{Deserialize, Serialize};
use std::fs::File;
use std::io::Write;

#[derive(Serialize, Deserialize, Debug, PartialEq)]
pub(crate) struct PackageOutputSpec {
module: String,
in_source: bool,
suffix: String,
}

pub(crate) fn get_package_output_specs(config: &crate::config::Config) -> Vec<PackageOutputSpec> {
config
.get_package_specs()
.iter()
.map(|spec| PackageOutputSpec {
module: spec.module.as_str().to_string(),
in_source: spec.in_source,
suffix: config.get_suffix(spec),
})
.collect()
}

// In order to have a loose coupling with the compiler, we don't want to have a hard dependency on the compiler's structs
// We can use this struct to parse the compiler-info.json file
// If something is not there, that is fine, we will treat it as a mismatch
Expand All @@ -19,6 +38,7 @@ struct CompilerInfoFile {
bsc_hash: String,
rescript_config_hash: String,
source_map_args: Vec<String>,
package_output_specs: Vec<PackageOutputSpec>,
runtime_path: String,
generated_at: String,
}
Expand All @@ -28,37 +48,69 @@ pub enum CompilerCheckResult {
CleanedPackagesDueToCompiler,
}

fn remove_package_outputs(package: &packages::Package, output_specs: &[PackageOutputSpec]) {
let Some(source_files) = &package.source_files else {
return;
};

for output_spec in output_specs {
let output_dir = match output_spec.module.as_str() {
"commonjs" => package.get_js_path(),
"esmodule" => package.get_esmodule_path(),
_ => continue,
};

source_files
.keys()
Comment on lines +63 to +64

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove outputs for sources missing from discovery

When a dependency loses a source file in the same update that changes the root package specs, source_files contains only the dependency's current files, so the previous output for the removed module is never visited here. Verification subsequently calls clean_package, deleting the old AST/compiler state before cleanup_previous_build can discover that removed module; the build then emits the remaining modules under the new spec but leaves the removed module's old .cjs/.mjs output behind. Use the previous build manifest or scan the previous output locations rather than limiting removal to current source keys.

Useful? React with 👍 / 👎.

.filter(|source_file| {
source_file
.extension()
.and_then(|extension| extension.to_str())
.is_some_and(helpers::is_implementation_file)
})
.for_each(|source_file| {
let source_file = if output_spec.in_source {
package.path.join(source_file)
} else {
output_dir.join(source_file)
};
clean::remove_js_file(&source_file, &output_spec.suffix);
});
}
}

fn get_rescript_config_hash(package: &packages::Package) -> Option<String> {
helpers::compute_file_hash(&package.config.path).map(|hash| hash.to_hex().to_string())
}

pub fn verify_compiler_info(
pub(crate) fn verify_compiler_info(
packages: &AHashMap<String, packages::Package>,
compiler: &CompilerInfo,
source_map_args: &[String],
package_output_specs: &[PackageOutputSpec],
) -> CompilerCheckResult {
let mismatched_packages = packages
.values()
.filter(|package| {
.filter_map(|package| {
let info_path = package.get_compiler_info_path();
let Ok(contents) = std::fs::read_to_string(&info_path) else {
// Can't read the compiler-info.json file, maybe there is no current build.
// We check if the ocaml build folder exists, if not, we assume the compiler is not installed
return logs::does_ocaml_build_compiler_log_exist(package);
return logs::does_ocaml_build_compiler_log_exist(package).then_some((package, None));
};

let parsed: Result<CompilerInfoFile, _> = serde_json::from_str(&contents);
let parsed = match parsed {
Ok(p) => p,
Err(_) => return true, // unknown or invalid format -> treat as mismatch
Err(_) => return Some((package, None)), // unknown or invalid format -> treat as mismatch
Comment on lines 102 to +105

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Migrate compiler-info files from the previous schema

When the first build with this version both upgrades Rewatch and changes package-specs, every compiler-info.json written by the previous version lacks the newly required package_output_specs field, so deserialization deterministically takes this branch and records no previous specs. The later cleanup therefore skips generated-output removal while deleting compiler assets, and recompilation leaves the old suffix or output directory alongside the new one. Fresh evidence beyond the earlier stale-output comment is the newly required schema field, which makes this path unavoidable for all pre-change metadata; deserialize it as optional or perform conservative output cleanup during migration.

Useful? React with 👍 / 👎.

};

let current_bsc_path_str = compiler.bsc_path.to_string_lossy();
let current_bsc_hash_hex = compiler.bsc_hash.to_hex().to_string();
let current_runtime_path_str = compiler.runtime_path.to_string_lossy();
let current_rescript_config_hash = match get_rescript_config_hash(package) {
Some(hash) => hash,
None => return true, // can't compute hash -> treat as mismatch
None => return Some((package, None)), // can't compute hash -> treat as mismatch
};

let mut mismatch = false;
Expand Down Expand Up @@ -107,24 +159,43 @@ pub fn verify_compiler_info(
);
mismatch = true;
}
let package_output_specs_changed = parsed.package_output_specs != package_output_specs;
if package_output_specs_changed {
log::debug!(
"compiler-info mismatch for {}: package_output_specs changed (stored={:?}, current={:?})",
package.name,
parsed.package_output_specs,
package_output_specs
);
mismatch = true;
}

mismatch
mismatch.then(|| {
let previous_output_specs =
package_output_specs_changed.then_some(parsed.package_output_specs);
(package, previous_output_specs)
})
})
.collect::<Vec<_>>();

let cleaned_count = mismatched_packages.len();
mismatched_packages.par_iter().for_each(|package| {
// suppress progress printing during init to avoid breaking step output
clean::clean_package(false, true, package);
});
mismatched_packages
.into_par_iter()
.for_each(|(package, previous_output_specs)| {
if let Some(previous_output_specs) = previous_output_specs {
remove_package_outputs(package, &previous_output_specs);
}
// suppress progress printing during init to avoid breaking step output
clean::clean_package(false, true, package);
});
if cleaned_count == 0 {
CompilerCheckResult::SameCompilerAsLastRun
} else {
CompilerCheckResult::CleanedPackagesDueToCompiler
}
}

pub fn write_compiler_info(build_state: &BuildCommandState) {
pub(crate) fn write_compiler_info(build_state: &BuildCommandState) {
let bsc_path = build_state.compiler_info.bsc_path.to_string_lossy().to_string();
let bsc_hash = build_state.compiler_info.bsc_hash.to_hex().to_string();
let runtime_path = build_state
Expand All @@ -135,6 +206,7 @@ pub fn write_compiler_info(build_state: &BuildCommandState) {
let source_map_args = build_state
.get_root_config()
.get_source_map_args(build_state.source_map_command);
let package_output_specs = get_package_output_specs(build_state.get_root_config());
// derive version from the crate version
let version = env!("CARGO_PKG_VERSION").to_string();
let generated_at = crate::helpers::get_system_time().to_string();
Expand All @@ -147,6 +219,7 @@ pub fn write_compiler_info(build_state: &BuildCommandState) {
bsc_hash: &'a str,
rescript_config_hash: String,
source_map_args: &'a [String],
package_output_specs: &'a [PackageOutputSpec],
runtime_path: &'a str,
generated_at: &'a str,
}
Expand All @@ -159,6 +232,7 @@ pub fn write_compiler_info(build_state: &BuildCommandState) {
bsc_hash: &bsc_hash,
rescript_config_hash: rescript_config_hash.to_hex().to_string(),
source_map_args: &source_map_args,
package_output_specs: &package_output_specs,
runtime_path: &runtime_path,
generated_at: &generated_at,
};
Expand Down Expand Up @@ -225,12 +299,13 @@ pub fn write_compiler_info(build_state: &BuildCommandState) {
#[cfg(test)]
mod tests {
use super::*;
use crate::build::packages::{Namespace, Package};
use crate::build::packages::{Namespace, Package, SourceFileMeta};
use crate::config;
use ahash::{AHashMap, AHashSet};
use serde_json::json;
use std::fs;
use std::path::Path;
use std::time::SystemTime;
use tempfile::TempDir;

fn test_compiler(root: &Path) -> CompilerInfo {
Expand Down Expand Up @@ -268,7 +343,12 @@ mod tests {
}
}

fn write_test_compiler_info(package: &Package, compiler: &CompilerInfo, source_map_args: Vec<&str>) {
fn write_test_compiler_info(
package: &Package,
compiler: &CompilerInfo,
source_map_args: Vec<&str>,
package_output_specs: &[PackageOutputSpec],
) {
fs::create_dir_all(package.get_build_path()).expect("build directory should be created");
fs::create_dir_all(package.get_ocaml_build_path()).expect("ocaml build directory should be created");

Expand All @@ -280,6 +360,7 @@ mod tests {
"bsc_hash": compiler.bsc_hash.to_hex().to_string(),
"rescript_config_hash": rescript_config_hash,
"source_map_args": source_map_args,
"package_output_specs": package_output_specs,
"runtime_path": compiler.runtime_path.to_string_lossy().to_string(),
"generated_at": "test",
});
Expand All @@ -304,9 +385,20 @@ mod tests {
let package = test_package(temp_dir.path(), "dep");
let build_path = package.get_build_path();
let source_map_args = vec!["-bs-source-map".to_string(), "linked".to_string()];
write_test_compiler_info(&package, &compiler, vec!["-bs-source-map", "linked"]);
let package_output_specs = get_package_output_specs(&package.config);
write_test_compiler_info(
&package,
&compiler,
vec!["-bs-source-map", "linked"],
&package_output_specs,
);

let result = verify_compiler_info(&packages_map(package), &compiler, &source_map_args);
let result = verify_compiler_info(
&packages_map(package),
&compiler,
&source_map_args,
&package_output_specs,
);

assert!(matches!(result, CompilerCheckResult::SameCompilerAsLastRun));
assert!(build_path.exists());
Expand All @@ -319,14 +411,70 @@ mod tests {
let package = test_package(temp_dir.path(), "dep");
let build_path = package.get_build_path();
let source_map_args = vec!["-bs-source-map".to_string(), "linked".to_string()];
write_test_compiler_info(&package, &compiler, vec!["-bs-source-map", "false"]);
let package_output_specs = get_package_output_specs(&package.config);
write_test_compiler_info(
&package,
&compiler,
vec!["-bs-source-map", "false"],
&package_output_specs,
);

let result = verify_compiler_info(
&packages_map(package),
&compiler,
&source_map_args,
&package_output_specs,
);

assert!(matches!(
result,
CompilerCheckResult::CleanedPackagesDueToCompiler
));
assert!(!build_path.exists());
}

#[test]
fn verify_compiler_info_cleans_package_when_package_output_specs_change() {
let temp_dir = TempDir::new().expect("temp dir should be created");
let compiler = test_compiler(temp_dir.path());
let mut package = test_package(temp_dir.path(), "dep");
let build_path = package.get_build_path();
let source_file = Path::new("src/Dep.res").to_path_buf();
package.source_files = Some(AHashMap::from_iter([(
source_file.clone(),
SourceFileMeta {
modified: SystemTime::now(),
is_type_dev: false,
},
)]));
let previous_output = package.get_js_path().join(source_file).with_extension("cjs");
fs::create_dir_all(previous_output.parent().expect("output should have a parent"))
.expect("output directory should be created");
fs::write(&previous_output, "generated output").expect("previous output should be written");
fs::write(previous_output.with_extension("cjs.map"), "source map")
.expect("previous source map should be written");
let source_map_args = Vec::new();
let package_output_specs = get_package_output_specs(&package.config);
let previous_package_output_specs = vec![PackageOutputSpec {
module: "commonjs".to_string(),
in_source: false,
suffix: ".cjs".to_string(),
}];
write_test_compiler_info(&package, &compiler, vec![], &previous_package_output_specs);

let result = verify_compiler_info(&packages_map(package), &compiler, &source_map_args);
let result = verify_compiler_info(
&packages_map(package),
&compiler,
&source_map_args,
&package_output_specs,
);

assert!(matches!(
result,
CompilerCheckResult::CleanedPackagesDueToCompiler
));
assert!(!build_path.exists());
assert!(!previous_output.exists());
assert!(!previous_output.with_extension("cjs.map").exists());
}
}
Loading
Loading