From 1fb85fd57b6531edbac1b9da8c63eb6c8214e353 Mon Sep 17 00:00:00 2001 From: Tim Fischer Date: Mon, 3 Aug 2026 09:28:06 +0200 Subject: [PATCH 1/3] Add `dev_dependencies` manifest section Dependencies that are only needed to work on a package itself, such as testbench infrastructure, currently have no way to be kept out of dependent projects. The `target` field on a dependency only filters it out of source listings; it is still resolved and inherited by everyone depending on the package. Add a `dev_dependencies` section (alias `dev-dependencies`) whose entries are resolved only when the package is the root package, and are never propagated to packages depending on it. Entries accept exactly the same fields as `dependencies`, so `remote` shorthands, `target` and `pass_targets` all keep working. Listing a package in both sections is an error. Deliberately a single section rather than named groups: target expressions already provide the selection axis, and a second grouping mechanism would overlap with them. `Manifest::root_dependencies` and `root_dependency` mark the sites where dev-dependencies enter the picture, so the root-only rule stays visible in the code. This also fixes `bender fusesoc --single` disagreeing with `bender fusesoc` about the root package's `depend` list, since the former reads the manifest directly while the latter derives it from the shared source tree. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 + book/src/dependencies.md | 61 +++++++- book/src/manifest.md | 8 ++ book/src/targets.md | 2 + src/cmd/fusesoc.rs | 10 +- src/cmd/parents.rs | 11 +- src/cmd/sources.rs | 3 +- src/config.rs | 99 +++++++++---- src/resolver.rs | 22 +++ src/sess.rs | 42 ++++-- tests/dev_deps.rs | 295 +++++++++++++++++++++++++++++++++++++++ 11 files changed, 502 insertions(+), 53 deletions(-) create mode 100644 tests/dev_deps.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index da509645..b0f55eac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## Unreleased +### Added +- Add a `dev_dependencies` manifest section (alias `dev-dependencies`) for dependencies that are only needed to work on the package itself. They are resolved when the package is the root package and are not propagated to packages depending on it. Entries accept the same fields as `dependencies`, including `target` and `pass_targets`; listing a package in both sections is an error. Older Bender versions ignore the section with warning `W03`, which is the correct behavior for a consumer. ## 0.32.1 - 2026-07-07 ### Added diff --git a/book/src/dependencies.md b/book/src/dependencies.md index 6f8b97c5..91f76fb4 100644 --- a/book/src/dependencies.md +++ b/book/src/dependencies.md @@ -42,6 +42,65 @@ dependencies: my_local_ip: { path: "../local_ips/my_ip" } ``` +## Dev-Dependencies + +Packages listed under `dev_dependencies` are only needed to work on the package itself. They are resolved when the package is the **root package** — the one Bender is invoked in — and are **not** propagated to packages that depend on it. + +```yaml +dependencies: + common_cells: "1.39.0" + +dev_dependencies: + common_verification: { version: "0.2.5", target: test } +``` + +The two sections take exactly the same entries: Git, path and revision dependencies, `remote` shorthands, `target` filters and `pass_targets` all behave identically. The only difference is propagation. Listing the same package in both sections is an error. + +> **Note:** For compatibility with Cargo's spelling, `dev-dependencies` is accepted as an alias for `dev_dependencies`. + +This is the mechanism to reach for when a package's testbench needs verification IP that its consumers should not inherit. A downstream project depending on such a package resolves only its `dependencies`; the `dev_dependencies` are ignored entirely and never appear in the downstream [`Bender.lock`](./lockfile.md). + +### Choosing Between `target` and `dev_dependencies` + +The two features are orthogonal and solve different problems: + +| | Effect | +|---|---| +| `target` on a dependency | Filters the dependency out of source listings when the target is inactive. The dependency is still resolved and locked, for the root package *and* for everyone depending on it. | +| `dev_dependencies` | Removes the dependency from the graph entirely for anyone depending on this package. Within the root package it behaves exactly like a regular dependency. | + +They compose: a dev-dependency may carry a `target`, in which case it is resolved for the root package but only contributes sources when that target is active. This is usually what you want, and it also removes the need for a grouping mechanism — [target expressions](./targets.md) already select subsets of your dev-dependencies: + +```yaml +dev_dependencies: + my_vip: { version: "0.2", target: test } + my_tb_utils: { path: "../tb_utils", target: tb } +``` + +```bash +bender script vsim -t tb +``` + +### Keeping Dev-Only Sources Consistent + +Because Bender emits one flat file list for the whole graph, source groups of a dependency are visible to downstream projects even though its dev-dependencies are not. A package whose sources reference a dev-dependency must therefore keep those sources behind a target that downstream projects do not enable, or the generated file list will reference modules that are no longer in the graph. + +The [recommended target conventions](./targets.md#recommended-conventions) draw exactly this line: + +- Code under `target: test` is *reusable* verification IP that consumers are expected to enable. Anything it needs belongs in `dependencies`. +- Code under `target: tb` is *non-reusable* testbench and testharness code that consumers never enable. Anything it needs belongs in `dev_dependencies`. + +```yaml +dev_dependencies: + my_tb_utils: { path: "../tb_utils", target: tb } + +sources: + - src/core.sv + - target: tb + files: + - tb/tb_core.sv # may use my_tb_utils; consumers never enable `tb` +``` + ## Remotes To avoid repeating full Git URLs, you can define `remotes` in your manifest. @@ -96,7 +155,7 @@ remotes: Dependencies can be conditionally included or configured using targets. For details on how to use target expressions or pass targets to dependencies, see the [Targets](./targets.md) documentation. -> **Note:** A `target` on a dependency only filters that dependency out of *source listings and generated scripts*. It does **not** affect dependency resolution: every dependency declared in [`Bender.yml`](./manifest.md) is still resolved and recorded in [`Bender.lock`](./lockfile.md) regardless of which targets are active. +> **Note:** A `target` on a dependency only filters that dependency out of *source listings and generated scripts*. It does **not** affect dependency resolution: every dependency declared in [`Bender.yml`](./manifest.md) is still resolved and recorded in [`Bender.lock`](./lockfile.md) regardless of which targets are active. To keep a dependency out of dependent packages, use [dev-dependencies](#dev-dependencies). ## Git LFS Support diff --git a/book/src/manifest.md b/book/src/manifest.md index c42b7ab5..2ee3fd5e 100644 --- a/book/src/manifest.md +++ b/book/src/manifest.md @@ -49,6 +49,14 @@ dependencies: common_cells: { git: "https://github.com/pulp-platform/common_cells.git", version: "1.39" } ``` +Packages that are only needed to work on this package itself — testbench infrastructure, for example — belong in `dev_dependencies` instead. They are resolved only when this package is the root package, and are never propagated to packages that depend on it. See [Dev-Dependencies](./dependencies.md#dev-dependencies) for details. + +```yaml +# Packages needed only when working on this package itself. Optional. +dev_dependencies: + my_tb_utils: { git: "https://github.com/pulp-platform/my_tb_utils.git", version: "0.1" } +``` + The sources section lists the HDL source files belonging to this package. It is optional for packages that only provide headers or are otherwise used without their own source files. More details on the format can be found [here](./sources.md). ```yaml diff --git a/book/src/targets.md b/book/src/targets.md index d03601a4..a9478bbe 100644 --- a/book/src/targets.md +++ b/book/src/targets.md @@ -100,6 +100,8 @@ dependencies: common_verification: { version: "0.2", target: any(test, simulation) } ``` +A `target` here only filters the dependency out of source listings; the dependency is still resolved and inherited by packages depending on yours. To keep a dependency out of dependent packages entirely, declare it under [`dev_dependencies`](./dependencies.md#dev-dependencies) instead. The two compose, and target expressions are the intended way to select subsets of your dev-dependencies. + ## Built-in Targets Bender automatically activates certain targets based on the subcommand and output format. These "default targets" ensure that tool-specific workarounds or flow-specific files are included correctly. You can disable this behavior with the `--no-default-target` flag. diff --git a/src/cmd/fusesoc.rs b/src/cmd/fusesoc.rs index f9734b32..7372b6d4 100644 --- a/src/cmd/fusesoc.rs +++ b/src/cmd/fusesoc.rs @@ -69,7 +69,10 @@ pub fn run_single(sess: &Session, args: &FusesocArgs) -> Result<()> { .load_sources( sources, Some(name.as_str()), - sess.manifest.dependencies.keys().cloned().collect(), + sess.manifest + .root_dependencies() + .map(|(name, _)| name.clone()) + .collect(), IndexMap::new(), version_string.clone(), ) @@ -90,9 +93,8 @@ pub fn run_single(sess: &Session, args: &FusesocArgs) -> Result<()> { let fuse_depend_string = sess .manifest - .dependencies - .keys() - .map(|dep| { + .root_dependencies() + .map(|(dep, _)| { ( dep.to_string(), format!( diff --git a/src/cmd/parents.rs b/src/cmd/parents.rs index 7e5f6afd..901bd5ae 100644 --- a/src/cmd/parents.rs +++ b/src/cmd/parents.rs @@ -167,11 +167,11 @@ pub fn get_parent_array( targets: bool, ) -> Result>> { let mut map = IndexMap::>::new(); - if sess.manifest.dependencies.contains_key(dep) { + if let Some(root_dep) = sess.manifest.root_dependency(dep) { if targets { map.insert( sess.manifest.package.name.clone(), - match sess.manifest.dependencies.get(dep).unwrap() { + match root_dep { Dependency::Version { target: targetspec, pass_targets: tgts, @@ -199,11 +199,8 @@ pub fn get_parent_array( }, ); } else { - let dep_str = format!( - "{}", - DependencyConstraint::from(&sess.manifest.dependencies[dep]) - ); - let source = DependencySource::from(&sess.manifest.dependencies[dep]); + let dep_str = format!("{}", DependencyConstraint::from(root_dep)); + let source = DependencySource::from(root_dep); let dep_source = format_dep_source(&source, sess.root); map.insert( sess.manifest.package.name.clone(), diff --git a/src/cmd/sources.rs b/src/cmd/sources.rs index 9c189204..3fd5fdd6 100644 --- a/src/cmd/sources.rs +++ b/src/cmd/sources.rs @@ -163,8 +163,7 @@ pub fn get_passed_targets( if used_packages.contains(&sess.manifest.package.name) { required_packages.insert(sess.manifest.package.name.clone()); sess.manifest - .dependencies - .iter() + .root_dependencies() .for_each(|(name, dep)| match dep { Dependency::Version { target: filter, diff --git a/src/config.rs b/src/config.rs index b150f659..5b0c5b42 100644 --- a/src/config.rs +++ b/src/config.rs @@ -41,6 +41,11 @@ pub struct Manifest { pub package: Package, /// The dependencies. pub dependencies: IndexMap, + /// The dependencies only needed to work on this package itself. + /// + /// These are resolved only when this package is the root package. They are + /// never propagated to packages that depend on this one. + pub dev_dependencies: IndexMap, /// The source files. pub sources: Option, /// The include directories exported to dependent packages. @@ -55,10 +60,33 @@ pub struct Manifest { pub vendor_package: Vec, } +impl Manifest { + /// The dependencies of this manifest as seen when it is the root package. + /// + /// This is the regular dependencies followed by the dev-dependencies. Code + /// that operates on a dependency's manifest must use `dependencies` + /// directly instead, since dev-dependencies are not propagated to + /// dependent packages. + pub fn root_dependencies(&self) -> impl Iterator { + self.dependencies.iter().chain(self.dev_dependencies.iter()) + } + + /// Look up a dependency by name, as seen when this is the root package. + /// + /// Considers the dev-dependencies in addition to the regular dependencies. + /// See `root_dependencies`. + pub fn root_dependency(&self, name: &str) -> Option<&Dependency> { + self.dependencies + .get(name) + .or_else(|| self.dev_dependencies.get(name)) + } +} + impl PrefixPaths for Manifest { fn prefix_paths(self, prefix: &Path) -> Result { Ok(Manifest { dependencies: self.dependencies.prefix_paths(prefix)?, + dev_dependencies: self.dev_dependencies.prefix_paths(prefix)?, sources: self .sources .map_or(Ok::, Error>(None), |src| { @@ -443,6 +471,9 @@ pub struct PartialManifest { pub remotes: Option>>, /// The dependencies. pub dependencies: Option>>, + /// The dependencies only needed to work on this package itself. + #[serde(alias = "dev-dependencies")] + pub dev_dependencies: Option>>, /// The source files. pub sources: Option>, /// The include directories exported to dependent packages. @@ -478,6 +509,7 @@ impl PrefixPaths for PartialManifest { Ok(PartialManifest { remotes: self.remotes, dependencies: self.dependencies.prefix_paths(prefix)?, + dev_dependencies: self.dev_dependencies.prefix_paths(prefix)?, sources: self.sources.prefix_paths(prefix)?, export_include_dirs: match self.export_include_dirs { Some(vec_inc) => Some( @@ -549,31 +581,49 @@ impl Validate for PartialManifest { None }; - let deps = match self.dependencies { - Some(d) => d - .into_iter() - .map(|(k, v)| { - let dep_name = k.to_lowercase(); - - // We need to construct a new context for the dependency validation - // since we need the dependency name and the remote definitions - // to validate a dependency. - let dep_vctx = ValidationContext { - package_name: &dep_name, - pre_output: vctx.pre_output, - remotes: remotes.as_ref(), - default_remote, - }; - - let validated = v.validate(&dep_vctx).wrap_err_with(|| { - format!("In dependency `{dep_name}` of package `{}`.", pkg.name) - })?; - - Ok((dep_name, validated)) - }) - .collect::>>()?, - None => IndexMap::new(), + // `dependencies` and `dev_dependencies` are validated identically, only + // the wording of the error context differs. + let validate_deps = |deps: Option>>, + kind: &str| + -> Result> { + match deps { + Some(d) => d + .into_iter() + .map(|(k, v)| { + let dep_name = k.to_lowercase(); + + // We need to construct a new context for the dependency validation + // since we need the dependency name and the remote definitions + // to validate a dependency. + let dep_vctx = ValidationContext { + package_name: &dep_name, + pre_output: vctx.pre_output, + remotes: remotes.as_ref(), + default_remote, + }; + + let validated = v.validate(&dep_vctx).wrap_err_with(|| { + format!("In {kind} `{dep_name}` of package `{}`.", pkg.name) + })?; + + Ok((dep_name, validated)) + }) + .collect::>>(), + None => Ok(IndexMap::new()), + } }; + let deps = validate_deps(self.dependencies, "dependency")?; + let dev_deps = validate_deps(self.dev_dependencies, "dev-dependency")?; + if let Some(name) = dev_deps.keys().find(|name| deps.contains_key(*name)) { + bail!( + help = "Remove it from one of the two sections. A dev-dependency is \ + not propagated to dependent packages, so listing it in both \ + is ambiguous.", + "`{}` is listed as both a dependency and a dev-dependency of package `{}`.", + name, + pkg.name + ); + } let srcs = match self.sources { Some(s) => Some( s.validate(vctx) @@ -616,6 +666,7 @@ impl Validate for PartialManifest { Ok(Manifest { package: pkg, dependencies: deps, + dev_dependencies: dev_deps, sources: match srcs { Some(SourceFile::Group(srcs)) => Some(*srcs), Some(SourceFile::File(_)) diff --git a/src/resolver.rs b/src/resolver.rs index 741bf39a..0dd68f2c 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -152,6 +152,16 @@ impl<'ctx> DependencyResolver<'ctx> { &io, )?; + // Load the dev-dependencies in the root manifest. These are only + // resolved for the root package; the dev-dependencies of the packages + // in the table are ignored. + self.register_dependencies_in_manifest( + &self.sess.manifest.dev_dependencies, + &self.sess.manifest.package.name, + &rt, + &io, + )?; + let mut _iteration = 0; let mut any_changes = true; while any_changes { @@ -573,12 +583,24 @@ impl<'ctx> DependencyResolver<'ctx> { // cons_map: dep_name->(parent_name, constraint, source) let cons_map = { let mut map = IndexMap::<&str, Vec<(&str, DependencyConstraint, DependencyRef)>>::new(); + let root_pkg_name = self + .sess + .intern_string(self.sess.manifest.package.name.clone()); let dep_iter = once(self.sess.manifest) .chain(self.table.values().filter_map(|dep| dep.manifest)) .flat_map(|m| { let pkg_name = self.sess.intern_string(m.package.name.clone()); m.dependencies.iter().map(move |(n, d)| (n, (pkg_name, d))) }) + // Only the root package's dev-dependencies constrain the + // resolution; those of the other packages are ignored. + .chain( + self.sess + .manifest + .dev_dependencies + .iter() + .map(move |(n, d)| (n, (root_pkg_name, d))), + ) .map(|(name, (pkg_name, dep))| { (name, (pkg_name, self.checked_out.get(name).unwrap_or(dep))) }) diff --git a/src/sess.rs b/src/sess.rs index 272933d0..7989e515 100644 --- a/src/sess.rs +++ b/src/sess.rs @@ -256,7 +256,7 @@ impl<'ctx> Session<'ctx> { let mut ranks: IndexMap = graph.keys().map(|&id| (id, 0)).collect(); let mut pending = IndexSet::new(); - for name in self.manifest.dependencies.keys() { + for (name, _) in self.manifest.root_dependencies() { if !(names.contains_key(name)) { bail!( help = "You may need to run `bender update`", @@ -265,7 +265,11 @@ impl<'ctx> Session<'ctx> { ); } } - pending.extend(self.manifest.dependencies.keys().map(|name| names[name])); + pending.extend( + self.manifest + .root_dependencies() + .map(|(name, _)| names[name]), + ); let mut cyclic = false; while !pending.is_empty() { let mut current_pending = IndexSet::new(); @@ -1756,12 +1760,22 @@ impl<'io, 'sess: 'io, 'ctx: 'sess> SessionIo<'sess, 'ctx> { let files = ranks .into_iter() - .chain(once(vec![Some(self.sess.manifest)])) - .map(|manifests| { + .map(|manifests| (manifests, false)) + .chain(once((vec![Some(self.sess.manifest)], true))) + .map(|(manifests, is_root)| { let files = manifests .into_iter() .flatten() .map(|m| { + // The dev-dependencies only belong to the root package; + // they are not propagated to dependent packages. + let dependencies: IndexSet = if is_root { + m.root_dependencies() + .map(|(name, _)| name.clone()) + .collect() + } else { + m.dependencies.keys().cloned().collect() + }; // Collect include dirs from export_include_dirs of package and direct dependencies let mut export_include_dirs: IndexMap> = IndexMap::new(); @@ -1772,15 +1786,13 @@ impl<'io, 'sess: 'io, 'ctx: 'sess> SessionIo<'sess, 'ctx> { .map(|(trgt, path)| (trgt.clone(), path.as_path())) .collect(), ); - if !m.dependencies.is_empty() { - for i in m.dependencies.keys() { - if !all_export_include_dirs.contains_key(i) { - Warnings::ExportDirNameIssue(i.clone()).emit(); - export_include_dirs.insert(i.to_string(), Vec::new()); - } else { - export_include_dirs - .insert(i.to_string(), all_export_include_dirs[i].clone()); - } + for i in &dependencies { + if !all_export_include_dirs.contains_key(i) { + Warnings::ExportDirNameIssue(i.clone()).emit(); + export_include_dirs.insert(i.to_string(), Vec::new()); + } else { + export_include_dirs + .insert(i.to_string(), all_export_include_dirs[i].clone()); } } if let Some(s) = m.sources.as_ref() { @@ -1788,7 +1800,7 @@ impl<'io, 'sess: 'io, 'ctx: 'sess> SessionIo<'sess, 'ctx> { .load_sources( s, Some(m.package.name.as_str()), - m.dependencies.keys().cloned().collect(), + dependencies, export_include_dirs, match self.sess.dependency_with_name(m.package.name.as_str()) { Ok(dep_id) => self.sess.dependency(dep_id).version.clone(), @@ -1802,7 +1814,7 @@ impl<'io, 'sess: 'io, 'ctx: 'sess> SessionIo<'sess, 'ctx> { // get_package_list can discover transitive deps. SourceFile::Group(Box::new(SourceGroup { package: Some(m.package.name.as_str()), - dependencies: m.dependencies.keys().cloned().collect(), + dependencies, export_incdirs: export_include_dirs, version: match self .sess diff --git a/tests/dev_deps.rs b/tests/dev_deps.rs new file mode 100644 index 00000000..f09f65c3 --- /dev/null +++ b/tests/dev_deps.rs @@ -0,0 +1,295 @@ +// Copyright (c) 2026 ETH Zurich +// Tim Fischer + +//! Tests for the `dev_dependencies` manifest section. +//! +//! The fixture is built from scratch in a temporary directory and only uses +//! path dependencies, so the tests need no network access. + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Output; + +use assert_cmd::cargo; + +/// Write `contents` to `path`, creating the parent directories as needed. +fn write_file(path: &Path, contents: &str) { + fs::create_dir_all(path.parent().unwrap()).expect("Failed to create directory"); + fs::write(path, contents).expect("Failed to write file"); +} + +/// Create a package with a single source file at `root/name`. +fn write_package(root: &Path, name: &str, manifest: &str) { + write_file(&root.join(name).join("Bender.yml"), manifest); + write_file( + &root.join(name).join("src").join(format!("{}.sv", name)), + &format!("module {};\nendmodule\n", name), + ); +} + +/// Build the fixture and return its root directory. +/// +/// The dependency structure is: +/// +/// - `root_pkg` depends on `lib` and dev-depends on `vip` +/// - `lib` dev-depends on `lib_only`, which must not reach `root_pkg` +fn setup(case: &str) -> PathBuf { + let root = Path::new("tests/tmp/dev_deps").join(case); + if root.exists() { + fs::remove_dir_all(&root).expect("Failed to clean fixture directory"); + } + fs::create_dir_all(&root).expect("Failed to create fixture directory"); + + write_package( + &root, + "lib", + "package:\n name: lib\n\ + dev_dependencies:\n lib_only: { path: ../lib_only }\n\ + sources:\n - src/lib.sv\n", + ); + write_package( + &root, + "vip", + "package:\n name: vip\nsources:\n - src/vip.sv\n", + ); + write_package( + &root, + "lib_only", + "package:\n name: lib_only\nsources:\n - src/lib_only.sv\n", + ); + + root +} + +/// Write the root manifest of the fixture. +fn write_root_manifest(root: &Path, manifest: &str) { + write_file(&root.join("Bender.yml"), manifest); + write_file(&root.join("src").join("top.sv"), "module top;\nendmodule\n"); + write_file( + &root.join("tb").join("tb_top.sv"), + "module tb_top;\nendmodule\n", + ); +} + +/// The default root manifest: a regular dependency plus a dev-dependency. +const ROOT_MANIFEST: &str = "package:\n name: root_pkg\n\ + dependencies:\n lib: { path: ./lib }\n\ + dev_dependencies:\n vip: { path: ./vip }\n\ + sources:\n - src/top.sv\n - target: test\n files:\n - tb/tb_top.sv\n"; + +/// Run bender in the fixture directory. +fn run_bender(root: &Path, args: &[&str]) -> Output { + let mut full_args = vec!["-d", root.to_str().unwrap()]; + full_args.extend(args); + + cargo::cargo_bin_cmd!() + .args(&full_args) + .output() + .expect("Failed to execute bender binary") +} + +/// Run bender expecting success; returns stdout. +fn run_bender_ok(root: &Path, args: &[&str]) -> String { + let out = run_bender(root, args); + assert!( + out.status.success(), + "bender {:?} failed.\nstdout:\n{}\nstderr:\n{}", + args, + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8(out.stdout).expect("stdout must be utf-8") +} + +/// Run bender expecting failure; returns stderr. +fn run_bender_failing(root: &Path, args: &[&str]) -> String { + let out = run_bender(root, args); + assert!( + !out.status.success(), + "bender {:?} unexpectedly succeeded.\nstdout:\n{}", + args, + String::from_utf8_lossy(&out.stdout) + ); + String::from_utf8(out.stderr).expect("stderr must be utf-8") +} + +/// The dev-dependencies of the root package are resolved, the dev-dependencies +/// of a dependency are not. +#[test] +fn dev_deps_are_root_only() { + let root = setup("root_only"); + write_root_manifest(&root, ROOT_MANIFEST); + + // Packages of the same rank are printed on a single line. + let packages: Vec = run_bender_ok(&root, &["packages"]) + .split_whitespace() + .map(str::to_string) + .collect(); + + assert!( + packages.iter().any(|p| p == "lib"), + "regular dependency `lib` missing from {:?}", + packages + ); + assert!( + packages.iter().any(|p| p == "vip"), + "dev-dependency `vip` of the root package missing from {:?}", + packages + ); + assert!( + !packages.iter().any(|p| p == "lib_only"), + "dev-dependency `lib_only` of `lib` was propagated into {:?}", + packages + ); +} + +/// The dev-dependencies of the root package end up in the lockfile, the +/// dev-dependencies of a dependency do not. +#[test] +fn dev_deps_in_lockfile() { + let root = setup("lockfile"); + write_root_manifest(&root, ROOT_MANIFEST); + + run_bender_ok(&root, &["update"]); + let lock = fs::read_to_string(root.join("Bender.lock")).expect("Failed to read Bender.lock"); + + assert!( + lock.contains("lib:"), + "`lib` missing from lockfile:\n{}", + lock + ); + assert!( + lock.contains("vip:"), + "`vip` missing from lockfile:\n{}", + lock + ); + assert!( + !lock.contains("lib_only"), + "`lib_only` leaked into lockfile:\n{}", + lock + ); +} + +/// The sources of a dev-dependency are available to the root package, and the +/// `target` field filters them as it does for a regular dependency. +#[test] +fn dev_dep_sources_follow_targets() { + let root = setup("targets"); + write_root_manifest( + &root, + "package:\n name: root_pkg\n\ + dependencies:\n lib: { path: ./lib }\n\ + dev_dependencies:\n vip: { path: ./vip, target: test }\n\ + sources:\n - src/top.sv\n - target: test\n files:\n - tb/tb_top.sv\n", + ); + + let without_test = run_bender_ok(&root, &["script", "flist"]); + assert!( + !without_test.contains("vip.sv"), + "target-gated dev-dependency leaked without `-t test`:\n{}", + without_test + ); + + let with_test = run_bender_ok(&root, &["script", "flist", "-t", "test"]); + assert!( + with_test.contains("vip.sv"), + "dev-dependency sources missing with `-t test`:\n{}", + with_test + ); + assert!( + with_test.contains("tb_top.sv"), + "root testbench sources missing with `-t test`:\n{}", + with_test + ); +} + +/// Listing the same package as both a dependency and a dev-dependency is an +/// error. +#[test] +fn duplicate_dep_and_dev_dep_is_rejected() { + let root = setup("duplicate"); + write_root_manifest( + &root, + "package:\n name: root_pkg\n\ + dependencies:\n lib: { path: ./lib }\n\ + dev_dependencies:\n lib: { path: ./lib }\n\ + sources:\n - src/top.sv\n", + ); + + let stderr = run_bender_failing(&root, &["packages"]); + assert!( + stderr.contains("both a dependency and a dev-dependency"), + "unexpected error message:\n{}", + stderr + ); +} + +/// The `dev-dependencies` spelling is accepted as an alias. +#[test] +fn dashed_alias_is_accepted() { + let root = setup("alias"); + write_root_manifest( + &root, + "package:\n name: root_pkg\n\ + dependencies:\n lib: { path: ./lib }\n\ + dev-dependencies:\n vip: { path: ./vip }\n\ + sources:\n - src/top.sv\n", + ); + + let packages = run_bender_ok(&root, &["packages"]); + assert!( + packages.split_whitespace().any(|p| p == "vip"), + "`dev-dependencies` alias not honoured:\n{}", + packages + ); +} + +/// A package that is both a dev-dependency of the root package and a regular +/// transitive dependency resolves to a single entry. +#[test] +fn dev_dep_overlapping_transitive_dep() { + let root = setup("overlap"); + // Make `lib` depend on `vip` regularly, while the root dev-depends on it. + write_package( + &root, + "lib", + "package:\n name: lib\n\ + dependencies:\n vip: { path: ../vip }\n\ + sources:\n - src/lib.sv\n", + ); + write_root_manifest(&root, ROOT_MANIFEST); + + run_bender_ok(&root, &["update"]); + let lock = fs::read_to_string(root.join("Bender.lock")).expect("Failed to read Bender.lock"); + assert_eq!( + lock.matches("\n vip:").count(), + 1, + "`vip` should appear exactly once in the lockfile:\n{}", + lock + ); + + // `vip` must be ranked below `lib`, since `lib` depends on it. + let flist = run_bender_ok(&root, &["script", "flist"]); + let vip_pos = flist.find("vip.sv").expect("vip sources missing"); + let lib_pos = flist.find("lib.sv").expect("lib sources missing"); + assert!( + vip_pos < lib_pos, + "`vip` must come before `lib` in the file list:\n{}", + flist + ); +} + +/// `bender parents` reports the root package as a parent of its +/// dev-dependencies. +#[test] +fn parents_reports_root_dev_deps() { + let root = setup("parents"); + write_root_manifest(&root, ROOT_MANIFEST); + + let parents = run_bender_ok(&root, &["parents", "vip"]); + assert!( + parents.contains("root_pkg"), + "root package missing from parents of dev-dependency:\n{}", + parents + ); +} From e3462183da61442efc01ee85586ca91cd1093855 Mon Sep 17 00:00:00 2001 From: Tim Fischer Date: Mon, 3 Aug 2026 10:39:45 +0200 Subject: [PATCH 2/3] tests: Move dev-dependency tests into config unit tests and `resolution.rs` The manifest-level cases (section separation, the `dev-dependencies` alias, field parity with `dependencies`, and the duplicate-section error) are pure `PartialManifest::validate` behavior and do not need a subprocess. Move them to `#[cfg(test)] mod tests` in `config.rs`, alongside the existing unit tests in `progress.rs` and `diagnostic.rs`. What remains genuinely needs an end-to-end run against a multi-package graph, so keep it as an integration test but name the file for what it covers rather than for one feature, so later resolution and source-tree tests have a home. Co-Authored-By: Claude Opus 5 --- src/config.rs | 104 ++++++++++++++++ tests/{dev_deps.rs => resolution.rs} | 171 +++++++++------------------ 2 files changed, 162 insertions(+), 113 deletions(-) rename tests/{dev_deps.rs => resolution.rs} (64%) diff --git a/src/config.rs b/src/config.rs index 5b0c5b42..3b6b51c4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2095,3 +2095,107 @@ fn env_string_from_string(path_str: &str) -> Result { pub(crate) fn env_path_from_string(path_str: &str) -> Result { Ok(PathBuf::from(env_string_from_string(path_str)?)) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Parse and validate a manifest, ignoring whether the source files exist. + fn parse_manifest(yaml: &str) -> Result { + let partial: PartialManifest = + serde_yaml_ng::from_str(yaml).expect("Failed to parse manifest"); + partial.validate_ignore_sources() + } + + #[test] + fn dev_dependencies_are_kept_separate() { + let manifest = parse_manifest( + "package:\n name: pkg\n\ + dependencies:\n lib: { path: ../lib }\n\ + dev_dependencies:\n vip: { path: ../vip }\n", + ) + .unwrap(); + + assert_eq!(manifest.dependencies.keys().collect::>(), ["lib"]); + assert_eq!( + manifest.dev_dependencies.keys().collect::>(), + ["vip"] + ); + // The root package sees both, in that order. + assert_eq!( + manifest + .root_dependencies() + .map(|(name, _)| name.as_str()) + .collect::>(), + ["lib", "vip"] + ); + assert!(manifest.root_dependency("vip").is_some()); + assert!(manifest.root_dependency("nope").is_none()); + } + + #[test] + fn dev_dependencies_accept_the_same_fields() { + let manifest = parse_manifest( + "package:\n name: pkg\n\ + remotes:\n pulp: \"https://github.com/pulp-platform\"\n\ + dev_dependencies:\n\ + \x20 vip: { version: \"0.2\", target: test, pass_targets: [\"debug\"] }\n", + ) + .unwrap(); + + match &manifest.dev_dependencies["vip"] { + Dependency::GitVersion { + target, + pass_targets, + url, + .. + } => { + assert_eq!(target.to_string(), "test"); + assert_eq!(pass_targets.len(), 1); + assert!(url.contains("pulp-platform"), "unexpected url {url}"); + } + other => panic!("expected a git version dependency, got {other:?}"), + } + } + + #[test] + fn dev_dependencies_dashed_alias_is_accepted() { + let manifest = parse_manifest( + "package:\n name: pkg\n\ + dev-dependencies:\n vip: { path: ../vip }\n", + ) + .unwrap(); + + assert_eq!( + manifest.dev_dependencies.keys().collect::>(), + ["vip"] + ); + } + + #[test] + fn dependency_in_both_sections_is_rejected() { + let err = parse_manifest( + "package:\n name: pkg\n\ + dependencies:\n lib: { path: ../lib }\n\ + dev_dependencies:\n lib: { path: ../lib }\n", + ) + .unwrap_err(); + + assert!( + format!("{err:?}").contains("both a dependency and a dev-dependency"), + "unexpected error: {err:?}" + ); + } + + #[test] + fn manifest_without_dev_dependencies_has_none() { + let manifest = parse_manifest( + "package:\n name: pkg\n\ + dependencies:\n lib: { path: ../lib }\n", + ) + .unwrap(); + + assert!(manifest.dev_dependencies.is_empty()); + assert_eq!(manifest.root_dependencies().count(), 1); + } +} diff --git a/tests/dev_deps.rs b/tests/resolution.rs similarity index 64% rename from tests/dev_deps.rs rename to tests/resolution.rs index f09f65c3..b924815a 100644 --- a/tests/dev_deps.rs +++ b/tests/resolution.rs @@ -1,14 +1,18 @@ // Copyright (c) 2026 ETH Zurich // Tim Fischer -//! Tests for the `dev_dependencies` manifest section. +//! End-to-end tests for dependency resolution and the resulting source tree. //! -//! The fixture is built from scratch in a temporary directory and only uses -//! path dependencies, so the tests need no network access. +//! Manifest parsing and validation is covered by the unit tests in +//! `src/config.rs`; the tests here run the actual binary and assert on the +//! resolved graph, the lockfile and the generated file lists. +//! +//! Fixtures are built from scratch under `tests/tmp/` and only use path +//! dependencies, so no network access is required and each case gets a clean +//! directory. use std::fs; use std::path::{Path, PathBuf}; -use std::process::Output; use assert_cmd::cargo; @@ -27,18 +31,44 @@ fn write_package(root: &Path, name: &str, manifest: &str) { ); } -/// Build the fixture and return its root directory. +/// Create an empty fixture directory for `case`. +fn fixture(case: &str) -> PathBuf { + let root = Path::new("tests/tmp").join(case); + if root.exists() { + fs::remove_dir_all(&root).expect("Failed to clean fixture directory"); + } + fs::create_dir_all(&root).expect("Failed to create fixture directory"); + root +} + +/// Run bender in the fixture directory, expecting success; returns stdout. +fn run_bender(root: &Path, args: &[&str]) -> String { + let mut full_args = vec!["-d", root.to_str().unwrap()]; + full_args.extend(args); + + let out = cargo::cargo_bin_cmd!() + .args(&full_args) + .output() + .expect("Failed to execute bender binary"); + + assert!( + out.status.success(), + "bender {:?} failed.\nstdout:\n{}\nstderr:\n{}", + args, + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8(out.stdout).expect("stdout must be utf-8") +} + +/// Build the standard dev-dependency fixture and return its root directory. /// /// The dependency structure is: /// /// - `root_pkg` depends on `lib` and dev-depends on `vip` /// - `lib` dev-depends on `lib_only`, which must not reach `root_pkg` -fn setup(case: &str) -> PathBuf { - let root = Path::new("tests/tmp/dev_deps").join(case); - if root.exists() { - fs::remove_dir_all(&root).expect("Failed to clean fixture directory"); - } - fs::create_dir_all(&root).expect("Failed to create fixture directory"); +fn dev_dep_fixture(case: &str, root_manifest: &str) -> PathBuf { + let root = fixture(case); write_package( &root, @@ -58,70 +88,30 @@ fn setup(case: &str) -> PathBuf { "package:\n name: lib_only\nsources:\n - src/lib_only.sv\n", ); - root -} - -/// Write the root manifest of the fixture. -fn write_root_manifest(root: &Path, manifest: &str) { - write_file(&root.join("Bender.yml"), manifest); + write_file(&root.join("Bender.yml"), root_manifest); write_file(&root.join("src").join("top.sv"), "module top;\nendmodule\n"); write_file( &root.join("tb").join("tb_top.sv"), "module tb_top;\nendmodule\n", ); + + root } -/// The default root manifest: a regular dependency plus a dev-dependency. +/// The standard root manifest: a regular dependency plus a dev-dependency. const ROOT_MANIFEST: &str = "package:\n name: root_pkg\n\ dependencies:\n lib: { path: ./lib }\n\ dev_dependencies:\n vip: { path: ./vip }\n\ sources:\n - src/top.sv\n - target: test\n files:\n - tb/tb_top.sv\n"; -/// Run bender in the fixture directory. -fn run_bender(root: &Path, args: &[&str]) -> Output { - let mut full_args = vec!["-d", root.to_str().unwrap()]; - full_args.extend(args); - - cargo::cargo_bin_cmd!() - .args(&full_args) - .output() - .expect("Failed to execute bender binary") -} - -/// Run bender expecting success; returns stdout. -fn run_bender_ok(root: &Path, args: &[&str]) -> String { - let out = run_bender(root, args); - assert!( - out.status.success(), - "bender {:?} failed.\nstdout:\n{}\nstderr:\n{}", - args, - String::from_utf8_lossy(&out.stdout), - String::from_utf8_lossy(&out.stderr) - ); - String::from_utf8(out.stdout).expect("stdout must be utf-8") -} - -/// Run bender expecting failure; returns stderr. -fn run_bender_failing(root: &Path, args: &[&str]) -> String { - let out = run_bender(root, args); - assert!( - !out.status.success(), - "bender {:?} unexpectedly succeeded.\nstdout:\n{}", - args, - String::from_utf8_lossy(&out.stdout) - ); - String::from_utf8(out.stderr).expect("stderr must be utf-8") -} - /// The dev-dependencies of the root package are resolved, the dev-dependencies /// of a dependency are not. #[test] fn dev_deps_are_root_only() { - let root = setup("root_only"); - write_root_manifest(&root, ROOT_MANIFEST); + let root = dev_dep_fixture("dev_deps_root_only", ROOT_MANIFEST); // Packages of the same rank are printed on a single line. - let packages: Vec = run_bender_ok(&root, &["packages"]) + let packages: Vec = run_bender(&root, &["packages"]) .split_whitespace() .map(str::to_string) .collect(); @@ -147,10 +137,9 @@ fn dev_deps_are_root_only() { /// dev-dependencies of a dependency do not. #[test] fn dev_deps_in_lockfile() { - let root = setup("lockfile"); - write_root_manifest(&root, ROOT_MANIFEST); + let root = dev_dep_fixture("dev_deps_lockfile", ROOT_MANIFEST); - run_bender_ok(&root, &["update"]); + run_bender(&root, &["update"]); let lock = fs::read_to_string(root.join("Bender.lock")).expect("Failed to read Bender.lock"); assert!( @@ -174,23 +163,22 @@ fn dev_deps_in_lockfile() { /// `target` field filters them as it does for a regular dependency. #[test] fn dev_dep_sources_follow_targets() { - let root = setup("targets"); - write_root_manifest( - &root, + let root = dev_dep_fixture( + "dev_deps_targets", "package:\n name: root_pkg\n\ dependencies:\n lib: { path: ./lib }\n\ dev_dependencies:\n vip: { path: ./vip, target: test }\n\ sources:\n - src/top.sv\n - target: test\n files:\n - tb/tb_top.sv\n", ); - let without_test = run_bender_ok(&root, &["script", "flist"]); + let without_test = run_bender(&root, &["script", "flist"]); assert!( !without_test.contains("vip.sv"), "target-gated dev-dependency leaked without `-t test`:\n{}", without_test ); - let with_test = run_bender_ok(&root, &["script", "flist", "-t", "test"]); + let with_test = run_bender(&root, &["script", "flist", "-t", "test"]); assert!( with_test.contains("vip.sv"), "dev-dependency sources missing with `-t test`:\n{}", @@ -203,52 +191,11 @@ fn dev_dep_sources_follow_targets() { ); } -/// Listing the same package as both a dependency and a dev-dependency is an -/// error. -#[test] -fn duplicate_dep_and_dev_dep_is_rejected() { - let root = setup("duplicate"); - write_root_manifest( - &root, - "package:\n name: root_pkg\n\ - dependencies:\n lib: { path: ./lib }\n\ - dev_dependencies:\n lib: { path: ./lib }\n\ - sources:\n - src/top.sv\n", - ); - - let stderr = run_bender_failing(&root, &["packages"]); - assert!( - stderr.contains("both a dependency and a dev-dependency"), - "unexpected error message:\n{}", - stderr - ); -} - -/// The `dev-dependencies` spelling is accepted as an alias. -#[test] -fn dashed_alias_is_accepted() { - let root = setup("alias"); - write_root_manifest( - &root, - "package:\n name: root_pkg\n\ - dependencies:\n lib: { path: ./lib }\n\ - dev-dependencies:\n vip: { path: ./vip }\n\ - sources:\n - src/top.sv\n", - ); - - let packages = run_bender_ok(&root, &["packages"]); - assert!( - packages.split_whitespace().any(|p| p == "vip"), - "`dev-dependencies` alias not honoured:\n{}", - packages - ); -} - /// A package that is both a dev-dependency of the root package and a regular /// transitive dependency resolves to a single entry. #[test] fn dev_dep_overlapping_transitive_dep() { - let root = setup("overlap"); + let root = dev_dep_fixture("dev_deps_overlap", ROOT_MANIFEST); // Make `lib` depend on `vip` regularly, while the root dev-depends on it. write_package( &root, @@ -257,9 +204,8 @@ fn dev_dep_overlapping_transitive_dep() { dependencies:\n vip: { path: ../vip }\n\ sources:\n - src/lib.sv\n", ); - write_root_manifest(&root, ROOT_MANIFEST); - run_bender_ok(&root, &["update"]); + run_bender(&root, &["update"]); let lock = fs::read_to_string(root.join("Bender.lock")).expect("Failed to read Bender.lock"); assert_eq!( lock.matches("\n vip:").count(), @@ -269,7 +215,7 @@ fn dev_dep_overlapping_transitive_dep() { ); // `vip` must be ranked below `lib`, since `lib` depends on it. - let flist = run_bender_ok(&root, &["script", "flist"]); + let flist = run_bender(&root, &["script", "flist"]); let vip_pos = flist.find("vip.sv").expect("vip sources missing"); let lib_pos = flist.find("lib.sv").expect("lib sources missing"); assert!( @@ -283,10 +229,9 @@ fn dev_dep_overlapping_transitive_dep() { /// dev-dependencies. #[test] fn parents_reports_root_dev_deps() { - let root = setup("parents"); - write_root_manifest(&root, ROOT_MANIFEST); + let root = dev_dep_fixture("dev_deps_parents", ROOT_MANIFEST); - let parents = run_bender_ok(&root, &["parents", "vip"]); + let parents = run_bender(&root, &["parents", "vip"]); assert!( parents.contains("root_pkg"), "root package missing from parents of dev-dependency:\n{}", From cd6a7b2da95b7ddbc31ed37ed44aa731633f0e12 Mon Sep 17 00:00:00 2001 From: Tim Fischer Date: Mon, 3 Aug 2026 10:51:13 +0200 Subject: [PATCH 3/3] Drop the `dev-dependencies` alias in favour of snake_case only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every key in the manifest format is snake_case, and the alias was the only serde alias or rename in `config.rs`, so it would have been the sole kebab-case spelling accepted anywhere in a `Bender.yml`. The aliases bender does carry are all CLI backward-compatibility shims; there is nothing to stay compatible with for a brand-new section. The kebab-case spelling now falls through to the unknown-field path, which names the offending key: warning[W03]: Ignoring unknown field dev-dependencies in package X. ╰─› help: Check for typos in dev-dependencies or remove it [...] Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- book/src/dependencies.md | 2 +- src/config.rs | 11 +++++------ 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0f55eac..4407bb49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a ## Unreleased ### Added -- Add a `dev_dependencies` manifest section (alias `dev-dependencies`) for dependencies that are only needed to work on the package itself. They are resolved when the package is the root package and are not propagated to packages depending on it. Entries accept the same fields as `dependencies`, including `target` and `pass_targets`; listing a package in both sections is an error. Older Bender versions ignore the section with warning `W03`, which is the correct behavior for a consumer. +- Add a `dev_dependencies` manifest section for dependencies that are only needed to work on the package itself. They are resolved when the package is the root package and are not propagated to packages depending on it. Entries accept the same fields as `dependencies`, including `target` and `pass_targets`; listing a package in both sections is an error. Older Bender versions ignore the section with warning `W03`, which is the correct behavior for a consumer. ## 0.32.1 - 2026-07-07 ### Added diff --git a/book/src/dependencies.md b/book/src/dependencies.md index 91f76fb4..9f7d39d7 100644 --- a/book/src/dependencies.md +++ b/book/src/dependencies.md @@ -56,7 +56,7 @@ dev_dependencies: The two sections take exactly the same entries: Git, path and revision dependencies, `remote` shorthands, `target` filters and `pass_targets` all behave identically. The only difference is propagation. Listing the same package in both sections is an error. -> **Note:** For compatibility with Cargo's spelling, `dev-dependencies` is accepted as an alias for `dev_dependencies`. +> **Note:** The section is spelled `dev_dependencies`, matching the snake_case used throughout the manifest. Cargo's kebab-case `dev-dependencies` is *not* accepted; it is reported as an unknown field via warning `W03`. This is the mechanism to reach for when a package's testbench needs verification IP that its consumers should not inherit. A downstream project depending on such a package resolves only its `dependencies`; the `dev_dependencies` are ignored entirely and never appear in the downstream [`Bender.lock`](./lockfile.md). diff --git a/src/config.rs b/src/config.rs index 3b6b51c4..1e7be194 100644 --- a/src/config.rs +++ b/src/config.rs @@ -472,7 +472,6 @@ pub struct PartialManifest { /// The dependencies. pub dependencies: Option>>, /// The dependencies only needed to work on this package itself. - #[serde(alias = "dev-dependencies")] pub dev_dependencies: Option>>, /// The source files. pub sources: Option>, @@ -2159,17 +2158,17 @@ mod tests { } #[test] - fn dev_dependencies_dashed_alias_is_accepted() { + fn dev_dependencies_reject_kebab_case_spelling() { + // The manifest format is uniformly snake_case; the kebab-case spelling + // must fall through to the unknown-field path rather than silently + // becoming a second accepted name. let manifest = parse_manifest( "package:\n name: pkg\n\ dev-dependencies:\n vip: { path: ../vip }\n", ) .unwrap(); - assert_eq!( - manifest.dev_dependencies.keys().collect::>(), - ["vip"] - ); + assert!(manifest.dev_dependencies.is_empty()); } #[test]