Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 9 additions & 2 deletions src/cli/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,17 @@ fn collect_file_layers_with_env(
(FileLayerTrace::Automatic { project_scope }, None, outcome)
},
|path| {
let (load_warning, outcome) = load_layers_from_path_with_warning(path);
// `-C` behaves as a working-directory change for CLI paths,
// including an explicit configuration selector.
let effective_path = cli
.directory
.as_deref()
.filter(|_| path.is_relative())
.map_or_else(|| path.to_path_buf(), |directory| directory.join(path));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep explicit config paths anchored to the process CWD

When -C/--directory is combined with a relative --config (or relative NETSUKE_CONFIG), this now resolves the selector beneath the project directory rather than the invoking shell's working directory. That contradicts the public contract in docs/users-guide.md:896-897 and the design invariant in docs/netsuke-design.md:3117-3120, and can silently load a same-named project config instead of the explicitly selected file; keep the resolved selector path unchanged while continuing to use cli.directory only for automatic project discovery.

Useful? React with 👍 / 👎.

let (load_warning, outcome) = load_layers_from_path_with_warning(&effective_path);
(
FileLayerTrace::Explicit {
path: BoundedConfigPath::from_path(Some(path)),
path: BoundedConfigPath::from_path(Some(&effective_path)),
},
load_warning,
outcome,
Expand Down
28 changes: 28 additions & 0 deletions src/cli/discovery_layer_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,34 @@ fn injected_automatic_discovery_uses_xdg_config_home() -> Result<()> {
Ok(())
}

/// An explicit relative configuration file uses the CLI working directory.
#[test]
fn explicit_relative_config_uses_cli_directory() -> Result<()> {
let temp = tempdir().context("create temp dir")?;
let config_path = temp.path().join("cli.toml");
test_support::fs::write(&config_path, "emoji = \"always\"\n")
.context("write explicit config")?;
let cli = Cli {
config: Some("cli.toml".into()),
directory: Some(temp.path().to_path_buf()),
..Cli::default()
};

let discovered = discover_file_layers(&cli, &TestEnv::default());

ensure!(
discovered.first_error().is_none(),
"explicit relative config should load from the CLI directory"
);
let paths = discovered
.layers()
.iter()
.filter_map(|layer| layer.path().map(|path| path.as_str().to_owned()))
.collect::<Vec<_>>();
assert_eq!(paths, vec![config_path.to_string_lossy().into_owned()]);
Ok(())
}

/// Discovered configuration candidates retain the outcome that their content
/// warrants; an unreadable candidate is never mistaken for an absent one.
#[rstest]
Expand Down
35 changes: 0 additions & 35 deletions tests/bdd/fixtures/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,8 @@ use std::collections::HashMap;
use std::ffi::OsString;
use std::path::PathBuf;
use std::sync::MutexGuard;
use test_support::CwdGuard;
use test_support::env_lock::EnvLock;
use test_support::http::HttpServer;

#[derive(Debug)]
struct GlobalStateGuard {
env_lock: EnvLock,
cwd_guard: CwdGuard,
}

/// Combined test world for all BDD scenarios.
///
/// Non-Clone types are stored in `RefCell<Option<T>>` to allow interior
Expand Down Expand Up @@ -149,28 +141,9 @@ pub struct TestWorld {
// Environment state
/// Values supplied to child Netsuke processes for scenario-tracked variables.
pub env_vars_forward: RefCell<HashMap<String, OsString>>,
/// Scenario-scoped lock for the few remaining process-global CWD operations.
global_state_lock: RefCell<Option<GlobalStateGuard>>,
}

impl TestWorld {
/// Acquire the scenario lock before changing the process working directory.
///
/// # Errors
///
/// Returns an error if the current working directory cannot be captured.
pub fn ensure_global_state_lock(&self) -> std::io::Result<()> {
if self.global_state_lock.borrow().is_none() {
let env_lock = EnvLock::acquire();
let cwd_guard = CwdGuard::acquire()?;
*self.global_state_lock.borrow_mut() = Some(GlobalStateGuard {
env_lock,
cwd_guard,
});
}
Ok(())
}

/// Set or remove a variable in the child-process environment specification.
pub fn track_env_var(&self, key: String, forward_value: Option<OsString>) {
if let Some(value) = forward_value {
Expand Down Expand Up @@ -206,14 +179,6 @@ impl Drop for TestWorld {
self.localization_guard.borrow_mut().take();
self.localization_lock.borrow_mut().take();
self.env_vars_forward.borrow_mut().clear();
if let Some(GlobalStateGuard {
env_lock,
cwd_guard,
}) = self.global_state_lock.borrow_mut().take()
{
drop(cwd_guard);
drop(env_lock);
}
self.stdlib_text.clear();
}
}
Expand Down
32 changes: 7 additions & 25 deletions tests/bdd/steps/configuration_discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ fn a_temporary_workspace(world: &TestWorld) -> Result<()> {
}

/// Write `content` to `file_name` inside `world`'s temp directory.
/// Set `chdir` to `true` for project-scope configs so discovery works
/// without an explicit path override.
fn write_config_file(world: &TestWorld, file_name: &str, content: &str, chdir: bool) -> Result<()> {
fn write_config_file(world: &TestWorld, file_name: &str, content: &str) -> Result<()> {
let temp_dir = world
.temp_dir
.borrow()
Expand All @@ -37,12 +35,6 @@ fn write_config_file(world: &TestWorld, file_name: &str, content: &str, chdir: b
dir.write(file_name, content.as_bytes())
.with_context(|| format!("failed to write {file_name} in {temp_dir_utf8}"))?;

if chdir {
// Acquire scenario-scoped lock before process-global CWD mutation
world.ensure_global_state_lock()?;
std::env::set_current_dir(&temp_dir).context("failed to change to temp directory")?;
}

Ok(())
}

Expand All @@ -59,12 +51,12 @@ emoji = "{emoji}"
jobs = {jobs}
"#
);
write_config_file(world, file_name.as_str(), &content, true)
write_config_file(world, file_name.as_str(), &content)
}

#[given("a malformed project config file {file_name:string}")]
fn malformed_project_config(world: &TestWorld, file_name: FileName) -> Result<()> {
write_config_file(world, file_name.as_str(), "emoji = \"always\n", true)
write_config_file(world, file_name.as_str(), "emoji = \"always\n")
}

/// Returns the TOML snippet for a config file that sets only `emoji`.
Expand All @@ -78,12 +70,7 @@ fn project_config_with_emoji(
file_name: FileName,
emoji: EmojiPolicy,
) -> Result<()> {
write_config_file(
world,
file_name.as_str(),
&emoji_config_content(emoji),
true,
)
write_config_file(world, file_name.as_str(), &emoji_config_content(emoji))
}

#[given("a project config file {file_name:string} with emoji {emoji:string} and JSON {json}")]
Expand All @@ -99,7 +86,7 @@ emoji = "{emoji}"
json = {json}
"#
);
write_config_file(world, file_name.as_str(), &content, true)
write_config_file(world, file_name.as_str(), &content)
}

#[given("a project config file {file_name:string} with default targets {targets:string}")]
Expand All @@ -123,7 +110,7 @@ fn project_config_with_default_targets(
default_targets = {targets_toml}
"
);
write_config_file(world, file_name.as_str(), &content, true)
write_config_file(world, file_name.as_str(), &content)
}

#[given("a custom config file {file_name:string} with emoji {emoji:string}")]
Expand All @@ -132,12 +119,7 @@ fn custom_config_with_emoji(
file_name: FileName,
emoji: EmojiPolicy,
) -> Result<()> {
write_config_file(
world,
file_name.as_str(),
&emoji_config_content(emoji),
false,
)
write_config_file(world, file_name.as_str(), &emoji_config_content(emoji))
}

#[given("the environment variable {var_name:string} is set to {value:string}")]
Expand Down
17 changes: 2 additions & 15 deletions tests/bdd/steps/ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,23 +213,10 @@ fn graph_target_implicit_deps(world: &TestWorld, target: &str, paths: &str) -> R

/// Compile a manifest file to IR, storing result or error in state.
fn compile_manifest_impl(world: &TestWorld, path: &str) {
// Convert relative test data paths to absolute to avoid issues when CWD
// is changed by parallel tests. Also lock CWD to the project root so
// that glob patterns inside the manifest resolve correctly.
// The manifest path is absolute, while the process CWD stays at the
// project root, so relative glob patterns continue to resolve correctly.
let resolved = if std::path::Path::new(path).is_relative() && path.starts_with("tests/") {
let manifest_dir = env!("CARGO_MANIFEST_DIR");
if let Err(error) = world.ensure_global_state_lock() {
let outcome = Err(format!("failed to capture current directory: {error}"));
store_parse_outcome(&world.build_graph, &world.generation_error, outcome);
return;
}
if let Err(e) = std::env::set_current_dir(manifest_dir) {
let outcome = Err(format!(
"failed to set current directory to {manifest_dir} for path {path}: {e}"
));
store_parse_outcome(&world.build_graph, &world.generation_error, outcome);
return;
}
std::path::Path::new(manifest_dir)
.join(path)
.to_string_lossy()
Expand Down
24 changes: 2 additions & 22 deletions tests/bdd/steps/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,32 +68,12 @@ pub(super) fn get_string_from_string_or_list(
}

fn parse_manifest_inner(world: &TestWorld, path: &ManifestPath) {
// Convert relative test data paths to absolute to avoid issues when CWD is
// changed by parallel tests. Also lock CWD to the project root so that
// glob patterns inside the manifest resolve correctly.
// Convert relative test data paths to absolute while the process CWD stays
// at the project root, where relative glob patterns resolve correctly.
let manifest_path = if std::path::Path::new(path.as_str()).is_relative()
&& path.as_str().starts_with("tests/")
{
let manifest_dir = env!("CARGO_MANIFEST_DIR");
// Hold the env lock and set CWD to the project root so that relative
// glob patterns (e.g. `tests/data/glob_files/*.txt`) resolve correctly.
if let Err(error) = world.ensure_global_state_lock() {
store_parse_outcome(
&world.manifest,
&world.manifest_error,
Err(format!("failed to capture current directory: {error}")),
);
return;
}
// EnvLock is held; safe to mutate CWD for this scenario.
if let Err(e) = std::env::set_current_dir(manifest_dir) {
store_parse_outcome(
&world.manifest,
&world.manifest_error,
Err(format!("failed to set CWD to {manifest_dir}: {e}")),
);
return;
}
std::path::Path::new(manifest_dir)
.join(path.as_str())
.to_string_lossy()
Expand Down
Loading