refactor: Add `BuildString` to rattler_conda_types by hunger · Pull Request #2430 · conda/rattler

hunger · 2026-05-13T09:02:15Z

Description

... and use those in a second step.

How Has This Been Tested?

Unit tests: All existing tests pass unchanged.

AI Disclosure

This PR contains AI-generated content.
- I have tested any AI-generated content in my PR.
- I take responsibility for any AI-generated content in my PR.

Tools: Claude

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added sufficient tests to cover my changes.

wolfv

Looks good to me!

baszalmstra · 2026-05-13T11:58:40Z

    #[setter]
    pub fn set_build(&mut self, build: String) {
-        self.as_package_record_mut().build = build;
+        self.as_package_record_mut().build = BuildString::new_unchecked(build);


This should error on an invalid build string I think.

Looks like I accidentally un-fixed this.

wolfv · 2026-05-21T15:50:49Z

Sadly still some merge conflicts

wolfv · 2026-05-21T15:56:09Z


-    fn build(&self) -> &str {
-        &self.repodata_record.package_record.build
+    fn build(&self) -> Option<&BuildString> {


I think I dislike Option here - it would be better if BuildString had a special empty state internally, no? Then the hash implementation can also easily be part of the type etc?

baszalmstra · 2026-06-09T07:01:51Z

+        Ok(())
+    }
+
+    fn validate(value: &str) -> Result<(), BuildStringError> {


According to the CEP BuildStrings cannot be empty. That means we cant have a default implementation for build string.

I think we should have something like "unset" or "none" as the default value.

wolfv · 2026-05-21T15:57:19Z

+                record.name.as_normalized(),
+                record.version,
+            ),
+            None => format!("{}-{}.json", record.name.as_normalized(), record.version),


This is definitely invalid as it breaks assumptions with two - in these strings. We should better define some kind of placeholder empty state I think, like "empty".

wolfv · 2026-06-09T07:03:31Z

+        Ok(())
+    }
+
+    fn validate(value: &str) -> Result<(), BuildStringError> {


I think we should have something like "unset" or "none" as the default value.

Adds a newtype in rattler_conda_types::package wrapping a String. The validating constructor BuildString::new enforces the CEP26 character set (ASCII alphanumeric plus '_', '.', '+') and a 64-byte maximum length; BuildString::new_unchecked accepts any value as-is. The internal structure of the build string (prefix, hash, build number) is intentionally not exposed: callers treat the value as a single opaque token. Use BuildString::append / ::prepend (validating) or their _unchecked siblings to build composite values. Construction: - BuildString::new(value) -> Result (CEP26 validation) - BuildString::new_unchecked(value) -> Self (infallible) Mutation: - append / prepend(other) -> Result (validating) - append_unchecked / prepend_unchecked(other) Hash derives from the inner String, so source-identifier hashes in lockfiles keyed by build strings stay stable when an existing String field is migrated to BuildString. PartialEq against str / &str / String is provided so comparison call sites work without conversion.

Replaces String-typed build strings on the core records with the dedicated newtype: - PackageRecord::build, IndexJson::build, MinimalPrefixRecord::build, GenericVirtualPackage::build_string: String -> BuildString. HasArtifactIdentificationRefs::build now returns &BuildString; lockfile v5/v6/v7 conda models and v7 source models store Cow<'a, BuildString> instead of Cow<'a, str>. PackageRecord::new takes a BuildString directly so callers pick the strictness explicitly via BuildString::strict / ::unchecked. All call sites across rattler, rattler-bin, rattler_cache, rattler_index, rattler_lock, rattler_repodata_gateway, rattler_solve, rattler_virtual_packages, py-rattler and js-rattler are updated to match.

…entry points Address the second round of review feedback that arrived after the first refactor commit was pushed. - BuildString::append/prepend now also reject empty results, restoring the [1, 64]-byte invariant for combined values. Adds BuildStringError::Empty. - Drop the unused append_unchecked/prepend_unchecked variants; production code only ever needs the validating versions. - py-rattler PackageRecord(...) constructor (PyRecord::create) now returns PyResult and raises ValueError on invalid build strings, matching the set_build setter. - js-rattler PackageRecord JSON constructor and the simple_solve locked- packages path validate build strings via BuildString::new instead of going through the permissive serde / new_unchecked entry points. - Test fixtures that depended on the permissive behaviour migrated from py3-none-any / cp39-cp39-linux_x86_64 to underscore-separated build strings.

Makes "BuildString may be empty" expressible, but then it was via the Option before.

…meta file name round-trip Address review feedback on the BuildString design: - `BuildString::new` now rejects the empty string, matching CEP26 exactly (the regex requires at least one character). The empty value remains the explicit sentinel for "no build string" and is constructed via the new `BuildString::empty()` (or `Default`). Since the empty string is not a valid CEP26 build string it can never collide with a real build string, unlike placeholder values such as "unset" or "none". - Restore the canonical `<name>-<version>-<build>.json` conda-meta file name for records with an empty build string (trailing `-`). Dropping the build segment produced file names that `MinimalPrefixRecord::from_path` either rejects (single-word names) or silently mis-parses (hyphenated names), since it splits the file name on the last two `-` characters. Add a regression test for the round-trip. - Fix `InstallationResultRecord::into_prefix_record` to use the canonical segment order (it formatted `<build>-<version>-<name>.json`) and to look in the `conda-meta` directory (latent bug; the Min branch is currently unreachable). - Drop `#[serde(default, skip_serializing_if)]` from `PackageRecord.build` again: the `build` key is always serialized (as `""` when empty) and required when deserializing, matching conda and the previous behavior. - Make the Python and JavaScript `PackageRecord` constructors and `build` setters unchecked again: `PackageRecord.build` is not always a CEP26 conda build string -- wheel records carry the wheel tag verbatim (e.g. `py3-none-any`) and source packages carry an empty build. Restore the hyphenated wheel-tag test data accordingly. Strict validation remains on the `IndexJson` setter, which only ever holds conda build strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…der build string Removes the empty() constructor and Default impl from BuildString so an empty value can no longer be constructed through the API. Places that previously used the empty sentinel (virtual packages, lock-file parsing, tests) now use "0" instead, matching the conda convention for packages without a build identifier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

# Conflicts: # crates/rattler_cache/src/package_cache/mod.rs

hunger requested a review from wolfv May 13, 2026 09:02

github-actions Bot added the vouched label May 13, 2026

wolfv reviewed May 13, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/package/mod.rs Outdated

wolfv requested changes May 13, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/package/mod.rs Outdated

baszalmstra reviewed May 13, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/match_spec/mod.rs Outdated

baszalmstra reviewed May 13, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/package/mod.rs Outdated

baszalmstra reviewed May 13, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/package/mod.rs Outdated

baszalmstra requested changes May 13, 2026

View reviewed changes

hunger force-pushed the push-wyvzyoxwrmxm branch 2 times, most recently from f169025 to 9c76043 Compare May 13, 2026 11:03

wolfv reviewed May 13, 2026

View reviewed changes

wolfv changed the title ~~refactor: Add BuildString and BuildStringPrefix to rattler_conda_types~~ refactor: Add BuildString to rattler_conda_types May 13, 2026

baszalmstra requested changes May 13, 2026

View reviewed changes

hunger force-pushed the push-wyvzyoxwrmxm branch 2 times, most recently from df53350 to 1cd9c5e Compare May 18, 2026 12:36

wolfv reviewed May 18, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/package/build_string.rs Outdated

hunger force-pushed the push-wyvzyoxwrmxm branch 3 times, most recently from 67236c8 to e6ee71a Compare May 20, 2026 10:13

wolfv reviewed May 20, 2026

View reviewed changes

Comment thread crates/rattler_conda_types/src/repo_data/mod.rs Outdated

wolfv reviewed May 20, 2026

View reviewed changes

Comment thread ...ler_lock/src/snapshots/rattler_lock__source_identifier__tests__compute_test_data_hashes.snap Outdated

hunger force-pushed the push-wyvzyoxwrmxm branch from e6ee71a to 9fdf03d Compare May 21, 2026 14:39

wolfv reviewed May 21, 2026

View reviewed changes

hunger force-pushed the push-wyvzyoxwrmxm branch from 9fdf03d to aeaa3f5 Compare May 26, 2026 13:28

baszalmstra reviewed Jun 9, 2026

View reviewed changes

wolfv reviewed Jun 9, 2026

View reviewed changes

hunger added 3 commits August 6, 2026 14:32

hunger and others added 3 commits August 6, 2026 14:34

refactor(conda-types): Move from Option<BuildString> to BuildString

8632616

Makes "BuildString may be empty" expressible, but then it was via the Option before.

wolfv force-pushed the push-wyvzyoxwrmxm branch from 0161daa to b9c32c4 Compare August 6, 2026 18:38

Merge remote-tracking branch 'origin/main' into pr-2430

9f51dec

# Conflicts: # crates/rattler_cache/src/package_cache/mod.rs

wolfv closed this Aug 7, 2026

wolfv reopened this Aug 7, 2026

Uh oh!

Conversation

hunger commented May 13, 2026

Description

How Has This Been Tested?

AI Disclosure

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfv commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants