Test legacy-spk-version-tag behavior without flag enabled

In order to be able to test this behavior without having to run the test
suite twice, once with the feature enabled and once with it disabled,
make the related code available with or without the feature flag, and
make the behavior runtime configurable.

The default behavior is still set via the feature flag, but it can also
be enabled through a new hidden command line flag.

Change the demonstration test to not be conditional on the feature flag
being set.

Signed-off-by: J Robert Ray <jrray@jrray.org>

crates/spk-cli/common/Cargo.toml

@@ -8,10 +8,7 @@ version = { workspace = true }
 workspace = true
 
 [features]
-legacy-spk-version-tags = [
-    "spk-schema/legacy-spk-version-tags",
-    "spk-storage/legacy-spk-version-tags",
-]
+legacy-spk-version-tags = ["spk-storage/legacy-spk-version-tags"]
 migration-to-components = [
     "spk-build/migration-to-components",
     "spk-config/migration-to-components",

crates/spk-cli/common/src/flags.rs

@@ -841,6 +841,16 @@ pub struct Repositories {
     /// that attempts to make changes to a repository, even if the time is in the future.
     #[clap(long)]
     pub when: Option<spfs::tracking::TimeSpec>,
+
+    /// Enable support for legacy spk version tags in the repository.
+    ///
+    /// This causes extra file I/O but is required if the repository contains
+    /// any packages that were published with non-normalized version tags.
+    ///
+    /// This is enabled by default if spk is built with the legacy-spk-version-tags
+    /// feature flag enabled.
+    #[clap(long, hide = true)]
+    pub legacy_spk_version_tags: bool,
 }
 
 impl Repositories {
@@ -872,6 +882,9 @@ impl Repositories {
             if let Some(ts) = self.when.as_ref() {
                 repo.pin_at_time(ts);
             }
+            if self.legacy_spk_version_tags {
+                repo.set_legacy_spk_version_tags(true);
+            }
             repos.push(("local".into(), repo.into()));
         }
         for (name, ts) in enabled.iter() {
@@ -893,6 +906,9 @@ impl Repositories {
             if let Some(ts) = ts.as_ref().or(self.when.as_ref()) {
                 repo.pin_at_time(ts);
             }
+            if self.legacy_spk_version_tags {
+                repo.set_legacy_spk_version_tags(true);
+            }
             repos.push((name.to_string(), repo.into()));
         }
         Ok(repos.into_iter().collect())
@@ -930,6 +946,9 @@ impl Repositories {
             if let Some(ts) = self.when.as_ref() {
                 repo.pin_at_time(ts);
             }
+            if self.legacy_spk_version_tags {
+                repo.set_legacy_spk_version_tags(true);
+            }
             repos.push(("local".into(), repo.into()));
         }
         if self.local_repo_only {
@@ -953,6 +972,9 @@ impl Repositories {
             if let Some(ts) = ts.as_ref().or(self.when.as_ref()) {
                 repo.pin_at_time(ts);
             }
+            if self.legacy_spk_version_tags {
+                repo.set_legacy_spk_version_tags(true);
+            }
             repos.push((name.into(), repo.into()));
         }
         Ok(repos)

crates/spk-cli/group2/src/cmd_ls_test.rs

@@ -3,12 +3,17 @@
 // https://github.com/imageworks/spk
 
 use clap::Parser;
+use futures::prelude::*;
+use relative_path::RelativePathBuf;
 use spfs::config::Remote;
+use spfs::storage::EntryType;
 use spfs::RemoteAddress;
 use spk_schema::foundation::ident_component::Component;
+use spk_schema::ident_ops::VerbatimTagStrategy;
 use spk_schema::recipe;
 use spk_solve::spec;
 use spk_storage::fixtures::*;
+use spk_storage::RepositoryHandle;
 
 use super::{Ls, Output, Run};
 
@@ -501,15 +506,8 @@ async fn test_ls_shows_partially_deprecated_version() {
 /// When the legacy-spk-version-tags feature is enabled, and when a package
 /// is published with a non-normalized version tag, `spk ls` is expected to
 /// list the package.
-#[cfg(feature = "legacy-spk-version-tags")]
 #[tokio::test]
 async fn test_ls_succeeds_for_package_saved_with_legacy_version_tag() {
-    use futures::prelude::*;
-    use relative_path::RelativePathBuf;
-    use spfs::storage::EntryType;
-    use spk_schema::ident_ops::VerbatimTagStrategy;
-    use spk_storage::RepositoryHandle;
-
     let mut rt = spfs_runtime_with_tag_strategy::<VerbatimTagStrategy>().await;
     let remote_repo = spfsrepo().await;
 
@@ -551,7 +549,7 @@ async fn test_ls_succeeds_for_package_saved_with_legacy_version_tag() {
         _ => panic!("expected SPFSWithVerbatimTags"),
     }
 
-    let mut opt = Opt::try_parse_from(["ls", "my-local-pkg"]).unwrap();
+    let mut opt = Opt::try_parse_from(["ls", "--legacy-spk-version-tags", "my-local-pkg"]).unwrap();
     opt.ls.run().await.unwrap();
     assert_eq!(opt.ls.output.vec.len(), 1);
 }

crates/spk-schema/Cargo.toml

@@ -8,7 +8,6 @@ version = { workspace = true }
 workspace = true
 
 [features]
-legacy-spk-version-tags = ["spk-schema-foundation/legacy-spk-version-tags"]
 migration-to-components = ["spk-schema-foundation/migration-to-components"]
 
 [dependencies]

crates/spk-schema/crates/foundation/Cargo.toml

@@ -12,7 +12,6 @@ default = ["parsedbuf-serde"]
 # activates serde within the generated code from parsedbuf macros
 parsedbuf-serde = []
 migration-to-components = []
-legacy-spk-version-tags = []
 
 [dependencies]
 arc-swap = { workspace = true }

crates/spk-schema/crates/foundation/src/ident_ops/mod.rs

@@ -7,6 +7,10 @@ pub mod parsing;
 mod tag_path;
 
 pub use metadata_path::MetadataPath;
-#[cfg(feature = "legacy-spk-version-tags")]
-pub use tag_path::VerbatimTagStrategy;
-pub use tag_path::{NormalizedTagStrategy, TagPath, TagPathStrategy, TagPathStrategyType};
+pub use tag_path::{
+    NormalizedTagStrategy,
+    TagPath,
+    TagPathStrategy,
+    TagPathStrategyType,
+    VerbatimTagStrategy,
+};

crates/spk-schema/crates/foundation/src/ident_ops/tag_path.rs

@@ -10,7 +10,6 @@ pub enum TagPathStrategyType {
     /// Normalize the version in tag path.
     Normalized,
     /// Use the version as specified in the tag path.
-    #[cfg(feature = "legacy-spk-version-tags")]
     Verbatim,
 }
 
@@ -34,11 +33,9 @@ impl TagPathStrategy for NormalizedTagStrategy {
 /// When creating a tag path that contains a version, this strategy will
 /// render the version as specified in the version object, without any
 /// normalization.
-#[cfg(feature = "legacy-spk-version-tags")]
 #[derive(Debug)]
 pub struct VerbatimTagStrategy {}
 
-#[cfg(feature = "legacy-spk-version-tags")]
 impl TagPathStrategy for VerbatimTagStrategy {
     #[inline]
     fn strategy_type() -> TagPathStrategyType {

crates/spk-storage/src/fixtures.rs

@@ -11,9 +11,12 @@ use spfs::config::Remote;
 use spfs::prelude::*;
 use spfs::Result;
 use spk_schema::foundation::fixtures::*;
-#[cfg(feature = "legacy-spk-version-tags")]
-use spk_schema::ident_ops::VerbatimTagStrategy;
-use spk_schema::ident_ops::{NormalizedTagStrategy, TagPathStrategy, TagPathStrategyType};
+use spk_schema::ident_ops::{
+    NormalizedTagStrategy,
+    TagPathStrategy,
+    TagPathStrategyType,
+    VerbatimTagStrategy,
+};
 use tokio::sync::{Mutex, MutexGuard};
 
 use crate as storage;
@@ -147,7 +150,6 @@ where
                     )
                     .unwrap(),
                 ),
-                #[cfg(feature = "legacy-spk-version-tags")]
                 TagPathStrategyType::Verbatim => storage::RepositoryHandle::SPFSWithVerbatimTags(
                     storage::SpfsRepository::<VerbatimTagStrategy>::try_from(
                         NameAndRepositoryWithTagStrategy::<_, _, VerbatimTagStrategy>::new(

crates/spk-storage/src/storage/handle.rs

@@ -2,7 +2,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
 
-#[cfg(feature = "legacy-spk-version-tags")]
 use spk_schema::ident_ops::VerbatimTagStrategy;
 use spk_schema::{Spec, SpecRecipe};
 
@@ -14,7 +13,6 @@ type Handle = dyn Repository<Recipe = SpecRecipe, Package = Spec>;
 #[allow(clippy::large_enum_variant)]
 pub enum RepositoryHandle {
     SPFS(super::SpfsRepository),
-    #[cfg(feature = "legacy-spk-version-tags")]
     SPFSWithVerbatimTags(super::SpfsRepository<VerbatimTagStrategy>),
     Mem(super::MemRepository<SpecRecipe>),
     Runtime(super::RuntimeRepository),
@@ -34,16 +32,10 @@ impl RepositoryHandle {
         Self::Runtime(Default::default())
     }
 
-    #[cfg(feature = "legacy-spk-version-tags")]
     pub fn is_spfs(&self) -> bool {
         matches!(self, Self::SPFS(_) | Self::SPFSWithVerbatimTags(_))
     }
 
-    #[cfg(not(feature = "legacy-spk-version-tags"))]
-    pub fn is_spfs(&self) -> bool {
-        matches!(self, Self::SPFS(_))
-    }
-
     pub fn is_mem(&self) -> bool {
         matches!(self, Self::Mem(_))
     }
@@ -55,7 +47,6 @@ impl RepositoryHandle {
     pub fn to_repo(self) -> Box<Handle> {
         match self {
             Self::SPFS(repo) => Box::new(repo),
-            #[cfg(feature = "legacy-spk-version-tags")]
             Self::SPFSWithVerbatimTags(repo) => Box::new(repo),
             Self::Mem(repo) => Box::new(repo),
             Self::Runtime(repo) => Box::new(repo),
@@ -69,7 +60,6 @@ impl std::ops::Deref for RepositoryHandle {
     fn deref(&self) -> &Self::Target {
         match self {
             RepositoryHandle::SPFS(repo) => repo,
-            #[cfg(feature = "legacy-spk-version-tags")]
             RepositoryHandle::SPFSWithVerbatimTags(repo) => repo,
             RepositoryHandle::Mem(repo) => repo,
             RepositoryHandle::Runtime(repo) => repo,
@@ -81,7 +71,6 @@ impl std::ops::DerefMut for RepositoryHandle {
     fn deref_mut(&mut self) -> &mut Self::Target {
         match self {
             RepositoryHandle::SPFS(repo) => repo,
-            #[cfg(feature = "legacy-spk-version-tags")]
             RepositoryHandle::SPFSWithVerbatimTags(repo) => repo,
             RepositoryHandle::Mem(repo) => repo,
             RepositoryHandle::Runtime(repo) => repo,

crates/spk-storage/src/storage/spfs.rs

@@ -24,9 +24,7 @@ use spk_schema::foundation::version::{parse_version, Version};
 use spk_schema::ident::{ToAnyWithoutBuild, VersionIdent};
 use spk_schema::ident_build::parsing::embedded_source_package;
 use spk_schema::ident_build::{EmbeddedSource, EmbeddedSourcePackage};
-#[cfg(feature = "legacy-spk-version-tags")]
-use spk_schema::ident_ops::VerbatimTagStrategy;
-use spk_schema::ident_ops::{NormalizedTagStrategy, TagPath, TagPathStrategy};
+use spk_schema::ident_ops::{NormalizedTagStrategy, TagPath, TagPathStrategy, VerbatimTagStrategy};
 use spk_schema::spec_ops::{HasVersion, WithVersion};
 use spk_schema::version::VersionParts;
 use spk_schema::{AnyIdent, BuildIdent, FromYaml, Package, Recipe, Spec, SpecRecipe};
@@ -64,12 +62,9 @@ macro_rules! verbatim_build_package_tag_if_enabled {
 
 macro_rules! verbatim_tag_if_enabled {
     ($self:expr, $tag:tt, $output:ty, $ident:expr) => {{
-        #[cfg(feature = "legacy-spk-version-tags")]
-        {
+        if $self.legacy_spk_version_tags {
             $self.$tag::<VerbatimTagStrategy, $output>($ident)
-        }
-        #[cfg(not(feature = "legacy-spk-version-tags"))]
-        {
+        } else {
             $self.$tag::<NormalizedTagStrategy, $output>($ident)
         }
     }};
@@ -83,6 +78,7 @@ pub struct SpfsRepository<S = NormalizedTagStrategy> {
     cache_policy: AtomicPtr<CachePolicy>,
     caches: CachesForAddress,
     tag_strategy: PhantomData<S>,
+    legacy_spk_version_tags: bool,
 }
 
 impl<S> std::hash::Hash for SpfsRepository<S> {
@@ -144,6 +140,7 @@ impl<S: AsRef<str>, T: Into<spfs::storage::RepositoryHandle>> TryFrom<(S, T)> fo
             inner,
             cache_policy: AtomicPtr::new(Box::leak(Box::new(CachePolicy::CacheOk))),
             tag_strategy: PhantomData,
+            legacy_spk_version_tags: cfg!(feature = "legacy-spk-version-tags"),
         })
     }
 }
@@ -195,6 +192,7 @@ where
             inner,
             cache_policy: AtomicPtr::new(Box::leak(Box::new(CachePolicy::CacheOk))),
             tag_strategy: PhantomData,
+            legacy_spk_version_tags: cfg!(feature = "legacy-spk-version-tags"),
         })
     }
 }
@@ -210,6 +208,7 @@ impl SpfsRepository {
             inner,
             cache_policy: AtomicPtr::new(Box::leak(Box::new(CachePolicy::CacheOk))),
             tag_strategy: PhantomData,
+            legacy_spk_version_tags: cfg!(feature = "legacy-spk-version-tags"),
         })
     }
 
@@ -229,6 +228,11 @@ impl SpfsRepository {
             .query_pairs_mut()
             .append_pair("when", &ts.to_string());
     }
+
+    /// Enable or disable the use of legacy spk version tags
+    pub fn set_legacy_spk_version_tags(&mut self, enabled: bool) {
+        self.legacy_spk_version_tags = enabled;
+    }
 }
 
 impl<S> std::ops::Drop for SpfsRepository<S> {
@@ -345,7 +349,7 @@ where
 
         let mut builds = HashSet::new();
 
-        for pkg in Self::iter_possible_parts(pkg) {
+        for pkg in Self::iter_possible_parts(pkg, self.legacy_spk_version_tags) {
             let spec_base = verbatim_build_spec_tag_if_enabled!(self, &pkg);
             let package_base = verbatim_build_package_tag_if_enabled!(self, &pkg);
 
@@ -386,7 +390,7 @@ where
         let mut builds = HashSet::new();
 
         let pkg = pkg.to_any(Some(Build::Source));
-        for pkg in Self::iter_possible_parts(&pkg) {
+        for pkg in Self::iter_possible_parts(&pkg, self.legacy_spk_version_tags) {
             let mut base = verbatim_build_spec_tag_if_enabled!(self, &pkg);
             // the package tag contains the name and build, but we need to
             // remove the trailing build in order to list the containing 'folder'
@@ -1001,7 +1005,10 @@ where
     ///
     /// If spk is built without the `legacy-spk-version-tags` feature enabled,
     /// then only the one canonical normalized part will be returned.
-    fn iter_possible_parts<I>(pkg: &I) -> impl Iterator<Item = I::Output> + '_
+    fn iter_possible_parts<I>(
+        pkg: &I,
+        legacy_spk_version_tags: bool,
+    ) -> impl Iterator<Item = I::Output> + '_
     where
         I: HasVersion + WithVersion,
     {
@@ -1011,9 +1018,7 @@ where
             // Handle all the part lengths that are bigger than the normalized
             // parts, except for the normalized parts length itself, which may
             // be larger than 5 and not hit by this range.
-            .filter(move |num_parts| {
-                cfg!(feature = "legacy-spk-version-tags") && *num_parts > normalized_parts_len
-            })
+            .filter(move |num_parts| legacy_spk_version_tags && *num_parts > normalized_parts_len)
             // Then, handle the normalized parts length itself, which is
             // skipped by the filter above so it isn't processed twice,
             // and is handled even if the length is outside the above range.
@@ -1077,7 +1082,7 @@ where
         Fut: Future<Output = Result<R>>,
     {
         let mut first_resolve_err = None;
-        for pkg in Self::iter_possible_parts(pkg) {
+        for pkg in Self::iter_possible_parts(pkg, self.legacy_spk_version_tags) {
             let tag_path = tag_path(&pkg);
             let tag_spec = spfs::tracking::TagSpec::parse(tag_path.as_str())?;
             let tag = match self
@@ -1192,7 +1197,7 @@ where
     /// (with or without package components)
     async fn lookup_package(&self, pkg: &BuildIdent) -> Result<StoredPackage> {
         let mut first_resolve_err = None;
-        for pkg in Self::iter_possible_parts(pkg) {
+        for pkg in Self::iter_possible_parts(pkg, self.legacy_spk_version_tags) {
             let tag_path = verbatim_build_package_tag_if_enabled!(self, &pkg);
             let tag_specs: HashMap<Component, TagSpec> = self
                 .ls_tags(&tag_path)
@@ -1311,6 +1316,7 @@ pub async fn local_repository() -> Result<SpfsRepository> {
         inner,
         cache_policy: AtomicPtr::new(Box::leak(Box::new(CachePolicy::CacheOk))),
         tag_strategy: PhantomData,
+        legacy_spk_version_tags: cfg!(feature = "legacy-spk-version-tags"),
     })
 }
 
@@ -1328,5 +1334,6 @@ pub async fn remote_repository<S: AsRef<str>>(name: S) -> Result<SpfsRepository>
         inner,
         cache_policy: AtomicPtr::new(Box::leak(Box::new(CachePolicy::CacheOk))),
         tag_strategy: PhantomData,
+        legacy_spk_version_tags: cfg!(feature = "legacy-spk-version-tags"),
     })
 }

crates/spk/Cargo.toml

@@ -15,10 +15,7 @@ required-features = ["cli"]
 
 [features]
 default = ["cli"]
-legacy-spk-version-tags = [
-    "spk-schema/legacy-spk-version-tags",
-    "spk-storage/legacy-spk-version-tags",
-]
+legacy-spk-version-tags = ["spk-storage/legacy-spk-version-tags"]
 migration-to-components = [
     "spk-schema/migration-to-components",
     "spk-solve/migration-to-components",