From 4a0313914457c8ff442a0bf5fadf598047e2ccff Mon Sep 17 00:00:00 2001 From: Thien Trung Vuong Date: Thu, 14 May 2026 17:49:44 +0000 Subject: [PATCH] fix: balance conditional nesting when removing spec sections collectSectionRanges now adjusts each section range to maintain balanced %if/%endif nesting before removal. This prevents producing invalid specs when sections are adjacent to or wrapped in conditional blocks. Three cases are handled automatically: - Balanced conditionals inside a section (no change needed) - Trailing %if openers belonging to the next section (trimmed) - Trailing %endif from a wrapping conditional block (trimmed, leaving an empty but valid conditional wrapper) Conditionals interleaved with section content (spanning case) are detected and reported with an ErrConditionalSpansSections error. After each range removal, consecutive blank lines at the splice boundary are collapsed to prevent multi-blank gaps in the output. Fixes #144 --- docs/user/reference/config/overlays.md | 21 +- internal/rpm/spec/edit.go | 257 +++++++++++++++++++++++- internal/rpm/spec/edit_test.go | 266 +++++++++++++++++++++++++ 3 files changed, 528 insertions(+), 16 deletions(-) diff --git a/docs/user/reference/config/overlays.md b/docs/user/reference/config/overlays.md index e8ab1495..55061118 100644 --- a/docs/user/reference/config/overlays.md +++ b/docs/user/reference/config/overlays.md @@ -297,6 +297,11 @@ package = "devel" description = "Remove devel sub-package files section" ``` +> **Conditionals (`%if`/`%endif`):** The same conditional handling described below for +> `spec-remove-subpackage` applies here as well — boundary conditionals are preserved, +> and an error is returned if a conditional block is interleaved with section content +> in a way that cannot be cleanly separated. + ### Removing an Entire Sub-package The `spec-remove-subpackage` overlay removes **every** section associated with a given @@ -331,12 +336,16 @@ the overlay always removes every section associated with the sub-package. > written as `%files -n my-other-pkg`. Specs that mix both forms for the same sub-package > (uncommon but legal) require a separate overlay per form. -> **Limitation:** Like `spec-remove-section`, this overlay operates on whole-section line -> ranges and does **not** understand `%if`/`%endif` conditionals. If a sub-package is wrapped -> in a conditional block (e.g. `%if 0%{?with_devel} … %endif`), the `%endif` will typically -> be consumed as part of the trailing sub-package section while the `%if` remains, producing -> an invalid spec. For specs that conditionalize sub-packages, use a `spec-search-replace` -> overlay (or remove the conditional first via additional overlays) instead. +> **Conditionals (`%if`/`%endif`):** The overlay only removes section content — it does +> not remove `%if`/`%endif` lines that sit at section boundaries. Conditional directives +> that are entirely within a section (e.g. `%ifarch` … `%endif` guarding a `Requires` +> tag) are removed along with the section. Conditional directives that straddle a +> section boundary are left in place so the spec remains valid. For example, if a +> sub-package is wrapped in `%if 0%{?with_devel}` … `%endif`, removing the sub-package +> leaves an empty `%if` … `%endif` block behind (which is harmless). If a conditional +> block is interleaved with section content in a way that cannot be cleanly separated, +> an error is returned; use a `spec-search-replace` overlay to adjust the conditionals +> before removing the sub-package. ## Validation diff --git a/internal/rpm/spec/edit.go b/internal/rpm/spec/edit.go index ec59837e..9eb824fc 100644 --- a/internal/rpm/spec/edit.go +++ b/internal/rpm/spec/edit.go @@ -21,6 +21,11 @@ var ErrNoSuchTag = errors.New("no such tag") // ErrSectionNotFound is returned when a requested section does not exist in the spec. var ErrSectionNotFound = errors.New("section not found") +// ErrConditionalSpansSections is returned when a conditional block (%if/%endif) spans +// across section boundaries, making it impossible to safely remove the section without +// breaking the conditional nesting structure. +var ErrConditionalSpansSections = errors.New("conditional block spans across section boundaries") + // ErrPatternNotFound is returned when a search pattern does not match any content in the spec. var ErrPatternNotFound = errors.New("pattern not found") @@ -195,8 +200,10 @@ func tagFamily(tag string) string { return lower[:end] } -// conditionalDepthChange returns +1 for lines that open a conditional block (%if, %ifarch, etc.), +// conditionalDepthChange returns +1 for lines that open a conditional block, // -1 for %endif, and 0 for everything else. Comments are ignored. +// +// The recognized conditional openers are: %if, %ifarch, %ifnarch, %ifos, %ifnos. func conditionalDepthChange(rawLine string) int { trimmed := strings.TrimSpace(rawLine) if strings.HasPrefix(trimmed, "#") { @@ -209,15 +216,41 @@ func conditionalDepthChange(rawLine string) int { } lower := strings.ToLower(token[0]) - if lower == "%endif" { + + switch lower { + case "%endif": return -1 + case "%if", "%ifarch", "%ifnarch", "%ifos", "%ifnos": + return 1 + default: + return 0 } +} - if strings.HasPrefix(lower, "%if") { - return 1 +// isConditionalBranchDirective returns true for lines that are branch directives +// within a conditional block. These do not change nesting depth but mark branch +// boundaries within an enclosing %if/%endif pair. Comments are ignored. +// +// The recognized branch directives are: %else, %elif, %elifarch, %elifnarch, %elifos, %elifnos. +func isConditionalBranchDirective(rawLine string) bool { + trimmed := strings.TrimSpace(rawLine) + if strings.HasPrefix(trimmed, "#") { + return false } - return 0 + tokens := strings.Fields(trimmed) + if len(tokens) == 0 { + return false + } + + lower := strings.ToLower(tokens[0]) + + switch lower { + case "%else", "%elif", "%elifarch", "%elifnarch", "%elifos", "%elifnos": + return true + default: + return false + } } // InsertTag inserts a tag into the spec, placing it after the last existing tag from the @@ -792,11 +825,12 @@ func (s *Spec) RemoveSection(sectionName, packageName string) error { // whichever form the spec uses. Specs that mix both forms for the same sub-package // (uncommon but legal) require a call per form. // -// Limitation: this method operates on whole-section line ranges and does not understand -// `%if`/`%endif` conditionals. Sub-packages wrapped in a conditional block will leave -// an unbalanced `%if` (the `%endif` is typically consumed as part of the trailing -// sub-package section). Callers that need to handle conditionalized sub-packages must -// adjust the conditionals themselves. +// Conditional handling: section ranges are automatically trimmed to maintain balanced +// `%if`/`%endif` nesting. Sections wrapped in a conditional block will have trailing +// `%endif` lines excluded from the removal, leaving an empty (but valid) conditional +// wrapper. Trailing `%if` lines that belong to the next section are similarly excluded. +// 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) RemoveSubpackage(packageName string) error { slog.Debug("Removing sub-package from spec", "package", packageName) @@ -831,6 +865,13 @@ type sectionLineRange struct { // 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) { @@ -866,9 +907,205 @@ func (s *Spec) collectSectionRanges( 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. +type conditionalPair struct { + ifLine int + endifLine int +} + +// 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. +func collectConditionalPairs(rawLines []string) ([]conditionalPair, error) { + var ( + pairs []conditionalPair + stack []int + ) + + for lineNum, line := range rawLines { + switch conditionalDepthChange(line) { + case 1: + stack = append(stack, lineNum) + case -1: + if len(stack) == 0 { + return nil, fmt.Errorf("unmatched %%endif at line %d", lineNum+1) + } + + ifLine := stack[len(stack)-1] + stack = stack[:len(stack)-1] + + pairs = append(pairs, conditionalPair{ifLine: ifLine, endifLine: lineNum}) + } + } + + 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. diff --git a/internal/rpm/spec/edit_test.go b/internal/rpm/spec/edit_test.go index e9d1bd7b..1c63b806 100644 --- a/internal/rpm/spec/edit_test.go +++ b/internal/rpm/spec/edit_test.go @@ -1690,6 +1690,7 @@ Main. } } +//nolint:maintidx // Test table complexity scales with the number of conditional handling scenarios. func TestRemoveSubpackage(t *testing.T) { tests := []struct { name string @@ -1840,6 +1841,271 @@ Main description. %files /usr/bin/test +`, + }, + { + name: "handles balanced conditional inside section", + input: `Name: test + +%description +Main. + +%package devel +Summary: Devel +%ifarch x86_64 +Requires: special-x86-lib +%endif + +%description devel +Devel description. + +%files +/usr/bin/test +`, + packageName: "devel", + expectedOutput: `Name: test + +%description +Main. + +%files +/usr/bin/test +`, + }, + { + name: "trims trailing conditional opener belonging to next section", + input: `Name: test + +%description +Main. + +%files foo +/usr/share/foo + +%if 0 +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test +`, + packageName: "foo", + expectedOutput: `Name: test + +%description +Main. + +%if 0 +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test +`, + }, + { + name: "trims trailing endif from section wrapped in conditional", + input: `Name: test + +%description +Main. + +%if 0%{?with_devel} +%package devel +Summary: Devel + +%description devel +Devel description. + +%files devel +/usr/include/test.h +%endif + +%files +/usr/bin/test +`, + packageName: "devel", + expectedOutput: `Name: test + +%description +Main. + +%if 0%{?with_devel} +%endif + +%files +/usr/bin/test +`, + }, + { + name: "errors on conditional spanning across sections", + input: `Name: test + +%files foo +/usr/share/foo1 +%if 0%{?with_extra} +/usr/share/foo-extra + +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test +`, + packageName: "foo", + errorExpected: true, + errorContains: "conditional block spans", + }, + { + name: "errors on else branch inside straddling conditional", + input: `Name: test + +%description +Main. + +%if cond +%files foo +/usr/share/foo +%else +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test +`, + packageName: "foo", + errorExpected: true, + errorContains: "branch directive", + }, + { + name: "errors when trimmed zone contains section content in a balanced conditional", + input: `Name: test + +%description +Main. + +%if 0%{?with_devel} +%files devel +/usr/include/test.h +%endif +%if 0%{?with_extra} +/usr/include/extra.h +%endif + +%files +/usr/bin/test +`, + packageName: "devel", + errorExpected: true, + errorContains: "conditional block spans", + }, + { + name: "trims consecutive endifs from nested wrapping conditionals", + input: `Name: test + +%description +Main. + +%if A +%if B +%files devel +/usr/include/test.h +%endif +%endif + +%files +/usr/bin/test +`, + packageName: "devel", + expectedOutput: `Name: test + +%description +Main. + +%if A +%if B +%endif +%endif + +%files +/usr/bin/test +`, + }, + { + name: "trims consecutive if openers belonging to next sections", + input: `Name: test + +%description +Main. + +%files foo +/usr/share/foo + +%if 0 +%if 0 +%files bar +/usr/share/bar +%endif +%endif + +%files +/usr/bin/test +`, + packageName: "foo", + expectedOutput: `Name: test + +%description +Main. + +%if 0 +%if 0 +%files bar +/usr/share/bar +%endif +%endif + +%files +/usr/bin/test +`, + }, + { + name: "trims mixed endif then if at tail", + input: `Name: test + +%description +Main. + +%if A +%files devel +/usr/include/test.h +%endif +%if 0 +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test +`, + packageName: "devel", + expectedOutput: `Name: test + +%description +Main. + +%if A +%endif +%if 0 +%files bar +/usr/share/bar +%endif + +%files +/usr/bin/test `, }, }