From 911b6642425dedc89cc0e72566fc112324bb6722 Mon Sep 17 00:00:00 2001 From: Goulven CLEC'H Date: Sat, 25 Apr 2026 12:51:09 +0200 Subject: [PATCH 1/2] feat(core,github-action): configurable git tag format --- .../changesets/somber-iceseeker-marjatta.md | 13 + crates/sampo-core/src/config.rs | 263 ++++++++--- crates/sampo-core/src/lib.rs | 1 + crates/sampo-core/src/publish.rs | 137 +++++- crates/sampo-core/src/release_tests.rs | 15 +- crates/sampo-core/src/tag_template.rs | 435 ++++++++++++++++++ crates/sampo-github-action/src/main.rs | 77 ++-- .../sampo-github-action/tests/integration.rs | 4 +- crates/sampo/README.md | 12 +- 9 files changed, 808 insertions(+), 149 deletions(-) create mode 100644 .sampo/changesets/somber-iceseeker-marjatta.md create mode 100644 crates/sampo-core/src/tag_template.rs diff --git a/.sampo/changesets/somber-iceseeker-marjatta.md b/.sampo/changesets/somber-iceseeker-marjatta.md new file mode 100644 index 0000000..2ab5ec1 --- /dev/null +++ b/.sampo/changesets/somber-iceseeker-marjatta.md @@ -0,0 +1,13 @@ +--- +cargo/sampo-core: minor +cargo/sampo: minor +cargo/sampo-github-action: minor +--- + +Made git tag format configurable via new `tag_format` and `short_tags_format` options under `[git]`. Templates accept `{ecosystem}`, `{package_name}`, and `{version}`. Sampo refuses to publish when two packages would render to the same tag. + +**⚠️ breaking change:** The default `tag_format` is now `{ecosystem}-{package_name}-v{version}` (was `{package_name}-v{version}`) so cross-ecosystem packages with the same name get distinct tags. To keep the previous tag shape, set: +```toml +[git] +tag_format = "{package_name}-v{version}" +``` diff --git a/crates/sampo-core/src/config.rs b/crates/sampo-core/src/config.rs index 031cbdf..af3f377 100644 --- a/crates/sampo-core/src/config.rs +++ b/crates/sampo-core/src/config.rs @@ -1,9 +1,16 @@ use crate::errors::SampoError; +use crate::tag_template::TagTemplate; +use crate::types::PackageKind; use rustc_hash::FxHashSet; -use semver::Version; use std::collections::BTreeSet; use std::path::Path; +/// Includes the ecosystem so same-named packages across ecosystems get distinct tags. +pub const DEFAULT_TAG_FORMAT: &str = "{ecosystem}-{package_name}-v{version}"; + +/// Used by packages selected via `git.short_tags` (Packagist requires `vX.Y.Z`). +pub const DEFAULT_SHORT_TAGS_FORMAT: &str = "v{version}"; + /// Configuration for Sampo #[derive(Debug, Clone)] pub struct Config { @@ -24,8 +31,10 @@ pub struct Config { pub ignore: Vec, pub git_default_branch: Option, pub git_release_branches: Vec, - /// Package using short tag format (`v{version}`) for Packagist compatibility. + /// Package using short tag format for Packagist compatibility. pub git_short_tags: Option, + pub git_tag_format: TagTemplate, + pub git_short_tags_format: TagTemplate, } impl Default for Config { @@ -46,6 +55,8 @@ impl Default for Config { git_default_branch: None, git_release_branches: Vec::new(), git_short_tags: None, + git_tag_format: TagTemplate::from_static(DEFAULT_TAG_FORMAT), + git_short_tags_format: TagTemplate::from_static(DEFAULT_SHORT_TAGS_FORMAT), } } } @@ -274,40 +285,54 @@ impl Config { } } - let (git_default_branch, git_release_branches, git_short_tags) = value - .get("git") - .and_then(|v| v.as_table()) - .map(|git_table| { - let default_branch = git_table - .get("default_branch") - .and_then(|v| v.as_str()) - .map(|s| s.trim()) - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()); - - let release_branches = git_table - .get("release_branches") - .and_then(|v| v.as_array()) - .map(|arr| { - arr.iter() - .filter_map(|item| item.as_str()) - .map(|s| s.trim()) - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()) - .collect::>() - }) - .unwrap_or_default(); + let git_table = value.get("git").and_then(|v| v.as_table()); - let short_tags = git_table - .get("short_tags") - .and_then(|v| v.as_str()) + let git_default_branch = git_table + .and_then(|t| t.get("default_branch")) + .and_then(|v| v.as_str()) + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()); + + let git_release_branches = git_table + .and_then(|t| t.get("release_branches")) + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|item| item.as_str()) .map(|s| s.trim()) .filter(|s| !s.is_empty()) - .map(|s| s.to_string()); - - (default_branch, release_branches, short_tags) + .map(|s| s.to_string()) + .collect::>() }) - .unwrap_or((None, Vec::new(), None)); + .unwrap_or_default(); + + let git_short_tags = git_table + .and_then(|t| t.get("short_tags")) + .and_then(|v| v.as_str()) + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + .map(|s| s.to_string()); + + let git_tag_format = match git_table.and_then(|t| t.get("tag_format")) { + Some(value) => { + let raw = value + .as_str() + .ok_or_else(|| SampoError::Config("git.tag_format must be a string".into()))?; + TagTemplate::parse(raw)? + } + None => TagTemplate::from_static(DEFAULT_TAG_FORMAT), + }; + + let git_short_tags_format = match git_table.and_then(|t| t.get("short_tags_format")) { + Some(value) => { + let raw = value.as_str().ok_or_else(|| { + SampoError::Config("git.short_tags_format must be a string".into()) + })?; + TagTemplate::parse(raw)? + } + None => TagTemplate::from_static(DEFAULT_SHORT_TAGS_FORMAT), + }; Ok(Self { version, @@ -325,6 +350,8 @@ impl Config { git_default_branch, git_release_branches, git_short_tags, + git_tag_format, + git_short_tags_format, }) } @@ -347,45 +374,47 @@ impl Config { self.release_branches().contains(branch) } - /// Returns true if the given package should use short tag format (`v{version}`). + /// Returns true if the given package should use the short tag format. pub fn uses_short_tags(&self, package_name: &str) -> bool { self.git_short_tags .as_ref() .is_some_and(|name| name == package_name) } - /// Builds a git tag name for the given package and version. - pub fn build_tag_name(&self, package_name: &str, version: &str) -> String { + /// Returns the template that applies to the given package. + pub fn tag_template_for(&self, package_name: &str) -> &TagTemplate { if self.uses_short_tags(package_name) { - format!("v{}", version) + &self.git_short_tags_format } else { - format!("{}-v{}", package_name, version) + &self.git_tag_format } } - /// Parses a tag and returns (package_name, version). + /// Builds a git tag name for the given package and version. + pub fn build_tag_name(&self, kind: PackageKind, package_name: &str, version: &str) -> String { + self.tag_template_for(package_name) + .render(kind, package_name, version) + } + + /// Parses a tag and returns `(package_name, version)`. + /// + /// Tries the short-tag template first (it typically lacks `{package_name}` + /// and only matches the one configured package), then the regular template. pub fn parse_tag(&self, tag: &str) -> Option<(String, String)> { - if let Some(short_pkg) = self - .git_short_tags - .as_ref() - .filter(|_| tag.starts_with('v')) + if let Some(short_pkg) = self.git_short_tags.as_deref() + && let Some(captured) = self.git_short_tags_format.match_tag(tag) + && let Some(version) = captured.version { - let version_str = tag.trim_start_matches('v'); - if Version::parse(version_str).is_ok() { - return Some((short_pkg.clone(), version_str.to_string())); - } + let name = captured + .package_name + .unwrap_or_else(|| short_pkg.to_string()); + return Some((name, version)); } - // Iterate over all "-v" positions to handle prereleases containing "-v" (e.g., "pkg-v1.2.3-v1"). - for (idx, _) in tag.match_indices("-v") { - let name = &tag[..idx]; - let version = &tag[idx + 2..]; - if name.is_empty() || version.is_empty() { - continue; - } - if Version::parse(version).is_ok() { - return Some((name.to_string(), version.to_string())); - } + if let Some(captured) = self.git_tag_format.match_tag(tag) + && let (Some(name), Some(version)) = (captured.package_name, captured.version) + { + return Some((name, version)); } None @@ -765,13 +794,61 @@ mod tests { .unwrap(); let config = Config::load(temp.path()).unwrap(); - assert_eq!(config.build_tag_name("my-package", "1.2.3"), "v1.2.3"); assert_eq!( - config.build_tag_name("other-package", "1.2.3"), - "other-package-v1.2.3" + config.build_tag_name(PackageKind::Packagist, "my-package", "1.2.3"), + "v1.2.3" + ); + assert_eq!( + config.build_tag_name(PackageKind::Cargo, "other-package", "1.2.3"), + "cargo-other-package-v1.2.3" + ); + } + + #[test] + fn build_tag_name_default_includes_ecosystem() { + let temp = tempfile::tempdir().unwrap(); + let config = Config::load(temp.path()).unwrap(); + assert_eq!( + config.build_tag_name(PackageKind::Cargo, "sampo-core", "0.1.0"), + "cargo-sampo-core-v0.1.0" + ); + assert_eq!( + config.build_tag_name(PackageKind::Npm, "sampo-core", "0.1.0"), + "npm-sampo-core-v0.1.0" ); } + #[test] + fn build_tag_name_honours_custom_format() { + let temp = tempfile::tempdir().unwrap(); + fs::create_dir_all(temp.path().join(".sampo")).unwrap(); + fs::write( + temp.path().join(".sampo/config.toml"), + "[git]\ntag_format = \"{package_name}-v{version}\"\n", + ) + .unwrap(); + + let config = Config::load(temp.path()).unwrap(); + assert_eq!( + config.build_tag_name(PackageKind::Cargo, "my-crate", "1.2.3"), + "my-crate-v1.2.3" + ); + } + + #[test] + fn rejects_invalid_tag_format() { + let temp = tempfile::tempdir().unwrap(); + fs::create_dir_all(temp.path().join(".sampo")).unwrap(); + fs::write( + temp.path().join(".sampo/config.toml"), + "[git]\ntag_format = \"{kind}-v{version}\"\n", + ) + .unwrap(); + + let err = Config::load(temp.path()).unwrap_err(); + assert!(format!("{err}").contains("unknown placeholder")); + } + #[test] fn parse_tag_handles_short_format() { let temp = tempfile::tempdir().unwrap(); @@ -791,9 +868,8 @@ mod tests { config.parse_tag("v1.2.3-alpha.1"), Some(("my-package".to_string(), "1.2.3-alpha.1".to_string())) ); - // Standard format still works assert_eq!( - config.parse_tag("other-package-v1.2.3"), + config.parse_tag("cargo-other-package-v1.2.3"), Some(("other-package".to_string(), "1.2.3".to_string())) ); } @@ -855,25 +931,76 @@ mod tests { } #[test] - fn parse_tag_without_short_tags_config() { + fn parse_tag_with_default_format() { let temp = tempfile::tempdir().unwrap(); let config = Config::load(temp.path()).unwrap(); + // Without short_tags, the default format requires an ecosystem prefix. assert_eq!(config.parse_tag("v1.2.3"), None); + assert_eq!(config.parse_tag("my-package-v1.2.3"), None); assert_eq!( - config.parse_tag("my-package-v1.2.3"), + config.parse_tag("cargo-my-package-v1.2.3"), Some(("my-package".to_string(), "1.2.3".to_string())) ); assert_eq!( - config.parse_tag("my-package-v1.2.3-alpha.1"), + config.parse_tag("npm-my-package-v1.2.3-alpha.1"), Some(("my-package".to_string(), "1.2.3-alpha.1".to_string())) ); - // -v in prerelease requires semver validation to parse correctly + // -v in prerelease still parses correctly thanks to semver validation assert_eq!( - config.parse_tag("my-package-v1.2.3-v1"), + config.parse_tag("cargo-my-package-v1.2.3-v1"), Some(("my-package".to_string(), "1.2.3-v1".to_string())) ); - assert_eq!(config.parse_tag("my-package-vfoo"), None); - assert_eq!(config.parse_tag("my-package-v1.2"), None); + assert_eq!(config.parse_tag("cargo-my-package-vfoo"), None); + assert_eq!(config.parse_tag("cargo-my-package-v1.2"), None); + } + + #[test] + fn build_tag_name_combines_tag_format_and_short_tags_format() { + let temp = tempfile::tempdir().unwrap(); + fs::create_dir_all(temp.path().join(".sampo")).unwrap(); + fs::write( + temp.path().join(".sampo/config.toml"), + "[git]\n\ + tag_format = \"{package_name}-v{version}\"\n\ + short_tags = \"my-php-pkg\"\n\ + short_tags_format = \"release-{version}\"\n", + ) + .unwrap(); + + let config = Config::load(temp.path()).unwrap(); + assert_eq!( + config.build_tag_name(PackageKind::Cargo, "core", "1.0.0"), + "core-v1.0.0" + ); + assert_eq!( + config.build_tag_name(PackageKind::Packagist, "my-php-pkg", "1.0.0"), + "release-1.0.0" + ); + assert_eq!( + config.parse_tag("core-v1.0.0"), + Some(("core".to_string(), "1.0.0".to_string())) + ); + assert_eq!( + config.parse_tag("release-1.0.0"), + Some(("my-php-pkg".to_string(), "1.0.0".to_string())) + ); + } + + #[test] + fn parse_tag_with_legacy_format_via_config() { + let temp = tempfile::tempdir().unwrap(); + fs::create_dir_all(temp.path().join(".sampo")).unwrap(); + fs::write( + temp.path().join(".sampo/config.toml"), + "[git]\ntag_format = \"{package_name}-v{version}\"\n", + ) + .unwrap(); + + let config = Config::load(temp.path()).unwrap(); + assert_eq!( + config.parse_tag("my-package-v1.2.3"), + Some(("my-package".to_string(), "1.2.3".to_string())) + ); } } diff --git a/crates/sampo-core/src/lib.rs b/crates/sampo-core/src/lib.rs index 6d9634c..1bd59ec 100644 --- a/crates/sampo-core/src/lib.rs +++ b/crates/sampo-core/src/lib.rs @@ -10,6 +10,7 @@ pub mod prerelease; pub mod process; pub mod publish; pub mod release; +pub mod tag_template; pub mod types; pub mod workspace; diff --git a/crates/sampo-core/src/publish.rs b/crates/sampo-core/src/publish.rs index fcebac6..31df474 100644 --- a/crates/sampo-core/src/publish.rs +++ b/crates/sampo-core/src/publish.rs @@ -130,6 +130,8 @@ pub fn run_publish( }); } + detect_tag_collisions(&config, &all_non_ignored)?; + // Validate internal deps do not include non-publishable packages let mut errors: Vec = Vec::new(); for identifier in &publishable { @@ -242,12 +244,17 @@ pub fn run_publish( adapter.publish(manifest.as_path(), dry_run, &args)?; any_published = true; - let tag = config.build_tag_name(&package.name, &package.version); + let tag = config.build_tag_name(package.kind, &package.name, &package.version); // Tag immediately after successful publish to ensure partial failures still tag what succeeded if !dry_run { - if let Err(e) = tag_published_crate(&ws.root, &config, &package.name, &package.version) - { + if let Err(e) = tag_published_crate( + &ws.root, + &config, + package.kind, + &package.name, + &package.version, + ) { eprintln!( "Warning: failed to create tag for {}@{}: {}", package.name, package.version, e @@ -255,7 +262,13 @@ pub fn run_publish( } else { tags_to_create.push(tag); } - } else if !package_tag_exists(&ws.root, &config, &package.name, &package.version)? { + } else if !package_tag_exists( + &ws.root, + &config, + package.kind, + &package.name, + &package.version, + )? { tags_to_create.push(tag); } } @@ -270,18 +283,28 @@ pub fn run_publish( if publishable.contains(package.canonical_identifier()) { continue; } - if !package_tag_exists(&ws.root, &config, &package.name, &package.version)? { + if !package_tag_exists( + &ws.root, + &config, + package.kind, + &package.name, + &package.version, + )? { private_packages_to_tag.push(*package); } } if any_published || !private_packages_to_tag.is_empty() { for package in &private_packages_to_tag { - let tag = config.build_tag_name(&package.name, &package.version); + let tag = config.build_tag_name(package.kind, &package.name, &package.version); if !dry_run { - if let Err(e) = - tag_published_crate(&ws.root, &config, &package.name, &package.version) - { + if let Err(e) = tag_published_crate( + &ws.root, + &config, + package.kind, + &package.name, + &package.version, + ) { eprintln!( "Warning: failed to create tag for {}@{}: {}", package.name, package.version, e @@ -307,9 +330,38 @@ pub fn run_publish( }) } +/// Reject the publish run when two packages would render to the same git tag +/// (e.g. cross-ecosystem same-name collisions, or a custom template that drops +/// `{package_name}`). Scoped to all non-ignored packages — private ones get +/// tagged too. +fn detect_tag_collisions(config: &Config, packages: &[&PackageInfo]) -> Result<()> { + let mut seen: BTreeMap = BTreeMap::new(); + for package in packages { + let tag = config.build_tag_name(package.kind, &package.name, &package.version); + if let Some(existing) = seen.insert(tag.clone(), package) { + let template_hint = if config.uses_short_tags(&package.name) + || config.uses_short_tags(&existing.name) + { + "Adjust git.tag_format and/or git.short_tags_format to include {ecosystem} and/or {package_name}." + } else { + "Adjust git.tag_format to include {ecosystem} and/or {package_name}." + }; + return Err(SampoError::Publish(format!( + "tag collision: '{}' and '{}' both render to git tag '{}'. {}", + existing.canonical_identifier(), + package.canonical_identifier(), + tag, + template_hint + ))); + } + } + Ok(()) +} + fn package_tag_exists( repo_root: &Path, config: &Config, + kind: PackageKind, crate_name: &str, version: &str, ) -> Result { @@ -317,7 +369,7 @@ fn package_tag_exists( return Ok(false); } - let tag = config.build_tag_name(crate_name, version); + let tag = config.build_tag_name(kind, crate_name, version); let out = Command::new("git") .arg("-C") .arg(repo_root) @@ -341,6 +393,7 @@ fn package_tag_exists( pub fn tag_published_crate( repo_root: &Path, config: &Config, + kind: PackageKind, crate_name: &str, version: &str, ) -> Result { @@ -348,10 +401,10 @@ pub fn tag_published_crate( // Not a git repo, skip return Ok(false); } - if package_tag_exists(repo_root, config, crate_name, version)? { + if package_tag_exists(repo_root, config, kind, crate_name, version)? { return Ok(false); } - let tag = config.build_tag_name(crate_name, version); + let tag = config.build_tag_name(kind, crate_name, version); let msg = format!("Release {} {}", crate_name, version); let status = Command::new("git") @@ -467,6 +520,44 @@ mod tests { sync::{Mutex, MutexGuard, OnceLock}, }; + fn make_package(kind: PackageKind, name: &str, version: &str) -> PackageInfo { + PackageInfo { + name: name.to_string(), + identifier: PackageInfo::dependency_identifier(kind, name), + version: version.to_string(), + path: PathBuf::from(format!("crates/{name}")), + internal_deps: std::collections::BTreeSet::new(), + internal_dev_deps: std::collections::BTreeSet::new(), + kind, + } + } + + #[test] + fn detect_tag_collisions_passes_when_default_format_disambiguates() { + let cargo_pkg = make_package(PackageKind::Cargo, "shared", "1.0.0"); + let npm_pkg = make_package(PackageKind::Npm, "shared", "1.0.0"); + let packages = vec![&cargo_pkg, &npm_pkg]; + let config = Config::default(); + detect_tag_collisions(&config, &packages).expect("default format must disambiguate"); + } + + #[test] + fn detect_tag_collisions_errors_when_template_drops_ecosystem() { + let cargo_pkg = make_package(PackageKind::Cargo, "shared", "1.0.0"); + let npm_pkg = make_package(PackageKind::Npm, "shared", "1.0.0"); + let packages = vec![&cargo_pkg, &npm_pkg]; + let config = Config { + git_tag_format: crate::tag_template::TagTemplate::parse("{package_name}-v{version}") + .unwrap(), + ..Config::default() + }; + let err = detect_tag_collisions(&config, &packages).unwrap_err(); + let msg = format!("{err}"); + assert!(msg.contains("tag collision")); + assert!(msg.contains("cargo/shared")); + assert!(msg.contains("npm/shared")); + } + #[test] fn args_for_kind_returns_universal_only_when_no_ecosystem_args() { let extra = PublishExtraArgs { @@ -1463,11 +1554,11 @@ edition = "2021" // Verify each tag exists once assert!( - tag_lines.contains(&"publishable-crate-v1.0.0"), + tag_lines.contains(&"cargo-publishable-crate-v1.0.0"), "Missing tag for publishable crate" ); assert!( - tag_lines.contains(&"private-crate-v1.0.0"), + tag_lines.contains(&"cargo-private-crate-v1.0.0"), "Missing tag for private crate" ); @@ -1656,11 +1747,11 @@ edition = "2021" ); assert!( - tag_lines.contains(&"private-one-v1.0.0"), + tag_lines.contains(&"cargo-private-one-v1.0.0"), "Missing tag for private-one" ); assert!( - tag_lines.contains(&"private-two-v1.0.0"), + tag_lines.contains(&"cargo-private-two-v1.0.0"), "Missing tag for private-two" ); } @@ -1743,11 +1834,11 @@ edition = "2021" ); assert!( - tag_lines.contains(&"publishable-crate-v1.0.0"), + tag_lines.contains(&"cargo-publishable-crate-v1.0.0"), "Missing tag for publishable crate" ); assert!( - tag_lines.contains(&"private-crate-v1.0.0"), + tag_lines.contains(&"cargo-private-crate-v1.0.0"), "Private tag should have been created because publishable package was published" ); } @@ -1830,12 +1921,12 @@ edition = "2021" // Both packages should receive tags assert!( - tag_lines.contains(&"lib-core-v1.0.0"), + tag_lines.contains(&"cargo-lib-core-v1.0.0"), "Publishable package should receive a tag. Got tags: {:?}", tag_lines ); assert!( - tag_lines.contains(&"internal-service-v0.5.0"), + tag_lines.contains(&"cargo-internal-service-v0.5.0"), "Private package should receive a tag in mixed workspace. Got tags: {:?}", tag_lines ); @@ -2032,8 +2123,8 @@ edition = "2021" .output() .expect("git tag list should succeed"); let tags = String::from_utf8_lossy(&output.stdout); - assert!(tags.contains("lib-core-v1.0.0")); - assert!(tags.contains("internal-service-v0.5.0")); + assert!(tags.contains("cargo-lib-core-v1.0.0")); + assert!(tags.contains("cargo-internal-service-v0.5.0")); // Now: bump ONLY the private package (simulates a release PR affecting only private crates) let service_manifest = workspace.root.join("crates/internal-service/Cargo.toml"); @@ -2069,7 +2160,7 @@ edition = "2021" let tags = String::from_utf8_lossy(&output.stdout); assert!( - tags.contains("internal-service-v0.6.0"), + tags.contains("cargo-internal-service-v0.6.0"), "Private package should receive a tag even when the publishable package was skipped. \ This is the exact bug from the review. Got tags: {}", tags diff --git a/crates/sampo-core/src/release_tests.rs b/crates/sampo-core/src/release_tests.rs index 9cfb923..0d776f6 100644 --- a/crates/sampo-core/src/release_tests.rs +++ b/crates/sampo-core/src/release_tests.rs @@ -1603,21 +1603,8 @@ tempfile = "3.0" // Create config with fixed dependencies let config = Config { - version: 1, - github_repository: None, - changelog_show_commit_hash: true, - changelog_show_acknowledgments: true, - changelog_show_release_date: true, - changelog_release_date_format: "%Y-%m-%d".to_string(), - changelog_release_date_timezone: None, - changesets_tags: vec![], fixed_dependencies: vec![vec!["pkg-a".to_string(), "pkg-c".to_string()]], - linked_dependencies: vec![], - ignore_unpublished: false, - ignore: vec![], - git_default_branch: None, - git_release_branches: Vec::new(), - git_short_tags: None, + ..Config::default() }; // Create changeset that affects pkg-b only diff --git a/crates/sampo-core/src/tag_template.rs b/crates/sampo-core/src/tag_template.rs new file mode 100644 index 0000000..3eca3c2 --- /dev/null +++ b/crates/sampo-core/src/tag_template.rs @@ -0,0 +1,435 @@ +use crate::errors::SampoError; +use crate::types::PackageKind; +use semver::Version; + +/// Placeholder tokens supported by tag templates. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Placeholder { + Ecosystem, + PackageName, + Version, +} + +impl Placeholder { + fn from_token(token: &str) -> Option { + match token { + "ecosystem" => Some(Self::Ecosystem), + "package_name" => Some(Self::PackageName), + "version" => Some(Self::Version), + _ => None, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum Segment { + Literal(String), + Placeholder(Placeholder), +} + +/// A parsed git tag template (e.g. `"{ecosystem}-{package_name}-v{version}"`). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TagTemplate { + raw: String, + segments: Vec, +} + +/// Captured fields from a tag string matched against a template. +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct TagMatch { + pub ecosystem: Option, + pub package_name: Option, + pub version: Option, +} + +impl TagTemplate { + /// Recognised placeholders: `{ecosystem}`, `{package_name}`, `{version}`. + /// `{` / `}` are not escapable. + pub fn parse(input: &str) -> Result { + let trimmed = input.trim(); + if trimmed.is_empty() { + return Err(SampoError::Config( + "tag format template cannot be empty".to_string(), + )); + } + + let mut segments = Vec::new(); + let mut buffer = String::new(); + let mut chars = trimmed.char_indices().peekable(); + while let Some((i, c)) = chars.next() { + match c { + '{' => { + let rest = &trimmed[i + 1..]; + let close_offset = rest.find('}').ok_or_else(|| { + SampoError::Config(format!( + "tag format template '{trimmed}' has an unterminated placeholder" + )) + })?; + let token = &rest[..close_offset]; + let placeholder = Placeholder::from_token(token).ok_or_else(|| { + SampoError::Config(format!( + "tag format template '{trimmed}' contains unknown placeholder '{{{token}}}'. \ + Allowed placeholders: {{ecosystem}}, {{package_name}}, {{version}}." + )) + })?; + if !buffer.is_empty() { + segments.push(Segment::Literal(std::mem::take(&mut buffer))); + } + segments.push(Segment::Placeholder(placeholder)); + let resume = i + 1 + close_offset + 1; + while let Some(&(idx, _)) = chars.peek() { + if idx < resume { + chars.next(); + } else { + break; + } + } + } + '}' => { + return Err(SampoError::Config(format!( + "tag format template '{trimmed}' has an unmatched '}}'" + ))); + } + _ => buffer.push(c), + } + } + if !buffer.is_empty() { + segments.push(Segment::Literal(buffer)); + } + + if !segments + .iter() + .any(|s| matches!(s, Segment::Placeholder(Placeholder::Version))) + { + return Err(SampoError::Config(format!( + "tag format template '{trimmed}' must include the {{version}} placeholder" + ))); + } + + for window in segments.windows(2) { + if matches!(window[0], Segment::Placeholder(_)) + && matches!(window[1], Segment::Placeholder(_)) + { + return Err(SampoError::Config(format!( + "tag format template '{trimmed}' has adjacent placeholders without a separator; \ + insert a literal character (e.g. '-') between them" + ))); + } + } + + Ok(Self { + raw: trimmed.to_string(), + segments, + }) + } + + pub fn as_str(&self) -> &str { + &self.raw + } + + /// Panics on invalid input. For hard-coded defaults only. + pub(crate) fn from_static(input: &'static str) -> Self { + Self::parse(input) + .unwrap_or_else(|err| panic!("invalid built-in tag template '{input}': {err}")) + } + + pub fn contains(&self, placeholder: Placeholder) -> bool { + self.segments + .iter() + .any(|s| matches!(s, Segment::Placeholder(p) if *p == placeholder)) + } + + pub fn render(&self, kind: PackageKind, package_name: &str, version: &str) -> String { + let mut out = String::new(); + for segment in &self.segments { + match segment { + Segment::Literal(text) => out.push_str(text), + Segment::Placeholder(Placeholder::Ecosystem) => out.push_str(kind.as_str()), + Segment::Placeholder(Placeholder::PackageName) => out.push_str(package_name), + Segment::Placeholder(Placeholder::Version) => out.push_str(version), + } + } + out + } + + /// Returns `None` when the tag doesn't fit the template, the `{ecosystem}` + /// capture isn't a known kind, or `{version}` isn't valid semver. + pub fn match_tag(&self, tag: &str) -> Option { + let mut state = MatchState::default(); + if match_segments(&self.segments, tag, &mut state) { + Some(TagMatch { + ecosystem: state.ecosystem, + package_name: state.package_name, + version: state.version, + }) + } else { + None + } + } +} + +#[derive(Default, Clone)] +struct MatchState { + ecosystem: Option, + package_name: Option, + version: Option, +} + +fn match_segments(segments: &[Segment], input: &str, state: &mut MatchState) -> bool { + if segments.is_empty() { + return input.is_empty(); + } + + match &segments[0] { + Segment::Literal(text) => { + if let Some(rest) = input.strip_prefix(text.as_str()) { + match_segments(&segments[1..], rest, state) + } else { + false + } + } + Segment::Placeholder(placeholder) => { + // `parse()` rejects adjacent placeholders, so the next segment is + // either a bounding literal or absent (placeholder takes the rest). + let next_literal = match segments.get(1) { + Some(Segment::Literal(text)) => Some(text.as_str()), + _ => None, + }; + + let (candidates, advance_after_capture) = match next_literal { + Some(literal) => ( + enumerate_literal_positions(input, literal), + Some(literal.len()), + ), + None => (vec![input.len()], None), + }; + + for end in candidates { + let captured = &input[..end]; + let rest_start = end + advance_after_capture.unwrap_or(0); + if rest_start > input.len() { + continue; + } + let rest = &input[rest_start..]; + + let mut local = state.clone(); + if !apply_capture(&mut local, *placeholder, captured) { + continue; + } + + let next_segments = if advance_after_capture.is_some() { + &segments[2..] + } else { + &segments[1..] + }; + + if match_segments(next_segments, rest, &mut local) { + *state = local; + return true; + } + } + + false + } + } +} + +/// Non-overlapping byte offsets of `literal` in `input`. Realistic tag +/// separators (`-`, `-v`, `/`) don't self-overlap, so this is fine. +fn enumerate_literal_positions(input: &str, literal: &str) -> Vec { + if literal.is_empty() { + return vec![input.len()]; + } + let mut positions = Vec::new(); + let mut start = 0; + while let Some(pos) = input[start..].find(literal) { + let absolute = start + pos; + positions.push(absolute); + start = absolute + literal.len(); + } + positions +} + +fn apply_capture(state: &mut MatchState, placeholder: Placeholder, captured: &str) -> bool { + if captured.is_empty() { + return false; + } + match placeholder { + Placeholder::Ecosystem => match PackageKind::parse(captured) { + Some(kind) => { + if let Some(existing) = state.ecosystem + && existing != kind + { + return false; + } + state.ecosystem = Some(kind); + true + } + None => false, + }, + Placeholder::PackageName => { + if let Some(existing) = &state.package_name + && existing != captured + { + return false; + } + state.package_name = Some(captured.to_string()); + true + } + Placeholder::Version => { + if Version::parse(captured).is_err() { + return false; + } + if let Some(existing) = &state.version + && existing != captured + { + return false; + } + state.version = Some(captured.to_string()); + true + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn template(s: &str) -> TagTemplate { + TagTemplate::parse(s).unwrap() + } + + #[test] + fn parses_default_template() { + let t = template("{ecosystem}-{package_name}-v{version}"); + assert_eq!(t.as_str(), "{ecosystem}-{package_name}-v{version}"); + assert!(t.contains(Placeholder::Ecosystem)); + assert!(t.contains(Placeholder::PackageName)); + assert!(t.contains(Placeholder::Version)); + } + + #[test] + fn rejects_unknown_placeholder() { + let err = TagTemplate::parse("{kind}-{version}").unwrap_err(); + assert!(format!("{err}").contains("unknown placeholder")); + } + + #[test] + fn rejects_template_without_version() { + let err = TagTemplate::parse("{package_name}").unwrap_err(); + assert!(format!("{err}").contains("{version}")); + } + + #[test] + fn rejects_unterminated_placeholder() { + let err = TagTemplate::parse("{version").unwrap_err(); + assert!(format!("{err}").contains("unterminated")); + } + + #[test] + fn rejects_unmatched_close_brace() { + let err = TagTemplate::parse("v}{version}").unwrap_err(); + assert!(format!("{err}").contains("unmatched")); + } + + #[test] + fn renders_default_template() { + let t = template("{ecosystem}-{package_name}-v{version}"); + assert_eq!( + t.render(PackageKind::Cargo, "sampo-core", "1.2.3"), + "cargo-sampo-core-v1.2.3" + ); + assert_eq!( + t.render(PackageKind::Npm, "@scope/foo", "0.1.0"), + "npm-@scope/foo-v0.1.0" + ); + } + + #[test] + fn renders_short_template() { + let t = template("v{version}"); + assert_eq!( + t.render(PackageKind::Packagist, "ignored", "1.0.0"), + "v1.0.0" + ); + } + + #[test] + fn matches_default_template() { + let t = template("{ecosystem}-{package_name}-v{version}"); + let m = t.match_tag("cargo-my-pkg-v1.2.3").unwrap(); + assert_eq!(m.ecosystem, Some(PackageKind::Cargo)); + assert_eq!(m.package_name.as_deref(), Some("my-pkg")); + assert_eq!(m.version.as_deref(), Some("1.2.3")); + } + + #[test] + fn matches_legacy_template_with_v_in_prerelease() { + let t = template("{package_name}-v{version}"); + let m = t.match_tag("my-pkg-v1.2.3-v1").unwrap(); + assert_eq!(m.package_name.as_deref(), Some("my-pkg")); + assert_eq!(m.version.as_deref(), Some("1.2.3-v1")); + } + + #[test] + fn matches_short_template() { + let t = template("v{version}"); + let m = t.match_tag("v1.2.3").unwrap(); + assert_eq!(m.version.as_deref(), Some("1.2.3")); + assert!(m.package_name.is_none()); + assert!(m.ecosystem.is_none()); + } + + #[test] + fn rejects_invalid_semver() { + let t = template("v{version}"); + assert!(t.match_tag("vfoo").is_none()); + assert!(t.match_tag("v1.2").is_none()); + } + + #[test] + fn rejects_unknown_ecosystem() { + let t = template("{ecosystem}-{package_name}-v{version}"); + assert!(t.match_tag("ruby-foo-v1.2.3").is_none()); + } + + #[test] + fn matches_with_unusual_separator() { + let t = template("{ecosystem}/{package_name}@{version}"); + let m = t.match_tag("npm/foo@1.0.0").unwrap(); + assert_eq!(m.ecosystem, Some(PackageKind::Npm)); + assert_eq!(m.package_name.as_deref(), Some("foo")); + assert_eq!(m.version.as_deref(), Some("1.0.0")); + } + + #[test] + fn preserves_non_ascii_literal_segments() { + let t = template("{package_name}—v{version}"); + assert_eq!(t.render(PackageKind::Cargo, "café", "1.2.3"), "café—v1.2.3"); + let m = t.match_tag("café—v1.2.3").unwrap(); + assert_eq!(m.package_name.as_deref(), Some("café")); + assert_eq!(m.version.as_deref(), Some("1.2.3")); + } + + #[test] + fn rejects_empty_capture() { + let t = template("{package_name}-v{version}"); + assert!(t.match_tag("-v1.2.3").is_none()); + } + + #[test] + fn rejects_template_without_version_placeholder() { + assert!(TagTemplate::parse("{package_name}-stable").is_err()); + } + + #[test] + fn template_can_omit_optional_placeholders() { + let t = template("release-{version}"); + assert_eq!( + t.render(PackageKind::Cargo, "anything", "9.9.9"), + "release-9.9.9" + ); + let m = t.match_tag("release-9.9.9").unwrap(); + assert_eq!(m.version.as_deref(), Some("9.9.9")); + } +} diff --git a/crates/sampo-github-action/src/main.rs b/crates/sampo-github-action/src/main.rs index a2c6f65..1184df0 100644 --- a/crates/sampo-github-action/src/main.rs +++ b/crates/sampo-github-action/src/main.rs @@ -919,27 +919,15 @@ fn build_release_body_from_changelog(workspace: &Path, tag: &str) -> Option) -> Option<(String, String)> { - if let Some(cfg) = config { - cfg.parse_tag(tag) - } else { - // Fallback to standard format only when no config is available - parse_tag(tag) + match config { + Some(cfg) => cfg.parse_tag(tag), + None => SampoConfig::default().parse_tag(tag), } } -/// Parse tags in standard format `-v`. -fn parse_tag(tag: &str) -> Option<(String, String)> { - let idx = tag.rfind("-v")?; - let (name, ver) = tag.split_at(idx); - let version = ver.trim_start_matches("-v").to_string(); - if name.is_empty() || version.is_empty() { - return None; - } - Some((name.to_string(), version)) -} - fn tag_is_prerelease_with_config(tag: &str, config: Option<&SampoConfig>) -> bool { parse_tag_with_config(tag, config) .and_then(|(_name, version)| Version::parse(&version).ok()) @@ -1260,8 +1248,8 @@ mod tests { #[test] fn tag_pre_release_detection() { - assert!(tag_is_prerelease("sampo-v1.2.0-alpha.1")); - assert!(!tag_is_prerelease("sampo-v1.2.0")); + assert!(tag_is_prerelease("cargo-sampo-v1.2.0-alpha.1")); + assert!(!tag_is_prerelease("cargo-sampo-v1.2.0")); assert!(!tag_is_prerelease("invalid")); } @@ -1372,7 +1360,7 @@ mod tests { rename: Some("{{crate}}-{{version}}.tar.gz".to_string()), }]; - let assets = resolve_release_assets(workspace, "my-crate-v1.0.0", &specs) + let assets = resolve_release_assets(workspace, "cargo-my-crate-v1.0.0", &specs) .expect("asset resolution should succeed"); assert_eq!(assets.len(), 1); assert_eq!(assets[0].asset_name, "my-crate-1.0.0.tar.gz"); @@ -1380,21 +1368,22 @@ mod tests { } #[test] - fn test_parse_tag() { + fn parse_tag_with_default_template() { assert_eq!( - parse_tag("my-crate-v1.2.3"), - Some(("my-crate".into(), "1.2.3".into())) + parse_tag_with_config("cargo-my-crate-v1.2.3", None), + Some(("my-crate".to_string(), "1.2.3".to_string())) ); assert_eq!( - parse_tag("sampo-v0.9.0"), - Some(("sampo".into(), "0.9.0".into())) + parse_tag_with_config("npm-sampo-v0.9.0", None), + Some(("sampo".to_string(), "0.9.0".to_string())) ); assert_eq!( - parse_tag("sampo-github-action-v0.8.2"), - Some(("sampo-github-action".into(), "0.8.2".into())) + parse_tag_with_config("cargo-sampo-github-action-v0.8.2", None), + Some(("sampo-github-action".to_string(), "0.8.2".to_string())) ); - assert_eq!(parse_tag("nope"), None); - assert_eq!(parse_tag("-v1.0.0"), None); + assert_eq!(parse_tag_with_config("nope", None), None); + // Short format needs an explicit ecosystem prefix under the default template. + assert_eq!(parse_tag_with_config("v1.2.3", None), None); } #[test] @@ -1420,9 +1409,8 @@ mod tests { Some(("my-package".to_string(), "1.2.3-alpha.1".to_string())) ); - // Standard format still works for other packages assert_eq!( - parse_tag_with_config("other-package-v2.0.0", Some(&config)), + parse_tag_with_config("cargo-other-package-v2.0.0", Some(&config)), Some(("other-package".to_string(), "2.0.0".to_string())) ); @@ -1431,13 +1419,21 @@ mod tests { } #[test] - fn parse_tag_with_config_falls_back_without_config() { + fn parse_tag_honours_custom_tag_format() { + use std::fs; + let temp = tempfile::tempdir().unwrap(); + fs::create_dir_all(temp.path().join(".sampo")).unwrap(); + fs::write( + temp.path().join(".sampo/config.toml"), + "[git]\ntag_format = \"{package_name}-v{version}\"\n", + ) + .unwrap(); + + let config = SampoConfig::load(temp.path()).unwrap(); assert_eq!( - parse_tag_with_config("my-crate-v1.2.3", None), + parse_tag_with_config("my-crate-v1.2.3", Some(&config)), Some(("my-crate".to_string(), "1.2.3".to_string())) ); - - assert_eq!(parse_tag_with_config("v1.2.3", None), None); } #[test] @@ -1464,11 +1460,11 @@ mod tests { assert!(!tag_is_prerelease_with_config("v2.3.4", Some(&config))); assert!(tag_is_prerelease_with_config( - "other-v1.0.0-alpha.1", + "cargo-other-v1.0.0-alpha.1", Some(&config) )); assert!(!tag_is_prerelease_with_config( - "other-v1.0.0", + "cargo-other-v1.0.0", Some(&config) )); } @@ -1582,7 +1578,7 @@ mod tests { ]; // Test with sampo tag - should only match sampo binaries - let sampo_assets = resolve_release_assets(workspace, "sampo-v0.9.0", &specs) + let sampo_assets = resolve_release_assets(workspace, "cargo-sampo-v0.9.0", &specs) .expect("sampo asset resolution should succeed"); assert_eq!( sampo_assets.len(), @@ -1598,8 +1594,9 @@ mod tests { assert!(sampo_assets.iter().any(|a| a.asset_name.contains("darwin"))); // Test with sampo-github-action tag - should only match action binaries - let action_assets = resolve_release_assets(workspace, "sampo-github-action-v0.8.2", &specs) - .expect("action asset resolution should succeed"); + let action_assets = + resolve_release_assets(workspace, "cargo-sampo-github-action-v0.8.2", &specs) + .expect("action asset resolution should succeed"); assert_eq!( action_assets.len(), 2, diff --git a/crates/sampo-github-action/tests/integration.rs b/crates/sampo-github-action/tests/integration.rs index 3ec53e4..7c3cf29 100644 --- a/crates/sampo-github-action/tests/integration.rs +++ b/crates/sampo-github-action/tests/integration.rs @@ -498,14 +498,14 @@ fn test_publish_private_packages_creates_tags_and_reports_published() { // Verify that git tags were actually created for the private package let tag_output = Command::new("git") - .args(["tag", "--list", "foo-v*"]) + .args(["tag", "--list", "cargo-foo-v*"]) .current_dir(ws.path()) .output() .expect("failed to list git tags"); let tags = String::from_utf8_lossy(&tag_output.stdout); assert!( - tags.contains("foo-v0.1.0"), + tags.contains("cargo-foo-v0.1.0"), "Expected git tag for private package foo@0.1.0, got tags: {}", tags ); diff --git a/crates/sampo/README.md b/crates/sampo/README.md index 0ee1ede..8883f8d 100644 --- a/crates/sampo/README.md +++ b/crates/sampo/README.md @@ -162,10 +162,18 @@ linked = [["cargo/pkg-e", "cargo/pkg-f"], ["cargo/pkg-g", "cargo/pkg-h"]] > [!TIP] > At runtime you can override the detected branch with the `SAMPO_RELEASE_BRANCH` environment variable, which is useful for local testing or custom CI setups. -`short_tags`: Optional package name that should use short tag format (`v{version}`) instead of the standard format (`{package}-v{version}`). +`tag_format`: Template used for git tags created by `sampo publish` (default: `"{ecosystem}-{package_name}-v{version}"`). Supported placeholders: + +- `{ecosystem}` — `cargo`, `npm`, `hex`, `pypi`, or `packagist`. +- `{package_name}` — the package's local name. +- `{version}` — the released version (required). + +`short_tags`: Optional package name that should use the short tag format (see `short_tags_format`) instead of `tag_format`. > [!IMPORTANT] -> For publishable PHP packages, this option is required as Packagist only detects short version tags. Sadly, the Packagist adapter does not support monorepos with multiple publishable PHP packages, as `vX.Y.Z` tags cannot distinguish between packages. +> For publishable PHP packages, the short option is required as Packagist only detects short version tags. Sadly, the Packagist adapter does not support monorepos with multiple publishable PHP packages, since their short tag format cannot distinguish between them. + +`short_tags_format`: Template applied to the package selected by `short_tags` (default: `"v{version}"`). Same placeholders as `tag_format`. ### `[github]` section From 003330fc519eaaf6c1902e3603ef281c51bf7dff Mon Sep 17 00:00:00 2001 From: Goulven CLEC'H Date: Sun, 26 Apr 2026 12:44:01 +0200 Subject: [PATCH 2/2] keep tag-format default and detect tag conflicts at publish --- .../changesets/somber-iceseeker-marjatta.md | 8 +- crates/sampo-core/src/config.rs | 44 ++-- crates/sampo-core/src/publish.rs | 216 +++++++++++++++--- crates/sampo-github-action/src/main.rs | 38 +-- .../sampo-github-action/tests/integration.rs | 4 +- crates/sampo/README.md | 5 +- 6 files changed, 231 insertions(+), 84 deletions(-) diff --git a/.sampo/changesets/somber-iceseeker-marjatta.md b/.sampo/changesets/somber-iceseeker-marjatta.md index 2ab5ec1..9ae5e2f 100644 --- a/.sampo/changesets/somber-iceseeker-marjatta.md +++ b/.sampo/changesets/somber-iceseeker-marjatta.md @@ -4,10 +4,6 @@ cargo/sampo: minor cargo/sampo-github-action: minor --- -Made git tag format configurable via new `tag_format` and `short_tags_format` options under `[git]`. Templates accept `{ecosystem}`, `{package_name}`, and `{version}`. Sampo refuses to publish when two packages would render to the same tag. +Made git tag format configurable via new `tag_format` and `short_tags_format` options under `[git]`. Templates accept `{ecosystem}`, `{package_name}`, and `{version}`. -**⚠️ breaking change:** The default `tag_format` is now `{ecosystem}-{package_name}-v{version}` (was `{package_name}-v{version}`) so cross-ecosystem packages with the same name get distinct tags. To keep the previous tag shape, set: -```toml -[git] -tag_format = "{package_name}-v{version}" -``` +`sampo publish` now also detects cross-ecosystem tag conflicts: it errors when two packages would produce the same git tag for the release in flight, and warns when packages share a name across ecosystems with a template that doesn't include `{ecosystem}` (a future version bump would silently collide). Both diagnostics suggest setting `tag_format = "{ecosystem}-{package_name}-v{version}"` as the fix. diff --git a/crates/sampo-core/src/config.rs b/crates/sampo-core/src/config.rs index af3f377..c4abbea 100644 --- a/crates/sampo-core/src/config.rs +++ b/crates/sampo-core/src/config.rs @@ -5,8 +5,9 @@ use rustc_hash::FxHashSet; use std::collections::BTreeSet; use std::path::Path; -/// Includes the ecosystem so same-named packages across ecosystems get distinct tags. -pub const DEFAULT_TAG_FORMAT: &str = "{ecosystem}-{package_name}-v{version}"; +/// Stays narrow by default; cross-ecosystem same-name conflicts are caught at +/// publish time and surface a hint suggesting `{ecosystem}` in the template. +pub const DEFAULT_TAG_FORMAT: &str = "{package_name}-v{version}"; /// Used by packages selected via `git.short_tags` (Packagist requires `vX.Y.Z`). pub const DEFAULT_SHORT_TAGS_FORMAT: &str = "v{version}"; @@ -800,21 +801,21 @@ mod tests { ); assert_eq!( config.build_tag_name(PackageKind::Cargo, "other-package", "1.2.3"), - "cargo-other-package-v1.2.3" + "other-package-v1.2.3" ); } #[test] - fn build_tag_name_default_includes_ecosystem() { + fn build_tag_name_default_omits_ecosystem() { let temp = tempfile::tempdir().unwrap(); let config = Config::load(temp.path()).unwrap(); assert_eq!( config.build_tag_name(PackageKind::Cargo, "sampo-core", "0.1.0"), - "cargo-sampo-core-v0.1.0" + "sampo-core-v0.1.0" ); assert_eq!( config.build_tag_name(PackageKind::Npm, "sampo-core", "0.1.0"), - "npm-sampo-core-v0.1.0" + "sampo-core-v0.1.0" ); } @@ -824,14 +825,14 @@ mod tests { fs::create_dir_all(temp.path().join(".sampo")).unwrap(); fs::write( temp.path().join(".sampo/config.toml"), - "[git]\ntag_format = \"{package_name}-v{version}\"\n", + "[git]\ntag_format = \"{ecosystem}-{package_name}-v{version}\"\n", ) .unwrap(); let config = Config::load(temp.path()).unwrap(); assert_eq!( config.build_tag_name(PackageKind::Cargo, "my-crate", "1.2.3"), - "my-crate-v1.2.3" + "cargo-my-crate-v1.2.3" ); } @@ -869,7 +870,7 @@ mod tests { Some(("my-package".to_string(), "1.2.3-alpha.1".to_string())) ); assert_eq!( - config.parse_tag("cargo-other-package-v1.2.3"), + config.parse_tag("other-package-v1.2.3"), Some(("other-package".to_string(), "1.2.3".to_string())) ); } @@ -935,24 +936,23 @@ mod tests { let temp = tempfile::tempdir().unwrap(); let config = Config::load(temp.path()).unwrap(); - // Without short_tags, the default format requires an ecosystem prefix. - assert_eq!(config.parse_tag("v1.2.3"), None); - assert_eq!(config.parse_tag("my-package-v1.2.3"), None); assert_eq!( - config.parse_tag("cargo-my-package-v1.2.3"), + config.parse_tag("my-package-v1.2.3"), Some(("my-package".to_string(), "1.2.3".to_string())) ); assert_eq!( - config.parse_tag("npm-my-package-v1.2.3-alpha.1"), + config.parse_tag("my-package-v1.2.3-alpha.1"), Some(("my-package".to_string(), "1.2.3-alpha.1".to_string())) ); - // -v in prerelease still parses correctly thanks to semver validation + // -v in prerelease still parses correctly thanks to semver validation. assert_eq!( - config.parse_tag("cargo-my-package-v1.2.3-v1"), + config.parse_tag("my-package-v1.2.3-v1"), Some(("my-package".to_string(), "1.2.3-v1".to_string())) ); - assert_eq!(config.parse_tag("cargo-my-package-vfoo"), None); - assert_eq!(config.parse_tag("cargo-my-package-v1.2"), None); + // Tags without `{package_name}` are rejected by the default template. + assert_eq!(config.parse_tag("v1.2.3"), None); + assert_eq!(config.parse_tag("my-package-vfoo"), None); + assert_eq!(config.parse_tag("my-package-v1.2"), None); } #[test] @@ -988,19 +988,21 @@ mod tests { } #[test] - fn parse_tag_with_legacy_format_via_config() { + fn parse_tag_with_ecosystem_format_via_config() { let temp = tempfile::tempdir().unwrap(); fs::create_dir_all(temp.path().join(".sampo")).unwrap(); fs::write( temp.path().join(".sampo/config.toml"), - "[git]\ntag_format = \"{package_name}-v{version}\"\n", + "[git]\ntag_format = \"{ecosystem}-{package_name}-v{version}\"\n", ) .unwrap(); let config = Config::load(temp.path()).unwrap(); assert_eq!( - config.parse_tag("my-package-v1.2.3"), + config.parse_tag("cargo-my-package-v1.2.3"), Some(("my-package".to_string(), "1.2.3".to_string())) ); + // Legacy-shape tags are rejected once the user opts into the disambiguating template. + assert_eq!(config.parse_tag("my-package-v1.2.3"), None); } } diff --git a/crates/sampo-core/src/publish.rs b/crates/sampo-core/src/publish.rs index 31df474..8273813 100644 --- a/crates/sampo-core/src/publish.rs +++ b/crates/sampo-core/src/publish.rs @@ -1,4 +1,5 @@ use crate::adapters::PackageAdapter; +use crate::tag_template::Placeholder; use crate::types::{PackageInfo, PackageKind, PublishOutput}; use crate::{ Config, current_branch, discover_workspace, @@ -130,7 +131,9 @@ pub fn run_publish( }); } - detect_tag_collisions(&config, &all_non_ignored)?; + for warning in check_tag_conflicts(&config, &all_non_ignored)? { + eprintln!("Warning: {warning}"); + } // Validate internal deps do not include non-publishable packages let mut errors: Vec = Vec::new(); @@ -330,32 +333,101 @@ pub fn run_publish( }) } -/// Reject the publish run when two packages would render to the same git tag -/// (e.g. cross-ecosystem same-name collisions, or a custom template that drops -/// `{package_name}`). Scoped to all non-ignored packages — private ones get -/// tagged too. -fn detect_tag_collisions(config: &Config, packages: &[&PackageInfo]) -> Result<()> { - let mut seen: BTreeMap = BTreeMap::new(); +/// Two-tier tag-conflict diagnostics, scoped to all non-ignored packages +/// (private ones get tagged too): +/// +/// - Errors when two packages currently render to the *same* git tag (a release +/// right now would collide). +/// - Returns warnings when ≥2 packages share a name across ≥2 ecosystems and +/// the active template doesn't include `{ecosystem}` — they happen to render +/// distinct tags only because their versions differ today, and a future bump +/// that aligns the versions will collide silently. +/// +/// Both diagnostics suggest adding `{ecosystem}` to the user's `tag_format`. +/// Callers must surface the returned warnings — losing them silently defeats +/// the point of running this check. +#[must_use = "tag-conflict warnings must be surfaced to the user"] +fn check_tag_conflicts(config: &Config, packages: &[&PackageInfo]) -> Result> { + let mut by_tag: BTreeMap = BTreeMap::new(); for package in packages { let tag = config.build_tag_name(package.kind, &package.name, &package.version); - if let Some(existing) = seen.insert(tag.clone(), package) { - let template_hint = if config.uses_short_tags(&package.name) - || config.uses_short_tags(&existing.name) - { - "Adjust git.tag_format and/or git.short_tags_format to include {ecosystem} and/or {package_name}." - } else { - "Adjust git.tag_format to include {ecosystem} and/or {package_name}." - }; + if let Some(existing) = by_tag.insert(tag.clone(), package) { return Err(SampoError::Publish(format!( - "tag collision: '{}' and '{}' both render to git tag '{}'. {}", + "tag conflict: '{}' and '{}' both render to git tag '{}'. {}", existing.canonical_identifier(), package.canonical_identifier(), tag, - template_hint + tag_conflict_hint(config, &[existing, package]) ))); } } - Ok(()) + + let mut by_name: BTreeMap<&str, Vec<&PackageInfo>> = BTreeMap::new(); + for package in packages { + by_name + .entry(package.name.as_str()) + .or_default() + .push(*package); + } + + let mut warnings = Vec::new(); + for (name, group) in &by_name { + if group.len() < 2 { + continue; + } + let mut kinds: BTreeSet = BTreeSet::new(); + for package in group { + kinds.insert(package.kind); + } + if kinds.len() < 2 { + continue; + } + let any_template_disambiguates = group.iter().any(|package| { + config + .tag_template_for(&package.name) + .contains(Placeholder::Ecosystem) + }); + if any_template_disambiguates { + continue; + } + let mut identifiers: Vec = group + .iter() + .map(|package| format!("'{}'", package.canonical_identifier())) + .collect(); + identifiers.sort(); + warnings.push(format!( + "packages {} share the name '{}' across ecosystems; the current `tag_format` will collide once their versions match. {}", + join_with_and(&identifiers), + name, + tag_conflict_hint(config, group) + )); + } + + Ok(warnings) +} + +fn join_with_and(items: &[String]) -> String { + match items { + [] => String::new(), + [only] => only.clone(), + [first, second] => format!("{first} and {second}"), + [head @ .., last] => format!("{}, and {last}", head.join(", ")), + } +} + +/// Picks the right hint depending on whether any package in `group` is on the +/// short-tag path (so we mention `short_tags_format` too). +fn tag_conflict_hint(config: &Config, group: &[&PackageInfo]) -> String { + let touches_short_tags = group + .iter() + .any(|package| config.uses_short_tags(&package.name)); + if touches_short_tags { + "Add `{ecosystem}` to `git.tag_format` and/or `git.short_tags_format` in `.sampo/config.toml` to disambiguate (e.g. `tag_format = \"{ecosystem}-{package_name}-v{version}\"`)." + .to_string() + } else { + "Add `{ecosystem}` to `git.tag_format` in `.sampo/config.toml` to disambiguate (e.g. `tag_format = \"{ecosystem}-{package_name}-v{version}\"`)." + .to_string() + } } fn package_tag_exists( @@ -533,29 +605,97 @@ mod tests { } #[test] - fn detect_tag_collisions_passes_when_default_format_disambiguates() { + fn check_tag_conflicts_errors_on_same_rendered_tag() { + // Default template drops `{ecosystem}`, so two same-name same-version + // packages from different ecosystems collide right now. let cargo_pkg = make_package(PackageKind::Cargo, "shared", "1.0.0"); let npm_pkg = make_package(PackageKind::Npm, "shared", "1.0.0"); let packages = vec![&cargo_pkg, &npm_pkg]; let config = Config::default(); - detect_tag_collisions(&config, &packages).expect("default format must disambiguate"); + let err = check_tag_conflicts(&config, &packages).unwrap_err(); + let msg = format!("{err}"); + assert!(msg.contains("tag conflict"), "{msg}"); + assert!(msg.contains("cargo/shared"), "{msg}"); + assert!(msg.contains("npm/shared"), "{msg}"); + assert!(msg.contains("shared-v1.0.0"), "{msg}"); + assert!(msg.contains("{ecosystem}"), "{msg}"); + } + + #[test] + fn check_tag_conflicts_warns_when_cross_ecosystem_names_match() { + // Different versions today → no immediate collision, but a future + // alignment would. Default template lacks `{ecosystem}`, so warn. + let cargo_pkg = make_package(PackageKind::Cargo, "shared", "1.0.0"); + let npm_pkg = make_package(PackageKind::Npm, "shared", "2.3.4"); + let packages = vec![&cargo_pkg, &npm_pkg]; + let config = Config::default(); + let warnings = check_tag_conflicts(&config, &packages).expect("no immediate collision"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + let warning = &warnings[0]; + assert!(warning.contains("'cargo/shared'"), "{warning}"); + assert!(warning.contains("'npm/shared'"), "{warning}"); + assert!(warning.contains("share the name 'shared'"), "{warning}"); + assert!(warning.contains("{ecosystem}"), "{warning}"); } #[test] - fn detect_tag_collisions_errors_when_template_drops_ecosystem() { + fn check_tag_conflicts_silent_when_template_includes_ecosystem() { let cargo_pkg = make_package(PackageKind::Cargo, "shared", "1.0.0"); let npm_pkg = make_package(PackageKind::Npm, "shared", "1.0.0"); let packages = vec![&cargo_pkg, &npm_pkg]; let config = Config { - git_tag_format: crate::tag_template::TagTemplate::parse("{package_name}-v{version}") - .unwrap(), + git_tag_format: crate::tag_template::TagTemplate::parse( + "{ecosystem}-{package_name}-v{version}", + ) + .unwrap(), ..Config::default() }; - let err = detect_tag_collisions(&config, &packages).unwrap_err(); + let warnings = check_tag_conflicts(&config, &packages) + .expect("ecosystem in template prevents collisions"); + assert!(warnings.is_empty(), "{warnings:?}"); + } + + #[test] + fn check_tag_conflicts_silent_when_names_disjoint() { + let cargo_pkg = make_package(PackageKind::Cargo, "alpha", "1.0.0"); + let npm_pkg = make_package(PackageKind::Npm, "beta", "1.0.0"); + let packages = vec![&cargo_pkg, &npm_pkg]; + let config = Config::default(); + let warnings = check_tag_conflicts(&config, &packages).expect("no shared names"); + assert!(warnings.is_empty(), "{warnings:?}"); + } + + #[test] + fn check_tag_conflicts_short_tags_hint_mentions_short_format() { + // When at least one conflicting package sits on the short-tag path, + // the hint must mention `short_tags_format` too. + let cargo_pkg = make_package(PackageKind::Cargo, "alpha", "1.0.0"); + let php_pkg = make_package(PackageKind::Packagist, "beta", "1.0.0"); + let packages = vec![&cargo_pkg, &php_pkg]; + let config = Config { + git_tag_format: crate::tag_template::TagTemplate::parse("v{version}").unwrap(), + git_short_tags: Some("alpha".to_string()), + ..Config::default() + }; + let err = check_tag_conflicts(&config, &packages).unwrap_err(); let msg = format!("{err}"); - assert!(msg.contains("tag collision")); - assert!(msg.contains("cargo/shared")); - assert!(msg.contains("npm/shared")); + assert!(msg.contains("short_tags_format"), "{msg}"); + } + + #[test] + fn check_tag_conflicts_warning_uses_oxford_comma_for_three_groups() { + let cargo_pkg = make_package(PackageKind::Cargo, "shared", "1.0.0"); + let npm_pkg = make_package(PackageKind::Npm, "shared", "2.0.0"); + let hex_pkg = make_package(PackageKind::Hex, "shared", "3.0.0"); + let packages = vec![&cargo_pkg, &npm_pkg, &hex_pkg]; + let config = Config::default(); + let warnings = check_tag_conflicts(&config, &packages).expect("no immediate collision"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + let warning = &warnings[0]; + assert!( + warning.contains("'cargo/shared', 'hex/shared', and 'npm/shared'"), + "{warning}" + ); } #[test] @@ -1554,11 +1694,11 @@ edition = "2021" // Verify each tag exists once assert!( - tag_lines.contains(&"cargo-publishable-crate-v1.0.0"), + tag_lines.contains(&"publishable-crate-v1.0.0"), "Missing tag for publishable crate" ); assert!( - tag_lines.contains(&"cargo-private-crate-v1.0.0"), + tag_lines.contains(&"private-crate-v1.0.0"), "Missing tag for private crate" ); @@ -1747,11 +1887,11 @@ edition = "2021" ); assert!( - tag_lines.contains(&"cargo-private-one-v1.0.0"), + tag_lines.contains(&"private-one-v1.0.0"), "Missing tag for private-one" ); assert!( - tag_lines.contains(&"cargo-private-two-v1.0.0"), + tag_lines.contains(&"private-two-v1.0.0"), "Missing tag for private-two" ); } @@ -1834,11 +1974,11 @@ edition = "2021" ); assert!( - tag_lines.contains(&"cargo-publishable-crate-v1.0.0"), + tag_lines.contains(&"publishable-crate-v1.0.0"), "Missing tag for publishable crate" ); assert!( - tag_lines.contains(&"cargo-private-crate-v1.0.0"), + tag_lines.contains(&"private-crate-v1.0.0"), "Private tag should have been created because publishable package was published" ); } @@ -1921,12 +2061,12 @@ edition = "2021" // Both packages should receive tags assert!( - tag_lines.contains(&"cargo-lib-core-v1.0.0"), + tag_lines.contains(&"lib-core-v1.0.0"), "Publishable package should receive a tag. Got tags: {:?}", tag_lines ); assert!( - tag_lines.contains(&"cargo-internal-service-v0.5.0"), + tag_lines.contains(&"internal-service-v0.5.0"), "Private package should receive a tag in mixed workspace. Got tags: {:?}", tag_lines ); @@ -2123,8 +2263,8 @@ edition = "2021" .output() .expect("git tag list should succeed"); let tags = String::from_utf8_lossy(&output.stdout); - assert!(tags.contains("cargo-lib-core-v1.0.0")); - assert!(tags.contains("cargo-internal-service-v0.5.0")); + assert!(tags.contains("lib-core-v1.0.0")); + assert!(tags.contains("internal-service-v0.5.0")); // Now: bump ONLY the private package (simulates a release PR affecting only private crates) let service_manifest = workspace.root.join("crates/internal-service/Cargo.toml"); @@ -2160,7 +2300,7 @@ edition = "2021" let tags = String::from_utf8_lossy(&output.stdout); assert!( - tags.contains("cargo-internal-service-v0.6.0"), + tags.contains("internal-service-v0.6.0"), "Private package should receive a tag even when the publishable package was skipped. \ This is the exact bug from the review. Got tags: {}", tags diff --git a/crates/sampo-github-action/src/main.rs b/crates/sampo-github-action/src/main.rs index 1184df0..2ca1f8a 100644 --- a/crates/sampo-github-action/src/main.rs +++ b/crates/sampo-github-action/src/main.rs @@ -1248,8 +1248,8 @@ mod tests { #[test] fn tag_pre_release_detection() { - assert!(tag_is_prerelease("cargo-sampo-v1.2.0-alpha.1")); - assert!(!tag_is_prerelease("cargo-sampo-v1.2.0")); + assert!(tag_is_prerelease("sampo-v1.2.0-alpha.1")); + assert!(!tag_is_prerelease("sampo-v1.2.0")); assert!(!tag_is_prerelease("invalid")); } @@ -1360,7 +1360,7 @@ mod tests { rename: Some("{{crate}}-{{version}}.tar.gz".to_string()), }]; - let assets = resolve_release_assets(workspace, "cargo-my-crate-v1.0.0", &specs) + let assets = resolve_release_assets(workspace, "my-crate-v1.0.0", &specs) .expect("asset resolution should succeed"); assert_eq!(assets.len(), 1); assert_eq!(assets[0].asset_name, "my-crate-1.0.0.tar.gz"); @@ -1370,19 +1370,20 @@ mod tests { #[test] fn parse_tag_with_default_template() { assert_eq!( - parse_tag_with_config("cargo-my-crate-v1.2.3", None), + parse_tag_with_config("my-crate-v1.2.3", None), Some(("my-crate".to_string(), "1.2.3".to_string())) ); assert_eq!( - parse_tag_with_config("npm-sampo-v0.9.0", None), + parse_tag_with_config("sampo-v0.9.0", None), Some(("sampo".to_string(), "0.9.0".to_string())) ); assert_eq!( - parse_tag_with_config("cargo-sampo-github-action-v0.8.2", None), + parse_tag_with_config("sampo-github-action-v0.8.2", None), Some(("sampo-github-action".to_string(), "0.8.2".to_string())) ); assert_eq!(parse_tag_with_config("nope", None), None); - // Short format needs an explicit ecosystem prefix under the default template. + // Short tags require explicit `git.short_tags` config; under the + // default template a bare `v…` doesn't match the `{package_name}` slot. assert_eq!(parse_tag_with_config("v1.2.3", None), None); } @@ -1410,7 +1411,7 @@ mod tests { ); assert_eq!( - parse_tag_with_config("cargo-other-package-v2.0.0", Some(&config)), + parse_tag_with_config("other-package-v2.0.0", Some(&config)), Some(("other-package".to_string(), "2.0.0".to_string())) ); @@ -1425,15 +1426,21 @@ mod tests { fs::create_dir_all(temp.path().join(".sampo")).unwrap(); fs::write( temp.path().join(".sampo/config.toml"), - "[git]\ntag_format = \"{package_name}-v{version}\"\n", + "[git]\ntag_format = \"{ecosystem}-{package_name}-v{version}\"\n", ) .unwrap(); let config = SampoConfig::load(temp.path()).unwrap(); assert_eq!( - parse_tag_with_config("my-crate-v1.2.3", Some(&config)), + parse_tag_with_config("cargo-my-crate-v1.2.3", Some(&config)), Some(("my-crate".to_string(), "1.2.3".to_string())) ); + // Default-shape tags no longer match once the user opts into the + // disambiguating template. + assert_eq!( + parse_tag_with_config("my-crate-v1.2.3", Some(&config)), + None + ); } #[test] @@ -1460,11 +1467,11 @@ mod tests { assert!(!tag_is_prerelease_with_config("v2.3.4", Some(&config))); assert!(tag_is_prerelease_with_config( - "cargo-other-v1.0.0-alpha.1", + "other-v1.0.0-alpha.1", Some(&config) )); assert!(!tag_is_prerelease_with_config( - "cargo-other-v1.0.0", + "other-v1.0.0", Some(&config) )); } @@ -1578,7 +1585,7 @@ mod tests { ]; // Test with sampo tag - should only match sampo binaries - let sampo_assets = resolve_release_assets(workspace, "cargo-sampo-v0.9.0", &specs) + let sampo_assets = resolve_release_assets(workspace, "sampo-v0.9.0", &specs) .expect("sampo asset resolution should succeed"); assert_eq!( sampo_assets.len(), @@ -1594,9 +1601,8 @@ mod tests { assert!(sampo_assets.iter().any(|a| a.asset_name.contains("darwin"))); // Test with sampo-github-action tag - should only match action binaries - let action_assets = - resolve_release_assets(workspace, "cargo-sampo-github-action-v0.8.2", &specs) - .expect("action asset resolution should succeed"); + let action_assets = resolve_release_assets(workspace, "sampo-github-action-v0.8.2", &specs) + .expect("action asset resolution should succeed"); assert_eq!( action_assets.len(), 2, diff --git a/crates/sampo-github-action/tests/integration.rs b/crates/sampo-github-action/tests/integration.rs index 7c3cf29..3ec53e4 100644 --- a/crates/sampo-github-action/tests/integration.rs +++ b/crates/sampo-github-action/tests/integration.rs @@ -498,14 +498,14 @@ fn test_publish_private_packages_creates_tags_and_reports_published() { // Verify that git tags were actually created for the private package let tag_output = Command::new("git") - .args(["tag", "--list", "cargo-foo-v*"]) + .args(["tag", "--list", "foo-v*"]) .current_dir(ws.path()) .output() .expect("failed to list git tags"); let tags = String::from_utf8_lossy(&tag_output.stdout); assert!( - tags.contains("cargo-foo-v0.1.0"), + tags.contains("foo-v0.1.0"), "Expected git tag for private package foo@0.1.0, got tags: {}", tags ); diff --git a/crates/sampo/README.md b/crates/sampo/README.md index 8883f8d..46f6001 100644 --- a/crates/sampo/README.md +++ b/crates/sampo/README.md @@ -162,12 +162,15 @@ linked = [["cargo/pkg-e", "cargo/pkg-f"], ["cargo/pkg-g", "cargo/pkg-h"]] > [!TIP] > At runtime you can override the detected branch with the `SAMPO_RELEASE_BRANCH` environment variable, which is useful for local testing or custom CI setups. -`tag_format`: Template used for git tags created by `sampo publish` (default: `"{ecosystem}-{package_name}-v{version}"`). Supported placeholders: +`tag_format`: Template used for git tags created by `sampo publish` (default: `"{package_name}-v{version}"`). Supported placeholders: - `{ecosystem}` — `cargo`, `npm`, `hex`, `pypi`, or `packagist`. - `{package_name}` — the package's local name. - `{version}` — the released version (required). +> [!IMPORTANT] +> If your workspace ships packages with the same name across multiple ecosystems (e.g. a Cargo crate and an npm package both called `foo`), `sampo publish` warns you that the default template will eventually collide and suggests `tag_format = "{ecosystem}-{package_name}-v{version}"`. If two packages would already produce the same tag for the release in flight, the publish errors out with the same hint. + `short_tags`: Optional package name that should use the short tag format (see `short_tags_format`) instead of `tag_format`. > [!IMPORTANT]