feat(sources): add opt-in %fedora_upstream_* provenance macros (replaces #285) - #290
feat(sources): add opt-in %fedora_upstream_* provenance macros (replaces #285)#290liunan-ms wants to merge 1 commit into
Conversation
032907a to
881674c
Compare
881674c to
3282564
Compare
3282564 to
1749e01
Compare
1749e01 to
527ccb0
Compare
527ccb0 to
725c020
Compare
| } | ||
|
|
||
| if version != "" { | ||
| setMacroIfAbsent(macros, fedoraUpstreamVersionMacro, version) |
There was a problem hiding this comment.
This forwards the raw [Release] tag rather than the resolved Fedora release. For an upstream spec using [Release: %autorelease], this writes %fedora_upstream_release %autorelease; later expansion occurs against azldev’s overlay/synthetic Git history, not the pristine Fedora dist-git history. The same issue applies to macro-defined [Version]/[Release] values
There was a problem hiding this comment.
Thanks @Tonisal-byte ! %autorelease is now resolved eagerly at macros-file-write time, which happens when the upstream .git is still pristine (before overlays and synthetic commits are layered on). azldev shells out to rpmautospec calculate-release --complete-release <spec> in that checkout, so the count comes from Fedora's history.
I validated this end-to-end on fwupd-efi (Release: %autorelease): the built binary's .sbat section now shows:
fwupd-efi.fedora,1,The Fedora Project,fwupd-efi,1.8-1.fc43,https://src.fedoraproject.org/rpms/fwupd-efi
Macro-defined Version/Release (e.g. Version: %{majorver}.%{minorver}) are still emitted unexpanded, but because the macros file is loaded back into the same spec via %{load:...}, RPM expands them lazily at build time against the spec's own definitions, so the consuming spec sees the correct value. The one residual caveat is that lazy expansion resolves against the overlaid spec, but it only happens when an overlay redefined the version macros. I opted not to fully expand these eagerly (e.g. via rpmspec -q) because that'd be a much larger change than the bounded rpmautospec dependency and it's very rare in practice. It's documented as a known limitation.
725c020 to
e57b66f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/app/azldev/core/sources/upstream_provenance.go:211
rpmautospec calculate-releasefailures are currently logged without stderr, which makes diagnosing why%fedora_upstream_releasewas skipped much harder (and diverges from the established pattern elsewhere, e.g.internal/utils/git/git.go:244-256). Capture stderr on the exec.Cmd and include it in the warning when the command fails.
execCmd := exec.CommandContext(ctx, rpmautospecBinary, "calculate-release", "--complete-release", specPath)
execCmd.Dir = sourcesDir
cmd, err := p.cmdFactory.Command(execCmd)
if err != nil {
e57b66f to
f241454
Compare
f241454 to
bf25a3e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/app/azldev/core/sources/upstream_provenance.go:166
- Autorelease and dist-macro detection here is too narrow: it only recognizes the bare "%autorelease" substring and only expands "%{?dist}". Fedora specs can also use braced/conditional forms like "%{autorelease}" / "%{?autorelease}" (see internal/app/azldev/core/sources/release.go:25), and some specs use "%{dist}". In those cases this code will skip rpmautospec resolution and/or leave the dist macro to expand in the Azure Linux buildroot, producing an incorrect
%fedora_upstream_release.
if !strings.Contains(release, autoreleaseMacro) {
return strings.ReplaceAll(release, distPlaceholder, p.upstreamDistTag)
}
number := p.calculateAutorelease(ctx, componentName, sourcesDir, specPath)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/app/azldev/core/sources/upstream_provenance.go:165
- When a pristine Release tag contains
%autoreleasewith additional suffix/prefix text (e.g.%autorelease.1%{?dist}), the current implementation returnsnumber + p.upstreamDistTag, which discards the rest of the Release expression and can emit an incorrect%fedora_upstream_releasevalue. Preserve the full Release string by substituting%autoreleasewith the computed number (and still expanding%{?dist}).
return number + p.upstreamDistTag
internal/app/azldev/core/sources/upstream_provenance.go:189
- In dry-run mode,
cmd.RunAndGetOutputintentionally returns an empty string without executingrpmautospec, but the current parsing treats empty output as an invalid release number and logs a WARN. This produces noisy/incorrect warnings during dry runs. Skip%autoreleaseresolution entirely whenp.dryRunnable.DryRun()is true (debug-level is fine).
if p.cmdFactory == nil || !p.cmdFactory.CommandInSearchPath(rpmautospecBinary) {
slog.Warn("Skipping %fedora_upstream_release; rpmautospec is not available to resolve %autorelease",
"component", componentName)
return ""
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/app/azldev/core/sources/upstream_provenance.go:158
resolveUpstreamReleaseonly detects the bare%autoreleasetoken viastrings.Contains(release, "%autorelease"). Elsewhere in this package (release.go)%autoreleasedetection supports braced/conditional forms like%{autorelease}and%{?autorelease}. With the current logic, those forms will be treated as non-autorelease and could be emitted verbatim into%fedora_upstream_release, causing build-time expansion against the Azure Linux history/macros (the case this feature is trying to avoid). Also,%{dist}(non-conditional) won’t be substituted, so the macro can still pick up the buildroot’s dist tag instead of Fedora’s.
if !strings.Contains(release, autoreleaseMacro) {
return strings.ReplaceAll(release, distPlaceholder, p.upstreamDistTag)
}
docs/user/reference/config/components.md:222
- The example uses
%{fedora_upstream_version}/%{fedora_upstream_release}unconditionally, but this feature is best-effort (macros can be skipped if the spec can’t be parsed orrpmautospecis unavailable for%autorelease). Using the conditional form (%{?name}) avoids hard failures when the macros are absent.
%sbat_generate_metadata ... derived from grub2 %{fedora_upstream_version}-%{fedora_upstream_release}
ec9f204 to
51adab5
Compare
51adab5 to
f268a64
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
internal/app/azldev/core/sources/upstream_provenance.go:172
- The explanation about replacement order is misleading:
strings.ReplaceAlldoes exact substring replacement, so replacing%{dist}cannot partially match%{?dist}and leave a stray%{?behind. This comment could confuse future maintainers about why the order matters.
// replaceDistTag substitutes both the conditional (%{?dist}) and unconditional
// (%{dist}) dist macros in a Release value with the resolved Fedora dist tag.
// The conditional form is replaced first so the %{dist} pass does not match the
// inner {dist} of a %{?dist} token and leave a stray "%{?" behind.
internal/app/azldev/core/sources/upstream_provenance.go:223
- When
rpmautospec calculate-releasefails, the warning logs only include the Go error. Capturing stderr (like internal/utils/git/git.go:238-256) makes failures diagnosable, especially when rpmautospec emits useful details on stderr.
// --complete-release prints the release with any prerelease flags but without
// the dist tag, which is exactly what we combine with the resolved Fedora
// dist tag.
execCmd := exec.CommandContext(ctx, rpmautospecBinary, "calculate-release", "--complete-release", specPath)
execCmd.Dir = sourcesDir
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/app/azldev/core/sources/upstream_provenance.go:207
calculateAutoreleasesetsexecCmd.Dir = sourcesDirbut passesspecPaththat is already joined withsourcesDir(viafindSpecInDir). IfsourcesDir/specPathare relative (common forcomponent prepare-sources -o ./...),rpmautospecwill be invoked fromsourcesDirwith an argument like./out/comp/comp.spec, which resolves tosourcesDir/./out/comp/comp.specand fails to find the spec. Pass only the spec filename (orfilepath.Base(specPath)) when settingDir.
execCmd := exec.CommandContext(ctx, rpmautospecBinary, "calculate-release", "--complete-release", specPath)
execCmd.Dir = sourcesDir
f268a64 to
7c5614a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
internal/app/azldev/cmds/component/diffsources.go:104
component diff-sourceswiresWithUpstreamProvenance(...)but does not passWithMockProcessor(...). For opted-in Fedora upstream specs using%autorelease, this means%fedora_upstream_releasewill be skipped here even though build/render can resolve it, so diff output can diverge from build/render behavior.
preparer, err := sources.NewPreparer(sourceManager, env.FS(), env, env,
sources.WithUpstreamProvenance(sources.FedoraDistTag(distro.Ref.Name, distro.Version.ReleaseVer)))
if err != nil {
return nil, fmt.Errorf("failed to create source preparer:\n%w", err)
}
internal/app/azldev/core/sources/mockprocessor.go:296
git config --global --add safe.directory '*'disables Git’s dubious-ownership protection for all paths inside the chroot. Since the bind mount location is known (/tmp/provenance), this can be scoped to just that directory to reduce the blast radius while still avoiding ownership-mismatch failures.
// safe.directory guards against any residual host/chroot ownership mismatch
// on the bind-mounted .git, mirroring the render batch path. Wrapped in
// `sh -c` so the shell operators survive shellquote joining in CmdInChroot.
shellCmd := "git config --global --add safe.directory '*' && " +
"rpmautospec calculate-release --complete-release " + shellquote.Join(specInChroot)
internal/app/azldev/cmds/component/preparesources.go:166
prepare-sourcesenables upstream provenance viaWithUpstreamProvenance(...), but never supplies aMockProcessor(WithMockProcessor(...)). As a result, opted-in Fedora upstream components whose pristine spec uses%autoreleasewill consistently miss%fedora_upstream_releasein this code path (even whenWithGitRepois enabled), diverging from build/render behavior.
opts = append(opts,
sources.WithUpstreamProvenance(sources.FedoraDistTag(distro.Ref.Name, distro.Version.ReleaseVer)))
| // spec's basename within that directory. The raw command output is returned; | ||
| // the caller parses and validates it. The chroot is lazily initialized on first | ||
| // use, shared with [MockProcessor.BatchProcess]. | ||
| func (p *MockProcessor) CalculateRelease(ctx context.Context, specHostDir, specFilename string) (string, error) { |
There was a problem hiding this comment.
Add some unit tests for CalculateRelease please
Add a build.emit-upstream-provenance flag that, for Fedora upstream
components, injects %fedora_upstream_version and %fedora_upstream_release
macros into the component's generated macros file. Values are read from
the pristine upstream Fedora spec on disk (before overlays) and the
Release tag's %{?dist} is expanded to the resolved Fedora dist tag.
For specs using rpmautospec (Release: %autorelease), the pristine Fedora
release number is resolved by running `rpmautospec calculate-release`
against the upstream dist-git checkout while its history is still Fedora's
(before azldev's synthetic overlay commits are layered on). azldev appends
its own resolved .fc<N> dist tag rather than trusting the build root's
%{?dist} (the mock is Azure Linux, not Fedora). Resolution is best-effort:
if rpmautospec is unavailable or returns an unusable value, the release
macro is skipped with a warning instead of emitting a literal %autorelease.
Nothing is stored in the lock file; the macros are derived fresh at
render/build time from the pinned upstream commit. Opt-in so only
components that reference the macros (e.g. grub2/fwupd-efi SBAT) get a
macros file. User-defined macros of the same name win.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7c5614a to
0a53cbf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/app/azldev/cmds/component/diffsources.go:102
component diff-sourcesenablesWithUpstreamProvenance(...)but never passesWithMockProcessor(...). For Fedora upstream specs whose pristineRelease:uses rpmautospec%autorelease, this means%fedora_upstream_releasecan never be resolved in diff output (even when the project has a mock config), diverging fromcomponent render/component buildbehavior and the documented best-effort semantics (skip only when no mock config or rpmautospec fails).
preparer, err := sources.NewPreparer(sourceManager, env.FS(), env, env,
sources.WithUpstreamProvenance(sources.FedoraDistTag(distro.Ref.Name, distro.Version.ReleaseVer)))
if err != nil {
internal/app/azldev/cmds/component/preparesources.go:166
component prepare-sourceswiresWithUpstreamProvenance(...)but does not provideWithMockProcessor(...). As a result, opted-in Fedora upstream components whose pristine spec uses rpmautospec%autoreleasewill always skip emitting%fedora_upstream_releasein this command, even when a project mock config exists (unlikecomponent render/component build). Consider creating a mock processor for the command (best-effort when config exists) and passing it viasources.WithMockProcessor(...), with a corresponding cleanup at the command boundary.
opts = append(opts,
sources.WithUpstreamProvenance(sources.FedoraDistTag(distro.Ref.Name, distro.Version.ReleaseVer)))
Summary
This PR supports azldev emits
%fedora_upstream_versionand%fedora_upstream_releasemacros for Fedora upstream components that opt in via a newbuild.emit-upstream-provenanceflag. Values are read from the pristine upstream Fedora spec on disk (before overlays are applied) and the Release tag's%{?dist}token is expanded to the resolved Fedora dist tag (e.g..fc43). The macros are surfaced through the component's existing generated macros file.This is a replacement for #285 ("capture upstream version/release provenance and expose
%azl_upstream_*macros"). That PR stored the version and release in the per-component lock file and derived the macros at build time from the lock. This PR takes a different, simpler approach.Closed #285.
Advantages over #285
component update→ store in lockfile → read at render/buildupstream-name,upstream-epoch,upstream-version,upstream-release)updatecommandhasLockedUpstreamVersionReleasegateSourceIdentityplumbingResolveIdentity/ResolveSourceIdentitysignature change threaded through Fedora, RPM, and local providersgit show <commit>:<name>.specper component during identity resolution.azl.macrosfileupdaterun%azl_upstream_*%fedora_upstream_*(explicit about the source)Design
sources.WithUpstreamProvenance(distTag string)onSourcePreparer.distTagis the resolved%{?dist}expansion (FedoraDistTag(distroName, releaseVer)returns.fc<N>for Fedora,""otherwise, disabling the feature).writeMacrosFile(in the pre-overlay window). The pristine spec exists on disk right afterFetchComponent, before overlays rewrite it.parseSpecVersionReleasereads Version/Release verbatim via the existingspec.VisitTagsPackagepath; only%{?dist}is substituted.build.emit-upstream-provenance = truein[components.<name>.build]. Fingerprintable — flipping it changes rendered output. Registered with thecomponent historycollector.build.defineswins (macros are layered viasetMacroIfAbsent)..azl.macros+%{load:%{_sourcedir}/...}+Source9999path. No second mechanism.spec.type = upstream. Local, SRPM, and non-Fedora upstream components are skipped, so the flag is a no-op for them.WARNis logged and the macros are skipped; render/build proceeds without them.Consumer example (grub2 SBAT)
An accompanying comp.toml change in the azurelinux repo enables the flag and adds a spec-search-replace overlay:
Result: the built
sbat.csv'sgrub.rhline carries the pristine upstream Fedora NEVR (e.g.grub.rh,2,Red Hat,grub2,2.12-45.fc43,...) instead of the AZL build V-R (…azl4).Testing
mage unit— passing (7 new tests cover happy path, opt-out, local skip, non-Fedora skip, user-define wins, missing spec best-effort, empty release, and dist-tag mapping).mage check all— passing (linting, static analysis, license, schema).mage docsregenerated the JSON schema for the new field.grub2build succeeded and the SBAT overlay is verified working using command:Both EFI binaries (x64 and ia32) carry identical SBAT data:
Fix: AB#12798
ADR: AB#28331
Limitations (documented)
%{?dist}is expanded. A tag built from other macros (e.g.Version: %{majorver}.%{minorver}) is captured with those macros unexpanded.fedora_upstream_version/_releaseinbuild.undefinesdoes not suppress the injected macros. Useemit-upstream-provenance = false(or omit it) instead.