diff --git a/docs/user/reference/config/overlays.md b/docs/user/reference/config/overlays.md index 3ab187916..0e2af5ae7 100644 --- a/docs/user/reference/config/overlays.md +++ b/docs/user/reference/config/overlays.md @@ -10,7 +10,7 @@ Overlays are defined within a component's configuration in your TOML config file ### Spec Overlays -These overlays modify `.spec` files using the structured spec parser, allowing precise targeting of tags and sections. +These overlays modify `.spec` files with lexical tag operations and a structured parser for section-aware operations, allowing precise targeting of tags and sections. | Type | Description | Required Fields | |------|-------------|-----------------| @@ -20,6 +20,7 @@ These overlays modify `.spec` files using the structured spec parser, allowing p | `spec-update-tag` | Updates an existing tag; **fails if the tag doesn't exist** | `tag`, `value` | | `spec-remove-tag` | Removes a tag from the spec; **fails if the tag doesn't exist** | `tag` | | `spec-prepend-lines` | Prepends lines to the start of a section, or to the top of the file if `section` is omitted; **fails if a named section doesn't exist** | `lines` | +| `spec-prepend-all-lines` | Prepends lines to every matching named section; useful for repeated sections in conditional branches; **fails if no matching section exists** | `section`, `lines` | | `spec-append-lines` | Appends lines to the end of a section, or to the bottom of the file if `section` is omitted; **fails if a named section doesn't exist** | `lines` | | `spec-search-replace` | Regex-based search and replace on spec content; targets a single section if `section` is given, otherwise the entire spec | `regex` | | `spec-remove-section` | Removes an entire section from the spec; **fails if section doesn't exist** | `section` | @@ -27,6 +28,12 @@ These overlays modify `.spec` files using the structured spec parser, allowing p | `patch-add` | Adds a patch file and registers it in the spec (PatchN tag or %patchlist) | `source` | | `patch-remove` | Removes patch files and their spec references matching a glob pattern | `file` | +> **Conditional section wrappers:** `spec-remove-section` and +> `spec-remove-subpackage` preserve simple wrappers, but reject layouts that +> interleave removed sections with loose content whose linear RPM ownership +> cannot be preserved. Use a whole-spec `spec-search-replace` overlay to make +> an ambiguous layout unambiguous first, then remove the section. + ### File Overlays These overlays modify non-spec source files directly. They cannot be used on `.spec` files. These @@ -89,11 +96,11 @@ file = "vendor/**" # files inside the archive | Description | `description` | Human-readable explanation documenting the need for the change; helps identify overlays in error messages | All (optional) | | Tag | `tag` | The spec tag name (e.g., `BuildRequires`, `Requires`, `Version`) | `spec-add-tag`, `spec-insert-tag`, `spec-set-tag`, `spec-update-tag`, `spec-remove-tag` | | Value | `value` | The tag value to set, or value to match for removal | `spec-add-tag`, `spec-insert-tag`, `spec-set-tag`, `spec-update-tag`, `spec-remove-tag` (optional for matching) | -| Section | `section` | The spec section to target (e.g., `%build`, `%install`, `%files`, `%description`). Optional for `spec-prepend-lines`, `spec-append-lines`, and `spec-search-replace` — omit to target the entire spec file. Required for `spec-remove-section`. | `spec-prepend-lines` (optional), `spec-append-lines` (optional), `spec-search-replace` (optional), `spec-remove-section` | +| Section | `section` | The spec section to target (e.g., `%build`, `%install`, `%files`, `%description`). Optional for `spec-prepend-lines`, `spec-append-lines`, and `spec-search-replace` — omit to target the entire spec file. Required for `spec-prepend-all-lines` and `spec-remove-section`. | `spec-prepend-lines` (optional), `spec-prepend-all-lines`, `spec-append-lines` (optional), `spec-search-replace` (optional), `spec-remove-section` | | Package | `package` | The sub-package name for multi-package specs; omit to target the main package. Cannot be combined with an omitted `section` (a sub-package is always a sub-qualifier of a section). | All spec overlays (optional, except `spec-remove-subpackage` which **requires** it) | | Regex | `regex` | Regular expression pattern to match | `spec-search-replace`, `file-search-replace` | | Replacement | `replacement` | Literal replacement text; capture group references like `$1` are **not** expanded. Omit or leave empty to delete matched text. | `spec-search-replace`, `file-search-replace`, `file-rename` | -| Lines | `lines` | Array of text lines to insert | `spec-prepend-lines`, `spec-append-lines`, `file-prepend-lines` | +| Lines | `lines` | Array of text lines to insert | `spec-prepend-lines`, `spec-prepend-all-lines`, `spec-append-lines`, `file-prepend-lines` | | File | `file` | The name of the non-spec file to modify or add, or a glob pattern. When combined with the `archive` field, the glob is matched against files inside that source archive. | `file-prepend-lines`, `file-search-replace`, `file-add`, `file-remove`, `file-rename`, `patch-add` (optional), `patch-remove` | | Archive | `archive` | The source archive to extract, modify, and repack (e.g. `pkg-1.0.tar.gz`). When set, `file` is a glob matched relative to the archive's extraction root. | `file-remove`, `file-search-replace` (optional) | | Source | `source` | Path to source file for `file-add` and `patch-add`; relative paths are relative to the config file that defines the overlay (the overlay file if loaded via [`overlay-files`](#per-file-overlay-format), otherwise the component config) | `file-add`, `patch-add` | @@ -625,6 +632,31 @@ the overlay always removes every section associated with the sub-package. > an error is returned; use a `spec-search-replace` overlay to adjust the conditionals > before removing the sub-package. +## Known Limitations + +### Section-scoped operations and straddling conditionals + +Section-scoped tag and search/replace overlays use RPM's linear section ownership, including when a section header is inside a `%if` wrapper and its content continues past `%endif`: + +```spec +%if 0%{!?scl:1} +%package headless +Requires: binutils +%endif +# ← content below is still part of %package headless in RPM's view. +Recommends: default-yama-scope +``` + +In this pattern, `spec-remove-tag` with `package = "headless"` can remove +`Recommends`. Section removal remains more restrictive: it rejects wrappers +that interleave removed sections with loose content whose ownership cannot be +preserved. Use whole-spec `spec-search-replace` to make those layouts +unambiguous before removing a section. + +### Macro-generated sections + +Specs that use macros like `%ghc_lib_subpackage`, `%pyproject_extras_subpkg`, or `%fontpkg` generate sections at build time that are invisible to the static parser. Section-scoped overlays cannot target these generated sections. Use `spec-search-replace` for modifications that need to reach macro-generated content. + ## Validation Overlay configurations are validated when the config file is loaded. Validation checks: diff --git a/internal/app/azldev/agentskill/content/overlays.md.tmpl b/internal/app/azldev/agentskill/content/overlays.md.tmpl index e3b3a0625..0899198e1 100644 --- a/internal/app/azldev/agentskill/content/overlays.md.tmpl +++ b/internal/app/azldev/agentskill/content/overlays.md.tmpl @@ -61,6 +61,7 @@ the config loads, so a missing field fails fast rather than at apply time. | `spec-update-tag` | change an existing tag; fails if it is missing | `tag`, `value` | | `spec-remove-tag` | delete tag instances; without `value`, deletes every instance | `tag` | | `spec-prepend-lines` | insert lines at the top of a section (or the whole file) | `lines` | +| `spec-prepend-all-lines` | insert lines at the top of every matching named section | `section`, `lines` | | `spec-append-lines` | insert lines at the end of a section (or the whole file) | `lines` | | `spec-search-replace` | regex replace within a section (or the whole spec) | `regex` | | `spec-remove-section` | delete a whole section | `section` | @@ -114,6 +115,13 @@ the config loads, so a missing field fails fast rather than at apply time. For a multi-line change use a structured spec overlay (`spec-remove-section`, `spec-prepend-lines`/`spec-append-lines`, etc.). `file-search-replace` is different: it matches against the whole file, so multi-line patterns (and `(?s)`) work there. +- **Section deletion rejects ambiguous conditional section wrappers.** + `spec-remove-section` and `spec-remove-subpackage` preserve simple wrapped + sections, but reject wrappers that interleave removed sections with loose + content whose linear RPM ownership cannot be preserved. Section-scoped tag + and search/replace overlays do follow linear ownership across branches and + after `%endif`. Use whole-spec `spec-search-replace` to make an ambiguous + removal layout unambiguous first. - **`file` is a glob** (`**` supported) for the multi-file file overlays; for `file-add` and `file-rename` it is a single name, and `file-rename`'s `replacement` is a filename only (not a path). diff --git a/internal/app/azldev/core/sources/overlays.go b/internal/app/azldev/core/sources/overlays.go index 44c2ca5c5..806953b3a 100644 --- a/internal/app/azldev/core/sources/overlays.go +++ b/internal/app/azldev/core/sources/overlays.go @@ -154,6 +154,11 @@ func ApplySpecOverlay(overlay projectconfig.ComponentOverlay, openedSpec *spec.S return fmt.Errorf("failed to prepend lines to spec:\n%w", err) } } + case projectconfig.ComponentOverlayPrependAllSpecLines: + err := openedSpec.PrependLinesToAllSections(overlay.SectionName, overlay.PackageName, overlay.Lines) + if err != nil { + return fmt.Errorf("failed to prepend lines to all matching sections in spec:\n%w", err) + } case projectconfig.ComponentOverlayAppendSpecLines: if overlay.SectionName == "" { openedSpec.AppendLines(overlay.Lines) diff --git a/internal/app/azldev/core/sources/release.go b/internal/app/azldev/core/sources/release.go index 55dcad6cb..6c2c1579e 100644 --- a/internal/app/azldev/core/sources/release.go +++ b/internal/app/azldev/core/sources/release.go @@ -8,7 +8,6 @@ import ( "log/slog" "regexp" "strconv" - "strings" "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/components" "github.com/microsoft/azure-linux-dev-tools/internal/global/opctx" @@ -46,21 +45,12 @@ func GetReleaseTagValue(fs opctx.FS, specPath string) (string, error) { return "", fmt.Errorf("failed to parse spec %#q:\n%w", specPath, err) } - var releaseValue string - - err = openedSpec.VisitTagsPackage("", func(tagLine *spec.TagLine, _ *spec.Context) error { - if strings.EqualFold(tagLine.Tag, "Release") { - releaseValue = tagLine.Value - } - - return nil - }) + // Preserve the historical visitor behavior: when a spec contains multiple + // lexical Release tags (including conditional alternatives), the last one + // is the value considered for release calculation. + releaseValue, err := openedSpec.GetLastTag("", "Release") if err != nil { - return "", fmt.Errorf("failed to visit tags in spec %#q:\n%w", specPath, err) - } - - if releaseValue == "" { - return "", fmt.Errorf("release tag not found in spec %#q:\n%w", specPath, spec.ErrNoSuchTag) + return "", fmt.Errorf("failed to get Release tag from spec %#q:\n%w", specPath, err) } return releaseValue, nil diff --git a/internal/app/azldev/core/sources/release_test.go b/internal/app/azldev/core/sources/release_test.go index 13a0444ce..9eb2a6bde 100644 --- a/internal/app/azldev/core/sources/release_test.go +++ b/internal/app/azldev/core/sources/release_test.go @@ -108,6 +108,12 @@ func TestGetReleaseTagValue(t *testing.T) { {"static with dist", makeSpec("1%{?dist}"), "1%{?dist}", false}, {"autorelease", makeSpec("%autorelease"), "%autorelease", false}, {"braced autorelease", makeSpec("%{autorelease}"), "%{autorelease}", false}, + { + "duplicate and conditional releases use last lexical value", + "Name: test-package\n%if 0\nRelease: 1\n%else\nRelease: 2\n%endif\n", + "2", false, + }, + {"empty release is present", "Name: test-package\nRelease:\n", "", false}, {"no release tag", "Name: test-package\nVersion: 1.0.0\nSummary: Test\n", "", true}, } { t.Run(testCase.name, func(t *testing.T) { diff --git a/internal/app/azldev/core/sources/sourceprep.go b/internal/app/azldev/core/sources/sourceprep.go index 4d32c9834..e4d9edec4 100644 --- a/internal/app/azldev/core/sources/sourceprep.go +++ b/internal/app/azldev/core/sources/sourceprep.go @@ -1339,8 +1339,9 @@ func generateFileHeaderOverlay() []projectconfig.ComponentOverlay { } // synthesizeCheckSkipOverlays generates overlays to disable the %check section if configured. -// When check.skip is true, it prepends an 'exit 0' to the %check section with a comment -// explaining why the section was disabled. +// When check.skip is true, it prepends an 'exit 0' to every %check section in the spec with +// a comment explaining why the section was disabled. Uses [ComponentOverlayPrependAllSpecLines] +// to handle specs that contain multiple %check sections gated by different conditionals. func synthesizeCheckSkipOverlays(checkConfig projectconfig.CheckConfig) []projectconfig.ComponentOverlay { if !checkConfig.Skip { return nil @@ -1348,7 +1349,7 @@ func synthesizeCheckSkipOverlays(checkConfig projectconfig.CheckConfig) []projec return []projectconfig.ComponentOverlay{ { - Type: projectconfig.ComponentOverlayPrependSpecLines, + Type: projectconfig.ComponentOverlayPrependAllSpecLines, SectionName: "%check", Lines: []string{ "# Check section disabled: " + checkConfig.SkipReason, diff --git a/internal/app/azldev/core/sources/upstream_provenance.go b/internal/app/azldev/core/sources/upstream_provenance.go index 327269710..c609eed1b 100644 --- a/internal/app/azldev/core/sources/upstream_provenance.go +++ b/internal/app/azldev/core/sources/upstream_provenance.go @@ -6,6 +6,7 @@ package sources import ( "bytes" "context" + "errors" "fmt" "log/slog" "path/filepath" @@ -256,24 +257,26 @@ func parseSpecVersionRelease(fs opctx.FS, specPath string) (version, release str return "", "", fmt.Errorf("failed to parse spec %#q:\n%w", specPath, err) } - // VisitTagsPackage("") iterates tags in the base (unnamed) package, where - // Name/Version/Release live for a well-formed spec. - visitErr := parsed.VisitTagsPackage("", func(tagLine *spec.TagLine, _ *spec.Context) error { - switch strings.ToLower(tagLine.Tag) { - case "version": - if version == "" { - version = strings.TrimSpace(tagLine.Value) - } - case "release": - if release == "" { - release = strings.TrimSpace(tagLine.Value) - } - } + version, err = parsed.GetFirstNonEmptyTag("", "Version") + if err != nil && !errors.Is(err, spec.ErrNoSuchTag) { + return "", "", fmt.Errorf("failed to read Version tag in spec %#q:\n%w", specPath, err) + } + + if errors.Is(err, spec.ErrNoSuchTag) { + version = "" + } else { + version = strings.TrimSpace(version) + } + + release, err = parsed.GetFirstNonEmptyTag("", "Release") + if err != nil && !errors.Is(err, spec.ErrNoSuchTag) { + return "", "", fmt.Errorf("failed to read Release tag in spec %#q:\n%w", specPath, err) + } - return nil - }) - if visitErr != nil { - return "", "", fmt.Errorf("failed to scan spec tags in %#q:\n%w", specPath, visitErr) + if errors.Is(err, spec.ErrNoSuchTag) { + release = "" + } else { + release = strings.TrimSpace(release) } return version, release, nil diff --git a/internal/app/azldev/core/sources/upstream_provenance_internal_test.go b/internal/app/azldev/core/sources/upstream_provenance_internal_test.go index f62c37fa8..01806a017 100644 --- a/internal/app/azldev/core/sources/upstream_provenance_internal_test.go +++ b/internal/app/azldev/core/sources/upstream_provenance_internal_test.go @@ -79,6 +79,20 @@ func TestParseSpecVersionRelease(t *testing.T) { assert.Equal(t, "5%{?dist}", release, "release is captured verbatim, dist is expanded later") } +func TestParseSpecVersionReleaseUsesFirstNonEmptyTags(t *testing.T) { + memFS := afero.NewMemMapFs() + require.NoError(t, fileutils.MkdirAll(memFS, provenanceWorkDir)) + path := filepath.Join(provenanceWorkDir, "conditional.spec") + content := "Name: conditional\nVersion:\nRelease:\n%if 0\nVersion: 1\nRelease: 1\n" + + "%else\nVersion: 2\nRelease: 2\n%endif\n" + require.NoError(t, fileutils.WriteFile(memFS, path, []byte(content), fileperms.PublicFile)) + + version, release, err := parseSpecVersionRelease(memFS, path) + require.NoError(t, err) + assert.Equal(t, "1", version) + assert.Equal(t, "1", release) +} + func TestParseSpecVersionRelease_MissingFile(t *testing.T) { _, _, err := parseSpecVersionRelease(afero.NewMemMapFs(), "/does-not-exist.spec") require.Error(t, err) diff --git a/internal/projectconfig/overlay.go b/internal/projectconfig/overlay.go index e3827798e..42bf62982 100644 --- a/internal/projectconfig/overlay.go +++ b/internal/projectconfig/overlay.go @@ -20,7 +20,7 @@ import ( //nolint:recvcheck // HashInclude needs a value receiver for hashstructure; all other methods use pointer receivers. type ComponentOverlay struct { // The type of overlay to apply. - Type ComponentOverlayType `toml:"type" json:"type" validate:"required" jsonschema:"enum=spec-add-tag,enum=spec-insert-tag,enum=spec-set-tag,enum=spec-update-tag,enum=spec-remove-tag,enum=spec-prepend-lines,enum=spec-append-lines,enum=spec-search-replace,enum=spec-remove-section,enum=spec-remove-subpackage,enum=patch-add,enum=patch-remove,enum=file-prepend-lines,enum=file-search-replace,enum=file-add,enum=file-remove,enum=file-rename,title=Overlay type,description=The type of overlay to apply"` + Type ComponentOverlayType `toml:"type" json:"type" validate:"required" jsonschema:"enum=spec-add-tag,enum=spec-insert-tag,enum=spec-set-tag,enum=spec-update-tag,enum=spec-remove-tag,enum=spec-prepend-lines,enum=spec-prepend-all-lines,enum=spec-append-lines,enum=spec-search-replace,enum=spec-remove-section,enum=spec-remove-subpackage,enum=patch-add,enum=patch-remove,enum=file-prepend-lines,enum=file-search-replace,enum=file-add,enum=file-remove,enum=file-rename,title=Overlay type,description=The type of overlay to apply"` // Human readable description of overlay; primarily present to document the need for the change. Description string `toml:"description,omitempty" json:"description,omitempty" jsonschema:"title=Description,description=Human readable description of overlay" fingerprint:"-"` @@ -131,6 +131,7 @@ func (c *ComponentOverlay) ModifiesSpec() bool { c.Type == ComponentOverlayUpdateSpecTag || c.Type == ComponentOverlayRemoveSpecTag || c.Type == ComponentOverlayPrependSpecLines || + c.Type == ComponentOverlayPrependAllSpecLines || c.Type == ComponentOverlayAppendSpecLines || c.Type == ComponentOverlaySearchAndReplaceInSpec || c.Type == ComponentOverlayRemoveSection || @@ -260,6 +261,8 @@ const ( // ComponentOverlayPrependSpecLines is an overlay that prepends lines to a section in a spec; fails if the section // doesn't exist. ComponentOverlayPrependSpecLines ComponentOverlayType = "spec-prepend-lines" + // ComponentOverlayPrependAllSpecLines prepends lines to every matching section. + ComponentOverlayPrependAllSpecLines ComponentOverlayType = "spec-prepend-all-lines" // ComponentOverlayAppendSpecLines is an overlay that appends lines to a section in a spec; fails if the section // doesn't exist. ComponentOverlayAppendSpecLines ComponentOverlayType = "spec-append-lines" @@ -355,8 +358,8 @@ func (c *ComponentOverlay) validateRequiredFields(desc string) error { case ComponentOverlayAddSpecTag, ComponentOverlayInsertSpecTag, ComponentOverlaySetSpecTag, ComponentOverlayUpdateSpecTag, ComponentOverlayRemoveSpecTag: return c.validateSpecTagFields(desc) - case ComponentOverlayPrependSpecLines, ComponentOverlayAppendSpecLines: - return c.validateSpecLineOverlay(desc) + case ComponentOverlayPrependSpecLines, ComponentOverlayPrependAllSpecLines, ComponentOverlayAppendSpecLines: + return c.validateSpecLineOverlayType(desc) case ComponentOverlaySearchAndReplaceInSpec: return c.validateSpecSearchReplaceOverlay(desc) case ComponentOverlayPrependLinesToFile, ComponentOverlaySearchAndReplaceInFile: @@ -378,6 +381,18 @@ func (c *ComponentOverlay) validateRequiredFields(desc string) error { } } +func (c *ComponentOverlay) validateSpecLineOverlayType(desc string) error { + if err := c.validateSpecLineOverlay(desc); err != nil { + return err + } + + if c.Type == ComponentOverlayPrependAllSpecLines && c.SectionName == "" { + return fmt.Errorf("overlay type %#q requires %#q field: %s", c.Type, "section", desc) + } + + return nil +} + func (c *ComponentOverlay) validateSpecTagFields(desc string) error { if c.Tag == "" { return fmt.Errorf("overlay type %#q requires %#q field: %s", c.Type, "tag", desc) diff --git a/internal/projectconfig/overlay_test.go b/internal/projectconfig/overlay_test.go index 1313562fe..c054dcfe9 100644 --- a/internal/projectconfig/overlay_test.go +++ b/internal/projectconfig/overlay_test.go @@ -138,6 +138,25 @@ func TestComponentOverlay_Validate(t *testing.T) { errorExpected: true, errorContains: "lines", }, + // spec-prepend-all-lines tests + { + name: "spec-prepend-all-lines valid", + overlay: projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayPrependAllSpecLines, + SectionName: "%check", + Lines: []string{"exit 0"}, + }, + errorExpected: false, + }, + { + name: "spec-prepend-all-lines missing section", + overlay: projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayPrependAllSpecLines, + Lines: []string{"exit 0"}, + }, + errorExpected: true, + errorContains: "section", + }, // spec-append-lines tests { name: "spec-append-lines valid", diff --git a/internal/rpm/spec/edit.go b/internal/rpm/spec/edit.go index 0fdd056c3..b1f701df0 100644 --- a/internal/rpm/spec/edit.go +++ b/internal/rpm/spec/edit.go @@ -26,12 +26,33 @@ var ErrSectionNotFound = errors.New("section not found") // breaking the conditional nesting structure. var ErrConditionalSpansSections = errors.New("conditional block spans across section boundaries") +// ErrUnsafeMacroHoist is returned when removing a section would require moving +// a macro definition whose RPM scope or evaluation order cannot be preserved. +var ErrUnsafeMacroHoist = errors.New("unsafe macro hoist") + // ErrPatternNotFound is returned when a search pattern does not match any content in the spec. var ErrPatternNotFound = errors.New("pattern not found") -// SetTag sets the value of the given tag in the spec, under the specified package. It first -// attempts to update the first instance of the tag found in the spec; if no such tag exists, -// a new tag is added under the given package. +// visitLinearSpecLines reports each non-section-header line with the section +// that RPM assigns to it. Section ownership is lexical: a section declared +// inside a conditional remains active after its %endif, so this deliberately +// does not follow the structural tree's branch nesting. +func visitLinearSpecLines(lines []string, visit func(lineIdx int, secName, secPkg string)) { + secName, secPkg := "", "" + + for lineIdx, line := range lines { + if isSectionHeaderLine(line) { + secName, secPkg = getSectionNameAndPackageFromHeader(line) + + continue + } + + visit(lineIdx, secName, secPkg) + } +} + +// SetTag sets the value of the given tag in the spec, under the specified +// package. It updates matching instances, or adds a tag when none exists. func (s *Spec) SetTag(packageName string, tag string, value string) (err error) { err = s.UpdateExistingTag(packageName, tag, value) if err == nil { @@ -45,33 +66,38 @@ func (s *Spec) SetTag(packageName string, tag string, value string) (err error) return err } -// UpdateExistingTag looks for the first instance of the named tag in the given package; if it -// finds such a tag instance, it replaces its value with the provided value. If no such tag -// exists, it returns an error. +// UpdateExistingTag updates every instance of the named tag in the given +// package. If no such tag exists, it returns an error. func (s *Spec) UpdateExistingTag(packageName string, tag string, value string) (err error) { slog.Debug("Updating tag in spec", "package", packageName, "tag", tag, "newValue", value) tagToCompareAgainst := strings.ToLower(tag) - var updated bool + updated := false - err = s.VisitTagsPackage(packageName, func(tagLine *TagLine, ctx *Context) error { - if strings.ToLower(tagLine.Tag) != tagToCompareAgainst { - return nil + rawLines := append([]string(nil), s.rawLines...) + + visitLinearSpecLines(rawLines, func(lineIdx int, secName, secPkg string) { + if secPkg != packageName || !isTagBearingSection(secName) { + return } - ctx.ReplaceLine(fmt.Sprintf("%s: %s", tag, value)) + parsedTag, _, isTag := parseTagLine(rawLines[lineIdx]) + if !isTag || strings.ToLower(parsedTag) != tagToCompareAgainst { + return + } + rawLines[lineIdx] = fmt.Sprintf("%s: %s", tag, value) updated = true - - return nil }) if !updated { return fmt.Errorf("tag %#q not found in spec:\n%w", tag, ErrNoSuchTag) } - return err + s.rawLines = rawLines + + return nil } // RemoveTag removes all instances of the given tag from the spec, under the specified @@ -105,39 +131,72 @@ func (s *Spec) RemoveTag(packageName string, tag string, value string) (err erro return nil } -// VisitTags iterates over all tag lines across all packages, calling the visitor function -// for each one. The visitor receives the parsed [TagLine] and the mutation [Context]. -func (s *Spec) VisitTags(visitor func(tagLine *TagLine, ctx *Context) error) error { - return s.Visit(func(ctx *Context) error { - if ctx.Target.TargetType != SectionLineTarget { - return nil - } +// GetTag returns the value of the first instance of the named tag in the given package. +// Returns [ErrNoSuchTag] if the tag does not exist. +func (s *Spec) GetTag(packageName string, tag string) (string, error) { + return s.getTag(packageName, tag, true) +} - if ctx.Target.Line.Parsed.GetType() != Tag { - return nil - } +// GetLastTag returns the value of the last lexical instance of the named tag +// in the given package. It preserves the historical Release-tag selection +// behavior for callers that need it. Returns [ErrNoSuchTag] if the tag does +// not exist. +func (s *Spec) GetLastTag(packageName string, tag string) (string, error) { + return s.getTag(packageName, tag, false) +} - tagLine, isTagLine := ctx.Target.Line.Parsed.(*TagLine) - if !isTagLine { - return nil +// GetFirstNonEmptyTag returns the first lexical instance of tag in packageName +// whose value is not empty. Returns [ErrNoSuchTag] when no such tag exists. +func (s *Spec) GetFirstNonEmptyTag(packageName string, tag string) (string, error) { + tagToCompareAgainst := strings.ToLower(tag) + + var foundValue string + + visitLinearSpecLines(s.rawLines, func(lineIdx int, secName, secPkg string) { + if foundValue != "" || secPkg != packageName || !isTagBearingSection(secName) { + return } - return visitor(tagLine, ctx) + parsedTag, parsedValue, isTag := parseTagLine(s.rawLines[lineIdx]) + if isTag && strings.ToLower(parsedTag) == tagToCompareAgainst && strings.TrimSpace(parsedValue) != "" { + foundValue = parsedValue + } }) + + if foundValue == "" { + return "", fmt.Errorf("non-empty tag %#q not found in package %#q:\n%w", tag, packageName, ErrNoSuchTag) + } + + return foundValue, nil } -// VisitTagsPackage iterates over all tag lines in the given package, calling the visitor -// function for each one. The visitor receives the parsed [TagLine] and the mutation [Context]. -// This extracts the common target-type / package / tag-type filtering that many tag-oriented -// methods need. -func (s *Spec) VisitTagsPackage(packageName string, visitor func(tagLine *TagLine, ctx *Context) error) error { - return s.VisitTags(func(tagLine *TagLine, ctx *Context) error { - if ctx.CurrentSection.Package != packageName { - return nil +func (s *Spec) getTag(packageName string, tag string, first bool) (string, error) { + tagToCompareAgainst := strings.ToLower(tag) + + var ( + foundValue string + found bool + ) + + visitLinearSpecLines(s.rawLines, func(lineIdx int, secName, secPkg string) { + if (first && found) || secPkg != packageName || !isTagBearingSection(secName) { + return } - return visitor(tagLine, ctx) + parsedTag, parsedValue, isTag := parseTagLine(s.rawLines[lineIdx]) + if !isTag || strings.ToLower(parsedTag) != tagToCompareAgainst { + return + } + + foundValue = parsedValue + found = true }) + + if !found { + return "", fmt.Errorf("tag %#q not found in package %#q:\n%w", tag, packageName, ErrNoSuchTag) + } + + return foundValue, nil } // RemoveTagsMatching removes all tags in the given package for which the provided matcher @@ -146,19 +205,33 @@ func (s *Spec) VisitTagsPackage(packageName string, visitor func(tagLine *TagLin func (s *Spec) RemoveTagsMatching(packageName string, matcher func(tag, value string) bool) (int, error) { removed := 0 - err := s.VisitTagsPackage(packageName, func(tagLine *TagLine, ctx *Context) error { - if !matcher(tagLine.Tag, tagLine.Value) { - return nil + remove := make([]bool, len(s.rawLines)) + visitLinearSpecLines(s.rawLines, func(lineIdx int, secName, secPkg string) { + if secPkg != packageName || !isTagBearingSection(secName) { + return } - ctx.RemoveLine() + parsedTag, parsedValue, isTag := parseTagLine(s.rawLines[lineIdx]) + if isTag && matcher(parsedTag, parsedValue) { + remove[lineIdx] = true + removed++ + } + }) - removed++ + if removed == 0 { + return 0, nil + } - return nil - }) + rawLines := make([]string, 0, len(s.rawLines)-removed) + for lineIdx, line := range s.rawLines { + if !remove[lineIdx] { + rawLines = append(rawLines, line) + } + } - return removed, err + s.rawLines = rawLines + + return removed, nil } // AddTag adds the given tag to the spec, under the specified package (or globally if @@ -172,12 +245,7 @@ func (s *Spec) RemoveTagsMatching(packageName string, matcher func(tag, value st func (s *Spec) AddTag(packageName string, tag string, value string) (err error) { slog.Debug("Adding tag to spec", "package", packageName, "tag", tag, "value", value) - sectionName := "" - if packageName != "" { - sectionName = "%package" - } - - return s.AppendLinesToSection(sectionName, packageName, []string{fmt.Sprintf("%s: %s", tag, value)}) + return s.insertLinearTag(packageName, tag, value, false) } // tagFamily returns the "family" prefix of a tag name by stripping any trailing digits. @@ -263,7 +331,9 @@ func isConditionalBranchDirective(rawLine string) bool { // "source" family. // // If the chosen insertion point falls inside a conditional block (%if/%endif), the tag is -// placed after the closing %endif instead, so it remains unconditional. +// placed after the closing %endif when that location retains the target's lexical ownership. +// Otherwise it remains next to its anchor inside the conditional rather than being assigned +// to another package or section. // // Note: When inserting into a sub-package (non-empty packageName), the corresponding // %package section must already exist in the spec; otherwise, an [ErrSectionNotFound] @@ -271,246 +341,181 @@ func isConditionalBranchDirective(rawLine string) bool { func (s *Spec) InsertTag(packageName string, tag string, value string) error { slog.Debug("Inserting tag to spec", "package", packageName, "tag", tag, "value", value) - family := tagFamily(tag) - newLine := fmt.Sprintf("%s: %s", tag, value) - - sectionName := "" - if packageName != "" { - sectionName = "%package" - } - - result, err := s.findInsertTagPosition(sectionName, packageName, family) - if err != nil { - return err - } - - // Determine insertion point: prefer same-family, then any tag, then fall back to AddTag. - insertAfterLine := result.lastFamilyTagLineNum - if insertAfterLine < 0 { - insertAfterLine = result.lastAnyTagLineNum - } - - if insertAfterLine < 0 { - // No tags at all — fall back to AddTag behavior. - return s.AddTag(packageName, tag, value) - } - - // If the insertion point is inside a conditional block, move it forward past the - // closing %endif so the new tag doesn't become conditional. - insertAfterLine = s.skipPastConditional(insertAfterLine, result.sectionEndLineNum) - - // Insert after the found line (0-indexed, so insertAfterLine+1). - s.InsertLinesAt([]string{newLine}, insertAfterLine+1) - - return nil -} - -// insertTagScanResult holds the results of scanning a spec for a tag insertion point. -type insertTagScanResult struct { - lastFamilyTagLineNum int - lastAnyTagLineNum int - sectionEndLineNum int + return s.insertLinearTag(packageName, tag, value, true) } -// findInsertTagPosition scans the spec to find the best insertion point for a tag of the -// given family within the specified section/package. Returns the scan results or an error -// if the target section is not found. -func (s *Spec) findInsertTagPosition( - sectionName, packageName, family string, -) (insertTagScanResult, error) { - result := insertTagScanResult{ - lastFamilyTagLineNum: -1, - lastAnyTagLineNum: -1, - sectionEndLineNum: len(s.rawLines), - } - - sectionFound := false - - err := s.Visit(func(ctx *Context) error { - if ctx.Target.TargetType == SectionStartTarget { - if ctx.CurrentSection.SectName == sectionName && ctx.CurrentSection.Package == packageName { - sectionFound = true +//nolint:cyclop,gocognit,funlen // The linear scan deliberately handles each tag-placement case together. +func (s *Spec) insertLinearTag(packageName, tag, value string, preferFamily bool) error { + targetSection := "" + if packageName != "" { + targetSection = packageSectionName + } + + foundSection := packageName == "" + lastOwnedLine := -1 + lastAnyTag := -1 + lastFamilyTag := -1 + secName, secPkg := "", "" + owners := make([]struct{ section, pkg string }, len(s.rawLines)) + + for lineIdx, line := range s.rawLines { + if isSectionHeaderLine(line) { + secName, secPkg = getSectionNameAndPackageFromHeader(line) + if secName == targetSection && secPkg == packageName { + foundSection = true + lastOwnedLine = lineIdx } - } - if ctx.Target.TargetType == SectionEndTarget { - if ctx.CurrentSection.SectName == sectionName && ctx.CurrentSection.Package == packageName { - result.sectionEndLineNum = ctx.CurrentLineNum - } + continue } - if ctx.Target.TargetType != SectionLineTarget { - return nil - } + owners[lineIdx] = struct{ section, pkg string }{secName, secPkg} - if ctx.CurrentSection.SectName != sectionName || ctx.CurrentSection.Package != packageName { - return nil + if secName != targetSection || secPkg != packageName { + continue } - if ctx.Target.Line.Parsed.GetType() != Tag { - return nil - } + lastOwnedLine = lineIdx - tagLine, ok := ctx.Target.Line.Parsed.(*TagLine) - if !ok { - return nil + if !isTagBearingSection(secName) { + continue } - result.lastAnyTagLineNum = ctx.CurrentLineNum - - if tagFamily(tagLine.Tag) == family { - result.lastFamilyTagLineNum = ctx.CurrentLineNum + parsedTag, _, isTag := parseTagLine(line) + if !isTag { + continue } - return nil - }) - if err != nil { - return result, fmt.Errorf("failed to scan spec for tag insertion point:\n%w", err) + lastAnyTag = lineIdx + if tagFamily(parsedTag) == tagFamily(tag) { + lastFamilyTag = lineIdx + } } - if !sectionFound { - return result, fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) + if !foundSection { + return fmt.Errorf("section %#q (package=%#q) not found:\n%w", targetSection, packageName, ErrSectionNotFound) } - return result, nil -} + insertAfter := lastOwnedLine + if preferFamily && lastAnyTag >= 0 { + insertAfter = lastAnyTag + if lastFamilyTag >= 0 { + insertAfter = lastFamilyTag + } -// skipPastConditional checks whether lineNum falls inside a conditional block by computing -// the conditional nesting depth from the start of the file up to that line. If depth > 0, -// it scans forward to find the %endif that brings depth back to 0 and returns that line -// number. Otherwise it returns lineNum unchanged. -func (s *Spec) skipPastConditional(lineNum int, sectionEnd int) int { - // Compute conditional depth at the insertion point by scanning from the start. - depth := 0 - for i := 0; i <= lineNum && i < len(s.rawLines); i++ { - depth += conditionalDepthChange(s.rawLines[i]) - } + pairs, err := collectConditionalPairs(s.rawLines) + if err != nil { + return fmt.Errorf("parsing conditional structure:\n%w", err) + } - if depth <= 0 { - return lineNum - } + unconditionalAfter := insertAfter + for _, pair := range pairs { + if pair.ifLine < insertAfter && insertAfter < pair.endifLine && pair.endifLine > insertAfter { + unconditionalAfter = pair.endifLine + } + } - // Scan forward to find the %endif that closes the conditional. - for i := lineNum + 1; i < sectionEnd && i < len(s.rawLines); i++ { - depth += conditionalDepthChange(s.rawLines[i]) - if depth <= 0 { - return i + if owners[unconditionalAfter].section == targetSection && + owners[unconditionalAfter].pkg == packageName { + insertAfter = unconditionalAfter } } - // Could not find a closing %endif within the section; return the original position. - return lineNum + newLine := fmt.Sprintf("%s: %s", tag, value) + if insertAfter < 0 { + s.rawLines = append([]string{newLine}, s.rawLines...) + } else { + s.rawLines = append(s.rawLines, "") + copy(s.rawLines[insertAfter+2:], s.rawLines[insertAfter+1:]) + s.rawLines[insertAfter+1] = newLine + } + + return nil } -// PrependLines prepends the given lines to the very top of the spec file. This is a -// whole-file edit, distinct from section-targeted editing, which applies within a specific -// section rather than to the raw file contents. +// PrependLines prepends the given lines to the very top of the spec file. func (s *Spec) PrependLines(lines []string) { slog.Debug("Prepending lines to spec file", "lines", lines) - s.rawLines = append(append([]string{}, lines...), s.rawLines...) } -// AppendLines appends the given lines at the very bottom of the spec file. This is a -// whole-file edit, distinct from section-targeted editing, which applies within a specific -// section rather than to the raw file contents. +// AppendLines appends the given lines at the very bottom of the spec file. func (s *Spec) AppendLines(lines []string) { slog.Debug("Appending lines to spec file", "lines", lines) - s.rawLines = append(s.rawLines, lines...) } -// PrependLinesToSection prepends the given lines to the start of the specified section, placing -// them just after the section header (or at the top of the file in the global section). An error -// is returned if the identified section cannot be found in the spec. +// PrependLinesToSection prepends the given lines to the start of the first section matching +// the specified name and package, placing them just after the section header (or at the top +// of the file in the global section). An error is returned if the identified section cannot +// be found in the spec. func (s *Spec) PrependLinesToSection(sectionName, packageName string, lines []string) (err error) { slog.Debug("Prepending lines to spec", "section", sectionName, "package", packageName, "lines", lines) - var updated bool - - err = s.Visit(func(ctx *Context) error { - // Make sure this is a section start. - if ctx.Target.TargetType != SectionStartTarget { - return nil + return s.mutateTree(func(tree *specTree) error { + sect := tree.Section(sectionName, packageName) + if sect == nil { + return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) } - // Make sure section name matches. - if ctx.CurrentSection.SectName != sectionName { - return nil - } + sect.PrependLines(lines) - // Make sure package name matches. - if ctx.CurrentSection.Package != packageName { - return nil - } + return nil + }) +} + +// PrependLinesToAllSections prepends the given lines to the start of every section matching +// the specified name and package, placing them just after each section header. This is useful +// when a spec contains multiple sections with the same name (e.g., two %check sections gated +// by different conditionals) and all of them need the same modification. +// An error is returned if no matching section exists. +func (s *Spec) PrependLinesToAllSections(sectionName, packageName string, lines []string) (err error) { + slog.Debug("Prepending lines to all matching sections", "section", sectionName, "package", packageName, "lines", lines) - // Insert the lines. The global section doesn't have a header line, so we insert the - // lines *before* the start. For all other sections, including sub-package %package - // sections, we need to make sure we insert the lines after the header line of the - // section. - if ctx.CurrentSection.SectName == "" && ctx.CurrentSection.Package == "" { - ctx.InsertLinesBefore(lines) - } else { - ctx.InsertLinesAfter(lines) + return s.mutateTree(func(tree *specTree) error { + sections := tree.Sections(sectionName, packageName) + if len(sections) == 0 { + return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) } - // Note that we've made an update. - updated = true + for _, sect := range sections { + sect.PrependLines(lines) + } return nil }) - - if !updated { - return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) - } - - return err } // AppendLinesToSection appends the given lines at the end of the specified section, placing -// them just after the current last line of the section. An error is returned if the identified -// section cannot be found in the spec. +// them just after the current last line of the section's content. When a conditional block +// (%if/%endif) straddles the section boundary, the appended lines are placed before the +// conditional — they do not land inside it. +// +// An error is returned if the identified section cannot be found in the spec. func (s *Spec) AppendLinesToSection(sectionName, packageName string, lines []string) (err error) { slog.Debug("Appending lines to spec", "section", sectionName, "package", packageName, "lines", lines) - var updated bool - - err = s.Visit(func(ctx *Context) error { - // Make sure this is a section start. - if ctx.Target.TargetType != SectionEndTarget { - return nil - } - - // Make sure section name matches. - if ctx.CurrentSection.SectName != sectionName { - return nil + return s.mutateTree(func(tree *specTree) error { + sect := tree.Section(sectionName, packageName) + if sect == nil { + return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) } - // Make sure package name matches. - if ctx.CurrentSection.Package != packageName { - return nil - } - - // Insert the line. - ctx.InsertLinesBefore(lines) - - // Note that we've made an update. - updated = true + sect.AppendLines(lines) return nil }) - - if !updated { - return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) - } - - return err } // SearchAndReplace performs a regex-based search-and-replace against all lines in the specified // section. If `sectionName` is empty, the operation acts against all sections. If no matches were // found to replace, an error is returned. The replacement is performed literally; regex capture // group references like $1 are not expanded. +// +// Unlike [specTree.VisitAllLines] (which skips structural lines), this function +// walks every line in the tree including macro definitions (%define/%global) and +// conditional directives (%if/%else/%endif), so patterns that match those lines +// are found correctly. func (s *Spec) SearchAndReplace(sectionName, packageName, regex, replacement string) (err error) { slog.Debug("Searching and replacing in spec", "section", sectionName, @@ -525,39 +530,43 @@ func (s *Spec) SearchAndReplace(sectionName, packageName, regex, replacement str return fmt.Errorf("failed to compile regex %#q:\n%w", regex, err) } - var updated bool + if sectionName == "" && packageName == "" { + updated := false - err = s.Visit(func(ctx *Context) error { - // Make sure this is a section line. - if ctx.Target.TargetType != SectionLineTarget { - return nil - } + rawLines := append([]string(nil), s.rawLines...) - // Make sure section name matches (or was omitted). - if sectionName != "" && ctx.CurrentSection.SectName != sectionName { - return nil + for lineIdx, line := range rawLines { + if replacementLine := compiledRegex.ReplaceAllLiteralString(line, replacement); replacementLine != line { + rawLines[lineIdx] = replacementLine + updated = true + } } - // Make sure package name matches (or was omitted). - if packageName != "" && ctx.CurrentSection.Package != packageName { - return nil + if !updated { + return fmt.Errorf( + "pattern %#q not found (section=%#q, package=%#q):\n%w", + regex, sectionName, packageName, ErrPatternNotFound, + ) } - // Get the line. - line := ctx.Target.Line.Text + s.rawLines = rawLines - // Try to replace. If no replacements were made, return. - updatedLine := compiledRegex.ReplaceAllLiteralString(line, replacement) - if line == updatedLine { - return nil - } + return nil + } - ctx.ReplaceLine(updatedLine) + rawLines := append([]string(nil), s.rawLines...) + updated := false - // Note that we've made an update. - updated = true + visitLinearSpecLines(rawLines, func(lineIdx int, secName, secPkg string) { + if (sectionName != "" && sectionName != secName) || + (packageName != "" && packageName != secPkg) { + return + } - return nil + if newLine := compiledRegex.ReplaceAllLiteralString(rawLines[lineIdx], replacement); newLine != rawLines[lineIdx] { + rawLines[lineIdx] = newLine + updated = true + } }) if !updated { @@ -567,7 +576,9 @@ func (s *Spec) SearchAndReplace(sectionName, packageName, regex, replacement str ) } - return err + s.rawLines = rawLines + + return nil } // AddChangelogEntry adds a changelog entry to the spec's changelog section. An error is returned if @@ -576,42 +587,26 @@ func (s *Spec) AddChangelogEntry(user, email, version, release string, time time slog.Debug("Adding changelog entry to spec", "user", user, "email", email, "version", version, "release", release, "details", details) - var updated bool - - err = s.Visit(func(ctx *Context) error { - // Make sure we're in the right section. - if ctx.Target.TargetType != SectionStartTarget { - return nil - } + formattedDate := time.Format("Mon Jan 02 2006") + header := fmt.Sprintf("* %s %s <%s> - %s-%s", formattedDate, user, email, version, release) - if ctx.CurrentSection.SectName != "%changelog" { - return nil - } + lines := []string{header} + for _, detail := range details { + lines = append(lines, "- "+detail) + } - // Insert an entry. - formattedDate := time.Format("Mon Jan 02 2006") - header := fmt.Sprintf("* %s %s <%s> - %s-%s", formattedDate, user, email, version, release) + lines = append(lines, "") - lines := []string{header} - for _, detail := range details { - lines = append(lines, "- "+detail) + return s.mutateTree(func(tree *specTree) error { + sect := tree.Section("%changelog", "") + if sect == nil { + return errors.New("existing changelog section could not be found") } - lines = append(lines, "") - - ctx.InsertLinesAfter(lines) - - // Note that we've made an update. - updated = true + sect.PrependLines(lines) return nil }) - - if !updated { - return errors.New("existing changelog section could not be found") - } - - return err } // ParsePatchTagNumber checks if the given tag name is a PatchN tag (case-insensitive) @@ -636,10 +631,8 @@ func ParsePatchTagNumber(tag string) (int, bool) { func (s *Spec) HasSection(sectionName string) (bool, error) { var found bool - err := s.Visit(func(ctx *Context) error { - if ctx.Target.TargetType == SectionStartTarget && ctx.CurrentSection.SectName == sectionName { - found = true - } + err := s.inspectTree(func(tree *specTree) error { + found = tree.HasSection(sectionName) return nil }) @@ -711,23 +704,34 @@ func (s *Spec) RemovePatchEntry(pattern string) error { func (s *Spec) removePatchTagsMatching(pattern string) (int, error) { removed := 0 - err := s.VisitTags(func(tagLine *TagLine, ctx *Context) error { - if _, ok := ParsePatchTagNumber(tagLine.Tag); !ok { - return nil - } + err := s.mutateTree(func(tree *specTree) error { + return tree.VisitAllLines(func(secName, _ string, line *lineHandle) error { + if !isTagBearingSection(secName) { + return nil + } - matched, matchErr := doublestar.Match(pattern, tagLine.Value) - if matchErr != nil { - return fmt.Errorf("failed to match glob pattern %#q against %#q:\n%w", pattern, tagLine.Value, matchErr) - } + parsedTag, parsedValue, isTag := parseTagLine(line.Text) + if !isTag { + return nil + } - if matched { - ctx.RemoveLine() + if _, ok := ParsePatchTagNumber(parsedTag); !ok { + return nil + } - removed++ - } + matched, matchErr := doublestar.Match(pattern, parsedValue) + if matchErr != nil { + return fmt.Errorf("failed to match glob pattern %#q against %#q:\n%w", pattern, parsedValue, matchErr) + } - return nil + if matched { + line.Remove() + + removed++ + } + + return nil + }) }) return removed, err @@ -738,32 +742,31 @@ func (s *Spec) removePatchTagsMatching(pattern string) (int, error) { func (s *Spec) removePatchlistEntriesMatching(pattern string) (int, error) { removed := 0 - err := s.Visit(func(ctx *Context) error { - if ctx.Target.TargetType != SectionLineTarget { + err := s.mutateTree(func(tree *specTree) error { + sect := tree.Section("%patchlist", "") + if sect == nil { return nil } - if ctx.CurrentSection.SectName != "%patchlist" { - return nil - } - - line := strings.TrimSpace(ctx.Target.Line.Text) - if line == "" { - return nil - } + return sect.VisitLines(func(line *lineHandle) error { + trimmed := strings.TrimSpace(line.Text) + if trimmed == "" { + return nil + } - matched, err := doublestar.Match(pattern, line) - if err != nil { - return fmt.Errorf("failed to match glob pattern %#q against %#q:\n%w", pattern, line, err) - } + matched, matchErr := doublestar.Match(pattern, trimmed) + if matchErr != nil { + return fmt.Errorf("failed to match glob pattern %#q against %#q:\n%w", pattern, trimmed, matchErr) + } - if matched { - ctx.RemoveLine() + if matched { + line.Remove() - removed++ - } + removed++ + } - return nil + return nil + }) }) return removed, err @@ -778,17 +781,28 @@ func (s *Spec) GetHighestPatchTagNumber() (int, error) { highest := -1 unnumberedCount := 0 - err := s.VisitTags(func(tagLine *TagLine, _ *Context) error { - num, isPatchTag := ParsePatchTagNumber(tagLine.Tag) - if isPatchTag && num > highest { - highest = num - } else if strings.EqualFold(tagLine.Tag, "patch") { - // Bare "Patch:" with no numeric suffix — RPM auto-numbers these - // sequentially starting from 0. - unnumberedCount++ - } + err := s.inspectTree(func(tree *specTree) error { + return tree.VisitAllLines(func(secName, _ string, line *lineHandle) error { + if !isTagBearingSection(secName) { + return nil + } - return nil + parsedTag, _, isTag := parseTagLine(line.Text) + if !isTag { + return nil + } + + num, isPatchTag := ParsePatchTagNumber(parsedTag) + if isPatchTag && num > highest { + highest = num + } else if strings.EqualFold(parsedTag, "patch") { + // Bare "Patch:" with no numeric suffix — RPM auto-numbers these + // sequentially starting from 0. + unnumberedCount++ + } + + return nil + }) }) // Unnumbered patches occupy slots 0..unnumberedCount-1. @@ -814,20 +828,14 @@ func (s *Spec) RemoveSection(sectionName, packageName string) error { return errors.New("cannot remove the global/preamble section") } - ranges, err := s.collectSectionRanges(func(sn, pn string) bool { - return sn == sectionName && pn == packageName - }) - if err != nil { - return fmt.Errorf("failed to scan spec for section %#q (package=%#q):\n%w", sectionName, packageName, err) - } - - if len(ranges) == 0 { - return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) - } - - s.removeRanges(ranges) + return s.mutateTree(func(tree *specTree) error { + matches := tree.Sections(sectionName, packageName) + if len(matches) == 0 { + return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) + } - return nil + return tree.RemoveSections(matches) + }) } // RemoveSubpackage removes every section in the spec that is associated with the given @@ -859,98 +867,14 @@ func (s *Spec) RemoveSubpackage(packageName string) error { return errors.New("cannot remove sub-package with empty name") } - ranges, err := s.collectSectionRanges(func(_, pn string) bool { - return pn == packageName - }) - if err != nil { - return fmt.Errorf("failed to scan spec for sub-package %#q:\n%w", packageName, err) - } - - if len(ranges) == 0 { - return fmt.Errorf("sub-package %#q not found:\n%w", packageName, ErrSectionNotFound) - } - - s.removeRanges(ranges) - - return nil -} - -// sectionLineRange identifies a half-open `[start, end)` range of raw line numbers -// covering one section, from its header line through (but not including) the start -// of the next section. -type sectionLineRange struct { - start int - end int -} - -// collectSectionRanges walks the spec and returns one [sectionLineRange] for every -// section whose `(sectName, packageName)` pair satisfies the predicate, in the order -// they appear in the spec. -// -// Each returned range is adjusted to maintain conditional balance: if a range would -// include trailing `%if` or `%endif` lines that create a nesting imbalance, those -// lines are trimmed from the range so that removing the range does not break the -// spec's conditional structure. If a conditional block is interleaved with section -// content in a way that cannot be resolved by trimming, an [ErrConditionalSpansSections] -// error is returned. -func (s *Spec) collectSectionRanges( - matches func(sectName, packageName string) bool, -) ([]sectionLineRange, error) { - var ( - ranges []sectionLineRange - curStart = -1 - ) - - err := s.Visit(func(ctx *Context) error { - matched := matches(ctx.CurrentSection.SectName, ctx.CurrentSection.Package) - - //nolint:exhaustive // We intentionally only react to section boundaries. - switch ctx.Target.TargetType { - case SectionStartTarget: - if matched { - curStart = ctx.CurrentLineNum - } - case SectionEndTarget: - if matched && curStart >= 0 { - ranges = append(ranges, sectionLineRange{start: curStart, end: ctx.CurrentLineNum}) - curStart = -1 - } + return s.mutateTree(func(tree *specTree) error { + matches := tree.SectionsByPackage(packageName) + if len(matches) == 0 { + return fmt.Errorf("sub-package %#q not found:\n%w", packageName, ErrSectionNotFound) } - return nil + return tree.RemoveSections(matches) }) - - // Defensive fallback: today [Spec.Visit] always emits a trailing SectionEndTarget at - // EOF, so this branch is unreachable. We keep it so that this helper does not silently - // misbehave if that invariant ever changes (a section running to EOF would otherwise - // be silently dropped from the result). - if curStart >= 0 { - ranges = append(ranges, sectionLineRange{start: curStart, end: len(s.rawLines)}) - } - - // Skip conditional balancing when no matching ranges were found, so callers - // get the expected empty-result / not-found behavior rather than a conditional - // parse error from an unrelated part of the spec. - if len(ranges) == 0 { - return ranges, err - } - - // Balance each range to avoid breaking conditional nesting. - pairs, pairErr := collectConditionalPairs(s.rawLines) - if pairErr != nil { - return nil, fmt.Errorf("failed to parse conditional structure:\n%w", pairErr) - } - - for idx := range ranges { - balanced, balanceErr := balanceRange(ranges[idx], s.rawLines, pairs) - if balanceErr != nil { - return nil, balanceErr - } - - ranges[idx] = balanced - } - - return ranges, err } // conditionalPair represents a matched `%if`/`%endif` pair by their line numbers. @@ -960,15 +884,42 @@ type conditionalPair struct { } // collectConditionalPairs walks the raw lines and returns all matched `%if`/`%endif` -// pairs using a stack. Nested pairs are properly matched. Returns an error if there -// are unmatched `%if` or `%endif` directives. +// pairs using a stack. Nested pairs are properly matched. Lines inside +// macro definition continuations are skipped — `%if`/`%endif` that appear +// inside multi-line `%define`/`%global` bodies (e.g. `%define foo() \` … +// `%if …\` … `%endif\`) are RPM macro body text, not structural conditionals. +// However, `%if`/`%endif` inside general shell continuations (e.g. +// `configure \` … `%if …` … `%endif`) ARE structural — RPM evaluates them +// as preprocessor directives before shell interpretation. Returns an error if +// there are unmatched `%if` or `%endif` directives. func collectConditionalPairs(rawLines []string) ([]conditionalPair, error) { var ( pairs []conditionalPair stack []int ) + inMacroCont := false + braceDepth := 0 + macroStart := -1 + for lineNum, line := range rawLines { + if inMacroCont { + braceDepth = macroBraceDepthAfter(line, braceDepth) + inMacroCont = strings.HasSuffix(line, "\\") || braceDepth > 0 + + continue + } + + // Only skip continuations that start from a %define/%global line — + // those are macro body text where %if/%endif are not structural. + if _, isMacro := isMacroDefLine(line); isMacro { + macroStart = lineNum + braceDepth = macroBraceDepthAfter(line, 0) + inMacroCont = strings.HasSuffix(line, "\\") || braceDepth > 0 + + continue + } + switch conditionalDepthChange(line) { case 1: stack = append(stack, lineNum) @@ -984,154 +935,13 @@ func collectConditionalPairs(rawLines []string) ([]conditionalPair, error) { } } + if braceDepth > 0 { + return nil, fmt.Errorf("unterminated macro construct at line %d", macroStart+1) + } + if len(stack) > 0 { return nil, fmt.Errorf("unmatched %%if at line %d", stack[0]+1) } return pairs, nil } - -// balanceRange adjusts a section line range so that removing it does not leave -// unbalanced `%if`/`%endif` directives in the spec. It uses pre-computed conditional -// pairs to identify straddling conditionals — pairs where one half is inside the -// range and the other half is outside. -// -// Straddling conditional lines inside the range are excluded (the range is trimmed -// so they remain in the spec). This handles: -// - Trailing `%endif` from a wrapping conditional: excluded, leaving an empty -// `%if`/`%endif` wrapper. -// - Trailing `%if` belonging to the next section: excluded, keeping the next -// section's conditional intact. -// - Balanced pairs fully inside the range: removed along with the section content. -// -// If a straddling conditional is interleaved with real section content (not just -// other conditional directives and blank lines), an [ErrConditionalSpansSections] -// error is returned. -func balanceRange(sectionRange sectionLineRange, rawLines []string, pairs []conditionalPair) (sectionLineRange, error) { - // Find the earliest straddling line inside the range and validate that no - // straddling %if has real content after it. A pair straddles if exactly one - // of its lines falls within [sectionRange.start, sectionRange.end). - trimmed := sectionRange.end - - for _, pair := range pairs { - ifInside := pair.ifLine >= sectionRange.start && pair.ifLine < sectionRange.end - endifInside := pair.endifLine >= sectionRange.start && pair.endifLine < sectionRange.end - - if ifInside == endifInside { - // Both inside (fully contained) or both outside (irrelevant). - continue - } - - // Straddling: the line that's inside our range should be excluded. - var insideLine int - if ifInside { - insideLine = pair.ifLine - } else { - insideLine = pair.endifLine - } - - if insideLine < trimmed { - trimmed = insideLine - } - - // If the straddling line is an %if (opener inside, closer outside), - // check for real content between the %if and the range end. Such content - // would belong to this section but span into the next via the conditional. - if ifInside { - if err := validateNoContentAfter(pair.ifLine, sectionRange.end, rawLines); err != nil { - return sectionRange, fmt.Errorf( - "section at lines %d-%d has a conditional block that spans into the next section; "+ - "use a spec-search-replace overlay to adjust conditionals before removing:\n%w", - sectionRange.start+1, sectionRange.end, ErrConditionalSpansSections, - ) - } - } - } - - // Check for %else/%elif branch directives that would be broken by the removal. - if err := validateNoBranchDirectivesInExternalConditional(sectionRange, rawLines, pairs); err != nil { - return sectionRange, err - } - - if trimmed == sectionRange.end { - // No straddling pairs — range is already balanced. - return sectionRange, nil - } - - // Validate: the trimmed zone [trimmed, sectionRange.end) will remain in the spec. - // If it contains real section content (not just conditional directives and blanks), - // we'd be leaving behind part of the section the caller asked to remove. - if err := validateNoContentAfter(trimmed-1, sectionRange.end, rawLines); err != nil { - return sectionRange, fmt.Errorf( - "section at lines %d-%d has a conditional block that spans into the next section; "+ - "use a spec-search-replace overlay to adjust conditionals before removing:\n%w", - sectionRange.start+1, sectionRange.end, ErrConditionalSpansSections, - ) - } - - return sectionLineRange{start: sectionRange.start, end: trimmed}, nil -} - -// validateNoContentAfter checks that there is no real section content (non-blank, -// non-conditional lines) between startLine and endLine. Returns an error if any -// such content is found. -func validateNoContentAfter(startLine, endLine int, rawLines []string) error { - for lineNum := startLine + 1; lineNum < endLine; lineNum++ { - if !isBlankOrComment(rawLines[lineNum]) && conditionalDepthChange(rawLines[lineNum]) == 0 { - return fmt.Errorf("real content found at line %d", lineNum+1) - } - } - - return nil -} - -// validateNoBranchDirectivesInExternalConditional checks that the section range -// does not contain any `%else`/`%elif` branch directives whose enclosing -// `%if`/`%endif` pair extends beyond the range. Removing such a branch directive -// while keeping the enclosing conditional would change which branch is active. -func validateNoBranchDirectivesInExternalConditional( - sectionRange sectionLineRange, - rawLines []string, - pairs []conditionalPair, -) error { - for lineNum := sectionRange.start; lineNum < sectionRange.end; lineNum++ { - if !isConditionalBranchDirective(rawLines[lineNum]) { - continue - } - - for _, pair := range pairs { - if pair.ifLine <= lineNum && pair.endifLine >= lineNum { - pairFullyInside := pair.ifLine >= sectionRange.start && pair.endifLine < sectionRange.end - - if !pairFullyInside { - return fmt.Errorf( - "section at lines %d-%d contains a %%else/%%elif branch directive inside a "+ - "conditional block that extends beyond the section boundary; "+ - "use a spec-search-replace overlay to adjust conditionals before removing:\n%w", - sectionRange.start+1, sectionRange.end, ErrConditionalSpansSections, - ) - } - - break - } - } - } - - return nil -} - -// isBlankOrComment returns true if the line is empty, whitespace-only, or a comment. -func isBlankOrComment(line string) bool { - trimmed := strings.TrimSpace(line) - - return trimmed == "" || strings.HasPrefix(trimmed, "#") -} - -// removeRanges deletes the given line ranges from the spec. Ranges must be -// non-overlapping and in ascending order (as produced by [Spec.collectSectionRanges]); -// they are removed from last to first so earlier indices remain valid. -func (s *Spec) removeRanges(ranges []sectionLineRange) { - for i := len(ranges) - 1; i >= 0; i-- { - s.RemoveLines(ranges[i].start, ranges[i].end) - } -} diff --git a/internal/rpm/spec/edit_test.go b/internal/rpm/spec/edit_test.go index 959c5f104..9eda024f5 100644 --- a/internal/rpm/spec/edit_test.go +++ b/internal/rpm/spec/edit_test.go @@ -155,6 +155,156 @@ Name: value } } +func TestGetTagDistinguishesEmptyAndDuplicateTags(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(strings.Join([]string{"Release:", "Release: 2", ""}, "\n"))) + require.NoError(t, err) + + value, err := specFile.GetTag("", "Release") + require.NoError(t, err) + assert.Empty(t, value) + + last, err := specFile.GetLastTag("", "Release") + require.NoError(t, err) + assert.Equal(t, "2", last) +} + +func TestSearchAndReplaceRepairsMalformedWholeSpecConditionals(t *testing.T) { + tests := []struct { + name, input, regex, replacement, expected string + }{ + {"unmatched endif", "Name: test\n%endif\n", `^%endif$`, "# %endif", "Name: test\n# %endif\n"}, + {"unmatched if", "Name: test\n%if 1\n", `^%if 1$`, "# %if 1", "Name: test\n# %if 1\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) + require.NoError(t, specFile.SearchAndReplace("", "", testCase.regex, testCase.replacement)) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, testCase.expected, output.String()) + }) + } +} + +func TestScopedRemovalRejectsConditionalSectionWrappers(t *testing.T) { + tests := []struct { + name string + body string + }{ + {"then", "%files foo\n/foo\n%if 1\n/foo-extra\n%files bar\n/bar\n%endif\n"}, + {"else", "%files foo\n/foo\n%if 1\n%files bar\n/bar\n%else\n/foo-extra\n%endif\n"}, + {"elif", "%files foo\n/foo\n%if 1\n%files bar\n/bar\n%elif 0\n/foo-extra\n%endif\n"}, + {"post endif", strings.Join([]string{ + "%package headless", + "Recommends: default-yama-scope", + "%if 1", + "%package gui", + "%endif", + "Recommends: default-yama-scope", + "", + }, "\n")}, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(testCase.body)) + require.NoError(t, err) + + if testCase.name == "post endif" { + err = specFile.RemoveSubpackage("headless") + } else { + err = specFile.RemoveSection("%files", "foo") + } + + require.ErrorIs(t, err, spec.ErrConditionalSpansSections) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, testCase.body, output.String()) + }) + } +} + +func TestScopedTagOperationsUseLinearOwnershipAcrossConditionalWrappers(t *testing.T) { + input := strings.Join([]string{ + "%if 0%{!?scl:1}", + "%package headless", + "Requires: binutils", + "%endif", + "Recommends: default-yama-scope", + "", + }, "\n") + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + value, err := specFile.GetTag("headless", "Recommends") + require.NoError(t, err) + assert.Equal(t, "default-yama-scope", value) + require.NoError(t, specFile.UpdateExistingTag("headless", "Recommends", "updated-yama-scope")) + require.NoError(t, specFile.RemoveTag("headless", "Recommends", "updated-yama-scope")) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, strings.Replace(input, "Recommends: default-yama-scope\n", "", 1), output.String()) +} + +func TestScopedSearchReplaceDoesNotAssignRequestedOwnership(t *testing.T) { + input := "%files foo\n/foo\n%if 1\n%files bar\n/bar\n%else\n/foo-extra\n%endif\n" + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.SearchAndReplace("%files", "baz", "^/foo-extra$", "/changed") + require.ErrorIs(t, err, spec.ErrPatternNotFound) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, input, output.String()) +} + +func TestUpdateExistingTagUpdatesAllMatchingOccurrences(t *testing.T) { + input := "Release: 1\n%if 0\nRelease: 2\n%else\nRelease: 3\n%endif\n" + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.NoError(t, specFile.UpdateExistingTag("", "Release", "4")) + release, err := specFile.GetLastTag("", "Release") + require.NoError(t, err) + assert.Equal(t, "4", release) +} + +func TestPublicEditsPreserveEmptyConditionalBranches(t *testing.T) { + input := "Name: test\n%if 1\n%elif 0\n%else\n%endif\n" + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + require.NoError(t, specFile.UpdateExistingTag("", "Name", "updated")) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, "Name: updated\n%if 1\n%elif 0\n%else\n%endif\n", output.String()) +} + +func TestRemovalDoesNotMisparseMacroContinuationBranches(t *testing.T) { + input := strings.Join([]string{ + "%if 1", + "%package tests", + "%define macro \\", + "%else \\", + "body", + "%endif", + }, "\n") + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.NoError(t, specFile.RemoveSubpackage("tests")) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Equal(t, "%if 1\n%endif\n", output.String()) +} + func TestUpdateTag(t *testing.T) { tests := []struct { name string @@ -778,6 +928,64 @@ BuildRequires: gcc tag: "Source9999", value: "macros.azl.macros", }, + { + name: "insert stays with package across else transition", + input: `%if 0%{?with_foo} +%package foo +Source0: foo.tar.gz +%else +%package bar +Summary: Bar +%endif +Requires: bar-runtime +`, + expectedOutput: `%if 0%{?with_foo} +%package foo +Source0: foo.tar.gz +Source1: foo-extra.tar.gz +%else +%package bar +Summary: Bar +%endif +Requires: bar-runtime +`, + packageName: "foo", + tag: "Source1", + value: "foo-extra.tar.gz", + }, + { + name: "insert stays with package across elif and nested transitions", + input: `%if 0%{?with_outer} +%if 0%{?with_foo} +%package foo +Source0: foo.tar.gz +%elif 0%{?with_bar} +%package bar +Summary: Bar +%else +%package baz +Summary: Baz +%endif +%endif +`, + expectedOutput: `%if 0%{?with_outer} +%if 0%{?with_foo} +%package foo +Source0: foo.tar.gz +Source1: foo-extra.tar.gz +%elif 0%{?with_bar} +%package bar +Summary: Bar +%else +%package baz +Summary: Baz +%endif +%endif +`, + packageName: "foo", + tag: "Source1", + value: "foo-extra.tar.gz", + }, } for _, test := range tests { @@ -871,6 +1079,116 @@ func TestSearchAndReplace(t *testing.T) { require.Equal(t, expected, actual.String()) }) + + t.Run("replace in macro definition", func(t *testing.T) { + input := ` +%global with_doc 1 +Name: test + +%build +make +` + + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + expected := strings.ReplaceAll(input, "%global with_doc 1", "%global with_doc 0") + + err = specFile.SearchAndReplace("", "", `%global with_doc 1`, "%global with_doc 0") + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + require.Equal(t, expected, actual.String()) + }) + + t.Run("replace in conditional header", func(t *testing.T) { + input := ` +Name: test + +%if 0%{?fedora} +BuildRequires: fedora-only +%endif + +%build +make +` + + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + expected := strings.ReplaceAll(input, "%if 0%{?fedora}", "%if 0%{?rhel}") + + err = specFile.SearchAndReplace("", "", `^%if 0%\{\?fedora\}$`, "%if 0%{?rhel}") + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + require.Equal(t, expected, actual.String()) + }) + + t.Run("replace in multi-line macro definition", func(t *testing.T) { + input := ` +%global cfg_content --gcc-triple=%{_target_cpu}-redhat-linux \ + --extra-flag=old +Name: test + +%build +make +` + + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + expected := strings.ReplaceAll(input, "redhat-linux", "azl-linux") + + err = specFile.SearchAndReplace("", "", `redhat-linux`, "azl-linux") + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + require.Equal(t, expected, actual.String()) + }) + + t.Run("replace in wrapper else branch with section filter", func(t *testing.T) { + input := ` +Name: test + +%files +/usr/bin/test + +%ifarch x86_64 +%files nonlinux +%{_datadir}/syslinux/*.exe +%else +%exclude %{_datadir}/syslinux/*.exe +%endif +` + + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + expected := strings.ReplaceAll(input, "%exclude %{_datadir}/syslinux/*.exe", "") + + err = specFile.SearchAndReplace("%files", "", `^%exclude %\{_datadir\}/syslinux/\*\.exe$`, "") + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + require.Equal(t, expected, actual.String()) + }) } func TestAddChangelogEntry(t *testing.T) { @@ -1175,6 +1493,99 @@ Name: test err = specFile.PrependLinesToSection("%description", "", []string{"New line"}) require.Error(t, err) }) + + t.Run("prepends to first matching section only", func(t *testing.T) { + input := ` +Name: test + +%if %{with tests} +%check +%pytest +%endif + +%check +%pyproject_check_import +` + + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.PrependLinesToSection("%check", "", []string{"# disabled", "exit 0"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + assert.Equal(t, ` +Name: test + +%if %{with tests} +%check +# disabled +exit 0 +%pytest +%endif + +%check +%pyproject_check_import +`, actual.String()) + }) +} + +func TestPrependLinesToAllSections(t *testing.T) { + t.Run("prepends to all matching sections", func(t *testing.T) { + input := ` +Name: test + +%if %{with tests} +%check +%pytest +%endif + +%check +%pyproject_check_import +` + + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.PrependLinesToAllSections("%check", "", []string{"# disabled", "exit 0"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + assert.Equal(t, ` +Name: test + +%if %{with tests} +%check +# disabled +exit 0 +%pytest +%endif + +%check +# disabled +exit 0 +%pyproject_check_import +`, actual.String()) + }) + + t.Run("no such section", func(t *testing.T) { + input := ` +Name: test-no-check +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.PrependLinesToAllSections("%check", "", []string{"exit 0"}) + require.Error(t, err) + }) } func TestAppendLinesToSection(t *testing.T) { @@ -1228,6 +1639,246 @@ Name: test err = specFile.AppendLinesToSection("%description", "", []string{"New line"}) require.Error(t, err) }) + + // ---- Conditional boundary tests ---- + // + // These tests document AppendLinesToSection behavior at conditional boundaries. + // When a section is followed by a %if wrapper that contains the next section, + // the tree parser correctly identifies the wrapper boundary. Appended lines + // land at the end of the section body, before the wrapper. + + t.Run("appends before conditional wrapping next section", func(t *testing.T) { + // The %if wraps %install, not %build. "echo done" belongs in %build. + input := ` +Name: test + +%build +make + +%if %{with_docs} +%install +install.sh +%endif +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AppendLinesToSection("%build", "", []string{"echo done"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + // Tree-based code correctly places "echo done" before the wrapper %if. + assert.Equal(t, ` +Name: test + +%build +make + +echo done +%if %{with_docs} +%install +install.sh +%endif +`, actual.String()) + }) + + t.Run("appends before nested conditionals wrapping next section", func(t *testing.T) { + input := ` +Name: test + +%build +make + +%if %{with_docs} +%ifarch x86_64 +%install +install.sh +%endif +%endif +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AppendLinesToSection("%build", "", []string{"echo done"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + // Tree-based code correctly places "echo done" before the outer wrapper. + assert.Equal(t, ` +Name: test + +%build +make + +echo done +%if %{with_docs} +%ifarch x86_64 +%install +install.sh +%endif +%endif +`, actual.String()) + }) + + t.Run("appends correctly when section has own balanced conditional", func(t *testing.T) { + // %if/%endif is fully within %build, so the boundary is correct. + input := ` +Name: test + +%build +%if %{with_docs} +make docs +%endif +make + +%install +install.sh +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AppendLinesToSection("%build", "", []string{"echo done"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + assert.Equal(t, ` +Name: test + +%build +%if %{with_docs} +make docs +%endif +make + +echo done +%install +install.sh +`, actual.String()) + }) + + t.Run("appends correctly when no conditionals at boundary", func(t *testing.T) { + input := ` +Name: test + +%build +make + +%install +install.sh +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AppendLinesToSection("%build", "", []string{"echo done"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + assert.Equal(t, ` +Name: test + +%build +make + +echo done +%install +install.sh +`, actual.String()) + }) + + t.Run("appends before preamble conditional wrapping next section", func(t *testing.T) { + // In the preamble, a trailing %if wrapper contains %description. + // The tree parser correctly identifies the wrapper boundary. + input := ` +Name: test +Source0: test.tar.gz + +%if %{with_docs} +%description +A test package. +%endif + +%build +make +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AppendLinesToSection("", "", []string{"Vendor: Microsoft"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + // Tree-based code correctly places "Vendor: Microsoft" before the wrapper. + assert.Equal(t, ` +Name: test +Source0: test.tar.gz + +Vendor: Microsoft +%if %{with_docs} +%description +A test package. +%endif + +%build +make +`, actual.String()) + }) + + t.Run("appends inside wrapper when section is in conditional", func(t *testing.T) { + // %files modules-extra-matched is inside %ifnarch. The tree correctly + // scopes the section within the conditional, so appended lines land + // inside the wrapper (before %endif). + input := ` +Name: test + +%ifnarch noarch +%files modules-extra-matched +%endif + +%changelog +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + err = specFile.AppendLinesToSection("%files", "modules-extra-matched", []string{"", "# ANCHOR"}) + require.NoError(t, err) + + actual := new(bytes.Buffer) + + err = specFile.Serialize(actual) + require.NoError(t, err) + + assert.Equal(t, ` +Name: test + +%ifnarch noarch +%files modules-extra-matched + +# ANCHOR +%endif + +%changelog +`, actual.String()) + }) } func TestHasSection(t *testing.T) { @@ -1538,105 +2189,105 @@ func TestParsePatchTagNumber(t *testing.T) { } } -func TestVisitTags(t *testing.T) { - input := `Name: main-pkg +func TestContinuationSuppressesStructuralParsing(t *testing.T) { + t.Run("section keyword in continuation body is not a section start", func(t *testing.T) { + input := `Name: test Version: 1.0 -Patch0: main.patch -%package devel -Summary: Development files -Patch1: devel.patch +%description +A package. + +%install +echo \ +%files \ +done -%package -n other -Summary: Other package -Patch2: other.patch +%files +/usr/bin/test ` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) - tests := []struct { - name string - expectedTags []string - }{ - { - name: "visits tags across all packages", - expectedTags: []string{"Name", "Version", "Patch0", "Summary", "Patch1", "Summary", "Patch2"}, - }, - } + // %files inside the continuation should NOT be treated as a section start. + // Only one real %files section should exist. + found, err := specFile.HasSection("%files") + require.NoError(t, err) + assert.True(t, found, "real %%files section should be found") + }) - for _, testCase := range tests { - t.Run(testCase.name, func(t *testing.T) { - sf, err := spec.OpenSpec(strings.NewReader(input)) - require.NoError(t, err) + t.Run("tag-like line in continuation body is not a tag", func(t *testing.T) { + input := `Name: test +Version: 1.0 - var tags []string +%description +A package. - err = sf.VisitTags(func(tagLine *spec.TagLine, _ *spec.Context) error { - tags = append(tags, tagLine.Tag) +%install +echo \ +Name: fake \ +done +` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) - return nil - }) - require.NoError(t, err) - assert.Equal(t, testCase.expectedTags, tags) - }) - } -} + // The "Name: fake" inside the continuation should not be found as a tag. + // GetTag should return the real "Name: test" from the preamble. + value, err := specFile.GetTag("", "Name") + require.NoError(t, err) + assert.Equal(t, "test", value, "should find the real Name tag, not the continuation body") + }) -func TestVisitTagsPackage(t *testing.T) { - input := `Name: main-pkg + t.Run("normal parsing resumes after continuation ends", func(t *testing.T) { + input := `Name: test Version: 1.0 -Patch0: main.patch -%package devel -Summary: Development files -Patch1: devel.patch +%description +A package. -%package -n other -Summary: Other package -Patch2: other.patch +%build +echo \ +%install \ +done + +%install +make install + +%files +/usr/bin/test ` + specFile, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) - tests := []struct { - name string - packageName string - expectedTags []string - }{ - { - name: "global package only", - packageName: "", - expectedTags: []string{"Name", "Version", "Patch0"}, - }, - { - name: "devel sub-package only", - packageName: "devel", - expectedTags: []string{"Summary", "Patch1"}, - }, - { - name: "other sub-package only", - packageName: "other", - expectedTags: []string{"Summary", "Patch2"}, - }, - { - name: "non-existing package returns no tags", - packageName: "nonexistent", - expectedTags: nil, - }, - } + found, err := specFile.HasSection("%install") + require.NoError(t, err) + assert.True(t, found, "real %%install section after continuation should be found") - for _, testCase := range tests { - t.Run(testCase.name, func(t *testing.T) { - sf, err := spec.OpenSpec(strings.NewReader(input)) - require.NoError(t, err) + found, err = specFile.HasSection("%files") + require.NoError(t, err) + assert.True(t, found, "%%files section should be found") + }) - var tags []string + t.Run("chained multi-line continuation", func(t *testing.T) { + input := `Name: test +Version: 1.0 - err = sf.VisitTagsPackage(testCase.packageName, func(tagLine *spec.TagLine, _ *spec.Context) error { - tags = append(tags, tagLine.Tag) +%build +echo \ +%description \ +%files \ +%install \ +done +` + sf, err := spec.OpenSpec(strings.NewReader(input)) + require.NoError(t, err) - return nil - }) + // None of the keywords in the continuation chain should create sections. + for _, sect := range []string{"%description", "%files", "%install"} { + found, err := sf.HasSection(sect) require.NoError(t, err) - assert.Equal(t, testCase.expectedTags, tags) - }) - } + assert.False(t, found, "%%s in continuation chain should not be a section", sect) + } + }) } func TestRemoveSection(t *testing.T) { @@ -2087,7 +2738,7 @@ Main. errorContains: "conditional block spans", }, { - name: "errors on else branch inside straddling conditional", + name: "removes section from one branch while else branch retains sections", input: `Name: test %description @@ -2104,9 +2755,21 @@ Main. %files /usr/bin/test `, - packageName: "foo", - errorExpected: true, - errorContains: "branch directive", + packageName: "foo", + expectedOutput: `Name: test + +%description +Main. + +%if cond +%else +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test +`, }, { name: "errors when trimmed zone contains section content in a balanced conditional", diff --git a/internal/rpm/spec/spec.go b/internal/rpm/spec/spec.go index 1ab04206b..0b199d849 100644 --- a/internal/rpm/spec/spec.go +++ b/internal/rpm/spec/spec.go @@ -7,7 +7,6 @@ import ( "bufio" "fmt" "io" - "regexp" "slices" "strings" ) @@ -136,20 +135,6 @@ func (*RawLine) GetType() ParsedLineType { return Raw } -type parseState struct { - currentSect SectionTarget -} - -func newParseState() parseState { - return parseState{ - currentSect: SectionTarget{ - SectType: PackageSection, - SectName: "", - Package: "", - }, - } -} - // OpenSpec reads in the contents of an RPM spec file from the provided reader, returning a [Spec] object. // An error is returned if the reader cannot be fully read (e.g., I/O error or line exceeds buffer size). func OpenSpec(reader io.Reader) (*Spec, error) { @@ -207,110 +192,6 @@ func (s *Spec) InsertLinesAt(insertedLines []string, lineNumber int) { s.rawLines = slices.Insert(s.rawLines, lineNumber, insertedLines...) } -// Context provides context information to a visitor function when visiting a spec. -type Context struct { - // Target is the current visit target. - Target VisitTarget - // RawLine is the raw text of the current line, if applicable (nil otherwise). - RawLine *string - // CurrentSection is the current section being visited. - CurrentSection SectionTarget - // CurrentLineNum is the current (0-indexed) line number being visited. - CurrentLineNum int - - // parseStateBeforeCurrentLine represents the parse state for this spec as it was - // before parsing the current line. - parseStateBeforeCurrentLine parseState - // parseState represents the parse state for this spec after having parsed the - // current line. - parseState parseState - // nextLineNumToParse is the next (0-indexed) line number that will be parsed - // during visiting; note that not all parsed lines will be send to the visitor, - // though. - nextLineNumToParse int - // nextLineNumToVisit is the next (0-indexed) line number that will be visited. - nextLineNumToVisit int - // spec is the spec being visited. - spec *Spec -} - -// InsertLinesBefore inserts the provided lines just before the line currently being visited, -// updating the context accordingly. The next line to be visited will be the line following -// the current one being visited. -func (ctx *Context) InsertLinesBefore(lines []string) { - ctx.spec.InsertLinesAt(lines, ctx.CurrentLineNum) - - // Account for the displacement from the inserted lines. We will parse the - // new lines but not visit them, nor will we visit the current line again. - // This will require rollback back to the previous parse state first. - ctx.parseState = ctx.parseStateBeforeCurrentLine - ctx.nextLineNumToParse = ctx.CurrentLineNum - ctx.nextLineNumToVisit = ctx.CurrentLineNum + len(lines) + 1 -} - -// InsertLinesAfter inserts the provided lines just after the line currently being visited, -// updating the context accordingly. The next line to be visited will be the line following -// the newly inserted lines. -func (ctx *Context) InsertLinesAfter(lines []string) { - ctx.spec.InsertLinesAt(lines, ctx.CurrentLineNum+1) - - // Skip ahead past the newly inserted lines. - ctx.nextLineNumToParse = ctx.CurrentLineNum + 1 - ctx.nextLineNumToVisit += len(lines) -} - -// RemoveLine removes the line currently being visited, updating the context accordingly. -// The next line to be visited will be the line that followed the removed line. -func (ctx *Context) RemoveLine() { - ctx.spec.RemoveLine(ctx.CurrentLineNum) - - // Account for the removed line. We will reparse the new current line and revisit it. - // This will require rolling back to the previous parse state first. - ctx.parseState = ctx.parseStateBeforeCurrentLine - ctx.nextLineNumToParse = ctx.CurrentLineNum - ctx.nextLineNumToVisit = ctx.CurrentLineNum -} - -// ReplaceLine replaces the line currently being visited with the provided replacement line, -// updating the context accordingly. -func (ctx *Context) ReplaceLine(replacement string) { - ctx.spec.ReplaceLine(ctx.CurrentLineNum, replacement) - - // Account for the replaced line. We will reparse the current line, but not revisit it. - // This will require rolling back to the previous parse state. - ctx.parseState = ctx.parseStateBeforeCurrentLine - ctx.nextLineNumToParse = ctx.CurrentLineNum - ctx.nextLineNumToVisit = ctx.CurrentLineNum + 1 -} - -// VisitTarget encapsulates the current target of a visit operation. -type VisitTarget struct { - // TargetType is the type of the current visit target. - TargetType VisitTargetType - // Optionally, provides more detail about the target's section. - // Left nil when the target is not part of a section. - Section *SectionTarget - // Optionally, provides more detail about the target's line. Left nil - // when the target is not a line. - Line *Line -} - -// VisitTargetType indicates the type of a visit target. -type VisitTargetType string - -const ( - // SpecStartTarget indicates the start of the spec. - SpecStartTarget VisitTargetType = "SpecStart" - // SectionStartTarget indicates the start of a section. - SectionStartTarget VisitTargetType = "SectionStart" - // SectionLineTarget indicates a line within a section. - SectionLineTarget VisitTargetType = "SectionLine" - // SectionEndTarget indicates the end of a section. - SectionEndTarget VisitTargetType = "SectionEnd" - // SpecEndTarget indicates the end of the spec. - SpecEndTarget VisitTargetType = "SpecEnd" -) - // SectionTarget encapsulates information about the current section context. type SectionTarget struct { // SectName is the name of the section, e.g. "%description". @@ -322,199 +203,6 @@ type SectionTarget struct { Package string } -// Visitor is the type of a visitor function that can be passed to [Spec.Visit]. -type Visitor = func(ctx *Context) error - -// Visit walks through the spec, invoking the provided visitor function for each relevant target. -// -// State Management Invariants: -// - nextLineNumToParse: The next 0-indexed line number to parse. May be less than or equal to -// CurrentLineNum when context mutations (InsertLinesBefore, RemoveLine, ReplaceLine) require -// re-parsing the current position. -// - nextLineNumToVisit: The next 0-indexed line number to send to the visitor. A line is visited -// only if CurrentLineNum >= nextLineNumToVisit, allowing mutations to skip visiting newly -// inserted lines or re-visit lines after removal. -// - Context mutation methods update these values to maintain correct traversal after modifications. -// -//nolint:funlen -func (s *Spec) Visit(visitor Visitor) error { - ctx := Context{ - Target: VisitTarget{TargetType: SpecStartTarget}, - CurrentSection: newParseState().currentSect, - CurrentLineNum: 0, - parseState: newParseState(), - parseStateBeforeCurrentLine: newParseState(), - nextLineNumToVisit: 0, - nextLineNumToParse: 1, - spec: s, - } - - // Visit the spec start. - err := visitor(&ctx) - if err != nil { - return err - } - - // Visit the preamble start. - ctx.Target = VisitTarget{ - TargetType: SectionStartTarget, - Section: &ctx.CurrentSection, - } - - err = visitor(&ctx) - if err != nil { - return err - } - - // Go through the lines. - for ctx.CurrentLineNum < len(ctx.spec.rawLines) { - var parsedLine ParsedLine - - rawLine := ctx.spec.rawLines[ctx.CurrentLineNum] - - ctx.parseStateBeforeCurrentLine = ctx.parseState - parsedLine, ctx.parseState = parseSpecLine(rawLine, ctx.parseStateBeforeCurrentLine) - - if _, ok := parsedLine.(*SectionStartLine); ok { - // Visit the end of the preceding section. - ctx.Target = VisitTarget{ - TargetType: SectionEndTarget, - Section: &ctx.CurrentSection, - } - - ctx.RawLine = nil - - // Skip visiting if this line was inserted or we're re-parsing after a mutation. - if ctx.CurrentLineNum >= ctx.nextLineNumToVisit { - err = visitor(&ctx) - if err != nil { - return err - } - } - - // Visit the start of the new section. - ctx.CurrentSection = ctx.parseState.currentSect - ctx.RawLine = &rawLine - ctx.Target = VisitTarget{ - TargetType: SectionStartTarget, - Section: &ctx.CurrentSection, - } - } else { - ctx.RawLine = &rawLine - ctx.Target = VisitTarget{ - TargetType: SectionLineTarget, - Line: &Line{ - Text: rawLine, - Parsed: parsedLine, - }, - } - } - - // Visit the line (if so requested). - if ctx.CurrentLineNum >= ctx.nextLineNumToVisit { - err = visitor(&ctx) - if err != nil { - return err - } - } - - // Move to whatever is the next line that we need to parse; note that it - // may be the same as the previous line number in case that line got removed. - // It's also possible that we won't send the line to the visitor. - ctx.CurrentLineNum = ctx.nextLineNumToParse - - // Update next *next* lines to visit/parse. - ctx.nextLineNumToParse++ - ctx.nextLineNumToVisit = max(ctx.CurrentLineNum, ctx.nextLineNumToVisit) - } - - // Visit the end of the last section. - ctx.Target = VisitTarget{ - TargetType: SectionEndTarget, - Section: &ctx.CurrentSection, - } - - ctx.RawLine = nil - - err = visitor(&ctx) - if err != nil { - return err - } - - // Visit the spec end. - ctx.Target = VisitTarget{TargetType: SpecEndTarget} - ctx.RawLine = nil - - err = visitor(&ctx) - if err != nil { - return err - } - - return nil -} - -func parseSpecLine(physicalText string, state parseState) (ParsedLine, parseState) { - parsedLine := newParsedLine(physicalText, state) - - if sectionStartLine, ok := parsedLine.(*SectionStartLine); ok { - state.currentSect.SectType = sectionStartLine.SectType - state.currentSect.SectName = sectionStartLine.SectName - state.currentSect.Package = getPackageNameForSection(sectionStartLine.SectType, sectionStartLine.Tokens) - } - - return parsedLine, state -} - -func newParsedLine(physicalText string, state parseState) ParsedLine { - logicalLine := physicalText - if strings.HasPrefix(physicalText, "#") { - logicalLine = "" - } - - logicalLine = strings.TrimSpace(logicalLine) - - return parseLogicalLine(logicalLine, state) -} - -var tagRegex = regexp.MustCompile(`^\s*([^\s:]+):\s*(.*?)\s*$`) - -func parseLogicalLine(logicalLine string, state parseState) ParsedLine { - tokens := strings.Fields(logicalLine) - if len(tokens) == 0 { - return &RawLine{} - } - - // See if this appears to be the start of a new section. - if strings.HasPrefix(tokens[0], "%") { - if newSectionType, known := sectionTypesByName[strings.ToLower(tokens[0])]; known { - return &SectionStartLine{ - SectType: newSectionType, - SectName: tokens[0], - Tokens: tokens, - } - } - } - - // Otherwise, if we're currently in a package section, see if this looks like a line that defines - // one or more tags. - if state.currentSect.SectType == PackageSection { - const reSubmatchCount = 3 - - matches := tagRegex.FindStringSubmatch(logicalLine) - if len(matches) == reSubmatchCount { - return &TagLine{ - Tag: matches[1], - Value: matches[2], - } - } - } - - // This doesn't appear to be the start of a section, nor a tag definition; treat it as a raw line. - return &RawLine{ - Content: logicalLine, - } -} - func getPackageNameForSection(sectionType SectionType, headerTokens []string) string { switch sectionType { case SourceFileListSection: diff --git a/internal/rpm/spec/spec_test.go b/internal/rpm/spec/spec_test.go index 88167abfd..b8c242e4d 100644 --- a/internal/rpm/spec/spec_test.go +++ b/internal/rpm/spec/spec_test.go @@ -54,12 +54,8 @@ func TestOpenSpec_EmptyInput(t *testing.T) { require.NoError(t, err) // Empty spec is parseable but has no tags. - err = sf.VisitTags(func(_ *spec.TagLine, _ *spec.Context) error { - t.Fatal("no tags should be visited in an empty spec") - - return nil - }) - require.NoError(t, err) + _, err = sf.GetTag("", "Name") + require.ErrorIs(t, err, spec.ErrNoSuchTag) } func TestOpenSpec_BinaryContent(t *testing.T) { diff --git a/internal/rpm/spec/testdata/specs/comment-only-conditional.spec b/internal/rpm/spec/testdata/specs/comment-only-conditional.spec new file mode 100644 index 000000000..be663d109 --- /dev/null +++ b/internal/rpm/spec/testdata/specs/comment-only-conditional.spec @@ -0,0 +1,32 @@ +Name: comment-only-conditional +Version: 1.0 +Release: 1 +Summary: %%if blocks whose entire body is comments and blank lines +License: MIT + +%if 0%{?with_future} +# Reserved for the upcoming foo backend. +# Empty until upstream finalizes the API. + +# Track: https://example.invalid/issues/42 +%endif + +%description +Fixture: a top-level conditional whose body contains only RPM-spec comments +and blank lines, plus a guard inside a script section with the same shape. + +%build +%if 0%{?with_future} +# TODO(future): wire up the foo backend once it lands upstream. +%endif +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/comment-only-conditional + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/elif-chain.spec b/internal/rpm/spec/testdata/specs/elif-chain.spec new file mode 100644 index 000000000..7c9507652 --- /dev/null +++ b/internal/rpm/spec/testdata/specs/elif-chain.spec @@ -0,0 +1,39 @@ +Name: elif-chain +Version: 1.0 +Release: 1 +Summary: %%if / %%elif / %%else chain inside preamble +License: MIT + +%if 0%{?rhel} >= 10 +Requires: rhel10-runtime +BuildRequires: rhel10-devel +%elif 0%{?rhel} >= 9 +Requires: rhel9-runtime +BuildRequires: rhel9-devel +%elif 0%{?fedora} >= 40 +Requires: fedora-runtime +BuildRequires: fedora-devel +%elif 0%{?suse_version} +Requires: suse-runtime +BuildRequires: suse-devel +%else +Requires: generic-runtime +BuildRequires: generic-devel +%endif + +%description +Fixture: deep %%elif chain with terminal %%else, content-style conditional +(no section headers in any branch). + +%build +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/elif-chain + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/elif-with-sections.spec b/internal/rpm/spec/testdata/specs/elif-with-sections.spec new file mode 100644 index 000000000..d3811051b --- /dev/null +++ b/internal/rpm/spec/testdata/specs/elif-with-sections.spec @@ -0,0 +1,52 @@ +Name: elif-with-sections +Version: 1.0 +Release: 1 +Summary: %%elif branches that each contain entire %%package sections +License: MIT + +%description +Fixture: %%elif chain where every branch (including %%else) introduces a +distinct %%package + %%description + %%files trio. Each conditional branch +acts as a wrapper, not as in-section content. + +%if 0%{?rhel} +%package rhel-extras +Summary: RHEL-specific extras + +%description rhel-extras +Extras only built for RHEL. + +%files rhel-extras +/usr/share/elif-with-sections/rhel +%elif 0%{?fedora} +%package fedora-extras +Summary: Fedora-specific extras + +%description fedora-extras +Extras only built for Fedora. + +%files fedora-extras +/usr/share/elif-with-sections/fedora +%else +%package generic-extras +Summary: Generic extras + +%description generic-extras +Fallback extras for all other distros. + +%files generic-extras +/usr/share/elif-with-sections/generic +%endif + +%build +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/elif-with-sections + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/if-with-continuation.spec b/internal/rpm/spec/testdata/specs/if-with-continuation.spec new file mode 100644 index 000000000..c3648167f --- /dev/null +++ b/internal/rpm/spec/testdata/specs/if-with-continuation.spec @@ -0,0 +1,34 @@ +Name: if-with-continuation +Version: 1.0 +Release: 1 +Summary: %%if condition that spans multiple lines via backslash continuation +License: MIT + +%global _is_long_arch \ + 0%{?rhel} >= 9 || \ + 0%{?fedora} >= 40 || \ + 0%{?suse_version} >= 1550 + +%if %{_is_long_arch} && \ + %{undefined disable_long_arch} && \ + "%{_arch}" != "armv7hl" +BuildRequires: long-arch-support +Requires: long-arch-runtime +%endif + +%description +Fixture: backslash-continuation inside an %%if condition itself (not just in +the body) and in a %%global that the condition references. + +%build +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/if-with-continuation + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/macro-conditional.spec b/internal/rpm/spec/testdata/specs/macro-conditional.spec new file mode 100644 index 000000000..4ed659dba --- /dev/null +++ b/internal/rpm/spec/testdata/specs/macro-conditional.spec @@ -0,0 +1,44 @@ +Name: macro-conditional +Version: 1.0 +Release: 1%{?dist} +Summary: Fixture with %if/%endif inside macro continuation bodies +License: MIT + +# Parameterized macro with %if/%endif in the body (kernel pattern). +# The %if here is RPM macro body text, NOT a structural conditional. +%define kernel_reqprovconf(o) \ +%if %{-o:0}%{!-o:1}\ +Provides: kernel = %{version}-%{release}\ +Provides: %{name} = %{version}-%{release}\ +%endif\ +%{nil} + +# Global macro with conditional (ghc pattern). +%global obsoletes_pkg() \ +%if %{defined old_name}\ +Obsoletes: %{old_name}%{?1:-%1} < %{version}-%{release}\ +Provides: %{old_name}%{?1:-%1} = %{version}-%{release}\ +%endif\ +%{nil} + +# Real structural conditional (should still be parsed). +%if 0%{?fedora} +BuildRequires: fedora-only-dep +%endif + +%description +A spec testing that %if/%endif inside backslash-continued macro +definitions are treated as macro body text, not structural conditionals. + +%build +make %{?_smp_mflags} + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/macro-conditional + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial build. diff --git a/internal/rpm/spec/testdata/specs/macro-continuation.spec b/internal/rpm/spec/testdata/specs/macro-continuation.spec new file mode 100644 index 000000000..7852d1d2a --- /dev/null +++ b/internal/rpm/spec/testdata/specs/macro-continuation.spec @@ -0,0 +1,33 @@ +Name: macro-continuation +Version: 1.0 +Release: 1 +Summary: %%define / %%global with backslash continuation +License: MIT + +%global cmake_flags \ + -DENABLE_FOO=ON \ + -DENABLE_BAR=OFF \ + -DCMAKE_BUILD_TYPE=Release + +%define configure_args \ + --prefix=%{_prefix} \ + --libdir=%{_libdir} \ + --sysconfdir=%{_sysconfdir} + +%description +Fixture: %%define / %%global with backslash continuation lines. + +%build +cmake %{cmake_flags} . +./configure %{configure_args} +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/macro-continuation + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/macro-with-parameters.spec b/internal/rpm/spec/testdata/specs/macro-with-parameters.spec new file mode 100644 index 000000000..b0042af2b --- /dev/null +++ b/internal/rpm/spec/testdata/specs/macro-with-parameters.spec @@ -0,0 +1,35 @@ +Name: macro-with-parameters +Version: 1.0 +Release: 1 +Summary: %%define macros that accept positional parameters +License: MIT + +%define uname_suffix() %{?1:+%{1}} +%define uname_variant() %{lua: + local v = rpm.expand("%{?1}") + if v == "" then return "" end + return "-" .. v +} + +%define build_with(opt) \ +%{expand:%%global _with_%{1} --with-%{1}} \ +%global _enable_%{1} 1 + +%description +Fixture: parameterized %%define macros — empty-arg, lua body, and a +multi-line definition that itself expands further %%global calls. + +%build +%{build_with foo} +%{build_with bar} +make %{?_smp_mflags} + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/macro-with-parameters + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/multi-package-mixed.spec b/internal/rpm/spec/testdata/specs/multi-package-mixed.spec new file mode 100644 index 000000000..81dd89031 --- /dev/null +++ b/internal/rpm/spec/testdata/specs/multi-package-mixed.spec @@ -0,0 +1,69 @@ +Name: multi-package-mixed +Version: 1.0 +Release: 1 +Summary: Multiple subpackages mixed with conditionals and macros +License: MIT +URL: https://example.invalid/ +Source0: %{name}-%{version}.tar.gz + +%global commit_id 0123456789abcdef0123456789abcdef01234567 +%define short_commit %(echo %{commit_id} | cut -c1-7) + +%description +Fixture: realistic multi-subpackage layout combining %%package -n +renaming, mixed conditional wrappers, and shared macros. Exercises tag +walks, section enumeration, and per-package filtering against a +non-trivial topology. + +%package devel +Summary: Development files for %{name} +Requires: %{name}%{?_isa} = %{version}-%{release} + +%description devel +Headers and link-time helpers for building against %{name}. + +%package -n lib%{name} +Summary: Runtime library for %{name} +Provides: bundled(%{name}-internal) = %{short_commit} + +%description -n lib%{name} +Just the shared library, suitable for stand-alone consumption. + +%if 0%{?with_docs} +%package doc +Summary: Documentation for %{name} +BuildArch: noarch + +%description doc +HTML and man pages for %{name}, built from the in-tree sources. +%endif + +%prep +%autosetup -n %{name}-%{version} + +%build +%configure +%make_build + +%install +%make_install + +%files +%license LICENSE +/usr/bin/multi-package-mixed + +%files devel +/usr/include/%{name}/ + +%files -n lib%{name} +/usr/lib64/lib%{name}.so.* + +%if 0%{?with_docs} +%files doc +%doc README.md +%doc docs/html/ +%endif + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/nested-wrappers.spec b/internal/rpm/spec/testdata/specs/nested-wrappers.spec new file mode 100644 index 000000000..7d3488cdb --- /dev/null +++ b/internal/rpm/spec/testdata/specs/nested-wrappers.spec @@ -0,0 +1,43 @@ +Name: nested-wrappers +Version: 1.0 +Release: 1 +Summary: %%if wrappers nested inside other %%if wrappers across sections +License: MIT + +%description +Fixture: outer %%if wraps the %%package devel section, which itself contains +an inner %%if that wraps %%description devel / %%files devel. + +%if 0%{?with_devel} +%package devel +Summary: Development files +Requires: %{name} = %{version}-%{release} + +%if 0%{?with_devel_docs} +%description devel +Devel files for nested-wrappers, including extra documentation. + +%files devel +/usr/include/nested-wrappers.h +/usr/share/doc/nested-wrappers/devel/ +%else +%description devel +Devel files for nested-wrappers. + +%files devel +/usr/include/nested-wrappers.h +%endif +%endif + +%build +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/nested-wrappers + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/script-section-tag-shaped.spec b/internal/rpm/spec/testdata/specs/script-section-tag-shaped.spec new file mode 100644 index 000000000..c91992bc2 --- /dev/null +++ b/internal/rpm/spec/testdata/specs/script-section-tag-shaped.spec @@ -0,0 +1,43 @@ +Name: script-section-tag-shaped +Version: 1.0 +Release: 1 +Summary: Tag-shaped shell lines inside script sections must not be parsed as tags +License: MIT + +%description +Fixture: script sections (%%build, %%install, %%post, %%pre, %%check) contain +shell commands whose arguments look exactly like spec tags +(`echo "Name: foo"`, `printf "Version: ...\n"`, etc.). Tag-edit operations +must skip these — only the preamble and %%package blocks accept tag writes. + +%build +echo "Name: not-a-tag-write" +printf "Version: still-not-a-tag\n" +echo "Requires: bash" >> .build-manifest +make + +%install +make install DESTDIR=%{buildroot} +cat < %{buildroot}/etc/%{name}.conf +Name: %{name} +Version: %{version} +EOF + +%check +echo "License: MIT" | tee -a check.log +make check + +%pre +echo "Conflicts: previous-version" >&2 + +%post +ldconfig +echo "Provides: %{name}-runtime" > /var/log/%{name}-post.log + +%files +/usr/bin/script-section-tag-shaped +/etc/%{name}.conf + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/straddling-wrapper.spec b/internal/rpm/spec/testdata/specs/straddling-wrapper.spec new file mode 100644 index 000000000..3f63284ea --- /dev/null +++ b/internal/rpm/spec/testdata/specs/straddling-wrapper.spec @@ -0,0 +1,30 @@ +Name: straddling-wrapper +Version: 1.0 +Release: 1 +Summary: %%if opens before a section header and %%endif closes inside it +License: MIT + +%description +Fixture: classic Fedora-style "straddling" conditional. The %%if directive +appears at the top level (between %%build and %%install) but is paired with +an %%endif that lives several sections later — bracketing %%install and +%%check into the conditional wrapper. + +%build +make + +%if 0%{?with_tests} +%install +make install DESTDIR=%{buildroot} +make install-tests DESTDIR=%{buildroot} + +%check +make check +%endif + +%files +/usr/bin/straddling-wrapper + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/subpackage-define-referenced.spec b/internal/rpm/spec/testdata/specs/subpackage-define-referenced.spec new file mode 100644 index 000000000..f430da4bd --- /dev/null +++ b/internal/rpm/spec/testdata/specs/subpackage-define-referenced.spec @@ -0,0 +1,39 @@ +Name: subpackage-define-referenced +Version: 1.0 +Release: 1 +Summary: %%define inside a subpackage referenced from %%install (issue #203 repro) +License: MIT + +%description +Fixture mirroring issue #203 -- the helper macro is defined inside the +test subpackage but referenced from the unconditional install section. +Removing the subpackage naively drops the macro and leaves dangling +references in surviving sections. + +%package tests +Summary: Tests for %{name} +Requires: %{name} = %{version}-%{release} + +%define testsdir %{_libdir}/%{name}/tests-src + +%description tests +The %{name}-tests rpm contains test fixtures for %{name}. + +%files tests +%{testsdir} + +%build +make + +%install +make install DESTDIR=%{buildroot} +mkdir -p %{buildroot}%{testsdir}/python +mkdir -p %{buildroot}%{testsdir}/scripts +install -p -m 0644 tests/Makefile.include %{buildroot}%{testsdir}/ + +%files +/usr/bin/subpackage-define-referenced + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec b/internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec new file mode 100644 index 000000000..b53837750 --- /dev/null +++ b/internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec @@ -0,0 +1,34 @@ +Name: subpackage-define-shadowed +Version: 1.0 +Release: 1 +Summary: Subpackage %%define shadows a surviving preamble macro +License: MIT + +%global toolsdir %{_libdir}/%{name} + +%description +Fixture verifying that a subpackage %%define whose name already has a +surviving definition in the preamble is NOT hoisted. The survivor reference +in %%install resolves to the existing preamble definition, so hoisting the +subpackage copy would wrongly clobber it. + +%package tools +Summary: Tools for %{name} + +%global toolsdir %{_libdir}/%{name}/tools-override + +%description tools +Tools for %{name}. + +%files tools +%{toolsdir} + +%install +mkdir -p %{buildroot}%{toolsdir} + +%files +/usr/bin/subpackage-define-shadowed + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/subpackage-define-transitive.spec b/internal/rpm/spec/testdata/specs/subpackage-define-transitive.spec new file mode 100644 index 000000000..1918887e7 --- /dev/null +++ b/internal/rpm/spec/testdata/specs/subpackage-define-transitive.spec @@ -0,0 +1,38 @@ +Name: subpackage-define-transitive +Version: 1.0 +Release: 1 +Summary: Transitive %%define chain inside a subpackage (issue #203 follow-up) +License: MIT + +%description +Fixture for conservative transitive macro-hoisting rejection: the subpackage +defines a chain of helper macros (%%testroot -> %%testsdir) and only the outer +one is referenced from the surviving %%install section. Removing the +subpackage must reject rather than reorder the dependency chain. + +%package tests +Summary: Tests for %{name} +Requires: %{name} = %{version}-%{release} + +%define testroot %{_libdir}/%{name} +%define testsdir %{testroot}/tests-src + +%description tests +The %{name}-tests rpm contains test fixtures for %{name}. + +%files tests +%{testsdir} + +%build +make + +%install +make install DESTDIR=%{buildroot} +mkdir -p %{buildroot}%{testsdir}/python + +%files +/usr/bin/subpackage-define-transitive + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata/specs/subpackage-define-unreferenced.spec b/internal/rpm/spec/testdata/specs/subpackage-define-unreferenced.spec new file mode 100644 index 000000000..d1fe37a2e --- /dev/null +++ b/internal/rpm/spec/testdata/specs/subpackage-define-unreferenced.spec @@ -0,0 +1,36 @@ +Name: subpackage-define-unreferenced +Version: 1.0 +Release: 1 +Summary: %%define inside a subpackage only referenced from within itself +License: MIT + +%description +Fixture companion to subpackage-define-referenced. The macro defined inside +the helper subpackage is only referenced from within the same subpackage, +so removing that subpackage should drop the macro cleanly without any +hoisting. + +%package tools +Summary: Helper tools for %{name} +Requires: %{name} = %{version}-%{release} + +%define toolsdir %{_libexecdir}/%{name}/tools + +%description tools +Helper command-line utilities used only with the tools subpackage. + +%files tools +%{toolsdir} + +%build +make + +%install +make install DESTDIR=%{buildroot} + +%files +/usr/bin/subpackage-define-unreferenced + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. diff --git a/internal/rpm/spec/testdata_test.go b/internal/rpm/spec/testdata_test.go new file mode 100644 index 000000000..dcf601bb8 --- /dev/null +++ b/internal/rpm/spec/testdata_test.go @@ -0,0 +1,815 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec //nolint:testpackage // Fixture suites directly exercise structural parser internals. + +import ( + "bytes" + "log/slog" + "math/rand/v2" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testdataSpecsDir is the directory containing curated hand-crafted spec +// fixtures harvested from real-world Fedora / Azure Linux patterns. +const testdataSpecsDir = "testdata/specs" + +// loadFixture reads a fixture file and returns its raw bytes. +func loadFixture(t *testing.T, name string) []byte { + t.Helper() + + path := filepath.Join(testdataSpecsDir, name) + + raw, err := os.ReadFile(path) + require.NoError(t, err, "reading fixture %s", path) + + return raw +} + +// openFixture parses a fixture into a *Spec. +func openFixture(t *testing.T, name string) *Spec { + t.Helper() + + raw := loadFixture(t, name) + + specObj, err := OpenSpec(bytes.NewReader(raw)) + require.NoError(t, err, "parsing fixture %s", name) + + return specObj +} + +// serializeSpec serializes a Spec to a string for assertion. +func serializeSpec(t *testing.T, s *Spec) string { + t.Helper() + + var buf bytes.Buffer + + require.NoError(t, s.Serialize(&buf)) + + return buf.String() +} + +// requireStructuralRoundTrip directly exercises parseTree and serializeTree +// while retaining the fixture's original bytes. +func requireStructuralRoundTrip(t *testing.T, input string) string { + t.Helper() + + specFile, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + tree, err := parseTree(specFile.rawLines) + require.NoError(t, err) + + specFile.rawLines = serializeTree(tree) + + return serializeSpec(t, specFile) +} + +// listFixtures returns the names (basename only) of all *.spec files in +// the curated testdata directory. +func listFixtures(t *testing.T) []string { + t.Helper() + + entries, err := os.ReadDir(testdataSpecsDir) + require.NoError(t, err) + + names := make([]string, 0, len(entries)) + + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".spec") { + continue + } + + names = append(names, entry.Name()) + } + + require.NotEmpty(t, names, "expected at least one fixture in %s", testdataSpecsDir) + + return names +} + +// --- Tier 1: round-trip preservation for every curated fixture. --- + +// TestTestdataRoundTrip parses every fixture in testdata/specs/, serializes +// it back to bytes, and asserts byte-for-byte equality with the source. +// Failures here indicate the parser/serializer is dropping or mutating lines +// for some real-world spec pattern. +func TestTestdataRoundTrip(t *testing.T) { + for _, name := range listFixtures(t) { + t.Run(name, func(t *testing.T) { + raw := loadFixture(t, name) + + got := requireStructuralRoundTrip(t, string(raw)) + assert.Equal(t, string(raw), got, "round-trip mismatch for %s", name) + }) + } +} + +// --- Tier 3: targeted edit-operation tests per fixture pattern. --- + +// TestTestdataAddTagPreservesStructure exercises AddTag against every fixture +// and asserts that: +// 1. The operation succeeds (no parse-state corruption from quirky inputs). +// 2. Serialized output still round-trips through OpenSpec (idempotent parser). +// 3. The injected tag is observable in the rendered output. +func TestTestdataAddTagPreservesStructure(t *testing.T) { + for _, name := range listFixtures(t) { + t.Run(name, func(t *testing.T) { + s := openFixture(t, name) + + require.NoError(t, s.AddTag("", "BuildRequires", "regression-marker")) + + out := serializeSpec(t, s) + assert.Contains(t, out, "BuildRequires: regression-marker", + "injected tag should appear in serialized output") + assert.Equal(t, out, requireStructuralRoundTrip(t, out), + "edited output must structurally parse and serialize") + + // Re-parse the result. The parser should accept its own output. + _, err := OpenSpec(bytes.NewReader([]byte(out))) + require.NoError(t, err, "serialized output should re-parse cleanly") + }) + } +} + +// TestTestdataHasSectionWalksWrappers verifies HasSection finds sections that +// live inside conditional wrappers (straddling, nested, elif-with-sections). +func TestTestdataHasSectionWalksWrappers(t *testing.T) { + cases := []struct { + fixture string + section string + want bool + }{ + // straddling-wrapper: %install and %check live inside %if 0%{?with_tests}. + {"straddling-wrapper.spec", "%install", true}, + {"straddling-wrapper.spec", "%check", true}, + {"straddling-wrapper.spec", "%files", true}, + {"straddling-wrapper.spec", "%post", false}, + + // nested-wrappers: %package devel lives inside %if 0%{?with_devel}. + {"nested-wrappers.spec", "%package", true}, + {"nested-wrappers.spec", "%description", true}, + {"nested-wrappers.spec", "%files", true}, + {"nested-wrappers.spec", "%check", false}, + + // elif-with-sections: each branch contributes a different %package, but + // from the spec's perspective at least one %package header exists. + {"elif-with-sections.spec", "%package", true}, + {"elif-with-sections.spec", "%files", true}, + + // multi-package-mixed: %if-wrapped %package doc. + {"multi-package-mixed.spec", "%package", true}, + {"multi-package-mixed.spec", "%changelog", true}, + } + + for _, testCase := range cases { + t.Run(testCase.fixture+"/"+testCase.section, func(t *testing.T) { + specObj := openFixture(t, testCase.fixture) + + got, err := specObj.HasSection(testCase.section) + require.NoError(t, err) + assert.Equal(t, testCase.want, got) + }) + } +} + +// TestTestdataAppendLinesToSection verifies AppendLinesToSection works through +// straddling and nested conditional wrappers — the lines must land inside the +// targeted section even when the section itself is inside an %if block. +func TestTestdataAppendLinesToSection(t *testing.T) { + cases := []struct { + fixture string + section string + pkg string + marker string + }{ + {"straddling-wrapper.spec", "%install", "", "echo straddling-marker"}, + {"nested-wrappers.spec", "%files", "devel", "/usr/share/nested-marker"}, + {"multi-package-mixed.spec", "%files", "devel", "/usr/share/multi-marker"}, + {"elif-with-sections.spec", "%files", "", "/usr/share/elif-marker"}, + } + + for _, testCase := range cases { + t.Run(testCase.fixture+"/"+testCase.section+"/"+testCase.pkg, func(t *testing.T) { + specObj := openFixture(t, testCase.fixture) + + require.NoError(t, specObj.AppendLinesToSection(testCase.section, testCase.pkg, []string{testCase.marker})) + + out := serializeSpec(t, specObj) + assert.Contains(t, out, testCase.marker, "marker should appear in serialized output") + + // Sanity: parser should accept its own output. + _, err := OpenSpec(bytes.NewReader([]byte(out))) + require.NoError(t, err) + }) + } +} + +// TestTestdataScriptSectionTagShapedSafety asserts that tag-walking operations +// targeting script sections leave shell lines that look like tags untouched. +// This guards against regressions in the isTagBearingSection filter. +func TestTestdataScriptSectionTagShapedSafety(t *testing.T) { + raw := loadFixture(t, "script-section-tag-shaped.spec") + + // Capture the script-section shell lines that look like tags. + scriptyLines := []string{ + `echo "Name: not-a-tag-write"`, + `printf "Version: still-not-a-tag\n"`, + `echo "Requires: bash" >> .build-manifest`, + `echo "License: MIT" | tee -a check.log`, + `echo "Conflicts: previous-version" >&2`, + `echo "Provides: %{name}-runtime" > /var/log/%{name}-post.log`, + } + + for _, line := range scriptyLines { + require.Contains(t, string(raw), line, "fixture must contain %q for the test to be meaningful", line) + } + + specObj, err := OpenSpec(bytes.NewReader(raw)) + require.NoError(t, err) + + // Try to remove every tag named "Name", "Version", "Requires", "License", + // "Conflicts", "Provides" in the main package. Only real preamble tags + // (Name, Version, Release, Summary, License at the top of the file) + // should be considered; the shell lines must be left alone. + for _, tag := range []string{"Name", "Version", "Requires", "License", "Conflicts", "Provides"} { + _, err := specObj.RemoveTagsMatching("", func(t, _ string) bool { + return strings.EqualFold(t, tag) + }) + require.NoError(t, err, "RemoveTagsMatching(%q) should not error", tag) + } + + out := serializeSpec(t, specObj) + + // Every shell-shaped line must still be present. + for _, line := range scriptyLines { + assert.Contains(t, out, line, + "script-section shell line %q must NOT be removed by tag operations", line) + } +} + +// TestTestdataSearchAndReplaceSectionScope verifies that SearchAndReplace +// confined to a section only touches that section. We replace a string that +// appears in both %install and %files of multi-package-mixed.spec, restricted +// to %install, and confirm %files is untouched. +func TestTestdataSearchAndReplaceSectionScope(t *testing.T) { + specObj := openFixture(t, "multi-package-mixed.spec") + + // %make_install appears only in %install for this fixture — replace with a + // marker, then confirm the marker shows up once. + require.NoError(t, specObj.SearchAndReplace("%install", "", "%make_install", "%make_install # PATCHED")) + + out := serializeSpec(t, specObj) + assert.Contains(t, out, "%make_install # PATCHED") + assert.Equal(t, 1, strings.Count(out, "# PATCHED"), + "section-scoped SearchAndReplace must apply exactly once") +} + +// --- Synthetic generator stress test. --- + +// syntheticBuilder composes the same primitive patterns the curated fixtures +// exercise (preamble tags, conditionals, macros, sections) into random spec +// inputs. The test asserts every generated input round-trips through the +// parser/serializer. +type syntheticBuilder struct { + rng *rand.Rand + out strings.Builder + pkgIdx int + condIdx int +} + +func (b *syntheticBuilder) line(s string) { + b.out.WriteString(s) + b.out.WriteByte('\n') +} + +func (b *syntheticBuilder) writePreamble(name string) { + b.line("Name: " + name) + b.line("Version: 1.0") + b.line("Release: 1") + b.line("Summary: Synthetic test fixture") + b.line("License: MIT") + b.line("") +} + +func (b *syntheticBuilder) writeMacroContinuation() { + b.line("%global synth_flags \\") + b.line(" --enable-foo \\") + b.line(" --enable-bar \\") + b.line(" --enable-baz") + b.line("") +} + +func (b *syntheticBuilder) writeIfWrapper(body func()) { + b.condIdx++ + b.line("%if 0%{?with_synth_" + strconv.Itoa(b.condIdx) + "}") + body() + b.line("%endif") + b.line("") +} + +func (b *syntheticBuilder) writeIfElseContent(then, els string) { + b.condIdx++ + b.line("%if 0%{?fedora}") + b.line(then) + b.line("%else") + b.line(els) + b.line("%endif") + b.line("") +} + +func (b *syntheticBuilder) writeElifChain() { + b.condIdx++ + b.line("%if 0%{?rhel}") + b.line("Requires: rhel-thing") + b.line("%elif 0%{?fedora}") + b.line("Requires: fedora-thing") + b.line("%elif 0%{?suse_version}") + b.line("Requires: suse-thing") + b.line("%else") + b.line("Requires: generic-thing") + b.line("%endif") + b.line("") +} + +func (b *syntheticBuilder) writeSubpackage() { + b.pkgIdx++ + + name := "sub" + strconv.Itoa(b.pkgIdx) + + b.line("%package " + name) + b.line("Summary: Sub-package " + name) + b.line("") + b.line("%description " + name) + b.line("Synthetic sub-package " + name + ".") + b.line("") + b.line("%files " + name) + b.line("/usr/share/synth/" + name) + b.line("") +} + +func (b *syntheticBuilder) writeScriptSection(name string) { + b.line(name) + b.line(`echo "Name: not-a-tag"`) + b.line(`printf "Version: still-not-a-tag\n"`) + b.line("make") + b.line("") +} + +func (b *syntheticBuilder) writeFooter() { + b.line("%changelog") + b.line("* Thu Jan 01 1970 Builder - 1.0-1") + b.line("- Synthetic.") +} + +// generateSyntheticSpec composes a random spec from the primitive patterns +// above. The output ends with a newline so byte-for-byte round-trip checks +// match Spec.Serialize behavior. +func generateSyntheticSpec(seed1, seed2 uint64) string { + //nolint:gosec // deterministic synthetic test data, not security-sensitive + builder := &syntheticBuilder{rng: rand.New(rand.NewPCG(seed1, seed2))} + + builder.writePreamble("synthetic") + + // Insert a randomized 0..4 mix of preamble-level primitives. + for range 4 { + switch builder.rng.IntN(5) { + case 0: + builder.writeMacroContinuation() + case 1: + builder.writeElifChain() + case 2: + builder.writeIfElseContent("BuildRequires: fedora-only", "BuildRequires: other") + case 3: + builder.writeIfWrapper(func() { + builder.writeSubpackage() + }) + case 4: + builder.writeSubpackage() + } + } + + builder.line("%description") + builder.line("Synthetic top-level description.") + builder.line("") + + for _, sect := range []string{"%prep", "%build", "%install", "%check"} { + builder.writeScriptSection(sect) + } + + builder.line("%files") + builder.line("/usr/bin/synthetic") + builder.line("") + + builder.writeFooter() + + return builder.out.String() +} + +// TestSyntheticSpecsRoundTrip generates 64 random spec bodies from primitive +// patterns and asserts every one round-trips byte-for-byte. Failures expose a +// composition the parser handles incorrectly even though the individual +// patterns work in isolation. +func TestSyntheticSpecsRoundTrip(t *testing.T) { + const iterations = 64 + + for iteration := range iterations { + //nolint:gosec // iteration is bounded by iterations, no overflow risk + seed1 := uint64(iteration + 1) + //nolint:gosec // iteration is bounded by iterations, no overflow risk + seed2 := uint64(iteration)*1099511628211 + 14695981039346656037 + + t.Run("seed_"+strconv.Itoa(iteration), func(t *testing.T) { + input := generateSyntheticSpec(seed1, seed2) + + out := requireStructuralRoundTrip(t, input) + assert.Equal(t, input, out, + "synthetic spec must round-trip (seed1=%d, seed2=%d)", seed1, seed2) + }) + } +} + +// TestSyntheticSpecsAddTag generates random specs and asserts AddTag preserves +// re-parseability. This is the random-composition analogue of +// TestTestdataAddTagPreservesStructure. +func TestSyntheticSpecsAddTag(t *testing.T) { + const iterations = 32 + + for iteration := range iterations { + //nolint:gosec // iteration is bounded by iterations, no overflow risk + seed1 := uint64(iteration + 1000) + //nolint:gosec // iteration is bounded by iterations, no overflow risk + seed2 := uint64(iteration)*0x9E3779B97F4A7C15 + 0xBF58476D1CE4E5B9 + + t.Run("seed_"+strconv.Itoa(iteration), func(t *testing.T) { + input := generateSyntheticSpec(seed1, seed2) + + s, err := OpenSpec(bytes.NewReader([]byte(input))) + require.NoError(t, err) + + require.NoError(t, s.AddTag("", "BuildRequires", "synth-marker")) + + out := serializeSpec(t, s) + assert.Contains(t, out, "BuildRequires: synth-marker") + assert.Equal(t, out, requireStructuralRoundTrip(t, out), + "edited output must structurally parse and serialize") + }) + } +} + +// --- Issue #203: macro hoisting on subpackage removal. --- + +// lineIndex returns the 0-based line index where target appears as an exact +// trimmed-line match in lines, or -1 if no such line exists. +func lineIndex(lines []string, target string) int { + for i, line := range lines { + if strings.TrimSpace(line) == target { + return i + } + } + + return -1 +} + +// hasLine reports whether any trimmed line in lines equals target. +func hasLine(lines []string, target string) bool { + return lineIndex(lines, target) >= 0 +} + +// hasLineWithPrefix reports whether any trimmed line in lines starts with +// prefix. Useful for header lines like `%package tests` where the trailing +// whitespace may vary. +func hasLineWithPrefix(lines []string, prefix string) bool { + for _, line := range lines { + if strings.HasPrefix(strings.TrimSpace(line), prefix) { + return true + } + } + + return false +} + +// TestTestdataRemoveSubpackageHoistsReferencedMacro is the issue #203 repro +// turned into a regression test. The fixture has `%define testsdir` inside +// `%package tests` and references `%{testsdir}` from `%install` (which is +// unconditional and survives subpackage removal). +// +// Required behavior: removing the `tests` subpackage must hoist the macro +// definition to the root level (before the first removed section) so the +// surviving references in `%install` still resolve. All sections targeting +// the `tests` subpackage must still be removed. +func TestTestdataRemoveSubpackageHoistsReferencedMacro(t *testing.T) { + specObj := openFixture(t, "subpackage-define-referenced.spec") + + require.NoError(t, specObj.RemoveSubpackage("tests")) + + out := serializeSpec(t, specObj) + outLines := strings.Split(out, "\n") + + // The macro definition must survive as its own header line. + macroLine := "%define testsdir %{_libdir}/%{name}/tests-src" + assert.True(t, hasLine(outLines, macroLine), + "referenced macro must be hoisted, not dropped with the subpackage") + + // All subpackage section headers must be gone (line-exact, ignoring the + // description text which legitimately mentions `%%package tests`). + assert.False(t, hasLineWithPrefix(outLines, "%package tests"), + "subpackage header must be removed") + assert.False(t, hasLineWithPrefix(outLines, "%description tests"), + "subpackage description must be removed") + assert.False(t, hasLineWithPrefix(outLines, "%files tests"), + "subpackage files must be removed") + + // The hoisted macro must appear before %install so the surviving + // `%{testsdir}` references resolve. + macroIdx := lineIndex(outLines, macroLine) + installIdx := lineIndex(outLines, "%install") + + require.GreaterOrEqual(t, macroIdx, 0, "hoisted macro must be in output") + require.GreaterOrEqual(t, installIdx, 0, "%install section must remain") + assert.Less(t, macroIdx, installIdx, + "hoisted macro must appear before %%install so the reference resolves") + + // The reference itself must still exist in %install. + assert.Contains(t, out, "%{buildroot}%{testsdir}/python", + "surviving %%install must still reference the hoisted macro") + + // Output must re-parse cleanly. + _, err := OpenSpec(bytes.NewReader([]byte(out))) + require.NoError(t, err, "spec must re-parse after subpackage removal") +} + +// TestTestdataRemoveSubpackageDoesNotHoistUnreferencedMacro verifies the +// negative case: when a `%define` inside a subpackage is only referenced from +// within that same subpackage, removal drops it cleanly (no hoisting needed, +// no noise added to the result). +func TestTestdataRemoveSubpackageDoesNotHoistUnreferencedMacro(t *testing.T) { + specObj := openFixture(t, "subpackage-define-unreferenced.spec") + + require.NoError(t, specObj.RemoveSubpackage("tools")) + + out := serializeSpec(t, specObj) + outLines := strings.Split(out, "\n") + + // The macro must be gone -- no `%define toolsdir ...` line anywhere. + assert.False(t, hasLineWithPrefix(outLines, "%define toolsdir"), + "unreferenced macro must be dropped along with the subpackage") + + // All subpackage section headers must be gone. + assert.False(t, hasLineWithPrefix(outLines, "%package tools"), + "subpackage header must be removed") + assert.False(t, hasLineWithPrefix(outLines, "%description tools"), + "subpackage description must be removed") + assert.False(t, hasLineWithPrefix(outLines, "%files tools"), + "subpackage files must be removed") + + // Output must re-parse cleanly. + _, err := OpenSpec(bytes.NewReader([]byte(out))) + require.NoError(t, err, "spec must re-parse after subpackage removal") +} + +func TestTestdataRemoveSubpackageRejectsTransitiveMacroHoist(t *testing.T) { + specObj := openFixture(t, "subpackage-define-transitive.spec") + + require.ErrorIs(t, specObj.RemoveSubpackage("tests"), ErrUnsafeMacroHoist) +} + +func TestRemoveSubpackageRejectsForwardGlobalDependencies(t *testing.T) { + input := `%package tests +%global foo %{bar} +%global bar value + +%description tests +Tests + +%install +echo %{foo}` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.ErrorIs(t, specObj.RemoveSubpackage("tests"), ErrUnsafeMacroHoist) +} + +func TestRemoveSubpackageRejectsHoistThatChangesEarlierConditional(t *testing.T) { + input := `%if 0%{?feature} +%build +echo enabled +%endif +%package tests +%global feature 1 +%description tests +Tests +%install +echo %{feature}` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.ErrorIs(t, specObj.RemoveSubpackage("tests"), ErrUnsafeMacroHoist) +} + +func TestRemoveSubpackageRejectsHoistWithSurvivingDependencyAfterPreamble(t *testing.T) { + input := `%description +%global bar value +%package tests +%global foo %{bar} +%description tests +Tests +%install +echo %{foo}` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.ErrorIs(t, specObj.RemoveSubpackage("tests"), ErrUnsafeMacroHoist) +} + +func TestRemoveSubpackagePreservesBraceDelimitedMacroAtomically(t *testing.T) { + input := `%package tests +%define helper() %{lua: + print("value") +} +%description tests +Tests +%install +%{helper}` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.NoError(t, specObj.RemoveSubpackage("tests")) + out := serializeSpec(t, specObj) + assert.Contains(t, out, "%define helper() %{lua:\n print(\"value\")\n}") + _, err = OpenSpec(strings.NewReader(out)) + require.NoError(t, err) +} + +func TestRemoveSubpackageIgnoresLiteralMacroBraces(t *testing.T) { + input := `%package tests +%global lbrace { + %global quoted_open "{" + %global quoted "}" + %global escaped %%{not-a-macro} +%description tests +Tests +%install +echo must-survive` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.NoError(t, specObj.RemoveSubpackage("tests")) + out := serializeSpec(t, specObj) + assert.Equal(t, "%install\necho must-survive\n", out) + assert.Equal(t, out, requireStructuralRoundTrip(t, out)) +} + +func TestRemoveSubpackageRejectsUnterminatedMacroConstructTransactionally(t *testing.T) { + input := `%package tests +%global broken %{lua: +print("still open") +%description tests +Tests +%install +echo must-survive` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.Error(t, specObj.RemoveSubpackage("tests")) + assert.Equal(t, input+"\n", serializeSpec(t, specObj)) +} + +func TestRemoveSubpackageRejectsDependencyStateChangesAcrossHoist(t *testing.T) { + for _, stateChange := range []string{ + "%global dep old", + "%global dep replacement", + "%undefine dep", + } { + t.Run(stateChange, func(t *testing.T) { + input := strings.Join([]string{ + "%global dep initial", + "%package tests", + stateChange, + "%global exported %{?dep:bad}%{!?dep:good}", + "%description tests", + "Tests", + "%install", + "echo %{exported}", + }, "\n") + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.ErrorIs(t, specObj.RemoveSubpackage("tests"), ErrUnsafeMacroHoist) + assert.Equal(t, input+"\n", serializeSpec(t, specObj)) + }) + } +} + +func TestRemoveSubpackagePreservesParameterizedMacroReference(t *testing.T) { + input := `%package tests +%define build_with() enabled +%description tests +Tests +%install +echo %{build_with foo}` + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + + require.NoError(t, specObj.RemoveSubpackage("tests")) + out := serializeSpec(t, specObj) + assert.Contains(t, out, "%define build_with() enabled") +} + +func TestTestdataRemoveSubpackageRejectsShadowedMacro(t *testing.T) { + specObj := openFixture(t, "subpackage-define-shadowed.spec") + + require.ErrorIs(t, specObj.RemoveSubpackage("tools"), ErrUnsafeMacroHoist) +} + +func TestRemoveSubpackageRejectsCyclicMacros(t *testing.T) { + input := `Name: cyclic-macros +Version: 1.0 +Release: 1 +Summary: Cyclic %%define chain inside a subpackage +License: MIT + +%description +Main package. + +%package tests +Summary: Tests + +%define alpha %{beta} +%define beta %{alpha} + +%files tests +%{alpha} + +%install +mkdir -p %{buildroot}%{alpha} + +%files +/usr/bin/cyclic-macros + +%changelog +* Thu Jan 01 1970 Builder - 1.0-1 +- Initial fixture. +` + + specObj, err := OpenSpec(bytes.NewReader([]byte(input))) + require.NoError(t, err) + + require.ErrorIs(t, specObj.RemoveSubpackage("tests"), ErrUnsafeMacroHoist) +} + +// TestRemoveSubpackageLogsHoistedMacro verifies that hoisting a referenced +// macro is surfaced at the default (Info) log level rather than relocating the +// definition silently. +func TestRemoveSubpackageLogsHoistedMacro(t *testing.T) { + var logBuf bytes.Buffer + + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelInfo}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + specObj := openFixture(t, "subpackage-define-referenced.spec") + require.NoError(t, specObj.RemoveSubpackage("tests")) + + logOutput := logBuf.String() + assert.Contains(t, logOutput, "Hoisted referenced macro to preamble", + "hoisting must be logged at Info level") + assert.Contains(t, logOutput, "testsdir", + "log must identify the hoisted macro by name") +} + +func TestRemoveSubpackageHoistsMacroReferencedBySectionHeaders(t *testing.T) { + input := strings.Join([]string{ + "Name: app", + "%package tests", + "%define suffix tests", + "%description tests", + "Tests", + "%package -n app-%{suffix}", + "Summary: Uses the surviving macro in a package header", + "%description -n app-%{suffix}", + "Application", + "%files -n app-%{suffix}", + "/usr/bin/app", + "%post -n app-%{suffix}", + "echo post", + }, "\n") + + specObj, err := OpenSpec(strings.NewReader(input)) + require.NoError(t, err) + require.NoError(t, specObj.RemoveSubpackage("tests")) + + out := serializeSpec(t, specObj) + assert.Contains(t, out, "%define suffix tests") + assert.Contains(t, out, "%package -n app-%{suffix}") + assert.Contains(t, out, "%description -n app-%{suffix}") + assert.Contains(t, out, "%files -n app-%{suffix}") + assert.Contains(t, out, "%post -n app-%{suffix}") +} diff --git a/internal/rpm/spec/tree.go b/internal/rpm/spec/tree.go new file mode 100644 index 000000000..c01fae255 --- /dev/null +++ b/internal/rpm/spec/tree.go @@ -0,0 +1,964 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "fmt" + "strings" +) + +// blockKind classifies what a [block] represents in the spec tree. +type blockKind int + +const ( + // rootBlock is the top-level container for the entire spec. + rootBlock blockKind = iota + // sectionBlock is a named section (e.g., %build, %package -n foo). + // The implicit preamble (before any section header) is also a [sectionBlock] + // with an empty [block.Name]. + sectionBlock + // conditionalBlock is a %if/%endif block. May wrap sections (at top level) + // or appear as content inside a section. + conditionalBlock + // textBlock is a contiguous run of raw text lines (leaf node). + textBlock + // macroDefBlock is a %define/%global directive, optionally spanning + // multiple lines via backslash continuation. + macroDefBlock +) + +// block is a recursive node in the spec's structural tree. +// +// The tree is built by [parseTree] and serialized back to lines by [serializeTree]. +// Operations find and manipulate blocks, then serialize to update [Spec.rawLines]. +type block struct { + // Kind classifies this block. + Kind blockKind + // Header is the opening line: section header, conditional directive, or macro + // definition line. Empty for [rootBlock] and [textBlock]. + Header string + // Name is the section keyword (e.g., "%build") or macro name (e.g., "buildflags"). + // Empty for [rootBlock], [conditionalBlock], and [textBlock]. + Name string + // Package is the sub-package name for section blocks (e.g., "devel", "foo"). + // Empty for sections that target the main package. + Package string + // Endif is the %endif line text for [conditionalBlock] nodes. + Endif string + // Lines holds raw text for [textBlock] and [macroDefBlock] leaf nodes + // (including continuation lines for multi-line macros). + Lines []string + // Children holds nested blocks. For [sectionBlock], these are the section's + // content. For [conditionalBlock], these are the "then" branch. For [rootBlock], + // these are top-level sections and conditional wrappers. + Children []*block + // Else holds the "else" branch blocks for [conditionalBlock] nodes. + // nil when there is no %else/%elif branch. + Else []*block + // ElseDirective is the %else/%elif directive line, if present. + ElseDirective string +} + +// parseTree parses raw spec lines into a [block] tree. +// +// The parser runs in two passes: +// 1. Collect conditional pairs (%if/%endif) and section header positions. +// 2. Build the tree, classifying each conditional as a wrapper (spans sections) +// or content block (fully inside a section) based on whether its body contains +// section headers. +// +// Line continuations (backslash at end of line) are respected: continuation bodies +// are never interpreted as section headers or conditional directives. +func parseTree(rawLines []string) (*block, error) { + pairs, err := collectConditionalPairs(rawLines) + if err != nil { + return nil, fmt.Errorf("parsing conditional structure:\n%w", err) + } + + pairByIf := make(map[int]conditionalPair, len(pairs)) + for _, p := range pairs { + pairByIf[p.ifLine] = p + } + + sectionHeaders := findSectionHeaderLines(rawLines) + + sectionHeaderSet := make(map[int]bool, len(sectionHeaders)) + for _, h := range sectionHeaders { + sectionHeaderSet[h] = true + } + + root := &block{Kind: rootBlock} + + err = buildBlockChildren(rawLines, 0, len(rawLines), pairByIf, sectionHeaderSet, root, true) + if err != nil { + return nil, fmt.Errorf("building spec tree:\n%w", err) + } + + // Wrap leading non-section children (preamble content) into an implicit + // preamble sectionBlock with empty Name, matching how Visit treats lines + // before the first section header. This allows findSectionBlock(root, "", "") + // to locate the preamble. + wrapPreamble(root) + + return root, nil +} + +// wrapPreamble wraps the leading non-section children of root into a preamble +// [sectionBlock] with empty Name and Package. If the root already starts with +// a [sectionBlock], no wrapping is needed. +func wrapPreamble(root *block) { + // Find the index of the first sectionBlock or section-wrapping conditionalBlock. + firstSectionIdx := -1 + + for childIdx, child := range root.Children { + if child.Kind == sectionBlock { + firstSectionIdx = childIdx + + break + } + + if child.Kind == conditionalBlock && containsSectionBlocks(child) { + firstSectionIdx = childIdx + + break + } + } + + // If everything is preamble (no sections) or nothing precedes the first section, + // still wrap in a preamble block for uniform access. + preambleEnd := firstSectionIdx + if preambleEnd < 0 { + preambleEnd = len(root.Children) + } + + if preambleEnd == 0 { + // Nothing to wrap, but insert an empty preamble for uniform lookup. + preamble := &block{Kind: sectionBlock, Name: "", Package: ""} + root.Children = append([]*block{preamble}, root.Children...) + + return + } + + preamble := &block{ + Kind: sectionBlock, + Name: "", + Package: "", + Children: root.Children[:preambleEnd], + } + + root.Children = append([]*block{preamble}, root.Children[preambleEnd:]...) +} + +// containsSectionBlocks checks if a block (typically a conditionalBlock) contains +// any sectionBlock children in any branch, recursing through %elif chains. +func containsSectionBlocks(block *block) bool { + for _, child := range block.Children { + if child.Kind == sectionBlock { + return true + } + + if child.Kind == conditionalBlock && containsSectionBlocks(child) { + return true + } + } + + for _, child := range block.Else { + if child.Kind == sectionBlock { + return true + } + + if child.Kind == conditionalBlock && containsSectionBlocks(child) { + return true + } + } + + return false +} + +// findSectionHeaderLines returns the 0-indexed line numbers of all section headers, +// respecting line continuations (backslash-terminated lines suppress the next line). +func findSectionHeaderLines(rawLines []string) []int { + var headers []int + + inCont := false + braceDepth := 0 + + for lineIdx, line := range rawLines { + if inCont { + braceDepth = macroBraceDepthAfter(line, braceDepth) + inCont = strings.HasSuffix(line, "\\") || braceDepth > 0 + + continue + } + + if isSectionHeaderLine(line) { + headers = append(headers, lineIdx) + } + + if _, isMacro := isMacroDefLine(line); isMacro { + braceDepth = macroBraceDepthAfter(line, 0) + inCont = strings.HasSuffix(line, "\\") || braceDepth > 0 + } else { + inCont = strings.HasSuffix(line, "\\") + } + } + + return headers +} + +// isSectionHeaderLine returns true if the line starts a new RPM spec section. +func isSectionHeaderLine(rawLine string) bool { + tokens := strings.Fields(strings.TrimSpace(rawLine)) + if len(tokens) == 0 { + return false + } + + _, known := sectionTypesByName[strings.ToLower(tokens[0])] + + return known +} + +// hasSectionHeaderInRange checks whether any line in [start, end) is a section header. +func hasSectionHeaderInRange(start, end int, sectionHeaderSet map[int]bool) bool { + for lineNum := start; lineNum < end; lineNum++ { + if sectionHeaderSet[lineNum] { + return true + } + } + + return false +} + +// buildBlockChildren parses lines in [start, end) and appends resulting blocks +// to parent.Children. topLevel indicates whether sections can appear (true at +// root level and inside conditional wrappers). +// +//nolint:funlen // Recursive parser with multiple block types. +func buildBlockChildren( + rawLines []string, + start, end int, + pairByIf map[int]conditionalPair, + sectionHeaderSet map[int]bool, + parent *block, + topLevel bool, +) error { + lineIdx := start + inCont := false + + var textBuf []string + + flushText := func() { + if len(textBuf) > 0 { + parent.Children = append(parent.Children, &block{ + Kind: textBlock, + Lines: textBuf, + }) + + textBuf = nil + } + } + + for lineIdx < end { + line := rawLines[lineIdx] + + if inCont { + textBuf = append(textBuf, line) + inCont = strings.HasSuffix(line, "\\") + lineIdx++ + + continue + } + + // Section headers (only at top level). + if topLevel && sectionHeaderSet[lineIdx] { + flushText() + + name, pkg := getSectionNameAndPackageFromHeader(line) + sectionBlock := &block{ + Kind: sectionBlock, + Header: line, + Name: name, + Package: pkg, + } + + sectionEnd := findTreeSectionEnd(lineIdx+1, end, pairByIf, sectionHeaderSet) + + err := buildBlockChildren(rawLines, lineIdx+1, sectionEnd, pairByIf, sectionHeaderSet, sectionBlock, false) + if err != nil { + return err + } + + parent.Children = append(parent.Children, sectionBlock) + lineIdx = sectionEnd + + continue + } + + // Conditional directives. + if conditionalDepthChange(line) == 1 { + flushText() + + pair, ok := pairByIf[lineIdx] + if !ok { + return fmt.Errorf("%%if at line %d has no matching pair", lineIdx+1) + } + + condBlock := &block{ + Kind: conditionalBlock, + Header: line, + Endif: rawLines[pair.endifLine], + } + + bodyStart := lineIdx + 1 + bodyEnd := pair.endifLine + + elseLine := findElseDirectiveLine(rawLines, bodyStart, bodyEnd) + + thenEnd := bodyEnd + if elseLine >= 0 { + thenEnd = elseLine + } + + isWrapper := hasSectionHeaderInRange(bodyStart, bodyEnd, sectionHeaderSet) + + if err := buildConditionalBranches( + rawLines, bodyStart, thenEnd, elseLine, bodyEnd, + pairByIf, sectionHeaderSet, condBlock, isWrapper, + ); err != nil { + return err + } + + parent.Children = append(parent.Children, condBlock) + lineIdx = pair.endifLine + 1 + + continue + } + + // Macro definitions. + if name, ok := isMacroDefLine(line); ok { + flushText() + + macroBlock, nextLineIdx, err := parseMacroDefBlock(rawLines, lineIdx, end, name) + if err != nil { + return err + } + + parent.Children = append(parent.Children, macroBlock) + lineIdx = nextLineIdx + + continue + } + + // Plain text line. + textBuf = append(textBuf, line) + inCont = strings.HasSuffix(line, "\\") + lineIdx++ + } + + flushText() + + return nil +} + +func parseMacroDefBlock(rawLines []string, start, end int, name string) (*block, int, error) { + macroBlock := &block{ + Kind: macroDefBlock, + Header: rawLines[start], + Name: name, + Lines: []string{rawLines[start]}, + } + + braceDepth := macroBraceDepthAfter(rawLines[start], 0) + + lineIdx := start + 1 + if !strings.HasSuffix(rawLines[start], "\\") && braceDepth == 0 { + return macroBlock, lineIdx, nil + } + + for lineIdx < end { + line := rawLines[lineIdx] + macroBlock.Lines = append(macroBlock.Lines, line) + braceDepth = macroBraceDepthAfter(line, braceDepth) + lineIdx++ + + if !strings.HasSuffix(line, "\\") && braceDepth == 0 { + return macroBlock, lineIdx, nil + } + } + + if braceDepth > 0 { + return nil, 0, fmt.Errorf("unterminated macro construct at line %d", start+1) + } + + return macroBlock, lineIdx, nil +} + +// buildConditionalBranches parses the then and optional else/elif branches of a +// conditional block. For %elif chains, the else branch contains a single nested +// [conditionalBlock] whose Header is the %elif directive, forming a linked list. +func buildConditionalBranches( + rawLines []string, + bodyStart, thenEnd, elseLine, bodyEnd int, + pairByIf map[int]conditionalPair, + sectionHeaderSet map[int]bool, + condBlock *block, + isWrapper bool, +) error { + err := buildBlockChildren(rawLines, bodyStart, thenEnd, pairByIf, sectionHeaderSet, condBlock, isWrapper) + if err != nil { + return err + } + + if elseLine < 0 { + return nil + } + + if isElifDirective(rawLines[elseLine]) { + // %elif: create a nested conditionalBlock forming a linked list. + // The inner block has no Endif — only the outermost block owns %endif. + inner := &block{ + Kind: conditionalBlock, + Header: rawLines[elseLine], + } + + // Find the next branch directive (%elif/%else) within the remaining body. + nextElse := findElseDirectiveLine(rawLines, elseLine+1, bodyEnd) + + nextThenEnd := bodyEnd + if nextElse >= 0 { + nextThenEnd = nextElse + } + + if err := buildConditionalBranches( + rawLines, elseLine+1, nextThenEnd, nextElse, bodyEnd, + pairByIf, sectionHeaderSet, inner, isWrapper, + ); err != nil { + return err + } + + condBlock.Else = []*block{inner} + } else { + // %else: terminal branch — store directive and parse content directly. + condBlock.ElseDirective = rawLines[elseLine] + elseContainer := &block{Kind: rootBlock} + + err := buildBlockChildren(rawLines, elseLine+1, bodyEnd, pairByIf, sectionHeaderSet, elseContainer, isWrapper) + if err != nil { + return err + } + + condBlock.Else = elseContainer.Children + } + + return nil +} + +// isElifDirective returns true if the line is a %elif/%elifarch/%elifnarch/%elifos/%elifnos +// directive (as opposed to a plain %else which is a terminal branch). +func isElifDirective(rawLine string) bool { + tokens := strings.Fields(strings.TrimSpace(rawLine)) + if len(tokens) == 0 { + return false + } + + lower := strings.ToLower(tokens[0]) + + return lower != "%else" && isConditionalBranchDirective(rawLine) +} + +// findTreeSectionEnd finds where a section ends: at the next section header at the +// same nesting level, or at a conditional that wraps sections. +func findTreeSectionEnd(start, end int, pairByIf map[int]conditionalPair, sectionHeaderSet map[int]bool) int { + lineIdx := start + + for lineIdx < end { + if sectionHeaderSet[lineIdx] { + return lineIdx + } + + if pair, ok := pairByIf[lineIdx]; ok { + if hasSectionHeaderInRange(lineIdx+1, pair.endifLine, sectionHeaderSet) { + return lineIdx + } + + lineIdx = pair.endifLine + 1 + + continue + } + + lineIdx++ + } + + return end +} + +// findElseDirectiveLine finds the %else/%elif line within [start, end) at +// conditional depth 0. +func findElseDirectiveLine(rawLines []string, start, end int) int { + depth := 0 + inMacroCont := false + braceDepth := 0 + + for lineIdx := start; lineIdx < end; lineIdx++ { + line := rawLines[lineIdx] + if inMacroCont { + braceDepth = macroBraceDepthAfter(line, braceDepth) + inMacroCont = strings.HasSuffix(line, "\\") || braceDepth > 0 + + continue + } + + if _, isMacro := isMacroDefLine(line); isMacro { + braceDepth = macroBraceDepthAfter(line, 0) + inMacroCont = strings.HasSuffix(line, "\\") || braceDepth > 0 + + continue + } + + d := conditionalDepthChange(line) + + switch { + case d == 1: + depth++ + case d == -1: + depth-- + case depth == 0 && isConditionalBranchDirective(line): + return lineIdx + } + } + + return -1 +} + +// isMacroDefLine returns the macro name if the line is a %define or %global directive. +func isMacroDefLine(rawLine string) (string, bool) { + trimmed := strings.TrimSpace(rawLine) + tokens := strings.Fields(trimmed) + + const minMacroDefTokens = 2 + + if len(tokens) < minMacroDefTokens { + return "", false + } + + lower := strings.ToLower(tokens[0]) + if lower == "%define" || lower == "%global" { + // Strip trailing parentheses from macro names with parameters, + // e.g. "%define foo(x)" → "foo". + name := tokens[1] + if idx := strings.IndexByte(name, '('); idx >= 0 { + name = name[:idx] + } + + return name, true + } + + return "", false +} + +// macroBraceDepthAfter tracks actual RPM `%{...}` constructs in a macro +// definition. Literal braces do not open a construct; a doubled percent is an +// escaped literal percent rather than a macro expansion. +func macroBraceDepthAfter(line string, depth int) int { + for idx := 0; idx < len(line); idx++ { + switch { + case line[idx] == '%' && idx+1 < len(line) && line[idx+1] == '{' && + (idx == 0 || line[idx-1] != '%'): + depth++ + idx++ + case line[idx] == '}' && depth > 0: + depth-- + } + } + + return depth +} + +// getSectionNameAndPackageFromHeader extracts the section keyword and package name +// from a section header line. Uses the existing [GetPackageNameFromSectionHeader] +// for package name extraction. +func getSectionNameAndPackageFromHeader(rawLine string) (string, string) { + tokens := strings.Fields(strings.TrimSpace(rawLine)) + if len(tokens) == 0 { + return "", "" + } + + sectName := tokens[0] + + sectType, ok := sectionTypesByName[strings.ToLower(sectName)] + if !ok { + return sectName, "" + } + + pkg := getPackageNameForSection(sectType, tokens) + + return sectName, pkg +} + +// serializeTree flattens a [block] tree back into raw spec lines. +// The result preserves all original whitespace, comments, and blank lines. +func serializeTree(block *block) []string { + var lines []string + + switch block.Kind { + case rootBlock: + for _, child := range block.Children { + lines = append(lines, serializeTree(child)...) + } + + case sectionBlock: + if block.Header != "" { + lines = append(lines, block.Header) + } + + for _, child := range block.Children { + lines = append(lines, serializeTree(child)...) + } + + case conditionalBlock: + lines = append(lines, block.Header) + + for _, child := range block.Children { + lines = append(lines, serializeTree(child)...) + } + + if block.ElseDirective != "" { + lines = append(lines, block.ElseDirective) + } + + if block.Else != nil { + for _, child := range block.Else { + lines = append(lines, serializeTree(child)...) + } + } + + if block.Endif != "" { + lines = append(lines, block.Endif) + } + + case textBlock: + lines = append(lines, block.Lines...) + + case macroDefBlock: + lines = append(lines, block.Lines...) + } + + return lines +} + +// --- Tree query helpers --- + +// walk performs a depth-first, pre-order traversal of the tree rooted at b, +// invoking visit on every block (starting with b itself). Traversal descends +// into Children and, for [conditionalBlock] nodes, the Else branch as well — +// mirroring the structural nesting used throughout the tree. +// +// If visit returns false, the subtree rooted at the current block is pruned: +// its descendants are skipped, but siblings are still visited. Returning true +// continues the traversal. walk is read-only; visitors must not add or remove +// blocks during traversal. +func walk(blk *block, visit func(*block) bool) { + if !visit(blk) { + return + } + + for _, child := range blk.Children { + walk(child, visit) + } + + if blk.Kind == conditionalBlock { + for _, child := range blk.Else { + walk(child, visit) + } + } +} + +// findSectionBlock finds the first section block (in document order) matching +// name and package, searching recursively through conditional wrappers. +func findSectionBlock(root *block, name, pkg string) *block { + var found *block + + walk(root, func(blk *block) bool { + if found != nil { + return false + } + + if blk.Kind == sectionBlock && blk.Name == name && blk.Package == pkg { + found = blk + + return false + } + + return true + }) + + return found +} + +// findAllSectionBlocks returns all section blocks matching name and package. +func findAllSectionBlocks(root *block, name, pkg string) []*block { + var results []*block + + walk(root, func(blk *block) bool { + if blk.Kind == sectionBlock && blk.Name == name && blk.Package == pkg { + results = append(results, blk) + } + + return true + }) + + return results +} + +// findAllSectionBlocksByPackage returns all section blocks matching a package name +// (any section name). +func findAllSectionBlocksByPackage(root *block, pkg string) []*block { + var results []*block + + walk(root, func(blk *block) bool { + if blk.Kind == sectionBlock && blk.Package == pkg { + results = append(results, blk) + } + + return true + }) + + return results +} + +// removeBlockFromParent removes a target block from any parent in the tree. +// It searches recursively through all [conditionalBlock] nesting levels. +func removeBlockFromParent(root *block, target *block) { + root.Children = filterBlocks(root.Children, target) + + for _, child := range root.Children { + if child.Kind == conditionalBlock { + removeFromConditional(child, target) + } + } +} + +func removeFromConditional(cond *block, target *block) { + cond.Children = filterBlocks(cond.Children, target) + + if cond.Else != nil { + cond.Else = filterBlocks(cond.Else, target) + } + + for _, child := range cond.Children { + if child.Kind == conditionalBlock { + removeFromConditional(child, target) + } + } + + for _, child := range cond.Else { + if child.Kind == conditionalBlock { + removeFromConditional(child, target) + } + } +} + +func filterBlocks(blocks []*block, exclude *block) []*block { + result := make([]*block, 0, len(blocks)) + + for _, b := range blocks { + if b != exclude { + result = append(result, b) + } + } + + return result +} + +// validateSectionRemoval checks that removing the given sections is safe. +// It detects patterns where the tree's structural section boundaries don't +// align with RPM's linear section ownership, which would produce incorrect +// output if sections were naively removed. +func validateSectionRemoval(root *block, toRemove []*block) error { + removeSet := make(map[*block]bool, len(toRemove)) + for _, b := range toRemove { + removeSet[b] = true + } + + // Check each level of the tree for unsafe patterns. + return validateRemovalInChildren(root.Children, removeSet) +} + +//nolint:cyclop // The checks enumerate distinct conditional ownership hazards. +func validateRemovalInChildren(children []*block, removeSet map[*block]bool) error { + for childIdx, child := range children { + if child.Kind != conditionalBlock { + continue + } + + if containsSectionBlocks(child) { + preceding := findPrecedingSection(children, childIdx) + related := (preceding != nil && removeSet[preceding]) || + containsRemovedSection(child, removeSet) + + if related && (hasMeaningfulLooseContent(child.Children) || + hasMeaningfulLooseContent(child.Else) || + hasLooseContentAfter(children, childIdx)) { + return fmt.Errorf("%%if block at %q "+ + "has ambiguous linear section ownership:\n%w", + child.Header, ErrConditionalSpansSections) + } + } + + // Check if removing sections from a wrapper would leave orphaned content + // in an adjacent non-wrapper conditional (case: adjacent content conditional + // after a wrapper whose only sections are being removed). + if wouldEmptyWrapper(child, removeSet) && childIdx+1 < len(children) { + next := children[childIdx+1] + if next.Kind == conditionalBlock && !containsSectionBlocks(next) && hasTextOrMacroContent(next.Children) { + return fmt.Errorf("content in %%if block at %q "+ + "would be orphaned after removing the preceding section:\n%w", + next.Header, ErrConditionalSpansSections) + } + } + + // Recurse into wrapper conditional's branches. + if err := validateRemovalInChildren(child.Children, removeSet); err != nil { + return err + } + + if child.Else != nil { + if err := validateRemovalInChildren(child.Else, removeSet); err != nil { + return err + } + } + } + + return nil +} + +func containsRemovedSection(blk *block, removeSet map[*block]bool) bool { + if blk.Kind == sectionBlock && removeSet[blk] { + return true + } + + for _, child := range blk.Children { + if containsRemovedSection(child, removeSet) { + return true + } + } + + for _, child := range blk.Else { + if containsRemovedSection(child, removeSet) { + return true + } + } + + return false +} + +func hasLooseContentAfter(children []*block, conditionalIdx int) bool { + for _, child := range children[conditionalIdx+1:] { + if child.Kind == sectionBlock { + return false + } + + if hasMeaningfulLooseContent([]*block{child}) { + return true + } + } + + return false +} + +func hasMeaningfulLooseContent(blocks []*block) bool { + for _, blk := range blocks { + switch blk.Kind { + case macroDefBlock: + return true + case textBlock: + for _, line := range blk.Lines { + if strings.TrimSpace(line) != "" { + return true + } + } + case rootBlock, sectionBlock, conditionalBlock: + if blk.Kind == conditionalBlock && + (hasMeaningfulLooseContent(blk.Children) || hasMeaningfulLooseContent(blk.Else)) { + return true + } + } + } + + return false +} + +// findPrecedingSection walks backwards from index i in children to find +// the most recent sectionBlock, skipping over text and other blocks. +func findPrecedingSection(children []*block, i int) *block { + for j := i - 1; j >= 0; j-- { + if children[j].Kind == sectionBlock { + return children[j] + } + } + + return nil +} + +func hasTextOrMacroContent(blocks []*block) bool { + for _, b := range blocks { + if b.Kind == textBlock || b.Kind == macroDefBlock { + return true + } + } + + return false +} + +// wouldEmptyWrapper checks if removing the targeted sections would leave +// a wrapper conditional with no section content in either branch. +func wouldEmptyWrapper(cond *block, removeSet map[*block]bool) bool { + if !containsSectionBlocks(cond) { + return false + } + + for _, child := range cond.Children { + if child.Kind == sectionBlock && !removeSet[child] { + return false + } + + if child.Kind == conditionalBlock && hasNonRemovedSectionsDeep(child, removeSet) { + return false + } + } + + for _, child := range cond.Else { + if child.Kind == sectionBlock && !removeSet[child] { + return false + } + + if child.Kind == conditionalBlock && hasNonRemovedSectionsDeep(child, removeSet) { + return false + } + } + + return true +} + +func hasNonRemovedSectionsDeep(block *block, removeSet map[*block]bool) bool { + if block.Kind == sectionBlock && !removeSet[block] { + return true + } + + for _, child := range block.Children { + if hasNonRemovedSectionsDeep(child, removeSet) { + return true + } + } + + if block.Kind == conditionalBlock { + for _, child := range block.Else { + if hasNonRemovedSectionsDeep(child, removeSet) { + return true + } + } + } + + return false +} diff --git a/internal/rpm/spec/tree_api.go b/internal/rpm/spec/tree_api.go new file mode 100644 index 000000000..7258308c9 --- /dev/null +++ b/internal/rpm/spec/tree_api.go @@ -0,0 +1,500 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "fmt" + "regexp" +) + +// specTree is an opaque handle wrapping the parsed structural tree of a spec. +// Operations on the tree are exposed via methods so callers in edit.go do not +// depend on the internal [block] representation. Obtain one via [Spec.mutateTree] +// or [Spec.inspectTree]. +type specTree struct { + root *block +} + +// sectionHandle is an opaque reference to a single section within a [specTree]. +// Returned by [specTree.Section] / [specTree.Sections] and used to apply edits +// to that section's content. +type sectionHandle struct { + block *block + tree *specTree +} + +// mutateTree parses the spec into a tree, runs mutate against it, and serializes +// the tree back into [Spec.rawLines]. If mutate returns an error, [Spec.rawLines] +// is left unchanged. +func (s *Spec) mutateTree(mutate func(*specTree) error) error { + root, err := parseTree(s.rawLines) + if err != nil { + return fmt.Errorf("parsing spec tree:\n%w", err) + } + + tree := &specTree{root: root} + if err := mutate(tree); err != nil { + return err + } + + s.rawLines = serializeTree(root) + + return nil +} + +// inspectTree parses the spec into a tree and passes it to inspect for read-only +// inspection. The tree is discarded after inspect returns; [Spec.rawLines] is +// never modified. +func (s *Spec) inspectTree(inspect func(*specTree) error) error { + root, err := parseTree(s.rawLines) + if err != nil { + return fmt.Errorf("parsing spec tree:\n%w", err) + } + + return inspect(&specTree{root: root}) +} + +// --- specTree query API --- + +// Section returns a handle to the first section matching name and pkg, or nil +// if no such section exists. Searches recursively into conditional wrappers. +func (t *specTree) Section(name, pkg string) *sectionHandle { + b := findSectionBlock(t.root, name, pkg) + if b == nil { + return nil + } + + return §ionHandle{block: b, tree: t} +} + +// HasSection reports whether the tree contains any section with the given name +// (regardless of package qualifier), including sections inside conditional +// wrappers. +func (t *specTree) HasSection(name string) bool { + return hasSectionWithName(t.root, name) +} + +func hasSectionWithName(blk *block, name string) bool { + found := false + + walk(blk, func(b *block) bool { + if b.Kind == sectionBlock && b.Name == name { + found = true + + return false + } + + return true + }) + + return found +} + +// Sections returns handles for every section matching name and pkg, including +// sections inside conditional branches (both %if and %else). +func (t *specTree) Sections(name, pkg string) []*sectionHandle { + return t.handles(findAllSectionBlocks(t.root, name, pkg)) +} + +// SectionsByPackage returns handles for every section associated with the given +// package name (regardless of section keyword). +func (t *specTree) SectionsByPackage(pkg string) []*sectionHandle { + return t.handles(findAllSectionBlocksByPackage(t.root, pkg)) +} + +func (t *specTree) handles(blocks []*block) []*sectionHandle { + hs := make([]*sectionHandle, len(blocks)) + for i, b := range blocks { + hs[i] = §ionHandle{block: b, tree: t} + } + + return hs +} + +// --- specTree mutation API --- + +// RemoveSections removes the given sections from the tree. Removal is validated +// as a set: if any one removal would orphan content or break a conditional's +// semantics, the entire operation fails and the tree is left unmodified. +// +// Macro definitions (`%define` / `%global`) that live inside the removed +// sections but are referenced by surviving content are automatically hoisted +// to the root level just before the first removed section. See +// [hoistReferencedMacros] for the full behavior. +func (t *specTree) RemoveSections(handles []*sectionHandle) error { + blocks := make([]*block, len(handles)) + for i, h := range handles { + blocks[i] = h.block + } + + if err := validateSectionRemoval(t.root, blocks); err != nil { + return err + } + + if err := hoistReferencedMacros(t.root, blocks); err != nil { + return err + } + + for _, b := range blocks { + removeBlockFromParent(t.root, b) + } + + return nil +} + +// --- sectionHandle accessors and mutations --- + +// Name returns the section's keyword (e.g. "%build"). Empty for the preamble. +func (h *sectionHandle) Name() string { return h.block.Name } + +// Package returns the section's package qualifier (e.g. "devel"). Empty for +// sections that target the main package. +func (h *sectionHandle) Package() string { return h.block.Package } + +// AppendLines appends the given lines as a new text block at the end of the +// section's content. +func (h *sectionHandle) AppendLines(lines []string) { + h.block.Children = append(h.block.Children, &block{ + Kind: textBlock, + Lines: lines, + }) +} + +// PrependLines inserts the given lines as a new text block at the start of the +// section's content (right after the section header). +func (h *sectionHandle) PrependLines(lines []string) { + newChild := &block{Kind: textBlock, Lines: lines} + h.block.Children = append([]*block{newChild}, h.block.Children...) +} + +// --- Line-level iteration & mutation --- + +// lineHandle is an opaque reference to a single content line within a tree. +// Mutations (Replace, Remove) are queued during iteration and applied when the +// enclosing [specTree.VisitAllLines] / [sectionHandle.VisitLines] call returns, +// so callers can mutate freely during the walk without invalidating indices. +type lineHandle struct { + // Text is the original line text. Mutations made via Replace do not update + // this field; callers should treat the visited handle as a single snapshot. + Text string + + block *block + idx int + replaced bool + removed bool + newText string +} + +// Replace marks the line for replacement with newText. A subsequent Remove +// overrides any prior Replace; subsequent Replace overrides any prior Remove. +func (lh *lineHandle) Replace(newText string) { + lh.replaced = true + lh.removed = false + lh.newText = newText +} + +// Remove marks the line for deletion. +func (lh *lineHandle) Remove() { + lh.removed = true + lh.replaced = false +} + +// VisitAllLines walks every content line in the spec (text-block lines only; +// macro definitions and section/conditional headers are skipped). The visitor +// receives the enclosing section name and package qualifier plus a handle that +// can buffer Replace/Remove mutations. Mutations are flushed after the walk. +// Returning a non-nil error stops iteration; buffered mutations made prior to +// the error are still flushed. +func (t *specTree) VisitAllLines(visit func(secName, secPkg string, lh *lineHandle) error) error { + var handles []*lineHandle + + visitErr := collectAndVisitLines(t.root, "", "", visit, &handles) + + flushLineMutations(handles) + + return visitErr +} + +// VisitLines walks every content line inside this section, including lines +// nested inside conditional branches. Macro definitions and section/conditional +// headers are skipped. See [specTree.VisitAllLines] for mutation semantics. +func (h *sectionHandle) VisitLines(visit func(lh *lineHandle) error) error { + var handles []*lineHandle + + wrap := func(_, _ string, lh *lineHandle) error { return visit(lh) } + + visitErr := collectAndVisitLines(h.block, h.block.Name, h.block.Package, wrap, &handles) + + flushLineMutations(handles) + + return visitErr +} + +// collectAndVisitLines walks blk, calls visit on every text-line, and records +// each handle for later mutation flushing. +// +//nolint:cyclop // Switch over blockKind with a small recursive call per kind; splitting hurts readability. +func collectAndVisitLines( + blk *block, + secName, secPkg string, + visit func(string, string, *lineHandle) error, + handles *[]*lineHandle, +) error { + switch blk.Kind { + case rootBlock: + for _, child := range blk.Children { + if err := collectAndVisitLines(child, secName, secPkg, visit, handles); err != nil { + return err + } + } + + case sectionBlock: + for _, child := range blk.Children { + if err := collectAndVisitLines(child, blk.Name, blk.Package, visit, handles); err != nil { + return err + } + } + + case conditionalBlock: + for _, child := range blk.Children { + if err := collectAndVisitLines(child, secName, secPkg, visit, handles); err != nil { + return err + } + } + + for _, child := range blk.Else { + if err := collectAndVisitLines(child, secName, secPkg, visit, handles); err != nil { + return err + } + } + + case textBlock: + for i, line := range blk.Lines { + handle := &lineHandle{Text: line, block: blk, idx: i} + *handles = append(*handles, handle) + + if err := visit(secName, secPkg, handle); err != nil { + return err + } + } + + case macroDefBlock: + // Macro definitions are not visited as content lines. + } + + return nil +} + +// flushLineMutations applies buffered Replace/Remove operations. +// Iterates handles in reverse insertion order so per-block removals don't +// invalidate the indices of yet-to-be-applied operations. +func flushLineMutations(handles []*lineHandle) { + for i := len(handles) - 1; i >= 0; i-- { + handle := handles[i] + + switch { + case handle.removed: + handle.block.Lines = append(handle.block.Lines[:handle.idx], handle.block.Lines[handle.idx+1:]...) + case handle.replaced: + handle.block.Lines[handle.idx] = handle.newText + } + } +} + +// --- Tag-aware insertion --- + +// InsertTag inserts a tag-style line into this section, placing it after the +// last existing tag from the same family (e.g., "Source9999" lands after the +// last Source* tag). If no same-family tag exists, the new tag goes after the +// last tag of any kind. If the section has no tags at all, the new line is +// appended to the section's end. +// +// If the chosen anchor tag lives inside a [conditionalBlock], the new tag is +// inserted after the entire conditional block instead, so it remains +// unconditional. +func (h *sectionHandle) InsertTag(tag, value, family string) { + newLine := fmt.Sprintf("%s: %s", tag, value) + + anchor := h.findTagInsertAnchor(family) + if anchor == nil { + h.AppendLines([]string{newLine}) + + return + } + + anchor.insertAfter(h.block, newLine) +} + +// tagInsertAnchor records where a new tag should be placed relative to an +// existing tag. Exactly one of inTextBlock or afterChild is set: +// - inTextBlock != nil: the anchor tag is a direct line inside a textBlock; +// the new line is spliced into that block right after lineIdx. +// - afterChild != nil: the anchor tag lives inside a top-level conditionalBlock +// of the section; the new line goes into a new sibling textBlock immediately +// after afterChild in the section's Children. +type tagInsertAnchor struct { + inTextBlock *block + lineIdx int + afterChild *block +} + +func (a *tagInsertAnchor) insertAfter(parent *block, newLine string) { + if a.inTextBlock != nil { + lines := a.inTextBlock.Lines + spliced := make([]string, 0, len(lines)+1) + spliced = append(spliced, lines[:a.lineIdx+1]...) + spliced = append(spliced, newLine) + spliced = append(spliced, lines[a.lineIdx+1:]...) + a.inTextBlock.Lines = spliced + + return + } + + childIdx := -1 + + for i, child := range parent.Children { + if child == a.afterChild { + childIdx = i + + break + } + } + + newChild := &block{Kind: textBlock, Lines: []string{newLine}} + + if childIdx < 0 { + // Defensive: anchor not found in parent.Children — append. + parent.Children = append(parent.Children, newChild) + + return + } + + spliced := make([]*block, 0, len(parent.Children)+1) + spliced = append(spliced, parent.Children[:childIdx+1]...) + spliced = append(spliced, newChild) + spliced = append(spliced, parent.Children[childIdx+1:]...) + parent.Children = spliced +} + +// findTagInsertAnchor walks the section's top-level children to find the last +// tag matching family (preferred) or the last tag of any kind (fallback). +// Returns nil if the section contains no tags. +func (h *sectionHandle) findTagInsertAnchor(family string) *tagInsertAnchor { + var lastAny, lastFamily *tagInsertAnchor + + for _, child := range h.block.Children { + switch child.Kind { + case textBlock: + for tagLineIdx, line := range child.Lines { + tag, _, isTag := parseTagLine(line) + if !isTag { + continue + } + + anchor := &tagInsertAnchor{inTextBlock: child, lineIdx: tagLineIdx} + lastAny = anchor + + if tagFamily(tag) == family { + lastFamily = anchor + } + } + + case conditionalBlock: + hasAny, hasFamily := scanConditionalForTags(child, family) + if hasAny { + anchor := &tagInsertAnchor{afterChild: child} + lastAny = anchor + + if hasFamily { + lastFamily = anchor + } + } + + case rootBlock, sectionBlock, macroDefBlock: + // Not encountered as a section child (or carry no tag lines). + } + } + + if lastFamily != nil { + return lastFamily + } + + return lastAny +} + +// scanConditionalForTags reports whether the conditional block (any branch, +// any nesting depth) contains at least one tag line, and whether any of those +// tags belongs to the given family. +func scanConditionalForTags(cond *block, family string) (hasAny, hasFamily bool) { + scan := func(blocks []*block) { + for _, b := range blocks { + a, f := scanForTags(b, family) + hasAny = hasAny || a + hasFamily = hasFamily || f + } + } + + scan(cond.Children) + scan(cond.Else) + + return hasAny, hasFamily +} + +func scanForTags(blk *block, family string) (hasAny, hasFamily bool) { + switch blk.Kind { + case textBlock: + for _, line := range blk.Lines { + tag, _, isTag := parseTagLine(line) + if !isTag { + continue + } + + hasAny = true + + if tagFamily(tag) == family { + hasFamily = true + } + } + + case conditionalBlock: + return scanConditionalForTags(blk, family) + + case rootBlock, sectionBlock, macroDefBlock: + // Not searched for tags here. + } + + return hasAny, hasFamily +} + +// tagRegex matches RPM tag lines in the form "Name: value". +var tagRegex = regexp.MustCompile(`^\s*([^\s:]+):\s*(.*?)\s*$`) + +// parseTagLine attempts to parse line as an RPM tag line ("Name: value"). +// Returns the tag name and value, or ok=false if line is not a tag. +func parseTagLine(line string) (tag, value string, ok bool) { + const reSubmatchCount = 3 + + matches := tagRegex.FindStringSubmatch(line) + if len(matches) != reSubmatchCount { + return "", "", false + } + + return matches[1], matches[2], true +} + +// packageSectionName is the canonical section name for sub-package definitions +// (the `%package ` directive). The preamble (empty section name) and these +// sections are the only places where tag-style lines (`Foo: bar`) carry semantic +// meaning; script-style sections such as `%build` may contain lines that match +// the tag regex but are not actually tags. +const packageSectionName = "%package" + +// isTagBearingSection reports whether a section keyword can legally hold RPM +// tag declarations (e.g. "Name:", "Source0:"). Only the preamble (empty name) +// and "%package" sections qualify. Script-style sections like "%build" may +// contain shell that happens to match the "word: word" pattern; we must avoid +// treating those as tags. +func isTagBearingSection(secName string) bool { + return secName == "" || secName == packageSectionName +} diff --git a/internal/rpm/spec/tree_hoist.go b/internal/rpm/spec/tree_hoist.go new file mode 100644 index 000000000..62a4e810f --- /dev/null +++ b/internal/rpm/spec/tree_hoist.go @@ -0,0 +1,474 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "fmt" + "log/slog" + "regexp" + "strings" +) + +// hoistReferencedMacros moves [macroDefBlock] children of soon-to-be-removed +// sections to the preamble when those macros are referenced by content that +// will survive removal. +// +// Motivation (issue #203): spec authors sometimes place `%define` inside a +// `%package` subpackage block (e.g. a `%define testsdir` under `%package tests`) +// even though the macro is referenced by an unconditional section like +// `%install`. Naively removing the subpackage drops the macro and leaves +// dangling `%{testsdir}` references in survivors. Hoisting preserves the +// definition at the end of the preamble (before any section) so survivors +// still resolve regardless of where in the file they sit. +// +// Hoisting is deliberately narrow because RPM macro definitions are ordered +// and scope-sensitive. It moves only one unconditional, unique definition +// with no dependency on another removed definition. Redefinitions, undefines, +// conditionals, and eager forward dependencies are rejected rather than +// guessing at a different evaluation order. +// +// This function mutates root in place. It must be called BEFORE removed +// blocks are detached from the tree so that the "referenced outside the +// removed subtrees" check can compute the survivor set correctly. +func hoistReferencedMacros(root *block, removed []*block) error { + if len(removed) == 0 { + return nil + } + + removedSet := blockSet(removed) + + macros := collectMacrosInSections(removed) + if len(macros) == 0 { + return nil + } + + var referenced []*block + + for _, macro := range macros { + if isMacroReferencedOutside(root, macro.Name, removedSet) { + referenced = append(referenced, macro) + } + } + + if len(referenced) == 0 { + return nil + } + + if len(referenced) != 1 { + return fmt.Errorf("cannot safely hoist multiple referenced macro definitions:\n%w", ErrUnsafeMacroHoist) + } + + macro := referenced[0] + if macroHasUnsafeHoistSemantics(root, macro, removedSet) { + return fmt.Errorf("cannot safely hoist macro %#q because its RPM scope or evaluation order is ambiguous:\n%w", + macro.Name, ErrUnsafeMacroHoist) + } + + // Hoisting moves a definition the caller didn't explicitly touch, so make + // it visible at the default log level rather than silently relocating it. + slog.Info("Hoisted referenced macro to preamble during section removal", + "macro", macro.Name, "definition", strings.TrimSpace(macro.Header)) + + hoistIntoPreamble(root, []*block{macro}) + + return nil +} + +//nolint:cyclop // The checks enumerate the independent RPM scope hazards. +func macroHasUnsafeHoistSemantics(root *block, candidate *block, removedSet map[*block]bool) bool { + definitions := 0 + conditional := false + undefined := false + + var visit func(*block, bool) + + visit = func(blk *block, inConditional bool) { + if blk.Kind == conditionalBlock { + inConditional = true + } + + if blk.Kind == macroDefBlock && blk.Name == candidate.Name { + definitions++ + conditional = conditional || inConditional + } + + if blk.Kind == textBlock || blk.Kind == macroDefBlock { + for _, line := range blk.Lines { + if name, ok := isUndefineLine(line); ok && name == candidate.Name { + undefined = true + } + } + } + + for _, child := range blk.Children { + visit(child, inConditional) + } + + for _, child := range blk.Else { + visit(child, inConditional) + } + } + visit(root, false) + + if definitions != 1 || conditional || undefined { + return true + } + + if hasSurvivingReferenceBeforeCandidate(root, candidate, removedSet) { + return true + } + + for _, name := range macroNamesReferencedIn(candidate.Lines) { + if name == candidate.Name { + return true + } + + if hasDefinitionInRemovedSections(root, name, removedSet) || + hasDefinitionOutsidePreamble(root, name, candidate) || + hasDependencyStateChangeBeforeCandidate(root, name, candidate) { + return true + } + } + + return false +} + +// hasDependencyStateChangeBeforeCandidate reports a define, redefine, or +// undefine event for name that relocation would cross while moving candidate +// to the end of the preamble. Eager macro definitions expand at definition +// time, so crossing any such event can change the candidate's value. +func hasDependencyStateChangeBeforeCandidate(root *block, name string, candidate *block) bool { + preamble := findSectionBlock(root, "", "") + found := false + seenCandidate := false + + var visit func(*block) + + visit = func(blk *block) { + if found || seenCandidate || blk == preamble { + return + } + + if blk == candidate { + seenCandidate = true + + return + } + + if blk.Kind == macroDefBlock && blk.Name == name { + found = true + + return + } + + if blk.Kind == textBlock || blk.Kind == macroDefBlock { + for _, line := range blk.Lines { + if undefined, ok := isUndefineLine(line); ok && undefined == name { + found = true + + return + } + } + } + + for _, child := range blk.Children { + visit(child) + } + + for _, child := range blk.Else { + visit(child) + } + } + visit(root) + + return found +} + +// hasSurvivingReferenceBeforeCandidate rejects relocation when it would make a +// macro visible to content that originally preceded its definition. +// +//nolint:cyclop // The switch covers every block kind while preserving lexical order. +func hasSurvivingReferenceBeforeCandidate(root, candidate *block, removedSet map[*block]bool) bool { + found := false + seenCandidate := false + + var visit func(*block, bool) + + visit = func(blk *block, inRemovedSection bool) { + if found { + return + } + + if blk.Kind == sectionBlock { + inRemovedSection = inRemovedSection || removedSet[blk] + } + + if blk == candidate { + seenCandidate = true + + return + } + + if seenCandidate { + return + } + + if !inRemovedSection { + pattern := macroReferencePattern(candidate.Name) + + switch blk.Kind { + case textBlock, macroDefBlock: + found = anyLineMatches(blk.Lines, pattern) + case conditionalBlock: + found = pattern.MatchString(blk.Header) || + (blk.ElseDirective != "" && pattern.MatchString(blk.ElseDirective)) + case sectionBlock: + found = pattern.MatchString(blk.Header) + case rootBlock: + } + } + + if found { + return + } + + for _, child := range blk.Children { + visit(child, inRemovedSection) + } + + for _, child := range blk.Else { + visit(child, inRemovedSection) + } + } + + visit(root, false) + + return found +} + +// hasDefinitionOutsidePreamble reports a dependency definition that would no +// longer be available when candidate is moved to the preamble. +func hasDefinitionOutsidePreamble(root *block, name string, candidate *block) bool { + preamble := findSectionBlock(root, "", "") + found := false + + walk(root, func(blk *block) bool { + if blk.Kind == macroDefBlock && blk != candidate && blk.Name == name && !isDescendant(preamble, blk) { + found = true + + return false + } + + return true + }) + + return found +} + +func isDescendant(root, target *block) bool { + if root == nil { + return false + } + + if root == target { + return true + } + + for _, child := range root.Children { + if isDescendant(child, target) { + return true + } + } + + for _, child := range root.Else { + if isDescendant(child, target) { + return true + } + } + + return false +} + +func hasDefinitionInRemovedSections(root *block, name string, removedSet map[*block]bool) bool { + found := false + + walk(root, func(blk *block) bool { + if blk.Kind == sectionBlock && removedSet[blk] { + walk(blk, func(descendant *block) bool { + if descendant.Kind == macroDefBlock && descendant.Name == name { + found = true + } + + return !found + }) + + return false + } + + return !found + }) + + return found +} + +// hoistIntoPreamble appends the given macro blocks to the end of the preamble +// section (the implicit section before the first section header), so they are +// defined before any section that might reference them. If no preamble section +// exists, the macros are prepended at the root as a fallback. +func hoistIntoPreamble(root *block, macros []*block) { + if preamble := findSectionBlock(root, "", ""); preamble != nil { + preamble.Children = append(preamble.Children, macros...) + + return + } + + root.Children = append(append([]*block{}, macros...), root.Children...) +} + +// isUndefineLine returns the macro name if the line is a %undefine directive. +func isUndefineLine(rawLine string) (string, bool) { + tokens := strings.Fields(strings.TrimSpace(rawLine)) + + const minUndefineTokens = 2 + + if len(tokens) < minUndefineTokens { + return "", false + } + + if strings.ToLower(tokens[0]) == "%undefine" { + return tokens[1], true + } + + return "", false +} + +// blockSet builds an identity-set of block pointers for O(1) lookup. +func blockSet(blocks []*block) map[*block]bool { + set := make(map[*block]bool, len(blocks)) + for _, b := range blocks { + set[b] = true + } + + return set +} + +// collectMacrosInSections gathers every [macroDefBlock] reachable from any of +// the given section blocks, preserving declaration order across sections. +func collectMacrosInSections(sections []*block) []*block { + var macros []*block + + for _, sec := range sections { + walk(sec, func(b *block) bool { + if b.Kind == macroDefBlock { + macros = append(macros, b) + } + + return true + }) + } + + return macros +} + +// isMacroReferencedOutside walks the tree looking for references to name in +// any block whose enclosing section is NOT in removedSet. References include +// the standard RPM forms: %{name}, %{?name}, %{!?name}, %{name:...}, and bare +// %name terminated by a non-word character. +func isMacroReferencedOutside(root *block, name string, removedSet map[*block]bool) bool { + pattern := macroReferencePattern(name) + found := false + + walk(root, func(blk *block) bool { + // Skip entire subtrees rooted at a removed section — references that + // live inside the removed content are going away too. + if blk.Kind == sectionBlock && removedSet[blk] { + return false + } + + if found { + return false + } + + switch blk.Kind { + case textBlock, macroDefBlock: + // A macro definition outside the removed set may itself reference + // the hoisted macro (e.g. `%define foo %{name}-suffix`). + if anyLineMatches(blk.Lines, pattern) { + found = true + } + case conditionalBlock: + // The %if / %else directives themselves can reference macros + // (e.g. `%if 0%{?with_foo}`). + if pattern.MatchString(blk.Header) || + (blk.ElseDirective != "" && pattern.MatchString(blk.ElseDirective)) { + found = true + } + case sectionBlock: + if pattern.MatchString(blk.Header) { + found = true + } + case rootBlock: + // Container: references live in descendants. + } + + return !found + }) + + return found +} + +// anyLineMatches reports whether any line in lines matches pattern. +func anyLineMatches(lines []string, pattern *regexp.Regexp) bool { + for _, line := range lines { + if pattern.MatchString(line) { + return true + } + } + + return false +} + +// macroReferencePattern builds a regexp that matches references to a named +// RPM macro. Supported forms: +// - %{name}, %{?name}, %{!?name} +// - %{name:default} (parameterized expansion) +// - bare %name terminated by a non-word character or end of string +// +// The bare form requires a word boundary so we don't match %nameOther. +func macroReferencePattern(name string) *regexp.Regexp { + quoted := regexp.QuoteMeta(name) + // Braced: %{ optional ! optional ? NAME ( } | : ... | whitespace args ) + // Bare: %NAME terminated by \b + pattern := `%(?:\{!?\??` + quoted + `(?:[}:]|\s)|` + quoted + `\b)` + + return regexp.MustCompile(pattern) +} + +// macroReferenceNamePattern captures the macro name from any reference form: +// braced (`%{name}`, `%{?name}`, `%{!?name}`, `%{name:...}`) or bare (`%name`). +// It is intentionally permissive — callers filter the captured names against +// the known macro set, so matching directives like `%if` is harmless. +var macroReferenceNamePattern = regexp.MustCompile(`%\{!?\??(\w+)|%(\w+)`) + +// macroNamesReferencedIn returns the names of all macros referenced anywhere in +// the given lines, in order of appearance (with duplicates). Used to discover a +// hoisted definition's dependencies on other removed macros. +func macroNamesReferencedIn(lines []string) []string { + var names []string + + for _, line := range lines { + for _, match := range macroReferenceNamePattern.FindAllStringSubmatch(line, -1) { + name := match[1] + if name == "" { + name = match[2] + } + + if name != "" { + names = append(names, name) + } + } + } + + return names +} diff --git a/internal/rpm/spec/tree_test.go b/internal/rpm/spec/tree_test.go new file mode 100644 index 000000000..8a1310f5b --- /dev/null +++ b/internal/rpm/spec/tree_test.go @@ -0,0 +1,763 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec //nolint:testpackage // Tests access unexported tree types (block, parseTree, etc.). + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- Round-trip tests: parse → serialize must equal original --- + +func TestParseTreeRoundTrip(t *testing.T) { + tests := []struct { + name string + input string + }{ + { + name: "simple spec", + input: `Name: simple +Version: 1.0 +Release: 1 +Summary: A simple package + +%description +A simple package. + +%build +make + +%install +make install + +%files +/usr/bin/simple`, + }, + { + name: "conditional inside section", + input: `Name: test +Version: 1.0 + +%build +%if 0%{?with_debug} +CFLAGS="-g" make +%else +CFLAGS="-O2" make +%endif`, + }, + { + name: "conditional wrapping sections", + input: `Name: test +Version: 1.0 + +%if 0%{?with_docs} +%package docs +Summary: Documentation + +%description docs +Full docs. +%endif + +%build +make`, + }, + { + name: "else branch with different sections", + input: `Name: test +Version: 1.0 + +%if 0%{?with_docs} +%package docs +Summary: Documentation +%description docs +Full docs. +%else +%package minimal-docs +Summary: Minimal documentation +%description minimal-docs +Minimal docs. +%endif + +%build +make`, + }, + { + name: "straddling conditional", + input: `Name: test +Version: 1.0 + +%build +make + +%if 0%{?with_extra} +%install +make install EXTRA=1 +%endif + +%files +/usr/bin/test`, + }, + { + name: "macro definitions", + input: `Name: test +Version: 1.0 + +%global debug_package %{nil} +%define _builddir %{_topdir}/BUILD + +%build +%define buildflags -O2 -Wall +CFLAGS="%{buildflags}" make`, + }, + { + name: "multi-line macro definition", + input: `Name: test +Version: 1.0 + +%define common_flags \ + -DENABLE_FEATURE=ON \ + -DCMAKE_BUILD_TYPE=Release + +%build +cmake %{common_flags} . +make`, + }, + { + name: "continuation with section keyword", + input: `Name: test +Version: 1.0 + +%global extra_config \ + %files \ + something + +%build +make + +%files +/usr/bin/test`, + }, + { + name: "nested conditionals", + input: `Name: test +Version: 1.0 + +%build +%if 0%{?with_feature} +%if 0%{?with_debug} +make debug +%else +make feature +%endif +%endif`, + }, + { + name: "complex real-world pattern", + input: `Name: complex +Version: 2.0 +Release: 1 +Summary: Complex real-world test +License: MIT + +%global debug_package %{nil} +%define _builddir %{_topdir}/BUILD + +%description +A complex package. + +%package devel +Summary: Development files +Requires: %{name} = %{version}-%{release} + +%description devel +Development files for complex. + +%if 0%{?with_docs} +%package docs +Summary: Documentation subpackage + +%description docs +Full documentation. +%endif + +%prep +%autosetup + +%build +%define buildroot_flags --prefix=%{_prefix} +%if 0%{?with_debug} +CFLAGS="-g" ./configure %{buildroot_flags} +%else +./configure %{buildroot_flags} +%endif +make %{?_smp_mflags} + +%install +make install DESTDIR=%{buildroot} + +%if 0%{?with_docs} +%files docs +%doc README.md +%endif + +%files +%license LICENSE +/usr/bin/complex + +%files devel +/usr/include/complex.h + +%changelog`, + }, + { + name: "empty spec", + input: ``, + }, + { + name: "preamble only", + input: `Name: preamble-only`, + }, + { + name: "comments and blank lines", + input: `# This is a comment +Name: test + +# Another comment + +%build +# Build comment +make + +%install +make install`, + }, + { + name: "multiple conditionals at top level", + input: `Name: test + +%if 0%{?with_a} +%package a +Summary: Package A +%endif + +%if 0%{?with_b} +%package b +Summary: Package B +%endif + +%build +make`, + }, + { + name: "elif chain", + input: `Name: test + +%if 0%{?rhel} +Requires: rhel-thing +%elif 0%{?fedora} +Requires: fedora-thing +%elif 0%{?suse} +Requires: suse-thing +%else +Requires: generic-thing +%endif + +%build +make`, + }, + { + name: "elif with sections in branches", + input: `Name: test + +%if 0%{?rhel} +%package rhel-extras +Summary: RHEL extras +%elif 0%{?fedora} +%package fedora-extras +Summary: Fedora extras +%else +%package generic-extras +Summary: Generic extras +%endif + +%build +make`, + }, + { + name: "elif without terminal else", + input: `Name: test + +%if 0%{?rhel} +Requires: rhel-thing +%elif 0%{?fedora} +Requires: fedora-thing +%endif + +%build +make`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lines := splitLines(tt.input) + + root, err := parseTree(lines) + require.NoError(t, err) + + serialized := serializeTree(root) + assert.Equal(t, lines, serialized, "round-trip should preserve all lines exactly") + }) + } +} + +//nolint:maintidx // Comprehensive table-driven structural test covering all block kinds. +func TestParseTreeStructure(t *testing.T) { + t.Run("simple spec sections", func(t *testing.T) { + input := `Name: test +Version: 1.0 + +%description +A test. + +%build +make + +%install +make install + +%files +/usr/bin/test` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + // Preamble is now wrapped in a sectionBlock with empty name. + // Named sections are sectionBlock children. + var sectionNames []string + + for _, child := range root.Children { + if child.Kind == sectionBlock { + sectionNames = append(sectionNames, child.Name) + } + } + + assert.Equal(t, []string{"", "%description", "%build", "%install", "%files"}, sectionNames) + }) + + t.Run("straddling conditional is wrapper not content", func(t *testing.T) { + input := `Name: test + +%build +make + +%if 0%{?with_extra} +%install +make install +%endif + +%files +/usr/bin/test` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + // %build should NOT contain the %if. + buildSect := findSectionBlock(root, "%build", "") + require.NotNil(t, buildSect) + + for _, child := range buildSect.Children { + assert.NotEqual(t, conditionalBlock, child.Kind, + "straddling %%if should be a sibling wrapper, not %%build content") + } + + // %install should be findable inside the conditional wrapper. + installSect := findSectionBlock(root, "%install", "") + assert.NotNil(t, installSect, "%%install should be findable inside conditional wrapper") + }) + + t.Run("conditional inside section is content", func(t *testing.T) { + input := `Name: test + +%build +%if 0%{?with_debug} +make debug +%else +make release +%endif` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + buildSect := findSectionBlock(root, "%build", "") + require.NotNil(t, buildSect) + + hasConditional := false + + for _, child := range buildSect.Children { + if child.Kind == conditionalBlock { + hasConditional = true + + break + } + } + + assert.True(t, hasConditional, "%%if inside section should be a content conditionalBlock") + }) + + t.Run("else branch with sections is wrapper", func(t *testing.T) { + input := `Name: test + +%if 0%{?with_docs} +%package docs +Summary: Docs +%else +%package minimal +Summary: Minimal +%endif + +%build +make` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + docsSect := findSectionBlock(root, "%package", "docs") + assert.NotNil(t, docsSect, "%%package docs in then branch") + + minSect := findSectionBlock(root, "%package", "minimal") + assert.NotNil(t, minSect, "%%package minimal in else branch") + }) + + t.Run("macro def recognized", func(t *testing.T) { + input := `Name: test + +%build +%define buildflags -O2 +CFLAGS="%{buildflags}" make` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + buildSect := findSectionBlock(root, "%build", "") + require.NotNil(t, buildSect) + + hasMacroDef := false + + for _, child := range buildSect.Children { + if child.Kind == macroDefBlock && child.Name == "buildflags" { + hasMacroDef = true + + break + } + } + + assert.True(t, hasMacroDef, "%%define should be recognized as macroDefBlock") + }) + + t.Run("multi-line macro continuation", func(t *testing.T) { + input := `Name: test + +%define flags \ + -DFOO=ON \ + -DBAR=OFF + +%build +make` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + // The macro should have 3 lines (header + 2 continuation). + var macroBlock *block + + for _, child := range root.Children { + if child.Kind == sectionBlock && child.Name == "" { + // Preamble — look for macro. + for _, pChild := range child.Children { + if pChild.Kind == macroDefBlock && pChild.Name == "flags" { + macroBlock = pChild + + break + } + } + } + + if child.Kind == macroDefBlock && child.Name == "flags" { + macroBlock = child + + break + } + } + + require.NotNil(t, macroBlock, "should find macroDefBlock for 'flags'") + assert.Len(t, macroBlock.Lines, 3, "multi-line macro should have 3 lines") + }) + + t.Run("continuation with section keyword not a section", func(t *testing.T) { + input := `Name: test + +%global extra \ + %files \ + stuff + +%build +make + +%files +/usr/bin/test` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + // Only one real %files section. + allFiles := findAllSectionBlocks(root, "%files", "") + assert.Len(t, allFiles, 1, "continuation body should not create phantom %%files section") + }) + + t.Run("find sections by package", func(t *testing.T) { + input := `Name: test + +%package devel +Summary: Dev + +%description devel +Dev files. + +%files devel +/usr/include/* + +%build +make` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + develSections := findAllSectionBlocksByPackage(root, "devel") + assert.Len(t, develSections, 3, "should find 3 sections for 'devel' package") + + var names []string + for _, s := range develSections { + names = append(names, s.Name) + } + + assert.Contains(t, names, "%package") + assert.Contains(t, names, "%description") + assert.Contains(t, names, "%files") + }) + + t.Run("elif chain structure", func(t *testing.T) { + input := `Name: test + +%if 0%{?rhel} +Requires: rhel-thing +%elif 0%{?fedora} +Requires: fedora-thing +%elif 0%{?suse} +Requires: suse-thing +%else +Requires: generic-thing +%endif + +%build +make` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + // Find the conditional block (inside preamble section as content). + var cond *block + + for _, child := range root.Children { + if child.Kind == sectionBlock && child.Name == "" { + for _, pc := range child.Children { + if pc.Kind == conditionalBlock { + cond = pc + + break + } + } + } + } + + require.NotNil(t, cond, "should find conditional in preamble") + assert.Equal(t, "%if 0%{?rhel}", cond.Header) + assert.Equal(t, "%endif", cond.Endif) + + // Then-branch: "Requires: rhel-thing" + require.Len(t, cond.Children, 1) + assert.Equal(t, textBlock, cond.Children[0].Kind) + assert.Equal(t, []string{"Requires: rhel-thing"}, cond.Children[0].Lines) + + // Else is a single conditionalBlock for %elif fedora. + require.Len(t, cond.Else, 1) + elif1 := cond.Else[0] + assert.Equal(t, conditionalBlock, elif1.Kind) + assert.Equal(t, "%elif 0%{?fedora}", elif1.Header) + assert.Empty(t, elif1.Endif, "inner elif should not own %%endif") + + // elif1 then-branch: "Requires: fedora-thing" + require.Len(t, elif1.Children, 1) + assert.Equal(t, []string{"Requires: fedora-thing"}, elif1.Children[0].Lines) + + // elif1 else is another conditionalBlock for %elif suse. + require.Len(t, elif1.Else, 1) + elif2 := elif1.Else[0] + assert.Equal(t, conditionalBlock, elif2.Kind) + assert.Equal(t, "%elif 0%{?suse}", elif2.Header) + assert.Empty(t, elif2.Endif) + + // elif2 then-branch: "Requires: suse-thing" + require.Len(t, elif2.Children, 1) + assert.Equal(t, []string{"Requires: suse-thing"}, elif2.Children[0].Lines) + + // elif2 else is a terminal %else with content blocks. + assert.Equal(t, "%else", elif2.ElseDirective) + require.Len(t, elif2.Else, 1) + assert.Equal(t, []string{"Requires: generic-thing"}, elif2.Else[0].Lines) + }) + + t.Run("elif with sections finds all packages", func(t *testing.T) { + input := `Name: test + +%if 0%{?rhel} +%package rhel-extras +Summary: RHEL extras +%elif 0%{?fedora} +%package fedora-extras +Summary: Fedora extras +%else +%package generic-extras +Summary: Generic extras +%endif + +%build +make` + lines := splitLines(input) + + root, err := parseTree(lines) + require.NoError(t, err) + + for _, pkg := range []string{"rhel-extras", "fedora-extras", "generic-extras"} { + sect := findSectionBlock(root, "%package", pkg) + assert.NotNil(t, sect, "should find %%package %s", pkg) + } + }) +} + +func TestParseTreeErrors(t *testing.T) { + t.Run("unmatched endif", func(t *testing.T) { + input := `Name: test +%endif` + lines := splitLines(input) + + _, err := parseTree(lines) + require.Error(t, err) + assert.Contains(t, err.Error(), "unmatched %endif") + }) + + t.Run("unmatched if", func(t *testing.T) { + input := `Name: test +%if 0%{?foo}` + lines := splitLines(input) + + _, err := parseTree(lines) + require.Error(t, err) + assert.Contains(t, err.Error(), "unmatched %if") + }) + + t.Run("unterminated macro construct", func(t *testing.T) { + input := `%global broken %{lua: +print("still open") +%install +echo must-not-be-consumed` + + _, err := parseTree(splitLines(input)) + require.Error(t, err) + assert.Contains(t, err.Error(), "unterminated macro construct") + }) +} + +func TestWhitespaceLinesDoNotPanic(t *testing.T) { + lines := []string{"", " \t ", "%if 0", " ", "%else", "\t", "%endif"} + + root, err := parseTree(lines) + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(root)) + assert.False(t, isElifDirective(" \t ")) +} + +func TestParseTreePreservesEmptyConditionalBranches(t *testing.T) { + tests := []string{ + "%if 1\n%else\n%endif", + "%if 1\n%elif 0\n%endif", + "%if 1\n%elif 0\n%else\n%endif", + } + + for _, input := range tests { + t.Run(input, func(t *testing.T) { + root, err := parseTree(splitLines(input)) + require.NoError(t, err) + assert.Equal(t, splitLines(input), serializeTree(root)) + }) + } +} + +func TestMacroContinuationsDoNotCreateConditionalBranches(t *testing.T) { + for _, directive := range []string{"%else", "%elif 0"} { + t.Run(directive, func(t *testing.T) { + input := strings.Join([]string{ + "%if 1", + "%define example \\", + directive + " \\", + " body", + "%endif", + }, "\n") + + root, err := parseTree(splitLines(input)) + require.NoError(t, err) + assert.Equal(t, splitLines(input), serializeTree(root)) + + cond := root.Children[0].Children[0] + assert.Empty(t, cond.Else) + assert.Empty(t, cond.ElseDirective) + }) + } +} + +func TestMacroConstructTrackingIgnoresLiteralAndEscapedBraces(t *testing.T) { + input := strings.Join([]string{ + "%package tests", + "%global lbrace {", + `%global quoted_open "{"`, + `%global quoted "}"`, + "%global escaped %%{not-a-macro}", + "%description tests", + "Tests", + "%install", + "echo must-survive", + }, "\n") + + root, err := parseTree(splitLines(input)) + require.NoError(t, err) + assert.Equal(t, splitLines(input), serializeTree(root)) +} + +// splitLines splits input into lines. For an empty string, returns a slice with +// one empty element (matching strings.Split behavior). +func splitLines(input string) []string { + return strings.Split(input, "\n") +} diff --git a/scenario/__snapshots__/TestSnapshotsContainer_config_generate-schema_stdout_1.snap b/scenario/__snapshots__/TestSnapshotsContainer_config_generate-schema_stdout_1.snap index 5f5542a20..839bb5bf9 100755 --- a/scenario/__snapshots__/TestSnapshotsContainer_config_generate-schema_stdout_1.snap +++ b/scenario/__snapshots__/TestSnapshotsContainer_config_generate-schema_stdout_1.snap @@ -229,6 +229,7 @@ "spec-update-tag", "spec-remove-tag", "spec-prepend-lines", + "spec-prepend-all-lines", "spec-append-lines", "spec-search-replace", "spec-remove-section", diff --git a/scenario/__snapshots__/TestSnapshots_config_generate-schema_stdout_1.snap b/scenario/__snapshots__/TestSnapshots_config_generate-schema_stdout_1.snap index 5f5542a20..839bb5bf9 100755 --- a/scenario/__snapshots__/TestSnapshots_config_generate-schema_stdout_1.snap +++ b/scenario/__snapshots__/TestSnapshots_config_generate-schema_stdout_1.snap @@ -229,6 +229,7 @@ "spec-update-tag", "spec-remove-tag", "spec-prepend-lines", + "spec-prepend-all-lines", "spec-append-lines", "spec-search-replace", "spec-remove-section", diff --git a/schemas/azldev.schema.json b/schemas/azldev.schema.json index 5f5542a20..839bb5bf9 100644 --- a/schemas/azldev.schema.json +++ b/schemas/azldev.schema.json @@ -229,6 +229,7 @@ "spec-update-tag", "spec-remove-tag", "spec-prepend-lines", + "spec-prepend-all-lines", "spec-append-lines", "spec-search-replace", "spec-remove-section",