feat: Static release bumped during dist-git generation#54
feat: Static release bumped during dist-git generation#54reubeno merged 5 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Enables automatic bumping of static RPM Release: values during synthetic dist-git generation (--with-git), so releases remain unique per synthetic commit when %autorelease is not used.
Changes:
- Add logic to detect
%autoreleasevs staticRelease:values and bump static releases by the number of synthetic commits. - Invoke the static release bump as part of synthetic history preparation before staging changes.
- Add unit tests for release detection, bumping, and Release tag extraction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/app/azldev/core/sources/sourceprep.go | Calls the new static release bump step during synthetic history creation. |
| internal/app/azldev/core/sources/releaseoverlay.go | Introduces helpers to read Release: from spec, detect %autorelease, bump static releases, and apply an overlay. |
| internal/app/azldev/core/sources/releaseoverlay_test.go | Adds tests covering %autorelease detection, static bumping, and Release tag parsing behavior. |
| func BumpStaticRelease(releaseValue string, commitCount int) (string, error) { | ||
| matches := staticReleasePattern.FindStringSubmatch(releaseValue) | ||
| if matches == nil { | ||
| return "", fmt.Errorf("release value %#q does not start with an integer", releaseValue) |
There was a problem hiding this comment.
We know we'll find specs that don't use autorelease and don't have a standard integer followed by %{?dist}.
Example: https://src.fedoraproject.org/rpms/kernel/blob/rawhide/f/kernel.spec
For these, we'll want some way to avoid this being a fatal error. From Dan's "3 categories", this would be that 3rd case.
For those it would be okay for us to manually manage their Release values, but we'd need a way to do so. We can use an overlay to set the Release tag's value, but would then also need a way to disable this logic from running (and failing).
Tonisal-byte
force-pushed
the
asalinas/bumpStaticRelease
branch
from
e76c666 to
4cb971d
Compare
Tonisal-byte
force-pushed
the
asalinas/bumpStaticRelease
branch
from
a0dd17f to
c67655a
Compare
| specPath string, | ||
| commits []CommitMetadata, | ||
| ) (err error) { | ||
| versionValue, err := GetSpecTagValue(p.fs, specPath, "Version") |
There was a problem hiding this comment.
I can see us running into issues due to macros. I know there are definitely specs that define Version in terms of macros.
I'm somewhat inclined to say we should hold off on the auto-changelog-updating for now? Did you mention you were considering separating it anyway?
There was a problem hiding this comment.
Yeah, I went ahead and split them up in the latest iteration, should look a lot cleaner now :)
Tonisal-byte
force-pushed
the
asalinas/bumpStaticRelease
branch
from
b77ea3b to
61bfa50
Compare
Tonisal-byte
force-pushed
the
asalinas/bumpStaticRelease
branch
from
61bfa50 to
e1eeb8a
Compare
| // This covers both the bare form (%autorelease) and the braced form (%{autorelease}). | ||
| var autoreleasePattern = regexp.MustCompile(`%(\{autorelease\}|autorelease($|\s))`) |
There was a problem hiding this comment.
The autoreleasePattern will not match cases where %autorelease is immediately followed by another macro/text (e.g. Release: %autorelease%{?dist}), which would cause tryBumpStaticRelease to treat it as “not autorelease” and attempt a static bump. If such spec forms are supported in your RPM ecosystem, adjust the pattern to treat %autorelease as a token (e.g., using a word boundary / broader terminator than ($|\\s)).
| // This covers both the bare form (%autorelease) and the braced form (%{autorelease}). | |
| var autoreleasePattern = regexp.MustCompile(`%(\{autorelease\}|autorelease($|\s))`) | |
| // This covers both the bare form (%autorelease) and the braced form (%{autorelease}), | |
| // including cases where the bare form is immediately followed by another macro/text | |
| // token boundary (for example, "%autorelease%{?dist}"). | |
| var autoreleasePattern = regexp.MustCompile(`%(\{autorelease\}|autorelease\b)`) |
| func TestReleaseUsesAutorelease(t *testing.T) { | ||
| for _, testCase := range []struct { | ||
| value string | ||
| expected bool | ||
| }{ | ||
| {"%autorelease", true}, | ||
| {"%{autorelease}", true}, | ||
| {"1", false}, | ||
| {"1%{?dist}", false}, | ||
| {"3%{?dist}.1", false}, | ||
| {"", false}, | ||
| } { | ||
| t.Run(testCase.value, func(t *testing.T) { | ||
| assert.Equal(t, testCase.expected, sources.ReleaseUsesAutorelease(testCase.value)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Two test issues here: (1) using testCase.value as the subtest name yields an empty subtest name for the \"\" case; prefer an explicit name field (like the other tests). (2) if this repo is built with Go versions prior to 1.22, the range variable capture pattern can lead to flaky tests; defensively rebind testCase := testCase inside the loop before t.Run(...).
This PR allows azldev to automatically update the static release number for spec which do not use
%autorelease. The new release number is calculated by taking the original release number and adding the number of synthetic commits (from the project repo). This behavior can only be observed when creating synthetic dist-git using the--with-gitflag on eitherprepare-sourcesorbuildcommands.