Skip to content

Commit 1f331c9

Browse files
committed
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
1 parent e799c2c commit 1f331c9

6 files changed

Lines changed: 271 additions & 19 deletions

File tree

internal/app/azldev/core/sources/overlays.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ import (
3030
// overlay that found no matches).
3131
var ErrOverlayDidNotApply = errors.New("overlay did not apply to target")
3232

33+
const componentOverlayAddSource projectconfig.ComponentOverlayType = "internal-source-add"
34+
3335
// isSpecFile returns true if the given file path refers to a spec file.
3436
func isSpecFile(filePath string) bool {
3537
return strings.HasSuffix(filePath, ".spec")
@@ -46,7 +48,7 @@ func ApplyOverlayToSources(
4648
sourcesDirPath, specPath string,
4749
) error {
4850
// Apply the spec component, if any.
49-
if overlay.ModifiesSpec() {
51+
if overlay.ModifiesSpec() || overlay.Type == componentOverlayAddSource {
5052
err := ApplySpecOverlayToFileInPlace(fs, overlay, specPath)
5153
if err != nil {
5254
return err
@@ -130,6 +132,11 @@ func ApplySpecOverlay(overlay projectconfig.ComponentOverlay, openedSpec *spec.S
130132
if err != nil {
131133
return fmt.Errorf("failed to insert tag %#q into spec:\n%w", overlay.Tag, err)
132134
}
135+
case componentOverlayAddSource:
136+
err := openedSpec.AddSourceEntry(overlay.Value)
137+
if err != nil {
138+
return fmt.Errorf("failed to add source entry to spec:\n%w", err)
139+
}
133140
case projectconfig.ComponentOverlayUpdateSpecTag:
134141
err := openedSpec.UpdateExistingTag(overlay.PackageName, overlay.Tag, overlay.Value)
135142
if err != nil {

internal/app/azldev/core/sources/overlays_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,22 @@ BuildRequires: gcc
136136
Source0: test.tar.gz
137137
Source9999: macros.azl.macros
138138
BuildRequires: gcc
139+
`,
140+
},
141+
{
142+
name: "add source avoids occupied preferred number",
143+
overlay: projectconfig.ComponentOverlay{
144+
Type: projectconfig.ComponentOverlayType("internal-source-add"),
145+
Value: "macros.azl.macros",
146+
},
147+
spec: `Name: name
148+
Source9999: upstream.file
149+
BuildRequires: gcc
150+
`,
151+
result: `Name: name
152+
Source9999: upstream.file
153+
Source10000: macros.azl.macros
154+
BuildRequires: gcc
139155
`,
140156
},
141157
{

internal/app/azldev/core/sources/sourceprep.go

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -426,26 +426,34 @@ func (p *sourcePreparerImpl) applyArchiveOverlayGroup(
426426
return repackedArchives, nil
427427
}
428428

429-
// collectOverlays gathers all overlays for a component into a single ordered slice:
430-
// macros-load first, then user overlays, followed by check-skip and file-header overlays.
429+
// collectOverlays gathers all overlays for a component into a single ordered slice: the
430+
// macros-load directive first, then user overlays, then check-skip overlays, then the
431+
// macros source registration, and finally the file-header overlay. The macros source
432+
// registration is deliberately ordered after user overlays so it claims the next free
433+
// source number without colliding with any sources the user added.
431434
func (p *sourcePreparerImpl) collectOverlays(
432435
component components.Component, macrosFileName string,
433436
) ([]projectconfig.ComponentOverlay, error) {
434437
config := component.GetConfig()
435438

436-
var allOverlays []projectconfig.ComponentOverlay
439+
var (
440+
allOverlays []projectconfig.ComponentOverlay
441+
macroSourceOverlays []projectconfig.ComponentOverlay
442+
)
437443

438444
if macrosFileName != "" {
439-
macroOverlays, err := synthesizeMacroLoadOverlays(macrosFileName)
445+
loadDirective, sourceRegistration, err := synthesizeMacroLoadOverlays(macrosFileName)
440446
if err != nil {
441447
return nil, fmt.Errorf("failed to compute macros load overlays:\n%w", err)
442448
}
443449

444-
allOverlays = append(allOverlays, macroOverlays...)
450+
allOverlays = append(allOverlays, loadDirective...)
451+
macroSourceOverlays = sourceRegistration
445452
}
446453

447454
allOverlays = append(allOverlays, config.Overlays...)
448455
allOverlays = append(allOverlays, synthesizeCheckSkipOverlays(config.Build.Check)...)
456+
allOverlays = append(allOverlays, macroSourceOverlays...)
449457
allOverlays = append(allOverlays, generateFileHeaderOverlay()...)
450458

451459
return allOverlays, nil
@@ -1288,19 +1296,25 @@ func renderMacrosFile(macros map[string]string) string {
12881296
return strings.Join(lines, "\n") + "\n"
12891297
}
12901298

1291-
func synthesizeMacroLoadOverlays(macrosFileName string) ([]projectconfig.ComponentOverlay, error) {
1299+
// synthesizeMacroLoadOverlays returns the overlays that wire a component's generated
1300+
// macros file into its spec. The load-directive overlay prepends the %{load:...} line; the
1301+
// source-registration overlay adds the macros file as a numbered Source. They are returned
1302+
// separately so the caller can apply the source registration after user overlays, letting
1303+
// it claim the next free source number without colliding with sources the user added.
1304+
func synthesizeMacroLoadOverlays(
1305+
macrosFileName string,
1306+
) (loadDirective, sourceRegistration []projectconfig.ComponentOverlay, err error) {
12921307
// Basic check that the macros file name is valid and doesn't require escaping.
12931308
if strings.ContainsFunc(macrosFileName, func(r rune) bool {
12941309
return !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != '.' && r != '-' && r != '_' && r != '+'
12951310
}) {
1296-
return nil, fmt.Errorf(
1311+
return nil, nil, fmt.Errorf(
12971312
"macros file name %#q contains invalid characters; does the component name contain invalid characters?",
12981313
macrosFileName,
12991314
)
13001315
}
13011316

1302-
// We inject an overlay to prepend a line to the spec to load the macros file.
1303-
return []projectconfig.ComponentOverlay{
1317+
loadDirective = []projectconfig.ComponentOverlay{
13041318
{
13051319
// Prepend the %{load:...} directive to the spec.
13061320
Type: projectconfig.ComponentOverlayPrependSpecLines,
@@ -1311,16 +1325,18 @@ func synthesizeMacroLoadOverlays(macrosFileName string) ([]projectconfig.Compone
13111325
"",
13121326
},
13131327
},
1328+
}
1329+
1330+
sourceRegistration = []projectconfig.ComponentOverlay{
13141331
{
13151332
// Ensure that the macros file is manifested as a source in the spec so that
13161333
// mock and other tools know it needs to be present in the build root.
1317-
// Use InsertSpecTag to place it after the last existing Source* tag, avoiding
1318-
// misplacement after macros like %fontpkg or inside %if conditionals.
1319-
Type: projectconfig.ComponentOverlayInsertSpecTag,
1320-
Tag: "Source9999", // Use a high number to avoid conflicts with existing sources.
1334+
Type: componentOverlayAddSource,
13211335
Value: macrosFileName,
13221336
},
1323-
}, nil
1337+
}
1338+
1339+
return loadDirective, sourceRegistration, nil
13241340
}
13251341

13261342
// generateFileHeaderOverlay generates an overlay that prepends a header to the spec.

internal/app/azldev/core/sources/sourceprep_test.go

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"errors"
88
"os"
99
"path/filepath"
10+
"regexp"
1011
"strings"
1112
"testing"
1213

@@ -94,11 +95,11 @@ func TestPrepareSources_Success(t *testing.T) {
9495
require.NoError(t, err)
9596
assert.False(t, exists, "macros file should not be created when there are no macros")
9697

97-
// Verify spec does NOT contain macro load or Source9999.
98+
// Verify spec does not contain a macro load or source entry.
9899
specContents, err := fileutils.ReadFile(ctx.FS(), outputSpecPath)
99100
require.NoError(t, err)
100101
assert.NotContains(t, string(specContents), "%{load:%{_sourcedir}/"+macrosFileName+"}")
101-
assert.NotContains(t, string(specContents), "Source9999")
102+
assert.NotContains(t, string(specContents), "Source0")
102103
}
103104

104105
// TestPrepareSources_ArchiveOverlayRehashesSourcesEntry is an end-to-end check
@@ -423,14 +424,51 @@ func TestPrepareSources_WritesMacrosFile(t *testing.T) {
423424
require.NoError(t, err)
424425
assert.Contains(t, string(contents), "%_with_feature 1")
425426

426-
// Verify spec has macro load directive and Source9999 tag.
427+
// Verify spec has macro load directive and a collision-free source tag.
427428
specPath := filepath.Join(testOutputDir, "my-package.spec")
428429
specContents, err := fileutils.ReadFile(ctx.FS(), specPath)
429430
require.NoError(t, err)
430431

431432
specStr := string(specContents)
432433
assert.Contains(t, specStr, "%{load:%{_sourcedir}/my-package"+sources.MacrosFileExtension+"}")
433-
assert.Contains(t, specStr, "Source9999")
434+
// Assert the macros file is registered under some SourceN tag, without pinning the number
435+
// (the allocator picks the next free source number based on the spec's existing sources).
436+
macrosSourcePattern := regexp.MustCompile(
437+
`(?m)^Source[0-9]+: my-package` + regexp.QuoteMeta(sources.MacrosFileExtension) + `$`,
438+
)
439+
assert.Regexp(t, macrosSourcePattern, specStr)
440+
}
441+
442+
func TestPrepareSources_MacroSourceTagCollision(t *testing.T) {
443+
const testOutputDir = "/output"
444+
445+
ctrl := gomock.NewController(t)
446+
component := components_testutils.NewMockComponent(ctrl)
447+
sourceManager := sourceproviders_test.NewMockSourceManager(ctrl)
448+
ctx := testctx.NewCtx()
449+
450+
component.EXPECT().GetName().AnyTimes().Return("my-package")
451+
component.EXPECT().GetConfig().AnyTimes().Return(&projectconfig.ComponentConfig{
452+
Build: projectconfig.ComponentBuildConfig{With: []string{"feature"}},
453+
})
454+
sourceManager.EXPECT().FetchFiles(gomock.Any(), component, testOutputDir).Return(nil)
455+
sourceManager.EXPECT().FetchComponent(gomock.Any(), component, testOutputDir, gomock.Any()).DoAndReturn(
456+
func(_ interface{}, _ interface{}, outputDir string, _ ...sourceproviders.FetchComponentOption) error {
457+
return fileutils.WriteFile(
458+
ctx.FS(), filepath.Join(outputDir, "my-package.spec"),
459+
[]byte("Name: my-package\nSource9999: upstream.file\n"), fileperms.PublicFile,
460+
)
461+
},
462+
)
463+
464+
preparer, err := sources.NewPreparer(sourceManager, ctx.FS(), ctx, ctx)
465+
require.NoError(t, err)
466+
require.NoError(t, preparer.PrepareSources(ctx, component, testOutputDir, true))
467+
468+
specContents, err := fileutils.ReadFile(ctx.FS(), filepath.Join(testOutputDir, "my-package.spec"))
469+
require.NoError(t, err)
470+
assert.Contains(t, string(specContents), "Source9999: upstream.file")
471+
assert.Contains(t, string(specContents), "Source10000: my-package"+sources.MacrosFileExtension)
434472
}
435473

436474
// Tests for GenerateMacrosFileContents - these test content generation in isolation.

internal/rpm/spec/edit.go

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"errors"
88
"fmt"
99
"log/slog"
10+
"math"
1011
"regexp"
1112
"strconv"
1213
"strings"
@@ -631,6 +632,77 @@ func ParsePatchTagNumber(tag string) (int, bool) {
631632
return num, true
632633
}
633634

635+
// ParseSourceTagNumber checks if the given tag name is a SourceN tag (case-insensitive)
636+
// and returns the numeric suffix N. Returns -1, false if the tag is not a SourceN tag
637+
// or the suffix is not a valid non-negative integer.
638+
func ParseSourceTagNumber(tag string) (int, bool) {
639+
suffix, found := strings.CutPrefix(strings.ToLower(tag), "source")
640+
if !found || suffix == "" {
641+
return -1, false
642+
}
643+
644+
num, err := strconv.Atoi(suffix)
645+
if err != nil || num < 0 {
646+
return -1, false
647+
}
648+
649+
return num, true
650+
}
651+
652+
// AddSourceEntry registers a source in the spec. It prefers the conventional high slot
653+
// (Source9999) when that number is free, matching azldev's historical output so already-rendered
654+
// specs stay byte-identical (idempotent). When Source9999 is already taken, it falls back to the
655+
// next number after the highest existing source tag. Automatically numbered bare Source tags are
656+
// included when determining occupancy and the highest number.
657+
func (s *Spec) AddSourceEntry(filename string) error {
658+
const preferredSourceTagNumber = 9999
659+
660+
highest := -1
661+
unnumberedCount := 0
662+
preferredOccupied := false
663+
664+
err := s.VisitTags(func(tagLine *TagLine, _ *Context) error {
665+
num, isSourceTag := ParseSourceTagNumber(tagLine.Tag)
666+
if isSourceTag {
667+
if num > highest {
668+
highest = num
669+
}
670+
671+
if num == preferredSourceTagNumber {
672+
preferredOccupied = true
673+
}
674+
} else if strings.EqualFold(tagLine.Tag, "source") {
675+
unnumberedCount++
676+
}
677+
678+
return nil
679+
})
680+
if err != nil {
681+
return fmt.Errorf("failed to scan for existing source tags:\n%w", err)
682+
}
683+
684+
// Bare Source tags auto-number sequentially from 0, occupying slots 0..unnumberedCount-1.
685+
if unnumberedCount-1 > highest {
686+
highest = unnumberedCount - 1
687+
}
688+
689+
if unnumberedCount > preferredSourceTagNumber {
690+
preferredOccupied = true
691+
}
692+
693+
num := preferredSourceTagNumber
694+
695+
if preferredOccupied {
696+
if highest == math.MaxInt {
697+
return errors.New("cannot allocate SourceN tag after maximum integer tag number")
698+
}
699+
700+
num = highest + 1
701+
}
702+
703+
return s.InsertTag("", fmt.Sprintf("Source%d", num), filename)
704+
}
705+
634706
// HasSection returns true if the spec contains a section with the given name.
635707
// The comparison is exact (case-sensitive), consistent with [AppendLinesToSection].
636708
func (s *Spec) HasSection(sectionName string) (bool, error) {

0 commit comments

Comments
 (0)