From 5202617ab825965719208b1b135bc8c9825c52b9 Mon Sep 17 00:00:00 2001 From: Nan Liu Date: Wed, 9 Sep 2026 00:18:27 +0000 Subject: [PATCH] fix(overlays): allocate macro source tag to avoid collisions The macro-load overlay hard-coded its source as Source9999, which collided with specs that already define that tag (e.g. texlive, which uses sources up to Source10133), failing with "source 9999 defined multiple times". Replace the fixed tag with a new internal source-add overlay that allocates the next number after the highest existing source tag at apply time, after user overlays have run. Add Spec.AddSourceEntry and ParseSourceTagNumber mirroring the existing patch-entry logic, and return the load directive and source registration as separate overlays so the registration is ordered last. Fixes #235 --- internal/app/azldev/core/sources/overlays.go | 9 +- .../app/azldev/core/sources/overlays_test.go | 16 +++ .../app/azldev/core/sources/sourceprep.go | 44 +++++--- .../azldev/core/sources/sourceprep_test.go | 46 +++++++- internal/rpm/spec/edit.go | 72 ++++++++++++ internal/rpm/spec/edit_test.go | 103 ++++++++++++++++++ 6 files changed, 271 insertions(+), 19 deletions(-) diff --git a/internal/app/azldev/core/sources/overlays.go b/internal/app/azldev/core/sources/overlays.go index 44c2ca5c5..91bac3bab 100644 --- a/internal/app/azldev/core/sources/overlays.go +++ b/internal/app/azldev/core/sources/overlays.go @@ -30,6 +30,8 @@ import ( // overlay that found no matches). var ErrOverlayDidNotApply = errors.New("overlay did not apply to target") +const componentOverlayAddSource projectconfig.ComponentOverlayType = "internal-source-add" + // isSpecFile returns true if the given file path refers to a spec file. func isSpecFile(filePath string) bool { return strings.HasSuffix(filePath, ".spec") @@ -46,7 +48,7 @@ func ApplyOverlayToSources( sourcesDirPath, specPath string, ) error { // Apply the spec component, if any. - if overlay.ModifiesSpec() { + if overlay.ModifiesSpec() || overlay.Type == componentOverlayAddSource { err := ApplySpecOverlayToFileInPlace(fs, overlay, specPath) if err != nil { return err @@ -130,6 +132,11 @@ func ApplySpecOverlay(overlay projectconfig.ComponentOverlay, openedSpec *spec.S if err != nil { return fmt.Errorf("failed to insert tag %#q into spec:\n%w", overlay.Tag, err) } + case componentOverlayAddSource: + err := openedSpec.AddSourceEntry(overlay.Value) + if err != nil { + return fmt.Errorf("failed to add source entry to spec:\n%w", err) + } case projectconfig.ComponentOverlayUpdateSpecTag: err := openedSpec.UpdateExistingTag(overlay.PackageName, overlay.Tag, overlay.Value) if err != nil { diff --git a/internal/app/azldev/core/sources/overlays_test.go b/internal/app/azldev/core/sources/overlays_test.go index 5e6a26d4d..646cfd0e6 100644 --- a/internal/app/azldev/core/sources/overlays_test.go +++ b/internal/app/azldev/core/sources/overlays_test.go @@ -136,6 +136,22 @@ BuildRequires: gcc Source0: test.tar.gz Source9999: macros.azl.macros BuildRequires: gcc +`, + }, + { + name: "add source avoids occupied preferred number", + overlay: projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayType("internal-source-add"), + Value: "macros.azl.macros", + }, + spec: `Name: name +Source9999: upstream.file +BuildRequires: gcc +`, + result: `Name: name +Source9999: upstream.file +Source10000: macros.azl.macros +BuildRequires: gcc `, }, { diff --git a/internal/app/azldev/core/sources/sourceprep.go b/internal/app/azldev/core/sources/sourceprep.go index 4d32c9834..d627bf135 100644 --- a/internal/app/azldev/core/sources/sourceprep.go +++ b/internal/app/azldev/core/sources/sourceprep.go @@ -426,26 +426,34 @@ func (p *sourcePreparerImpl) applyArchiveOverlayGroup( return repackedArchives, nil } -// collectOverlays gathers all overlays for a component into a single ordered slice: -// macros-load first, then user overlays, followed by check-skip and file-header overlays. +// collectOverlays gathers all overlays for a component into a single ordered slice: the +// macros-load directive first, then user overlays, then check-skip overlays, then the +// macros source registration, and finally the file-header overlay. The macros source +// registration is deliberately ordered after user overlays so it claims the next free +// source number without colliding with any sources the user added. func (p *sourcePreparerImpl) collectOverlays( component components.Component, macrosFileName string, ) ([]projectconfig.ComponentOverlay, error) { config := component.GetConfig() - var allOverlays []projectconfig.ComponentOverlay + var ( + allOverlays []projectconfig.ComponentOverlay + macroSourceOverlays []projectconfig.ComponentOverlay + ) if macrosFileName != "" { - macroOverlays, err := synthesizeMacroLoadOverlays(macrosFileName) + loadDirective, sourceRegistration, err := synthesizeMacroLoadOverlays(macrosFileName) if err != nil { return nil, fmt.Errorf("failed to compute macros load overlays:\n%w", err) } - allOverlays = append(allOverlays, macroOverlays...) + allOverlays = append(allOverlays, loadDirective...) + macroSourceOverlays = sourceRegistration } allOverlays = append(allOverlays, config.Overlays...) allOverlays = append(allOverlays, synthesizeCheckSkipOverlays(config.Build.Check)...) + allOverlays = append(allOverlays, macroSourceOverlays...) allOverlays = append(allOverlays, generateFileHeaderOverlay()...) return allOverlays, nil @@ -1288,19 +1296,25 @@ func renderMacrosFile(macros map[string]string) string { return strings.Join(lines, "\n") + "\n" } -func synthesizeMacroLoadOverlays(macrosFileName string) ([]projectconfig.ComponentOverlay, error) { +// synthesizeMacroLoadOverlays returns the overlays that wire a component's generated +// macros file into its spec. The load-directive overlay prepends the %{load:...} line; the +// source-registration overlay adds the macros file as a numbered Source. They are returned +// separately so the caller can apply the source registration after user overlays, letting +// it claim the next free source number without colliding with sources the user added. +func synthesizeMacroLoadOverlays( + macrosFileName string, +) (loadDirective, sourceRegistration []projectconfig.ComponentOverlay, err error) { // Basic check that the macros file name is valid and doesn't require escaping. if strings.ContainsFunc(macrosFileName, func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != '.' && r != '-' && r != '_' && r != '+' }) { - return nil, fmt.Errorf( + return nil, nil, fmt.Errorf( "macros file name %#q contains invalid characters; does the component name contain invalid characters?", macrosFileName, ) } - // We inject an overlay to prepend a line to the spec to load the macros file. - return []projectconfig.ComponentOverlay{ + loadDirective = []projectconfig.ComponentOverlay{ { // Prepend the %{load:...} directive to the spec. Type: projectconfig.ComponentOverlayPrependSpecLines, @@ -1311,16 +1325,18 @@ func synthesizeMacroLoadOverlays(macrosFileName string) ([]projectconfig.Compone "", }, }, + } + + sourceRegistration = []projectconfig.ComponentOverlay{ { // Ensure that the macros file is manifested as a source in the spec so that // mock and other tools know it needs to be present in the build root. - // Use InsertSpecTag to place it after the last existing Source* tag, avoiding - // misplacement after macros like %fontpkg or inside %if conditionals. - Type: projectconfig.ComponentOverlayInsertSpecTag, - Tag: "Source9999", // Use a high number to avoid conflicts with existing sources. + Type: componentOverlayAddSource, Value: macrosFileName, }, - }, nil + } + + return loadDirective, sourceRegistration, nil } // generateFileHeaderOverlay generates an overlay that prepends a header to the spec. diff --git a/internal/app/azldev/core/sources/sourceprep_test.go b/internal/app/azldev/core/sources/sourceprep_test.go index 8276689c3..579cc7afa 100644 --- a/internal/app/azldev/core/sources/sourceprep_test.go +++ b/internal/app/azldev/core/sources/sourceprep_test.go @@ -7,6 +7,7 @@ import ( "errors" "os" "path/filepath" + "regexp" "strings" "testing" @@ -94,11 +95,11 @@ func TestPrepareSources_Success(t *testing.T) { require.NoError(t, err) assert.False(t, exists, "macros file should not be created when there are no macros") - // Verify spec does NOT contain macro load or Source9999. + // Verify spec does not contain a macro load directive or a macros source entry. specContents, err := fileutils.ReadFile(ctx.FS(), outputSpecPath) require.NoError(t, err) assert.NotContains(t, string(specContents), "%{load:%{_sourcedir}/"+macrosFileName+"}") - assert.NotContains(t, string(specContents), "Source9999") + assert.NotContains(t, string(specContents), macrosFileName) } // TestPrepareSources_ArchiveOverlayRehashesSourcesEntry is an end-to-end check @@ -423,14 +424,51 @@ func TestPrepareSources_WritesMacrosFile(t *testing.T) { require.NoError(t, err) assert.Contains(t, string(contents), "%_with_feature 1") - // Verify spec has macro load directive and Source9999 tag. + // Verify spec has macro load directive and a collision-free source tag. specPath := filepath.Join(testOutputDir, "my-package.spec") specContents, err := fileutils.ReadFile(ctx.FS(), specPath) require.NoError(t, err) specStr := string(specContents) assert.Contains(t, specStr, "%{load:%{_sourcedir}/my-package"+sources.MacrosFileExtension+"}") - assert.Contains(t, specStr, "Source9999") + // Assert the macros file is registered under some SourceN tag, without pinning the number + // (the allocator picks the next free source number based on the spec's existing sources). + macrosSourcePattern := regexp.MustCompile( + `(?m)^Source[0-9]+: my-package` + regexp.QuoteMeta(sources.MacrosFileExtension) + `$`, + ) + assert.Regexp(t, macrosSourcePattern, specStr) +} + +func TestPrepareSources_MacroSourceTagCollision(t *testing.T) { + const testOutputDir = "/output" + + ctrl := gomock.NewController(t) + component := components_testutils.NewMockComponent(ctrl) + sourceManager := sourceproviders_test.NewMockSourceManager(ctrl) + ctx := testctx.NewCtx() + + component.EXPECT().GetName().AnyTimes().Return("my-package") + component.EXPECT().GetConfig().AnyTimes().Return(&projectconfig.ComponentConfig{ + Build: projectconfig.ComponentBuildConfig{With: []string{"feature"}}, + }) + sourceManager.EXPECT().FetchFiles(gomock.Any(), component, testOutputDir).Return(nil) + sourceManager.EXPECT().FetchComponent(gomock.Any(), component, testOutputDir, gomock.Any()).DoAndReturn( + func(_ interface{}, _ interface{}, outputDir string, _ ...sourceproviders.FetchComponentOption) error { + return fileutils.WriteFile( + ctx.FS(), filepath.Join(outputDir, "my-package.spec"), + []byte("Name: my-package\nSource9999: upstream.file\n"), fileperms.PublicFile, + ) + }, + ) + + preparer, err := sources.NewPreparer(sourceManager, ctx.FS(), ctx, ctx) + require.NoError(t, err) + require.NoError(t, preparer.PrepareSources(ctx, component, testOutputDir, true)) + + specContents, err := fileutils.ReadFile(ctx.FS(), filepath.Join(testOutputDir, "my-package.spec")) + require.NoError(t, err) + assert.Contains(t, string(specContents), "Source9999: upstream.file") + assert.Contains(t, string(specContents), "Source10000: my-package"+sources.MacrosFileExtension) } // Tests for GenerateMacrosFileContents - these test content generation in isolation. diff --git a/internal/rpm/spec/edit.go b/internal/rpm/spec/edit.go index 0fdd056c3..15df87c51 100644 --- a/internal/rpm/spec/edit.go +++ b/internal/rpm/spec/edit.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "log/slog" + "math" "regexp" "strconv" "strings" @@ -631,6 +632,77 @@ func ParsePatchTagNumber(tag string) (int, bool) { return num, true } +// ParseSourceTagNumber checks if the given tag name is a SourceN tag (case-insensitive) +// and returns the numeric suffix N. Returns -1, false if the tag is not a SourceN tag +// or the suffix is not a valid non-negative integer. +func ParseSourceTagNumber(tag string) (int, bool) { + suffix, found := strings.CutPrefix(strings.ToLower(tag), "source") + if !found || suffix == "" { + return -1, false + } + + num, err := strconv.Atoi(suffix) + if err != nil || num < 0 { + return -1, false + } + + return num, true +} + +// AddSourceEntry registers a source in the spec. It prefers the conventional high slot +// (Source9999) when that number is free, matching azldev's historical output so already-rendered +// specs stay byte-identical (idempotent). When Source9999 is already taken, it falls back to the +// next number after the highest existing source tag. Automatically numbered bare Source tags are +// included when determining occupancy and the highest number. +func (s *Spec) AddSourceEntry(filename string) error { + const preferredSourceTagNumber = 9999 + + highest := -1 + unnumberedCount := 0 + preferredOccupied := false + + err := s.VisitTags(func(tagLine *TagLine, _ *Context) error { + num, isSourceTag := ParseSourceTagNumber(tagLine.Tag) + if isSourceTag { + if num > highest { + highest = num + } + + if num == preferredSourceTagNumber { + preferredOccupied = true + } + } else if strings.EqualFold(tagLine.Tag, "source") { + unnumberedCount++ + } + + return nil + }) + if err != nil { + return fmt.Errorf("failed to scan for existing source tags:\n%w", err) + } + + // Bare Source tags auto-number sequentially from 0, occupying slots 0..unnumberedCount-1. + if unnumberedCount-1 > highest { + highest = unnumberedCount - 1 + } + + if unnumberedCount > preferredSourceTagNumber { + preferredOccupied = true + } + + num := preferredSourceTagNumber + + if preferredOccupied { + if highest == math.MaxInt { + return errors.New("cannot allocate SourceN tag after maximum integer tag number") + } + + num = highest + 1 + } + + return s.InsertTag("", fmt.Sprintf("Source%d", num), filename) +} + // HasSection returns true if the spec contains a section with the given name. // The comparison is exact (case-sensitive), consistent with [AppendLinesToSection]. func (s *Spec) HasSection(sectionName string) (bool, error) { diff --git a/internal/rpm/spec/edit_test.go b/internal/rpm/spec/edit_test.go index 959c5f104..336f7143e 100644 --- a/internal/rpm/spec/edit_test.go +++ b/internal/rpm/spec/edit_test.go @@ -5,6 +5,8 @@ package spec_test import ( "bytes" + "fmt" + "math" "strings" "testing" "time" @@ -1340,6 +1342,81 @@ func TestGetHighestPatchTagNumber(t *testing.T) { } } +func TestAddSourceEntry(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "prefers Source9999 when it is free", + input: "Name: test\nSource0: source.tar.gz\nVersion: 1.0\n", + expected: "Name: test\nSource0: source.tar.gz\nSource9999: macros.azl.macros\nVersion: 1.0\n", + }, + { + name: "prefers Source9999 when no sources exist", + input: "Name: test\nVersion: 1.0\n", + expected: "Name: test\nVersion: 1.0\nSource9999: macros.azl.macros\n", + }, + { + name: "avoids occupied preferred source number", + input: "Name: test\nSource9999: upstream.file\nVersion: 1.0\n", + expected: "Name: test\nSource9999: upstream.file\nSource10000: macros.azl.macros\nVersion: 1.0\n", + }, + { + name: "source tags are case insensitive", + input: "Name: test\nSOURCE9999: upstream.file\nsource9998: another.file\n", + expected: "Name: test\nSOURCE9999: upstream.file\nsource9998: another.file\nSource10000: macros.azl.macros\n", + }, + { + name: "bare source reserves automatically numbered slot", + input: "Name: test\nSource: source.tar.gz\nSource9999: upstream.file\n", + expected: "Name: test\nSource: source.tar.gz\nSource9999: upstream.file\nSource10000: macros.azl.macros\n", + }, + { + name: "allocates after source numbers above preferred range", + input: "Name: test\nSource9999: upstream.file\nSource10133: texlive.file\n", + expected: "Name: test\nSource9999: upstream.file\nSource10133: texlive.file\nSource10134: macros.azl.macros\n", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + require.NoError(t, err) + + err = specFile.AddSourceEntry("macros.azl.macros") + require.NoError(t, err) + + var output strings.Builder + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, testCase.expected, output.String()) + }) + } +} + +func TestAddSourceEntry_ManyBareSources(t *testing.T) { + input := "Name: test\n" + strings.Repeat("Source: automatically-numbered.tar.gz\n", 10001) + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.NoError(t, specFile.AddSourceEntry("macros.azl.macros")) + + var output strings.Builder + require.NoError(t, specFile.Serialize(&output)) + assert.Contains(t, output.String(), "Source10001: macros.azl.macros\n") +} + +func TestAddSourceEntry_MaximumIntegerTag(t *testing.T) { + // Source9999 occupied forces the fallback path; SourceMaxInt makes highest+1 overflow. + input := fmt.Sprintf("Name: test\nSource9999: upstream.file\nSource%d: max.file\n", math.MaxInt) + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AddSourceEntry("macros.azl.macros") + require.ErrorContains(t, err, "cannot allocate SourceN tag after maximum integer tag number") +} + func TestRemoveTagsMatching(t *testing.T) { tests := []struct { name string @@ -1538,6 +1615,32 @@ func TestParsePatchTagNumber(t *testing.T) { } } +func TestParseSourceTagNumber(t *testing.T) { + tests := []struct { + tag string + expectedNum int + expectedOK bool + }{ + {"Source0", 0, true}, + {"Source9999", 9999, true}, + {"source5", 5, true}, + {"SOURCE10000", 10000, true}, + {"Source-1", -1, false}, + {"Source", -1, false}, + {"SourceFoo", -1, false}, + {"Patch0", -1, false}, + {"", -1, false}, + } + + for _, testCase := range tests { + t.Run(testCase.tag, func(t *testing.T) { + num, ok := spec.ParseSourceTagNumber(testCase.tag) + assert.Equal(t, testCase.expectedNum, num) + assert.Equal(t, testCase.expectedOK, ok) + }) + } +} + func TestVisitTags(t *testing.T) { input := `Name: main-pkg Version: 1.0