From 9527eae3ffd9b376ca509bf25ce363eeb32a1bc5 Mon Sep 17 00:00:00 2001 From: Thien Trung Vuong Date: Mon, 24 Aug 2026 20:42:10 +0000 Subject: [PATCH 1/5] refactor(spec): add lossless structural parser Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/rpm/spec/tree.go | 898 ++++++++++++++++++++++ internal/rpm/spec/tree_raw_braces_test.go | 35 + internal/rpm/spec/tree_test.go | 192 +++++ 3 files changed, 1125 insertions(+) create mode 100644 internal/rpm/spec/tree.go create mode 100644 internal/rpm/spec/tree_raw_braces_test.go create mode 100644 internal/rpm/spec/tree_test.go diff --git a/internal/rpm/spec/tree.go b/internal/rpm/spec/tree.go new file mode 100644 index 000000000..291b3e709 --- /dev/null +++ b/internal/rpm/spec/tree.go @@ -0,0 +1,898 @@ +// 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 +} + +// treeConditionalPair represents a matched `%if`/`%endif` pair by their line numbers. +type treeConditionalPair struct { + ifLine int + endifLine int +} + +// 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. +// +// Only '%define' and '%global' continuation bodies are opaque. Ordinary script +// backslashes do not suppress RPM section headers or conditional directives. +func parseTree(rawLines []string) (*block, error) { + pairs, err := collectTreeConditionalPairs(rawLines) + if err != nil { + return nil, fmt.Errorf("parsing conditional structure:\n%w", err) + } + + pairByIf := make(map[int]treeConditionalPair, 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 +} + +// collectTreeConditionalPairs matches conditionals while treating macro bodies as +// opaque. Unlike the line-oriented editor helper, the structural parser must not +// interpret directive-shaped macro content. +func collectTreeConditionalPairs(rawLines []string) ([]treeConditionalPair, error) { + var ( + pairs []treeConditionalPair + stack []int + inMacroBody bool + parseState macroState + ) + + for lineNum, line := range rawLines { + if inMacroBody { + parseState, inMacroBody = macroBodyStateAfter(line, parseState) + + continue + } + + if _, isMacro := isMacroDefLine(line); isMacro { + parseState, inMacroBody = macroBodyStateAfter(line, macroState{}) + + continue + } + + 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, treeConditionalPair{ifLine: ifLine, endifLine: lineNum}) + } + } + + if len(stack) > 0 { + return nil, fmt.Errorf("unmatched %%if at line %d", stack[0]+1) + } + + return pairs, 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 + + inMacroBody := false + macroParseState := macroState{} + + for lineIdx, line := range rawLines { + if inMacroBody { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroParseState) + + continue + } + + if isSectionHeaderLine(line) { + headers = append(headers, lineIdx) + } + + if _, isMacro := isMacroDefLine(line); isMacro { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroState{}) + } + } + + 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]treeConditionalPair, + sectionHeaderSet map[int]bool, + parent *block, + topLevel bool, +) error { + lineIdx := start + + 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] + + // 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) + 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]}, + } + + state, continues := macroBodyStateAfter(rawLines[start], macroState{}) + + lineIdx := start + 1 + if !continues { + return macroBlock, lineIdx, nil + } + + for lineIdx < end { + line := rawLines[lineIdx] + macroBlock.Lines = append(macroBlock.Lines, line) + state, continues = macroBodyStateAfter(line, state) + lineIdx++ + + if !continues { + return macroBlock, lineIdx, nil + } + } + + return nil, 0, fmt.Errorf("unterminated macro construct at line %d", start+1) +} + +// 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]treeConditionalPair, + 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]treeConditionalPair, 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 + inMacroBody := false + macroParseState := macroState{} + + for lineIdx := start; lineIdx < end; lineIdx++ { + line := rawLines[lineIdx] + if inMacroBody { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroParseState) + + continue + } + + if _, isMacro := isMacroDefLine(line); isMacro { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroState{}) + + 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 +} + +type macroState struct { + depth int + escapedBraces int + shellBraces int + rawBraces int + lua *luaState +} + +type luaState struct { + braces int + nestedRPM int + quote byte + escaped bool + longClose string +} + +func (state macroState) open() bool { + return state.depth > 0 || state.escapedBraces > 0 || state.lua != nil +} + +// percentRunOpensBracedMacro reports whether the percent run at start ends in +// an active '%{' opener. RPM escapes percent pairs, leaving only odd runs live. +func percentRunOpensBracedMacro(content string, start int) bool { + if start >= len(content) || content[start] != '%' { + return false + } + + end := start + for end < len(content) && content[end] == '%' { + end++ + } + + return end < len(content) && content[end] == '{' && (end-start)%2 != 0 +} + +// macroBodyStateAfter advances the parser state for one physical macro-body +// line and reports whether the body continues onto another line. +func macroBodyStateAfter(line string, state macroState) (macroState, bool) { + state = macroStateAfter(line, state) + + return state, strings.HasSuffix(line, "\\") || state.open() +} + +// macroStateAfter tracks RPM macro constructs in a '%define'/'%global' body. +// Lua has its own syntax, so raw Lua braces, strings, comments, and nested RPM +// expansions are accounted for before deciding that the outer '%{lua:...}' +// expansion has ended. +// +//nolint:cyclop // Macro and shell delimiters require independent lexical states. +func macroStateAfter(line string, state macroState) macroState { + for idx := 0; idx < len(line); { + if state.lua != nil { + consumed, closed := state.lua.consume(line[idx:]) + idx += consumed + + if closed { + state.lua = nil + state.depth-- + } + + continue + } + + if state.escapedBraces > 0 { + state, idx = consumeEscapedBracedMacro(line, idx, state) + + continue + } + + switch { + case line[idx] == '%' && (idx == 0 || line[idx-1] != '%'): + state, idx = macroStateAfterPercentRun(line, idx, state) + case line[idx] == '$' && idx+1 < len(line) && line[idx+1] == '{': + state.shellBraces++ + idx += 2 + case line[idx] == '}' && state.shellBraces > 0: + state.shellBraces-- + idx++ + case line[idx] == '{' && state.depth > 0: + state.rawBraces++ + idx++ + case line[idx] == '}' && state.rawBraces > 0: + state.rawBraces-- + idx++ + case line[idx] == '}' && state.depth > 0: + state.depth-- + idx++ + default: + idx++ + } + } + + // Lua treats a backslash followed by a physical newline as one escaped + // newline. The next line starts with a fresh escape state. + if state.lua != nil { + state.lua.escaped = false + } + + return state +} + +func consumeEscapedBracedMacro(line string, idx int, state macroState) (macroState, int) { + switch line[idx] { + case '{': + state.escapedBraces++ + case '}': + state.escapedBraces-- + } + + return state, idx + 1 +} + +func macroStateAfterPercentRun(line string, idx int, state macroState) (macroState, int) { + runEnd := idx + for runEnd < len(line) && line[runEnd] == '%' { + runEnd++ + } + + if runEnd >= len(line) || line[runEnd] != '{' { + return state, runEnd + } + + if (runEnd-idx)%2 == 0 { + state.escapedBraces++ + + return state, runEnd + 1 + } + + state.depth++ + if strings.HasPrefix(line[runEnd-1:], "%{lua:") { + state.lua = &luaState{} + + return state, runEnd - 1 + len("%{lua:") + } + + return state, runEnd + 1 +} + +// consume scans one Lua body fragment. It returns whether the outer RPM Lua +// expansion closes. Lua line comments naturally end at the next physical line. +// +//nolint:cyclop,funlen // Lua lexical states must be recognized before structural braces. +func (state *luaState) consume(text string) (int, bool) { + const ( + longOpenLength = 2 + longCommentLength = 4 + ) + + for idx := 0; idx < len(text); { + if state.longClose != "" { + if strings.HasPrefix(text[idx:], state.longClose) { + idx += len(state.longClose) + state.longClose = "" + } else { + idx++ + } + + continue + } + + if state.quote != 0 { + switch { + case state.escaped: + state.escaped = false + case text[idx] == '\\': + state.escaped = true + case text[idx] == state.quote: + state.quote = 0 + } + + idx++ + + continue + } + + if delimiter, ok := luaLongDelimiter(text[idx:]); ok { + state.longClose = "]" + delimiter + "]" + idx += len(delimiter) + longOpenLength + + continue + } + + if strings.HasPrefix(text[idx:], "--") { + if delimiter, ok := luaLongDelimiter(text[idx+2:]); ok { + state.longClose = "]" + delimiter + "]" + idx += len(delimiter) + longCommentLength + + continue + } + + return len(text), false + } + + switch { + case text[idx] == '\'', text[idx] == '"': + state.quote = text[idx] + case text[idx] == '%' && idx+1 < len(text) && text[idx+1] == '{': + state.nestedRPM++ + idx++ + case text[idx] == '}': + switch { + case state.nestedRPM > 0: + state.nestedRPM-- + case state.braces > 0: + state.braces-- + default: + return idx + 1, true + } + case text[idx] == '{': + state.braces++ + } + + idx++ + } + + return len(text), false +} + +// luaLongDelimiter recognizes the '=' run in a Lua long-bracket opener. +func luaLongDelimiter(text string) (string, bool) { + if len(text) == 0 || text[0] != '[' { + return "", false + } + + idx := 1 + for idx < len(text) && text[idx] == '=' { + idx++ + } + + if idx >= len(text) || text[idx] != '[' { + return "", false + } + + return text[1:idx], true +} + +// 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 +} diff --git a/internal/rpm/spec/tree_raw_braces_test.go b/internal/rpm/spec/tree_raw_braces_test.go new file mode 100644 index 000000000..25dd16d92 --- /dev/null +++ b/internal/rpm/spec/tree_raw_braces_test.go @@ -0,0 +1,35 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec //nolint:testpackage // Tests access unexported parser tree types. + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseTreeKeepsRawBracesInsideExpandBody(t *testing.T) { + lines := []string{ + "%global date_corpus %{expand:", + "for date in 2024-02-29 2025-02-28; do", + ` if { test "${date#????-??-??}" = "$date"; }; then`, + " %if 0", + " printf '%s\\n' %{date}", + " %endif", + " fi", + "done", + "}", + "%build", + "echo %{date_corpus}", + } + + tree, err := parseTree(lines) + + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) + require.Len(t, tree.Children, 2) + require.Len(t, tree.Children[0].Children, 1) + assert.Equal(t, lines[:9], tree.Children[0].Children[0].Lines) +} diff --git a/internal/rpm/spec/tree_test.go b/internal/rpm/spec/tree_test.go new file mode 100644 index 000000000..2c88d51d2 --- /dev/null +++ b/internal/rpm/spec/tree_test.go @@ -0,0 +1,192 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec //nolint:testpackage // Tests access unexported parser tree types. + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseTreeRoundTrip(t *testing.T) { + tests := []struct { + name string + input string + }{ + {name: "empty", input: ""}, + {name: "whitespace", input: " \t \n\t"}, + {name: "comment-only conditional", input: "%if 1\n# then\n%else\n# else\n%endif"}, + {name: "empty else", input: "%if 1\n%else\n%endif"}, + {name: "terminal elif", input: "%if 1\n%elif 0\n%endif"}, + {name: "nested wrappers", input: strings.Join([]string{ + "%ifarch x86_64", "%package x", "%ifnos linux", "%description x", "ignored", + "%endif", "%else", "%package y", "%endif", + }, "\n")}, + {name: "elif with sections", input: strings.Join([]string{ + "%if 1", "%package first", "%elifarch x86_64", "%package second", "%else", + "%package third", "%endif", + }, "\n")}, + {name: "macro continuation with directives", input: "%if 1\n%define flags \\\n%else \\\n%if 0 \\\nbody\n%endif"}, + {name: "ordinary continuation followed by structure", input: strings.Join([]string{ + "%build", `configure \`, "%if 1", "make", "%endif", "%files", "/bin/example", + }, "\n")}, + {name: "parameterized macro", input: strings.Join([]string{ + `%define configure(name:) %{name} \`, " --enabled", "%build", "echo %{configure test}", + }, "\n")}, + {name: "lua raw braces strings and expansions", input: `%global helper %{lua: +local value = { nested = %{version}, literal = "}", escaped = "\}" } +print(value.nested) +} +%build +echo %{helper}`}, + {name: "macro expand body with shell parameter expansion", input: strings.Join([]string{ + "%define gobuild(o:) %{expand:", + " %if 0", + ` go build -tags="${BUILDTAGS:-}" %{?**}`, + " %else", + " go build %{?**}", + " %endif", + "}", + "Release: 7%{?dist}", + }, "\n")}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lines := splitLines(tt.input) + tree, err := parseTree(lines) + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) + }) + } +} + +func TestParseTreeRejectsMalformedInput(t *testing.T) { + tests := []struct { + name string + input string + }{ + {name: "unterminated conditional", input: "%if 1\n%build"}, + {name: "unterminated macro continuation", input: "%global flags \\\nbody \\"}, + {name: "unterminated lua macro", input: "%global helper %{lua:\nlocal value = {}\n%build"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := parseTree(splitLines(tt.input)) + require.Error(t, err) + }) + } +} + +func TestIsElifDirectiveIgnoresWhitespace(t *testing.T) { + assert.False(t, isElifDirective(" \t ")) + assert.True(t, isElifDirective("%elif 0")) +} + +func TestPercentRunOpensBracedMacro(t *testing.T) { + tests := []struct { + run string + opens bool + }{ + {run: "%%", opens: false}, + {run: "%%%", opens: true}, + {run: "%%%%", opens: false}, + {run: "%%%%%", opens: true}, + } + + for _, test := range tests { + t.Run(test.run, func(t *testing.T) { + assert.Equal(t, test.opens, percentRunOpensBracedMacro(test.run+"{macro}", 0)) + }) + } +} + +func TestParseTreeTreatsLivePercentRunMacroBodiesAsAtomic(t *testing.T) { + lines := []string{ + "%global helper %%%{", + "%endif", + "}", + "%build", + "echo %{helper}", + } + + tree, err := parseTree(lines) + + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) +} + +func TestParseTreeTreatsTrailingPercentRunsAsLiteralMacroContent(t *testing.T) { + tests := []struct { + name string + lines []string + }{ + { + name: "define single trailing percent", + lines: []string{"%define helper %", "%build", "echo %{helper}"}, + }, + { + name: "global even trailing percent run", + lines: []string{"%global helper %%", "%build", "echo %{helper}"}, + }, + { + name: "define odd trailing percent run", + lines: []string{"%define helper %%%", "%build", "echo %{helper}"}, + }, + { + name: "global even multiple trailing percent run", + lines: []string{"%global helper %%%%", "%build", "echo %{helper}"}, + }, + { + name: "continued intermediate line", + lines: []string{ + "%define helper \\", + "value %\\", + "final", + "%build", + "echo %{helper}", + }, + }, + { + name: "continued final line", + lines: []string{ + "%global helper \\", + "value %%%%", + "%build", + "echo %{helper}", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + tree, err := parseTree(test.lines) + require.NoError(t, err) + assert.Equal(t, test.lines, serializeTree(tree)) + }) + } +} + +func TestParseTreeKeepsEscapedBracedMacrosOpaqueInsideExpandBody(t *testing.T) { + lines := []string{ + "%global helper %{expand:", + "%%{literal}", + "%if 0", + "ignored", + "}", + "%build", + "echo %{helper}", + } + + tree, err := parseTree(lines) + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) +} + +func splitLines(input string) []string { + return strings.Split(input, "\n") +} From d66f84b7720401295444e8d5707d1521aa343b5b Mon Sep 17 00:00:00 2001 From: Thien Trung Vuong Date: Wed, 2 Sep 2026 20:38:30 +0000 Subject: [PATCH 2/5] refactor(spec): add structural tree API --- internal/rpm/spec/structural_spec.go | 9 + internal/rpm/spec/structural_tree_api.go | 310 ++++++++++ .../spec/structural_tree_api_internal_test.go | 555 ++++++++++++++++++ internal/rpm/spec/tree.go | 37 +- internal/rpm/spec/tree_test.go | 100 ++++ 5 files changed, 1005 insertions(+), 6 deletions(-) create mode 100644 internal/rpm/spec/structural_spec.go create mode 100644 internal/rpm/spec/structural_tree_api.go create mode 100644 internal/rpm/spec/structural_tree_api_internal_test.go diff --git a/internal/rpm/spec/structural_spec.go b/internal/rpm/spec/structural_spec.go new file mode 100644 index 000000000..a7bccbf03 --- /dev/null +++ b/internal/rpm/spec/structural_spec.go @@ -0,0 +1,9 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +// structuralSpec encapsulates the raw contents used by structural operations. +type structuralSpec struct { + rawLines []string +} diff --git a/internal/rpm/spec/structural_tree_api.go b/internal/rpm/spec/structural_tree_api.go new file mode 100644 index 000000000..fa9d104fa --- /dev/null +++ b/internal/rpm/spec/structural_tree_api.go @@ -0,0 +1,310 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "errors" + "fmt" + "strings" +) + +// specTree is an opaque handle for a parsed spec structure. +type specTree struct { + root *block +} + +// sectionHandle refers to one section in a [specTree]. +type sectionHandle struct { + block *block + tree *specTree +} + +// mutateTree parses the spec, applies mutate, and validates the resulting tree +// before replacing [structuralSpec.rawLines]. Errors leave the spec unchanged. +func (s *structuralSpec) 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 + } + + lines := serializeTree(root) + if _, err := parseTree(lines); err != nil { + return fmt.Errorf("validating mutated spec tree:\n%w", err) + } + + s.rawLines = lines + + return nil +} + +// inspectTree parses the spec and passes its structure to inspect without +// modifying [structuralSpec.rawLines]. +func (s *structuralSpec) 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}) +} + +// Section returns the first section with name and pkg, or nil if it is absent. +func (t *specTree) Section(name, pkg string) *sectionHandle { + for _, section := range t.Sections(name, pkg) { + return section + } + + return nil +} + +// HasSection reports whether a section with name is present for any package. +func (t *specTree) HasSection(name string) bool { + found := false + + walkBlocks(t.root, func(blk *block) bool { + if blk.Kind == sectionBlock && blk.Name == name { + found = true + } + + return !found + }) + + return found +} + +// Sections returns all matching sections in document order. +func (t *specTree) Sections(name, pkg string) []*sectionHandle { + var matches []*sectionHandle + + walkBlocks(t.root, func(blk *block) bool { + if blk.Kind == sectionBlock && blk.Name == name && blk.Package == pkg { + matches = append(matches, §ionHandle{block: blk, tree: t}) + } + + return true + }) + + return matches +} + +// SectionsByPackage returns every section associated with pkg in document order. +func (t *specTree) SectionsByPackage(pkg string) []*sectionHandle { + var matches []*sectionHandle + + walkBlocks(t.root, func(blk *block) bool { + if blk.Kind == sectionBlock && blk.Package == pkg { + matches = append(matches, §ionHandle{block: blk, tree: t}) + } + + return true + }) + + return matches +} + +// RemoveSections removes sections as one transaction. +func (t *specTree) RemoveSections(handles []*sectionHandle) error { + sections := make(map[*block]bool, len(handles)) + for _, handle := range handles { + if handle == nil || handle.tree != t || handle.block == nil { + return errors.New("section handle does not belong to this spec tree") + } + + if handle.block.Name == "" && handle.block.Package == "" { + return errors.New("cannot remove the global/preamble section") + } + + sections[handle.block] = true + } + + if err := validateSectionRemoval(t.root, sections); err != nil { + return err + } + + removeSections(t.root, sections) + + return nil +} + +// Name returns the section keyword. The preamble has an empty name. +func (h *sectionHandle) Name() string { + return h.block.Name +} + +// Package returns the section package qualifier. +func (h *sectionHandle) Package() string { + return h.block.Package +} + +// AppendLines appends lines to the section's content. +func (h *sectionHandle) AppendLines(lines []string) { + if len(lines) == 0 { + return + } + + h.block.Children = append(h.block.Children, &block{Kind: textBlock, Lines: lines}) +} + +// PrependLines inserts lines immediately after the section header. +func (h *sectionHandle) PrependLines(lines []string) { + if len(lines) == 0 { + return + } + + child := &block{Kind: textBlock, Lines: lines} + h.block.Children = append([]*block{child}, h.block.Children...) +} + +func walkBlocks(blk *block, visit func(*block) bool) bool { + if !visit(blk) { + return false + } + + for _, child := range blk.Children { + if !walkBlocks(child, visit) { + return false + } + } + + if blk.Kind == conditionalBlock { + for _, child := range blk.Else { + if !walkBlocks(child, visit) { + return false + } + } + } + + return true +} + +func removeSections(blk *block, removeSet map[*block]bool) { + blk.Children = removeSectionBlocks(blk.Children, removeSet) + if blk.Kind == conditionalBlock { + blk.Else = removeSectionBlocks(blk.Else, removeSet) + } + + for _, child := range blk.Children { + removeSections(child, removeSet) + } + + if blk.Kind == conditionalBlock { + for _, child := range blk.Else { + removeSections(child, removeSet) + } + } +} + +func removeSectionBlocks(blocks []*block, removeSet map[*block]bool) []*block { + result := make([]*block, 0, len(blocks)) + for _, blk := range blocks { + if !removeSet[blk] { + result = append(result, blk) + } + } + + return result +} + +func validateSectionRemoval(root *block, removeSet map[*block]bool) error { + return validateRemovalChildren(root.Children, removeSet, nil) +} + +func validateRemovalChildren(children []*block, removeSet map[*block]bool, preceding *block) error { + for index, child := range children { + if child.Kind == sectionBlock { + preceding = child + + continue + } + + if child.Kind != conditionalBlock { + continue + } + + if conditionalHasTextOrMacroContent(child) && containsSectionBlocks(child) { + if preceding != nil && removeSet[preceding] { + return fmt.Errorf("conditional block at %#q contains content belonging to the preceding section:\n%w", + child.Header, ErrConditionalSpansSections) + } + } + + if wouldEmptySectionWrapper(child, removeSet) && index+1 < len(children) { + next := children[index+1] + if next.Kind == conditionalBlock && !containsSectionBlocks(next) && conditionalHasTextOrMacroContent(next) { + return fmt.Errorf("content in conditional block at %#q would be orphaned after removing the preceding section:\n%w", + next.Header, ErrConditionalSpansSections) + } + } + + if err := validateRemovalChildren(child.Children, removeSet, preceding); err != nil { + return err + } + + if err := validateRemovalChildren(child.Else, removeSet, preceding); err != nil { + return err + } + } + + return nil +} + +func conditionalHasTextOrMacroContent(conditional *block) bool { + return hasTextOrMacroContent(conditional.Children) || hasTextOrMacroContent(conditional.Else) +} + +func hasTextOrMacroContent(blocks []*block) bool { + for _, blk := range blocks { + if blk.Kind == macroDefBlock { + return true + } + + if blk.Kind == textBlock { + for _, line := range blk.Lines { + trimmed := strings.TrimSpace(line) + if trimmed != "" && !strings.HasPrefix(trimmed, "#") { + return true + } + } + } + + if blk.Kind == conditionalBlock && + (hasTextOrMacroContent(blk.Children) || hasTextOrMacroContent(blk.Else)) { + return true + } + } + + return false +} + +func wouldEmptySectionWrapper(conditional *block, removeSet map[*block]bool) bool { + if !containsSectionBlocks(conditional) { + return false + } + + return !hasRemainingSection(conditional.Children, removeSet) && + !hasRemainingSection(conditional.Else, removeSet) +} + +func hasRemainingSection(blocks []*block, removeSet map[*block]bool) bool { + for _, blk := range blocks { + if blk.Kind == sectionBlock && !removeSet[blk] { + return true + } + + if hasRemainingSection(blk.Children, removeSet) { + return true + } + + if blk.Kind == conditionalBlock && hasRemainingSection(blk.Else, removeSet) { + return true + } + } + + return false +} diff --git a/internal/rpm/spec/structural_tree_api_internal_test.go b/internal/rpm/spec/structural_tree_api_internal_test.go new file mode 100644 index 000000000..db4759cac --- /dev/null +++ b/internal/rpm/spec/structural_tree_api_internal_test.go @@ -0,0 +1,555 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "errors" + "slices" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInspectTreeQueriesSectionsInDocumentOrder(t *testing.T) { + specification := newTreeAPISpec([]string{ + "Name: example", + "%package -n example-devel", + "%description -n example-devel", + "%ifarch x86_64", + "%files -n example-devel", + "%else", + "%files -n example-devel", + "%endif", + }) + + err := specification.inspectTree(func(tree *specTree) error { + section := tree.Section("%description", "example-devel") + require.NotNil(t, section) + assert.Equal(t, "%description", section.Name()) + assert.Equal(t, "example-devel", section.Package()) + assert.True(t, tree.HasSection("%files")) + assert.False(t, tree.HasSection("%check")) + assert.Len(t, tree.Sections("%files", "example-devel"), 2) + assert.Len(t, tree.SectionsByPackage("example-devel"), 4) + assert.Empty(t, tree.Sections("%build", "")) + + return nil + }) + + require.NoError(t, err) + assert.Equal(t, []string{ + "Name: example", + "%package -n example-devel", + "%description -n example-devel", + "%ifarch x86_64", + "%files -n example-devel", + "%else", + "%files -n example-devel", + "%endif", + }, specification.rawLines) +} + +func TestHasSectionRemainsFoundAfterLaterNonMatchingBlocks(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%build", + "make", + "%check", + "make check", + }) + + err := specification.inspectTree(func(tree *specTree) error { + assert.True(t, tree.HasSection("%build")) + + return nil + }) + + require.NoError(t, err) +} + +func TestHasSectionStopsAfterFirstMatch(t *testing.T) { + tree := &specTree{root: &block{ + Kind: rootBlock, + Children: []*block{ + {Kind: sectionBlock, Name: "%build"}, + nil, + }, + }} + + assert.NotPanics(t, func() { + assert.True(t, tree.HasSection("%build")) + }) +} + +func TestWalkBlocksStopsBeforeLaterSiblings(t *testing.T) { + first := &block{Kind: sectionBlock, Name: "%build"} + later := &block{Kind: sectionBlock, Name: "%check"} + root := &block{Kind: rootBlock, Children: []*block{first, later}} + + var visited []string + + walkBlocks(root, func(blk *block) bool { + visited = append(visited, blk.Name) + + return blk != first + }) + + assert.Equal(t, []string{"", "%build"}, visited) +} + +func TestWalkBlocksStopsInsideElseBeforeAncestorSiblings(t *testing.T) { + stop := &block{Kind: sectionBlock, Name: "%stop"} + innerElseLater := &block{Kind: sectionBlock, Name: "%inner-else-later"} + outerElseLater := &block{Kind: sectionBlock, Name: "%outer-else-later"} + rootLater := &block{Kind: sectionBlock, Name: "%root-later"} + inner := &block{ + Kind: conditionalBlock, + Header: "%if inner", + Else: []*block{stop, innerElseLater}, + } + outer := &block{ + Kind: conditionalBlock, + Header: "%if outer", + Else: []*block{inner, outerElseLater}, + } + root := &block{Kind: rootBlock, Children: []*block{outer, rootLater}} + + var visited []string + + walkBlocks(root, func(blk *block) bool { + switch { + case blk.Header != "": + visited = append(visited, blk.Header) + case blk.Kind == rootBlock: + visited = append(visited, "root") + default: + visited = append(visited, blk.Name) + } + + return blk != stop + }) + + assert.Equal(t, []string{"root", "%if outer", "%if inner", "%stop"}, visited) +} + +func TestTreeWrappersAreTransactional(t *testing.T) { + t.Run("callback error", func(t *testing.T) { + specification := newTreeAPISpec([]string{"%build", "make"}) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + tree.Section("%build", "").AppendLines([]string{"make install"}) + + return errors.New("stop") + }) + + require.EqualError(t, err, "stop") + assert.Equal(t, before, specification.rawLines) + }) + + t.Run("validation error", func(t *testing.T) { + specification := newTreeAPISpec([]string{"%if 1", "%build", "make", "%endif"}) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + tree.root.Children[1].Endif = "" + + return nil + }) + + require.Error(t, err) + assert.Equal(t, before, specification.rawLines) + }) + + t.Run("malformed source", func(t *testing.T) { + specification := newTreeAPISpec([]string{"%if 1", "%build"}) + before := append([]string(nil), specification.rawLines...) + + err := specification.inspectTree(func(*specTree) error { + t.Fatal("inspect callback must not run for malformed input") + + return nil + }) + + require.Error(t, err) + assert.Equal(t, before, specification.rawLines) + }) +} + +func TestSectionLinePrimitivesPreserveOrder(t *testing.T) { + specification := newTreeAPISpec([]string{"%build", "make"}) + + err := specification.mutateTree(func(tree *specTree) error { + section := tree.Section("%build", "") + require.NotNil(t, section) + section.PrependLines([]string{"setup"}) + section.AppendLines([]string{"make install"}) + + return nil + }) + + require.NoError(t, err) + assert.Equal(t, []string{"%build", "setup", "make", "make install"}, specification.rawLines) +} + +func TestSectionLinePrimitivesHandleEmptySections(t *testing.T) { + specification := newTreeAPISpec([]string{"%build", "%check"}) + + err := specification.mutateTree(func(tree *specTree) error { + section := tree.Section("%build", "") + require.NotNil(t, section) + section.AppendLines([]string{"make"}) + + return nil + }) + + require.NoError(t, err) + assert.Equal(t, []string{"%build", "make", "%check"}, specification.rawLines) +} + +func TestRemoveSectionsPreservesConditionalBalance(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%if 1", + "%package one", + "%description one", + "one", + "%else", + "%package two", + "%description two", + "two", + "%endif", + }) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("one")) + }) + + require.NoError(t, err) + assert.Equal(t, []string{ + "%if 1", + "%else", + "%package two", + "%description two", + "two", + "%endif", + }, specification.rawLines) + _, err = parseTree(specification.rawLines) + require.NoError(t, err) +} + +func TestRemoveSectionsRejectsPreambleTransactionally(t *testing.T) { + tests := []struct { + name string + removeFunc func(*specTree) []*sectionHandle + wantLines []string + }{ + { + name: "preamble", + removeFunc: func(tree *specTree) []*sectionHandle { + return tree.Sections("", "") + }, + wantLines: []string{ + "Name: example", + "%package example-devel", + "%description example-devel", + "Development files", + }, + }, + { + name: "preamble and section", + removeFunc: func(tree *specTree) []*sectionHandle { + return append(tree.Sections("", ""), tree.Sections("%package", "example-devel")...) + }, + wantLines: []string{ + "Name: example", + "%package example-devel", + "%description example-devel", + "Development files", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.wantLines) + before := slices.Clone(specification.rawLines) + + err := specification.mutateTree(func(tree *specTree) error { + before := serializeTree(tree.root) + + err := tree.RemoveSections(test.removeFunc(tree)) + + require.EqualError(t, err, "cannot remove the global/preamble section") + assert.Equal(t, before, serializeTree(tree.root)) + + return err + }) + + require.EqualError(t, err, "cannot remove the global/preamble section") + assert.Equal(t, before, specification.rawLines) + }) + } +} + +func TestRemoveSectionsAllowsMainPackageSection(t *testing.T) { + specification := newTreeAPISpec([]string{ + "Name: example", + "%build", + "make", + }) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.Sections("%build", "")) + }) + + require.NoError(t, err) + assert.Equal(t, []string{"Name: example"}, specification.rawLines) +} + +func TestRemoveSectionsRejectsOrphanedConditionalContent(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package one", + "%if 1", + "shared", + "%package two", + "%endif", + }) + + err := specification.inspectTree(func(tree *specTree) error { + before := serializeTree(tree.root) + err := tree.RemoveSections(tree.Sections("%package", "one")) + require.ErrorIs(t, err, ErrConditionalSpansSections) + assert.Equal(t, before, serializeTree(tree.root)) + + return nil + }) + + require.NoError(t, err) +} + +func TestRemoveSectionsRejectsOrphanedConditionalBranchContent(t *testing.T) { + tests := []struct { + name string + lines []string + }{ + { + name: "then", + lines: []string{ + "%package one", + "%if 1", + "orphan", + "%package two", + "%endif", + }, + }, + { + name: "else", + lines: []string{ + "%package one", + "%if 1", + "%package two", + "%else", + "orphan", + "%package three", + "%endif", + }, + }, + { + name: "elif", + lines: []string{ + "%package one", + "%if 1", + "%package two", + "%elif 0", + "orphan", + "%package three", + "%endif", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.lines) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.Sections("%package", "one")) + }) + + require.ErrorIs(t, err, ErrConditionalSpansSections) + assert.Equal(t, before, specification.rawLines) + }) + } +} + +func TestRemoveSectionsRejectsOrphanedAdjacentConditionalBranchContent(t *testing.T) { + tests := []struct { + name string + lines []string + }{ + { + name: "else", + lines: []string{ + "%if 1", + "%package one", + "%endif", + "%if 1", + "%else", + "orphan", + "%endif", + }, + }, + { + name: "elif", + lines: []string{ + "%if 1", + "%package one", + "%endif", + "%if 1", + "%elif 0", + "orphan", + "%endif", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.lines) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.Sections("%package", "one")) + }) + + require.ErrorIs(t, err, ErrConditionalSpansSections) + assert.Equal(t, before, specification.rawLines) + }) + } +} + +func TestRemoveSectionsAllowsIndependentConditionalBranchRemoval(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%if 1", + "%package one", + "%else", + "%package two", + "%endif", + }) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.Sections("%package", "one")) + }) + + require.NoError(t, err) + assert.Equal(t, []string{ + "%if 1", + "%else", + "%package two", + "%endif", + }, specification.rawLines) +} + +func TestRemoveSectionsAllowsEmptyOrCommentOnlyAdjacentElse(t *testing.T) { + tests := []struct { + name string + lines []string + }{ + { + name: "empty", + lines: []string{ + "%if 1", + "%package one", + "%endif", + "%if 1", + "%else", + "%endif", + }, + }, + { + name: "comment only", + lines: []string{ + "%if 1", + "%package one", + "%endif", + "%if 1", + "%else", + "# not content", + "%endif", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.lines) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.Sections("%package", "one")) + }) + + require.NoError(t, err) + }) + } +} + +func TestHasTextOrMacroContentRecursesThroughConditionalBranches(t *testing.T) { + directivesOnly := []*block{{ + Kind: conditionalBlock, + Header: "%if 1", + Else: []*block{{ + Kind: conditionalBlock, + Header: "%else", + }}, + }} + assert.False(t, hasTextOrMacroContent(directivesOnly)) + + nestedContent := []*block{{ + Kind: conditionalBlock, + Header: "%if 1", + Else: []*block{{ + Kind: conditionalBlock, + Header: "%else", + Children: []*block{{ + Kind: macroDefBlock, + Lines: []string{"%global helper value"}, + }}, + }}, + }} + assert.True(t, hasTextOrMacroContent(nestedContent)) + + assert.False(t, hasTextOrMacroContent([]*block{{ + Kind: textBlock, + Lines: []string{"", " ", "# comment"}, + }})) +} + +func TestTreeEditKeepsEscapedBracedMacrosOpaqueInsideExpandBody(t *testing.T) { + lines := []string{ + "%global helper %{expand:", + "%%{literal}", + "%if 0", + "ignored", + "}", + "%build", + "echo %{helper}", + } + specification := newTreeAPISpec(lines) + + err := specification.mutateTree(func(tree *specTree) error { + section := tree.Section("%build", "") + require.NotNil(t, section) + section.AppendLines([]string{"make"}) + + return nil + }) + + require.NoError(t, err) + assert.Equal(t, append(lines, "make"), specification.rawLines) +} + +func newTreeAPISpec(lines []string) *structuralSpec { + return &structuralSpec{rawLines: slices.Clone(lines)} +} diff --git a/internal/rpm/spec/tree.go b/internal/rpm/spec/tree.go index 291b3e709..e2d59a178 100644 --- a/internal/rpm/spec/tree.go +++ b/internal/rpm/spec/tree.go @@ -66,6 +66,11 @@ type treeConditionalPair struct { endifLine int } +type treeConditionalFrame struct { + ifLine int + elseLine int +} + // parseTree parses raw spec lines into a [block] tree. // // The parser runs in two passes: @@ -116,7 +121,7 @@ func parseTree(rawLines []string) (*block, error) { func collectTreeConditionalPairs(rawLines []string) ([]treeConditionalPair, error) { var ( pairs []treeConditionalPair - stack []int + stack []treeConditionalFrame inMacroBody bool parseState macroState ) @@ -136,21 +141,41 @@ func collectTreeConditionalPairs(rawLines []string) ([]treeConditionalPair, erro switch conditionalDepthChange(line) { case 1: - stack = append(stack, lineNum) + stack = append(stack, treeConditionalFrame{ifLine: lineNum, elseLine: -1}) case -1: if len(stack) == 0 { return nil, fmt.Errorf("unmatched %%endif at line %d", lineNum+1) } - ifLine := stack[len(stack)-1] + frame := stack[len(stack)-1] stack = stack[:len(stack)-1] - pairs = append(pairs, treeConditionalPair{ifLine: ifLine, endifLine: lineNum}) + pairs = append(pairs, treeConditionalPair{ifLine: frame.ifLine, endifLine: lineNum}) + case 0: + if !isConditionalBranchDirective(line) { + continue + } + + if len(stack) == 0 { + return nil, fmt.Errorf("conditional branch directive at line %d is outside a conditional", lineNum+1) + } + + frame := &stack[len(stack)-1] + if frame.elseLine >= 0 { + return nil, fmt.Errorf( + "conditional branch directive at line %d follows terminal %%else at line %d", + lineNum+1, frame.elseLine+1, + ) + } + + if !isElifDirective(line) { + frame.elseLine = lineNum + } } } if len(stack) > 0 { - return nil, fmt.Errorf("unmatched %%if at line %d", stack[0]+1) + return nil, fmt.Errorf("unmatched %%if at line %d", stack[0].ifLine+1) } return pairs, nil @@ -229,7 +254,7 @@ func containsSectionBlocks(block *block) bool { } // findSectionHeaderLines returns the 0-indexed line numbers of all section headers, -// respecting line continuations (backslash-terminated lines suppress the next line). +// suppressing recognition inside multiline '%define' and '%global' bodies. func findSectionHeaderLines(rawLines []string) []int { var headers []int diff --git a/internal/rpm/spec/tree_test.go b/internal/rpm/spec/tree_test.go index 2c88d51d2..614fc15ea 100644 --- a/internal/rpm/spec/tree_test.go +++ b/internal/rpm/spec/tree_test.go @@ -72,6 +72,18 @@ func TestParseTreeRejectsMalformedInput(t *testing.T) { {name: "unterminated conditional", input: "%if 1\n%build"}, {name: "unterminated macro continuation", input: "%global flags \\\nbody \\"}, {name: "unterminated lua macro", input: "%global helper %{lua:\nlocal value = {}\n%build"}, + {name: "else outside conditional", input: "%else"}, + {name: "elif outside conditional", input: "%elif 0"}, + {name: "elifarch outside conditional", input: "%elifarch x86_64"}, + {name: "elifnarch outside conditional", input: "%elifnarch x86_64"}, + {name: "elifos outside conditional", input: "%elifos linux"}, + {name: "elifnos outside conditional", input: "%elifnos linux"}, + {name: "duplicate else", input: "%if 1\n%else\n%else\n%endif"}, + {name: "elif after else", input: "%if 1\n%else\n%elif 0\n%endif"}, + {name: "elifarch after else", input: "%if 1\n%else\n%elifarch x86_64\n%endif"}, + {name: "elifnarch after else", input: "%if 1\n%else\n%elifnarch x86_64\n%endif"}, + {name: "elifos after else", input: "%if 1\n%else\n%elifos linux\n%endif"}, + {name: "elifnos after else", input: "%if 1\n%else\n%elifnos linux\n%endif"}, } for _, tt := range tests { @@ -82,6 +94,94 @@ func TestParseTreeRejectsMalformedInput(t *testing.T) { } } +func TestParseTreeAcceptsElifChainBeforeElse(t *testing.T) { + lines := []string{ + "%if 1", + "then", + "%elif 0", + "elif", + "%elifarch x86_64", + "elifarch", + "%elifnarch aarch64", + "elifnarch", + "%elifos linux", + "elifos", + "%elifnos linux", + "elifnos", + "%else", + "else", + "%endif", + } + + tree, err := parseTree(lines) + + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) +} + +func TestParseTreeAcceptsNestedConditionalBranches(t *testing.T) { + tests := []struct { + name string + lines []string + }{ + { + name: "outer elif after completed inner else", + lines: []string{ + "%if 1", + "%if 0", + "%else", + "%endif", + "%elif 0", + "%endif", + }, + }, + { + name: "nested elif inside outer else", + lines: []string{ + "%if 1", + "%else", + "%if 0", + "%elif 1", + "%endif", + "%endif", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tree, err := parseTree(tt.lines) + + require.NoError(t, err) + assert.Equal(t, tt.lines, serializeTree(tree)) + }) + } +} + +func TestParseTreeKeepsBranchLinesOpaqueInMultilineMacroBodies(t *testing.T) { + for _, macroHeader := range []string{"%define helper \\", "%global helper \\"} { + t.Run(macroHeader, func(t *testing.T) { + lines := []string{ + macroHeader, + "%else \\", + "%elif 0 \\", + "%elifarch x86_64 \\", + "%elifnarch aarch64 \\", + "%elifos linux \\", + "%elifnos linux \\", + "body", + "%build", + "echo %{helper}", + } + + tree, err := parseTree(lines) + + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) + }) + } +} + func TestIsElifDirectiveIgnoresWhitespace(t *testing.T) { assert.False(t, isElifDirective(" \t ")) assert.True(t, isElifDirective("%elif 0")) From e6d8c9ead2d91466f77ca917bcc0726073377309 Mon Sep 17 00:00:00 2001 From: Thien Trung Vuong Date: Wed, 2 Sep 2026 20:38:59 +0000 Subject: [PATCH 3/5] refactor(spec): add selectable spec editors --- internal/app/azldev/core/sources/overlays.go | 10 +- .../app/azldev/core/sources/overlays_test.go | 46 + internal/app/azldev/core/sources/release.go | 25 +- .../core/sources/release_internal_test.go | 38 + .../app/azldev/core/sources/release_test.go | 6 + .../app/azldev/core/sources/sourceprep.go | 9 +- .../core/sources/upstream_provenance.go | 8 +- .../upstream_provenance_internal_test.go | 43 + internal/rpm/spec/edit_test.go | 645 ++++++++-- internal/rpm/spec/editor.go | 229 ++++ internal/rpm/spec/{edit.go => legacy_edit.go} | 122 +- internal/rpm/spec/{spec.go => legacy_spec.go} | 191 +-- internal/rpm/spec/spec_test.go | 9 +- internal/rpm/spec/structural_edit.go | 1042 +++++++++++++++++ internal/rpm/spec/structural_spec.go | 279 ++++- internal/rpm/spec/structural_tree_api.go | 397 ++++++- .../spec/structural_tree_api_internal_test.go | 95 ++ 17 files changed, 2810 insertions(+), 384 deletions(-) create mode 100644 internal/rpm/spec/editor.go rename internal/rpm/spec/{edit.go => legacy_edit.go} (90%) rename internal/rpm/spec/{spec.go => legacy_spec.go} (70%) create mode 100644 internal/rpm/spec/structural_edit.go diff --git a/internal/app/azldev/core/sources/overlays.go b/internal/app/azldev/core/sources/overlays.go index 44c2ca5c5..9434e7efa 100644 --- a/internal/app/azldev/core/sources/overlays.go +++ b/internal/app/azldev/core/sources/overlays.go @@ -43,11 +43,11 @@ func ApplyOverlayToSources( dryRunnable opctx.DryRunnable, fs opctx.FS, overlay projectconfig.ComponentOverlay, - sourcesDirPath, specPath string, + sourcesDirPath, specPath string, options ...spec.OpenOption, ) error { // Apply the spec component, if any. if overlay.ModifiesSpec() { - err := ApplySpecOverlayToFileInPlace(fs, overlay, specPath) + err := ApplySpecOverlayToFileInPlace(fs, overlay, specPath, options...) if err != nil { return err } @@ -78,13 +78,15 @@ func ApplyOverlayToSources( // ApplySpecOverlayToFileInPlace applies the given overlay to the specified spec file. // Changes are made in-place. -func ApplySpecOverlayToFileInPlace(fs opctx.FS, overlay projectconfig.ComponentOverlay, specPath string) error { +func ApplySpecOverlayToFileInPlace( + fs opctx.FS, overlay projectconfig.ComponentOverlay, specPath string, options ...spec.OpenOption, +) error { specFile, err := fs.Open(specPath) if err != nil { return fmt.Errorf("failed to open spec %#q for reading:\n%w", specPath, err) } - openedSpec, err := spec.OpenSpec(specFile) + openedSpec, err := spec.OpenSpec(specFile, options...) specFile.Close() if err != nil { diff --git a/internal/app/azldev/core/sources/overlays_test.go b/internal/app/azldev/core/sources/overlays_test.go index 5e6a26d4d..240fbc42a 100644 --- a/internal/app/azldev/core/sources/overlays_test.go +++ b/internal/app/azldev/core/sources/overlays_test.go @@ -394,6 +394,52 @@ newname package } } +func TestApplySpecOverlay_ShimConditionalRepairAcrossOverlays(t *testing.T) { + openedSpec, err := spec.OpenSpec(strings.NewReader(`Name: shim-unsigned-%{efiarch} + +%build +%if 0%{?dbxfile} +echo dbx +%endif +cd build-%{efiarch} +cd build-%{efialtarch} +%install +`), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + require.NoError(t, sources.ApplySpecOverlay(projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlaySearchAndReplaceInSpec, + SectionName: "%build", + Regex: `^cd build-%\{efialtarch\}$`, + Replacement: "%if 0\ncd build-%{efialtarch}", + }, openedSpec)) + + var intermediate bytes.Buffer + require.NoError(t, openedSpec.Serialize(&intermediate)) + assert.Contains(t, intermediate.String(), "%if 0\ncd build-%{efialtarch}\n") + + require.NoError(t, sources.ApplySpecOverlay(projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayAppendSpecLines, + SectionName: "%build", + Lines: []string{"%endif"}, + }, openedSpec)) + + var result bytes.Buffer + require.NoError(t, openedSpec.Serialize(&result)) + assert.Equal(t, `Name: shim-unsigned-%{efiarch} + +%build +%if 0%{?dbxfile} +echo dbx +%endif +cd build-%{efiarch} +%if 0 +cd build-%{efialtarch} +%endif +%install +`, result.String()) +} + func TestApplyNonSpecOverlay(t *testing.T) { testCases := []struct { name string diff --git a/internal/app/azldev/core/sources/release.go b/internal/app/azldev/core/sources/release.go index 55dcad6cb..a4d71d66a 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" @@ -34,33 +33,21 @@ var staticReleasePattern = regexp.MustCompile(`^(\d+)(%\{\??dist\})?$`) // GetReleaseTagValue reads the Release tag value from the spec file at specPath. // It returns the raw value string as written in the spec (e.g. "1%{?dist}" or "%autorelease"). // Returns [spec.ErrNoSuchTag] if no Release tag is found. -func GetReleaseTagValue(fs opctx.FS, specPath string) (string, error) { +func GetReleaseTagValue(fs opctx.FS, specPath string, options ...spec.OpenOption) (string, error) { specFile, err := fs.Open(specPath) if err != nil { return "", fmt.Errorf("failed to open spec %#q:\n%w", specPath, err) } defer specFile.Close() - openedSpec, err := spec.OpenSpec(specFile) + openedSpec, err := spec.OpenSpec(specFile, options...) if err != nil { 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 - }) + 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 @@ -146,7 +133,7 @@ func (p *sourcePreparerImpl) readAndBumpRelease( return err } - releaseValue, err := GetReleaseTagValue(p.fs, specPath) + releaseValue, err := GetReleaseTagValue(p.fs, specPath, spec.WithEditor(p.specEditor)) if err != nil { return fmt.Errorf("failed to read Release tag for component %#q:\n%w", component.GetName(), err) @@ -187,7 +174,7 @@ func (p *sourcePreparerImpl) readAndBumpRelease( Value: newRelease, } - if err := ApplySpecOverlayToFileInPlace(p.fs, overlay, specPath); err != nil { + if err := ApplySpecOverlayToFileInPlace(p.fs, overlay, specPath, spec.WithEditor(p.specEditor)); err != nil { return fmt.Errorf("failed to apply release bump overlay for component %#q:\n%w", component.GetName(), err) } diff --git a/internal/app/azldev/core/sources/release_internal_test.go b/internal/app/azldev/core/sources/release_internal_test.go index 6a37e078a..e2728549a 100644 --- a/internal/app/azldev/core/sources/release_internal_test.go +++ b/internal/app/azldev/core/sources/release_internal_test.go @@ -103,6 +103,44 @@ func TestTryBumpStaticRelease_StaticBumps(t *testing.T) { assert.Contains(t, string(content), "Release: 4%{?dist}") } +func TestTryBumpStaticRelease_BumpsLastConditionalReleaseAndRereadsIt(t *testing.T) { + ctrl := gomock.NewController(t) + memFS := afero.NewMemMapFs() + preparer := newTestPreparer(memFS) + specDir := filepath.Join(testSourcesDir, "test-pkg") + require.NoError(t, fileutils.MkdirAll(memFS, specDir)) + specPath := filepath.Join(specDir, "test-pkg.spec") + require.NoError(t, fileutils.WriteFile(memFS, specPath, []byte(`Name: test-pkg +Version: 1.0.0 +%if 0 +Release: 1%{?dist} +%else +Release: 2%{?dist} +%endif +`), fileperms.PublicFile)) + + comp := mockComponent(ctrl, "test-pkg", &projectconfig.ComponentConfig{ + Release: projectconfig.ReleaseConfig{Calculation: projectconfig.ReleaseCalculationAuto}, + }) + + require.NoError(t, preparer.tryBumpStaticRelease(comp, specDir, 3)) + + release, err := GetReleaseTagValue(memFS, specPath) + require.NoError(t, err) + assert.Equal(t, "5%{?dist}", release) + + content, err := fileutils.ReadFile(memFS, specPath) + require.NoError(t, err) + assert.Equal(t, `Name: test-pkg +Version: 1.0.0 +%if 0 +Release: 5%{?dist} +%else +Release: 5%{?dist} +%endif +`, string(content)) +} + func TestTryBumpStaticRelease_StaticBumpsNonConditionalDist(t *testing.T) { ctrl := gomock.NewController(t) memFS := afero.NewMemMapFs() diff --git a/internal/app/azldev/core/sources/release_test.go b/internal/app/azldev/core/sources/release_test.go index 13a0444ce..6113ed75a 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}, + { + "last repeated conditional release", + "Name: test-package\nVersion: 1.0.0\n%if 0\nRelease: 1\n%else\nRelease: 2\n%endif\n", + "2", + 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..67d307910 100644 --- a/internal/app/azldev/core/sources/sourceprep.go +++ b/internal/app/azldev/core/sources/sourceprep.go @@ -24,6 +24,7 @@ import ( "github.com/microsoft/azure-linux-dev-tools/internal/projectconfig" "github.com/microsoft/azure-linux-dev-tools/internal/providers/sourceproviders" "github.com/microsoft/azure-linux-dev-tools/internal/providers/sourceproviders/fedorasource" + "github.com/microsoft/azure-linux-dev-tools/internal/rpm/spec" "github.com/microsoft/azure-linux-dev-tools/internal/utils/dirdiff" "github.com/microsoft/azure-linux-dev-tools/internal/utils/fileperms" "github.com/microsoft/azure-linux-dev-tools/internal/utils/fileutils" @@ -105,6 +106,10 @@ func WithDirtyDetection() PreparerOption { // Git-tracked files (spec, patches, scripts, configs) are still fetched from // the upstream clone. This is useful for rendering, where only the spec and // sidecar files are needed and downloading large source tarballs is unnecessary. +func WithSpecEditor(mode spec.EditorMode) PreparerOption { + return func(p *sourcePreparerImpl) { p.specEditor = mode } +} + func WithSkipLookaside() PreparerOption { return func(p *sourcePreparerImpl) { p.skipLookaside = true @@ -156,6 +161,7 @@ func WithAllowNoHashes() PreparerOption { // Standard implementation of the [SourcePreparer] interface. type sourcePreparerImpl struct { sourceManager sourceproviders.SourceManager + specEditor spec.EditorMode fs opctx.FS eventListener opctx.EventListener dryRunnable opctx.DryRunnable @@ -228,6 +234,7 @@ func NewPreparer( impl := &sourcePreparerImpl{ sourceManager: sourceManager, + specEditor: spec.EditorLegacy, fs: fs, eventListener: eventListener, dryRunnable: dryRunnable, @@ -1390,7 +1397,7 @@ func (p *sourcePreparerImpl) applyOverlayList( } if err := ApplyOverlayToSources( - p.dryRunnable, p.fs, overlay, sourcesDirPath, absSpecPath, + p.dryRunnable, p.fs, overlay, sourcesDirPath, absSpecPath, spec.WithEditor(p.specEditor), ); err != nil { return fmt.Errorf("failed to apply %#q overlay:\n%w", overlay.Type, err) } diff --git a/internal/app/azldev/core/sources/upstream_provenance.go b/internal/app/azldev/core/sources/upstream_provenance.go index 327269710..aa8b55578 100644 --- a/internal/app/azldev/core/sources/upstream_provenance.go +++ b/internal/app/azldev/core/sources/upstream_provenance.go @@ -119,7 +119,7 @@ func (p *sourcePreparerImpl) addUpstreamProvenanceMacros( return } - version, release, err := parseSpecVersionRelease(p.fs, specPath) + version, release, err := parseSpecVersionRelease(p.fs, specPath, spec.WithEditor(p.specEditor)) if err != nil { slog.Warn("Skipping upstream provenance macros; failed to parse spec", "component", component.GetName(), "error", err) @@ -245,13 +245,15 @@ func setMacroIfAbsent(macros map[string]string, name, value string) { // package of the spec at specPath. Values are captured verbatim (no macro // expansion beyond the caller's later %{?dist} substitution). Missing tags // yield empty strings; it is not an error for a tag to be absent. -func parseSpecVersionRelease(fs opctx.FS, specPath string) (version, release string, err error) { +func parseSpecVersionRelease( + fs opctx.FS, specPath string, options ...spec.OpenOption, +) (version, release string, err error) { data, err := fileutils.ReadFile(fs, specPath) if err != nil { return "", "", fmt.Errorf("failed to read spec %#q:\n%w", specPath, err) } - parsed, err := spec.OpenSpec(bytes.NewReader(data)) + parsed, err := spec.OpenSpec(bytes.NewReader(data), options...) if err != nil { return "", "", fmt.Errorf("failed to parse spec %#q:\n%w", specPath, err) } 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..c0c60eae2 100644 --- a/internal/app/azldev/core/sources/upstream_provenance_internal_test.go +++ b/internal/app/azldev/core/sources/upstream_provenance_internal_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/microsoft/azure-linux-dev-tools/internal/projectconfig" + "github.com/microsoft/azure-linux-dev-tools/internal/rpm/spec" "github.com/microsoft/azure-linux-dev-tools/internal/utils/fileperms" "github.com/microsoft/azure-linux-dev-tools/internal/utils/fileutils" "github.com/spf13/afero" @@ -79,6 +80,48 @@ func TestParseSpecVersionRelease(t *testing.T) { assert.Equal(t, "5%{?dist}", release, "release is captured verbatim, dist is expanded later") } +func TestParseSpecVersionReleaseReadsFirstRepeatedConditionalRelease(t *testing.T) { + memFS := afero.NewMemMapFs() + require.NoError(t, fileutils.MkdirAll(memFS, provenanceWorkDir)) + require.NoError(t, fileutils.WriteFile(memFS, filepath.Join(provenanceWorkDir, "grub2.spec"), []byte(`Name: grub2 +Version: 2.12 +%if 0 +Release: 5%{?dist} +%else +Release: 6%{?dist} +%endif +`), fileperms.PublicFile)) + + version, release, err := parseSpecVersionRelease(memFS, filepath.Join(provenanceWorkDir, "grub2.spec")) + require.NoError(t, err) + assert.Equal(t, "2.12", version) + assert.Equal(t, "5%{?dist}", release) +} + +func TestParseSpecVersionReleaseSkipsEmptyRepeatedConditionalTags(t *testing.T) { + for _, editor := range []spec.EditorMode{spec.EditorLegacy, spec.EditorStructural} { + t.Run(string(editor), func(t *testing.T) { + memFS := afero.NewMemMapFs() + require.NoError(t, fileutils.MkdirAll(memFS, provenanceWorkDir)) + require.NoError(t, fileutils.WriteFile(memFS, filepath.Join(provenanceWorkDir, "grub2.spec"), []byte(`Name: grub2 +%if 0 +Version: +Release: +%endif +Version: 2.12 +Release: 5%{?dist} +`), fileperms.PublicFile)) + + version, release, err := parseSpecVersionRelease( + memFS, filepath.Join(provenanceWorkDir, "grub2.spec"), spec.WithEditor(editor), + ) + require.NoError(t, err) + assert.Equal(t, "2.12", version) + assert.Equal(t, "5%{?dist}", release) + }) + } +} + func TestParseSpecVersionRelease_MissingFile(t *testing.T) { _, _, err := parseSpecVersionRelease(afero.NewMemMapFs(), "/does-not-exist.spec") require.Error(t, err) diff --git a/internal/rpm/spec/edit_test.go b/internal/rpm/spec/edit_test.go index 959c5f104..d4f22429e 100644 --- a/internal/rpm/spec/edit_test.go +++ b/internal/rpm/spec/edit_test.go @@ -14,6 +14,153 @@ import ( "github.com/stretchr/testify/require" ) +func TestVisitTags(t *testing.T) { + input := `Name: main-pkg +Version: 1.0 +Patch0: main.patch + +%package devel +Summary: Development files +Patch1: devel.patch + +%package -n other +Summary: Other package +Patch2: other.patch +` + + tests := []struct { + name string + options []spec.OpenOption + expectedTags []string + }{ + { + name: "default legacy editor", + expectedTags: []string{"Name", "Version", "Patch0", "Summary", "Patch1", "Summary", "Patch2"}, + }, + { + name: "structural editor", + options: []spec.OpenOption{spec.WithEditor(spec.EditorStructural)}, + expectedTags: []string{"Name", "Version", "Patch0", "Summary", "Patch1", "Summary", "Patch2"}, + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(input), testCase.options...) + require.NoError(t, err) + + var tags []string + + require.NoError(t, specFile.VisitTags(func(tagLine *spec.TagLine, _ *spec.Context) error { + tags = append(tags, tagLine.Tag) + + return nil + })) + assert.Equal(t, testCase.expectedTags, tags) + }) + } +} + +func TestStructuralVisitTagsUsesStructuralContent(t *testing.T) { + input := `Name: main +%global hidden() \ +Name: macro-body +%if 0 +Summary: conditional +%endif +%package devel +Summary: development +%build +Name: script-body +` + + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + var ( + tags []string + lineNumbers []int + ) + + require.NoError(t, specFile.VisitTags(func(tagLine *spec.TagLine, ctx *spec.Context) error { + tags = append(tags, tagLine.Tag) + + lineNumbers = append(lineNumbers, ctx.CurrentLineNum) + if tagLine.Tag == "Summary" && ctx.CurrentSection.Package == "devel" { + ctx.ReplaceLine("Summary: updated") + } + + return nil + })) + assert.Equal(t, []string{"Name", "Summary", "Summary"}, tags) + assert.Equal(t, []int{0, 4, 7}, lineNumbers) + + var output bytes.Buffer + require.NoError(t, specFile.Serialize(&output)) + assert.Contains(t, output.String(), "Summary: updated") + assert.Contains(t, output.String(), "Name: macro-body") + assert.Contains(t, output.String(), "Name: script-body") +} + +func TestVisitTagsPackage(t *testing.T) { + input := `Name: main-pkg +Version: 1.0 + +%package devel +Summary: Development files +Patch1: devel.patch +` + + tests := []struct { + name string + packageName string + options []spec.OpenOption + expectedTags []string + }{ + { + name: "default legacy editor filters package", + packageName: "devel", + expectedTags: []string{"Summary", "Patch1"}, + }, + { + name: "structural editor filters package and mutates it", + packageName: "devel", + options: []spec.OpenOption{spec.WithEditor(spec.EditorStructural)}, + expectedTags: []string{"Summary", "Patch1"}, + }, + { + name: "default legacy editor ignores unknown package", + packageName: "missing", + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(input), testCase.options...) + require.NoError(t, err) + + var tags []string + + require.NoError(t, specFile.VisitTagsPackage( + testCase.packageName, func(tagLine *spec.TagLine, ctx *spec.Context) error { + tags = append(tags, tagLine.Tag) + if testCase.name == "structural editor filters package and mutates it" && tagLine.Tag == "Summary" { + ctx.ReplaceLine("Summary: Structural mutation") + } + + return nil + })) + assert.Equal(t, testCase.expectedTags, tags) + + if testCase.options != nil { + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Contains(t, actual.String(), "Summary: Structural mutation") + } + }) + } +} + func TestSetTag(t *testing.T) { tests := []struct { name string @@ -133,7 +280,7 @@ Name: value for _, test := range tests { t.Run(test.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(test.input)) + specFile, err := spec.OpenSpec(strings.NewReader(test.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.SetTag(test.packageName, test.tag, test.value) @@ -241,7 +388,7 @@ Name: value for _, test := range tests { t.Run(test.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(test.input)) + specFile, err := spec.OpenSpec(strings.NewReader(test.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.UpdateExistingTag(test.packageName, test.tag, test.value) @@ -263,6 +410,29 @@ Name: value } } +func TestUpdateExistingTagUpdatesRepeatedConditionalTags(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(`Name: example +%if 0 +Release: 1 +%else +Release: 2 +%endif +`), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + require.NoError(t, specFile.UpdateExistingTag("", "Release", "3%{?dist}")) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, `Name: example +%if 0 +Release: 3%{?dist} +%else +Release: 3%{?dist} +%endif +`, actual.String()) +} + func TestRemoveTag(t *testing.T) { tests := []struct { name string @@ -385,7 +555,7 @@ Name: old for _, test := range tests { t.Run(test.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(test.input)) + specFile, err := spec.OpenSpec(strings.NewReader(test.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.RemoveTag(test.packageName, test.tag, test.value) @@ -500,7 +670,7 @@ BuildRequires: value for _, test := range tests { t.Run(test.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(test.input)) + specFile, err := spec.OpenSpec(strings.NewReader(test.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.AddTag(test.packageName, test.tag, test.value) @@ -522,6 +692,7 @@ BuildRequires: value } } +//nolint:maintidx // Table cases document tag insertion behavior. func TestInsertTag(t *testing.T) { tests := []struct { name string @@ -724,6 +895,85 @@ Source32: extra-x86_64.h %endif Source9999: macros.azl.macros BuildRequires: gcc +`, + tag: "Source9999", + value: "macros.azl.macros", + }, + { + name: "does not cross description before conditional end", + input: `Name: test +%if %{with extra} +Source0: extra.tar.gz +%description +Extra package description +%endif +`, + expectedOutput: `Name: test +%if %{with extra} +Source0: extra.tar.gz +Source9999: macros.azl.macros +%description +Extra package description +%endif +`, + tag: "Source9999", + value: "macros.azl.macros", + }, + { + name: "insert after last tag in conditional package variants", + input: `Name: main +%if 0 +%package -n test-package +Source0: first.tar.gz +%else +%package -n test-package +Source1: second.tar.gz +%endif +`, + expectedOutput: `Name: main +%if 0 +%package -n test-package +Source0: first.tar.gz +%else +%package -n test-package +Source1: second.tar.gz +%endif +Source9999: macros.azl.macros +`, + packageName: "test-package", + tag: "Source9999", + value: "macros.azl.macros", + }, + { + name: "ignore tags in macro bodies", + input: `%global generated \ +Source9999: generated.tar.gz +Name: main +`, + expectedOutput: `%global generated \ +Source9999: generated.tar.gz +Name: main +Vendor: Microsoft +`, + tag: "Vendor", + value: "Microsoft", + }, + { + name: "insert after nested conditional", + input: `Name: main +%if 1 +%if 1 +Source0: nested.tar.gz +%endif +%endif +`, + expectedOutput: `Name: main +%if 1 +%if 1 +Source0: nested.tar.gz +%endif +%endif +Source9999: macros.azl.macros `, tag: "Source9999", value: "macros.azl.macros", @@ -750,6 +1000,26 @@ Source32: jit-common.h %endif Source9999: macros.azl.macros BuildRequires: gcc +`, + tag: "Source9999", + value: "macros.azl.macros", + }, + { + name: "insert once after ifarch tools alternative", + input: `Name: test +%ifarch x86_64 +Source31: tools-x86_64.tar.gz +%else +Source31: tools-generic.tar.gz +%endif +`, + expectedOutput: `Name: test +%ifarch x86_64 +Source31: tools-x86_64.tar.gz +%else +Source31: tools-generic.tar.gz +%endif +Source9999: macros.azl.macros `, tag: "Source9999", value: "macros.azl.macros", @@ -782,7 +1052,7 @@ BuildRequires: gcc for _, test := range tests { t.Run(test.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(test.input)) + specFile, err := spec.OpenSpec(strings.NewReader(test.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.InsertTag(test.packageName, test.tag, test.value) @@ -813,7 +1083,7 @@ func TestSearchAndReplace(t *testing.T) { build.sh --vendor=contoso ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) expected := strings.ReplaceAll(input, "contoso", "azl") @@ -837,7 +1107,7 @@ func TestSearchAndReplace(t *testing.T) { build.sh --vendor=contoso ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.SearchAndReplace("", "", `vendor=non-existent`, "vendor=azl") @@ -856,7 +1126,7 @@ func TestSearchAndReplace(t *testing.T) { Something about contoso ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) expected := strings.ReplaceAll(input, "Something about contoso", "Something about azl") @@ -871,6 +1141,89 @@ func TestSearchAndReplace(t *testing.T) { require.Equal(t, expected, actual.String()) }) + + t.Run("replaces macro definitions and conditional directives", func(t *testing.T) { + input := `%global vendor contoso +%if "%{vendor}" == "contoso" +%build +echo contoso +%endif +` + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + err = specFile.SearchAndReplace("", "", "contoso", "azl") + require.NoError(t, err) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, `%global vendor azl +%if "%{vendor}" == "azl" +%build +echo azl +%endif +`, actual.String()) + }) + + t.Run("does not assign loose wrapper content to requested package", func(t *testing.T) { + input := `Name: test +%if 0 +%package tools +Summary: tools +%else +baz +%endif +` + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + err = specFile.SearchAndReplace("", "tools", "baz", "qux") + require.ErrorIs(t, err, spec.ErrPatternNotFound) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, input, actual.String()) + }) + + t.Run("replaces directive-shaped macro body lines in their enclosing package", func(t *testing.T) { + input := `%if 1 +%package tools +%global backslash body \ +%else backslash-marker +%global lua %{lua: +%elif lua-marker +%endif lua-marker +} +%define braces %{expand: +%else brace-marker +} +%else +%package other +%endif +` + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + require.NoError(t, specFile.SearchAndReplace("", "tools", "marker", "replaced")) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, `%if 1 +%package tools +%global backslash body \ +%else backslash-replaced +%global lua %{lua: +%elif lua-replaced +%endif lua-replaced +} +%define braces %{expand: +%else brace-replaced +} +%else +%package other +%endif +`, actual.String()) + }) } func TestAddChangelogEntry(t *testing.T) { @@ -888,7 +1241,7 @@ func TestAddChangelogEntry(t *testing.T) { Name: test ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.AddChangelogEntry(testUser, testEmail, testVersion, testRelease, testTime, []string{"Initial release"}) @@ -902,7 +1255,7 @@ Name: test %changelog ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.AddChangelogEntry( @@ -935,7 +1288,7 @@ Name: test * Wed Jan 01 2000 Test User - 0.0.1-1 - Initial release ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.AddChangelogEntry(testUser, testEmail, testVersion, testRelease, testTime, []string{"Update"}) @@ -961,7 +1314,7 @@ Name: test func TestPrependLines(t *testing.T) { t.Run("empty spec", func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader("")) + specFile, err := spec.OpenSpec(strings.NewReader(""), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) specFile.PrependLines([]string{"New line", "Next line"}) @@ -979,7 +1332,7 @@ Next line input := `%description A package. ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) specFile.PrependLines([]string{"# top comment"}) @@ -1001,7 +1354,7 @@ Version: 1.0 %description A package. ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) specFile.PrependLines([]string{"# header line 1", "# header line 2"}) @@ -1023,7 +1376,7 @@ A package. func TestAppendLines(t *testing.T) { t.Run("empty spec", func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader("")) + specFile, err := spec.OpenSpec(strings.NewReader(""), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) specFile.AppendLines([]string{"New line", "Next line"}) @@ -1047,7 +1400,7 @@ A package. * Mon Jan 01 2024 User - 1.0-1 - Initial release ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) specFile.AppendLines([]string{"# trailing comment"}) @@ -1071,7 +1424,7 @@ A package. t.Run("preamble only", func(t *testing.T) { input := `Name: test ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) specFile.AppendLines([]string{"# tail"}) @@ -1089,7 +1442,7 @@ A package. func TestPrependLinesToSection(t *testing.T) { t.Run("empty spec", func(t *testing.T) { input := "" - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.PrependLinesToSection("", "", []string{"New line", "Next line"}) @@ -1108,7 +1461,7 @@ Next line t.Run("global section", func(t *testing.T) { input := `Name: test ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.PrependLinesToSection("", "", []string{"New line", "Next line"}) @@ -1138,7 +1491,7 @@ This is another package. %build build.sh ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.PrependLinesToSection("%description", "foo", []string{"New line", "Next line"}) @@ -1169,7 +1522,7 @@ build.sh input := ` Name: test ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.PrependLinesToSection("%description", "", []string{"New line"}) @@ -1191,7 +1544,7 @@ This is another package. %build build.sh ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.AppendLinesToSection("%description", "foo", []string{"New line", "Next line"}) @@ -1222,12 +1575,87 @@ build.sh input := ` Name: test ` - specFile, err := spec.OpenSpec(strings.NewReader(input)) + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.AppendLinesToSection("%description", "", []string{"New line"}) require.Error(t, err) }) + t.Run("stays before a conditional wrapper for the next section", func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(`%build +make +%if 0 +%check +make check +%endif +`), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + require.NoError(t, specFile.AppendLinesToSection("%build", "", []string{"make install"})) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + + expected := []string{ + "%build", + "make", + "make install", + "%if 0", + "%check", + "make check", + "%endif", + } + assert.Equal(t, strings.Join(expected, "\n")+"\n", actual.String()) + }) +} + +func TestSectionLineEditsApplyToRepeatedConditionalSections(t *testing.T) { + input := `%if 0 +%description tools +disabled +%else +%description tools +enabled +%endif +` + + t.Run("prepend", func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + require.NoError(t, specFile.PrependLinesToSection("%description", "tools", []string{"marker"})) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, `%if 0 +%description tools +marker +disabled +%else +%description tools +marker +enabled +%endif +`, actual.String()) + }) + + t.Run("append", func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + require.NoError(t, specFile.AppendLinesToSection("%description", "tools", []string{"marker"})) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, `%if 0 +%description tools +disabled +marker +%else +%description tools +enabled +marker +%endif +`, actual.String()) + }) } func TestHasSection(t *testing.T) { @@ -1265,7 +1693,7 @@ func TestHasSection(t *testing.T) { for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) result, err := specFile.HasSection(testCase.sectionName) @@ -1330,7 +1758,7 @@ func TestGetHighestPatchTagNumber(t *testing.T) { for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) result, err := specFile.GetHighestPatchTagNumber() @@ -1397,7 +1825,7 @@ func TestRemoveTagsMatching(t *testing.T) { for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) count, err := specFile.RemoveTagsMatching(testCase.packageName, testCase.matcher) @@ -1488,7 +1916,7 @@ func TestRemovePatchEntry(t *testing.T) { for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.RemovePatchEntry(testCase.pattern) @@ -1511,6 +1939,38 @@ func TestRemovePatchEntry(t *testing.T) { } } +func TestPatchlistEditsApplyToRepeatedSections(t *testing.T) { + input := `Name: example +%if 0 +%patchlist +old.patch +%else +%patchlist +old.patch +%endif +` + + t.Run("add", func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + require.NoError(t, specFile.AddPatchEntry("", "new.patch")) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, 2, strings.Count(actual.String(), "new.patch")) + }) + + t.Run("remove", func(t *testing.T) { + specFile, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + require.NoError(t, specFile.RemovePatchEntry("old.patch")) + + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.NotContains(t, actual.String(), "old.patch") + }) +} + func TestParsePatchTagNumber(t *testing.T) { tests := []struct { tag string @@ -1538,107 +1998,6 @@ func TestParsePatchTagNumber(t *testing.T) { } } -func TestVisitTags(t *testing.T) { - input := `Name: main-pkg -Version: 1.0 -Patch0: main.patch - -%package devel -Summary: Development files -Patch1: devel.patch - -%package -n other -Summary: Other package -Patch2: other.patch -` - - tests := []struct { - name string - expectedTags []string - }{ - { - name: "visits tags across all packages", - expectedTags: []string{"Name", "Version", "Patch0", "Summary", "Patch1", "Summary", "Patch2"}, - }, - } - - for _, testCase := range tests { - t.Run(testCase.name, func(t *testing.T) { - sf, err := spec.OpenSpec(strings.NewReader(input)) - require.NoError(t, err) - - var tags []string - - err = sf.VisitTags(func(tagLine *spec.TagLine, _ *spec.Context) error { - tags = append(tags, tagLine.Tag) - - return nil - }) - require.NoError(t, err) - assert.Equal(t, testCase.expectedTags, tags) - }) - } -} - -func TestVisitTagsPackage(t *testing.T) { - input := `Name: main-pkg -Version: 1.0 -Patch0: main.patch - -%package devel -Summary: Development files -Patch1: devel.patch - -%package -n other -Summary: Other package -Patch2: other.patch -` - - 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, - }, - } - - for _, testCase := range tests { - t.Run(testCase.name, func(t *testing.T) { - sf, err := spec.OpenSpec(strings.NewReader(input)) - require.NoError(t, err) - - var tags []string - - err = sf.VisitTagsPackage(testCase.packageName, func(tagLine *spec.TagLine, _ *spec.Context) error { - tags = append(tags, tagLine.Tag) - - return nil - }) - require.NoError(t, err) - assert.Equal(t, testCase.expectedTags, tags) - }) - } -} - func TestRemoveSection(t *testing.T) { tests := []struct { name string @@ -1793,7 +2152,7 @@ Main. for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.RemoveSection(testCase.sectionName, testCase.packageName) @@ -1999,6 +2358,28 @@ Main. /usr/bin/test `, }, + { + name: "rejects nested conditional content orphaned by removal", + input: `Name: test + +%package devel +Summary: Devel + +%if 1 +%if 1 +shared content +%package tools +Summary: Tools +%endif +%endif + +%description tools +Tools description. +`, + packageName: "devel", + errorExpected: true, + errorContains: "conditional block spans across section boundaries", + }, { name: "trims trailing conditional opener belonging to next section", input: `Name: test @@ -2239,7 +2620,7 @@ Main. for _, testCase := range tests { t.Run(testCase.name, func(t *testing.T) { - specFile, err := spec.OpenSpec(strings.NewReader(testCase.input)) + specFile, err := spec.OpenSpec(strings.NewReader(testCase.input), spec.WithEditor(spec.EditorStructural)) require.NoError(t, err) err = specFile.RemoveSubpackage(testCase.packageName) @@ -2251,6 +2632,10 @@ Main. assert.Contains(t, err.Error(), testCase.errorContains) } + var actual bytes.Buffer + require.NoError(t, specFile.Serialize(&actual)) + assert.Equal(t, testCase.input, actual.String()) + return } diff --git a/internal/rpm/spec/editor.go b/internal/rpm/spec/editor.go new file mode 100644 index 000000000..b6b94f425 --- /dev/null +++ b/internal/rpm/spec/editor.go @@ -0,0 +1,229 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "fmt" + "io" + "time" +) + +// EditorMode identifies the implementation that edits an RPM spec. +type EditorMode string + +const ( + elseDirective = "%else" + + // EditorLegacy uses the established line-oriented editor. + EditorLegacy EditorMode = "legacy" + // EditorStructural uses the lossless structural editor. + EditorStructural EditorMode = "structural" +) + +type editorOptions struct { + mode EditorMode +} + +// OpenOption configures [OpenSpec]. +type OpenOption func(*editorOptions) + +// WithEditor selects the editor implementation used by [OpenSpec]. +func WithEditor(mode EditorMode) OpenOption { + return func(options *editorOptions) { + options.mode = mode + } +} + +//nolint:interfacebloat,inamedparam // The facade must cover the established public Spec API. +type specEditor interface { + Serialize(io.Writer) error + ReplaceLine(int, string) + RemoveLine(int) + RemoveLines(int, int) + InsertLinesAt([]string, int) + Visit(Visitor) error + VisitTags(func(*TagLine, *Context) error) error + VisitTagsPackage(string, func(*TagLine, *Context) error) error + SetTag(string, string, string) error + UpdateExistingTag(string, string, string) error + RemoveTag(string, string, string) error + RemoveTagsMatching(string, func(string, string) bool) (int, error) + AddTag(string, string, string) error + InsertTag(string, string, string) error + PrependLines([]string) + AppendLines([]string) + PrependLinesToSection(string, string, []string) error + AppendLinesToSection(string, string, []string) error + SearchAndReplace(string, string, string, string) error + AddChangelogEntry(string, string, string, string, time.Time, []string) error + HasSection(string) (bool, error) + AddPatchEntry(string, string) error + RemovePatchEntry(string) error + GetHighestPatchTagNumber() (int, error) + RemoveSection(string, string) error + RemoveSubpackage(string) error + GetTag(string, string) (string, error) + GetLastTag(string, string) (string, error) +} + +// Spec is the public facade for a configuration-selected RPM spec editor. +type Spec struct { + editor specEditor +} + +// OpenSpec reads a spec and selects its editor once. With no option, it preserves +// the established legacy behavior. +func OpenSpec(reader io.Reader, options ...OpenOption) (*Spec, error) { + config := editorOptions{mode: EditorLegacy} + for _, option := range options { + option(&config) + } + + var ( + editor specEditor + err error + ) + + switch config.mode { + case EditorLegacy, "": + editor, err = openLegacySpec(reader) + case EditorStructural: + editor, err = openStructuralSpec(reader) + default: + return nil, fmt.Errorf("unknown spec editor %#q", config.mode) + } + + if err != nil { + return nil, err + } + + return &Spec{editor: editor}, nil +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) Serialize(writer io.Writer) error { + return s.editor.Serialize(writer) +} + +func (s *Spec) ReplaceLine(lineNumber int, replacement string) { + s.editor.ReplaceLine(lineNumber, replacement) +} +func (s *Spec) RemoveLine(lineNumber int) { s.editor.RemoveLine(lineNumber) } +func (s *Spec) RemoveLines(start, end int) { s.editor.RemoveLines(start, end) } +func (s *Spec) InsertLinesAt(lines []string, lineNumber int) { + s.editor.InsertLinesAt(lines, lineNumber) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) Visit(visitor Visitor) error { + return s.editor.Visit(visitor) +} + +// 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]. +// +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) VisitTags(visitor func(tagLine *TagLine, ctx *Context) error) error { + return s.editor.VisitTags(visitor) +} + +// 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]. +// +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) VisitTagsPackage(packageName string, visitor func(tagLine *TagLine, ctx *Context) error) error { + return s.editor.VisitTagsPackage(packageName, visitor) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) SetTag(pkg, tag, value string) error { + return s.editor.SetTag(pkg, tag, value) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) UpdateExistingTag(pkg, tag, value string) error { + return s.editor.UpdateExistingTag(pkg, tag, value) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) RemoveTag(pkg, tag, value string) error { + return s.editor.RemoveTag(pkg, tag, value) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) RemoveTagsMatching(pkg string, matcher func(string, string) bool) (int, error) { + return s.editor.RemoveTagsMatching(pkg, matcher) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) AddTag(pkg, tag, value string) error { + return s.editor.AddTag(pkg, tag, value) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) InsertTag(pkg, tag, value string) error { + return s.editor.InsertTag(pkg, tag, value) +} +func (s *Spec) PrependLines(lines []string) { s.editor.PrependLines(lines) } +func (s *Spec) AppendLines(lines []string) { s.editor.AppendLines(lines) } + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) PrependLinesToSection(section, pkg string, lines []string) error { + return s.editor.PrependLinesToSection(section, pkg, lines) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) AppendLinesToSection(section, pkg string, lines []string) error { + return s.editor.AppendLinesToSection(section, pkg, lines) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) SearchAndReplace(section, pkg, regex, replacement string) error { + return s.editor.SearchAndReplace(section, pkg, regex, replacement) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) AddChangelogEntry(user, email, version, release string, at time.Time, details []string) error { + return s.editor.AddChangelogEntry(user, email, version, release, at, details) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) HasSection(section string) (bool, error) { + return s.editor.HasSection(section) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) AddPatchEntry(pkg, filename string) error { + return s.editor.AddPatchEntry(pkg, filename) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) RemovePatchEntry(pattern string) error { + return s.editor.RemovePatchEntry(pattern) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) GetHighestPatchTagNumber() (int, error) { + return s.editor.GetHighestPatchTagNumber() +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) RemoveSection(section, pkg string) error { + return s.editor.RemoveSection(section, pkg) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) RemoveSubpackage(pkg string) error { + return s.editor.RemoveSubpackage(pkg) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) GetTag(pkg, tag string) (string, error) { + return s.editor.GetTag(pkg, tag) +} + +//nolint:wrapcheck // Preserve errors returned by the selected editor. +func (s *Spec) GetLastTag(pkg, tag string) (string, error) { + return s.editor.GetLastTag(pkg, tag) +} diff --git a/internal/rpm/spec/edit.go b/internal/rpm/spec/legacy_edit.go similarity index 90% rename from internal/rpm/spec/edit.go rename to internal/rpm/spec/legacy_edit.go index 0fdd056c3..3f530beb6 100644 --- a/internal/rpm/spec/edit.go +++ b/internal/rpm/spec/legacy_edit.go @@ -32,7 +32,7 @@ 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. -func (s *Spec) SetTag(packageName string, tag string, value string) (err error) { +func (s *legacySpec) SetTag(packageName string, tag string, value string) (err error) { err = s.UpdateExistingTag(packageName, tag, value) if err == nil { return nil @@ -48,7 +48,7 @@ func (s *Spec) SetTag(packageName string, tag string, value string) (err error) // 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. -func (s *Spec) UpdateExistingTag(packageName string, tag string, value string) (err error) { +func (s *legacySpec) 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) @@ -78,7 +78,7 @@ func (s *Spec) UpdateExistingTag(packageName string, tag string, value string) ( // package (or globally if `packageName` is empty). If the provided `value` is non-empty, // then only tag instances whose values are as specified will be removed. This function // returns an error if a tag matching those criteria did not exist in the given package. -func (s *Spec) RemoveTag(packageName string, tag string, value string) (err error) { +func (s *legacySpec) RemoveTag(packageName string, tag string, value string) (err error) { slog.Debug("Removing tag from spec", "package", packageName, "tag", tag, "value", value) tagToCompareAgainst := strings.ToLower(tag) @@ -107,7 +107,7 @@ func (s *Spec) RemoveTag(packageName string, tag string, value string) (err erro // 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 { +func (s *legacySpec) VisitTags(visitor func(tagLine *TagLine, ctx *Context) error) error { return s.Visit(func(ctx *Context) error { if ctx.Target.TargetType != SectionLineTarget { return nil @@ -130,7 +130,7 @@ func (s *Spec) VisitTags(visitor func(tagLine *TagLine, ctx *Context) error) err // 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 { +func (s *legacySpec) 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 @@ -143,7 +143,7 @@ func (s *Spec) VisitTagsPackage(packageName string, visitor func(tagLine *TagLin // RemoveTagsMatching removes all tags in the given package for which the provided matcher // function returns true. The matcher receives the tag name and value as arguments. Returns // the number of tags removed. If no matching tags were found, returns 0 and no error. -func (s *Spec) RemoveTagsMatching(packageName string, matcher func(tag, value string) bool) (int, error) { +func (s *legacySpec) RemoveTagsMatching(packageName string, matcher func(tag, value string) bool) (int, error) { removed := 0 err := s.VisitTagsPackage(packageName, func(tagLine *TagLine, ctx *Context) error { @@ -169,7 +169,7 @@ func (s *Spec) RemoveTagsMatching(packageName string, matcher func(tag, value st // // Note: When adding to a sub-package (non-empty packageName), the corresponding %package // section must already exist in the spec; otherwise, an [ErrSectionNotFound] error is returned. -func (s *Spec) AddTag(packageName string, tag string, value string) (err error) { +func (s *legacySpec) AddTag(packageName string, tag string, value string) (err error) { slog.Debug("Adding tag to spec", "package", packageName, "tag", tag, "value", value) sectionName := "" @@ -232,6 +232,8 @@ func conditionalDepthChange(rawLine string) int { // 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, "#") { @@ -246,7 +248,7 @@ func isConditionalBranchDirective(rawLine string) bool { lower := strings.ToLower(tokens[0]) switch lower { - case "%else", "%elif", "%elifarch", "%elifnarch", "%elifos", "%elifnos": + case elseDirective, "%elif", "%elifarch", "%elifnarch", "%elifos", "%elifnos": return true default: return false @@ -268,7 +270,7 @@ func isConditionalBranchDirective(rawLine string) bool { // Note: When inserting into a sub-package (non-empty packageName), the corresponding // %package section must already exist in the spec; otherwise, an [ErrSectionNotFound] // error is returned. -func (s *Spec) InsertTag(packageName string, tag string, value string) error { +func (s *legacySpec) InsertTag(packageName string, tag string, value string) error { slog.Debug("Inserting tag to spec", "package", packageName, "tag", tag, "value", value) family := tagFamily(tag) @@ -315,7 +317,7 @@ type insertTagScanResult struct { // 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( +func (s *legacySpec) findInsertTagPosition( sectionName, packageName, family string, ) (insertTagScanResult, error) { result := insertTagScanResult{ @@ -379,7 +381,7 @@ func (s *Spec) findInsertTagPosition( // 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 { +func (s *legacySpec) 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++ { @@ -405,7 +407,7 @@ func (s *Spec) skipPastConditional(lineNum int, sectionEnd int) int { // 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. -func (s *Spec) PrependLines(lines []string) { +func (s *legacySpec) PrependLines(lines []string) { slog.Debug("Prepending lines to spec file", "lines", lines) s.rawLines = append(append([]string{}, lines...), s.rawLines...) @@ -414,7 +416,7 @@ func (s *Spec) PrependLines(lines []string) { // 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. -func (s *Spec) AppendLines(lines []string) { +func (s *legacySpec) AppendLines(lines []string) { slog.Debug("Appending lines to spec file", "lines", lines) s.rawLines = append(s.rawLines, lines...) @@ -423,7 +425,7 @@ func (s *Spec) AppendLines(lines []string) { // 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. -func (s *Spec) PrependLinesToSection(sectionName, packageName string, lines []string) (err error) { +func (s *legacySpec) PrependLinesToSection(sectionName, packageName string, lines []string) (err error) { slog.Debug("Prepending lines to spec", "section", sectionName, "package", packageName, "lines", lines) var updated bool @@ -470,7 +472,7 @@ func (s *Spec) PrependLinesToSection(sectionName, packageName string, lines []st // 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. -func (s *Spec) AppendLinesToSection(sectionName, packageName string, lines []string) (err error) { +func (s *legacySpec) AppendLinesToSection(sectionName, packageName string, lines []string) (err error) { slog.Debug("Appending lines to spec", "section", sectionName, "package", packageName, "lines", lines) var updated bool @@ -511,7 +513,7 @@ func (s *Spec) AppendLinesToSection(sectionName, packageName string, lines []str // 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. -func (s *Spec) SearchAndReplace(sectionName, packageName, regex, replacement string) (err error) { +func (s *legacySpec) SearchAndReplace(sectionName, packageName, regex, replacement string) (err error) { slog.Debug("Searching and replacing in spec", "section", sectionName, "package", packageName, @@ -572,7 +574,9 @@ func (s *Spec) SearchAndReplace(sectionName, packageName, regex, replacement str // AddChangelogEntry adds a changelog entry to the spec's changelog section. An error is returned if // no %changelog section exists in the spec. -func (s *Spec) AddChangelogEntry(user, email, version, release string, time time.Time, details []string) (err error) { +// +//nolint:lll +func (s *legacySpec) AddChangelogEntry(user, email, version, release string, time time.Time, details []string) (err error) { slog.Debug("Adding changelog entry to spec", "user", user, "email", email, "version", version, "release", release, "details", details) @@ -633,7 +637,7 @@ func ParsePatchTagNumber(tag string) (int, bool) { // HasSection returns true if the spec contains a section with the given name. // The comparison is exact (case-sensitive), consistent with [AppendLinesToSection]. -func (s *Spec) HasSection(sectionName string) (bool, error) { +func (s *legacySpec) HasSection(sectionName string) (bool, error) { var found bool err := s.Visit(func(ctx *Context) error { @@ -650,7 +654,7 @@ func (s *Spec) HasSection(sectionName string) (bool, error) { // AddPatchEntry registers a patch in the spec, either by appending to an existing %patchlist // section or by adding a new PatchN tag with the next available number. Returns an error // if the spec cannot be examined or updated. -func (s *Spec) AddPatchEntry(packageName, filename string) error { +func (s *legacySpec) AddPatchEntry(packageName, filename string) error { slog.Debug("Adding patch entry to spec", "package", packageName, "filename", filename) hasPatchlist, err := s.HasSection("%patchlist") @@ -673,7 +677,7 @@ func (s *Spec) AddPatchEntry(packageName, filename string) error { // RemovePatchEntry removes all references to patches matching the given pattern from the spec. // The pattern is a glob pattern (supporting doublestar syntax) matched against PatchN tag values // and %patchlist entries across all packages. Returns an error if no references matched the pattern. -func (s *Spec) RemovePatchEntry(pattern string) error { +func (s *legacySpec) RemovePatchEntry(pattern string) error { slog.Debug("Removing patch entry from spec", "pattern", pattern) totalRemoved := 0 @@ -708,7 +712,7 @@ func (s *Spec) RemovePatchEntry(pattern string) error { // removePatchTagsMatching removes all PatchN tags across all packages whose values match the // given glob pattern. Returns the number of tags removed. -func (s *Spec) removePatchTagsMatching(pattern string) (int, error) { +func (s *legacySpec) removePatchTagsMatching(pattern string) (int, error) { removed := 0 err := s.VisitTags(func(tagLine *TagLine, ctx *Context) error { @@ -735,7 +739,7 @@ func (s *Spec) removePatchTagsMatching(pattern string) (int, error) { // removePatchlistEntriesMatching removes lines from the %patchlist section whose trimmed content // matches the given glob pattern. Returns the number of entries removed. -func (s *Spec) removePatchlistEntriesMatching(pattern string) (int, error) { +func (s *legacySpec) removePatchlistEntriesMatching(pattern string) (int, error) { removed := 0 err := s.Visit(func(ctx *Context) error { @@ -774,7 +778,7 @@ func (s *Spec) removePatchlistEntriesMatching(pattern string) (int, error) { // suffix) are treated as auto-numbered starting from 0, consistent with RPM's behavior. // Returns -1 if no numbered PatchN tags and no unnumbered "Patch:" tags are found. Tags with // non-numeric suffixes (e.g., macro-based names like Patch%{n}) are silently skipped. -func (s *Spec) GetHighestPatchTagNumber() (int, error) { +func (s *legacySpec) GetHighestPatchTagNumber() (int, error) { highest := -1 unnumberedCount := 0 @@ -807,7 +811,7 @@ func (s *Spec) GetHighestPatchTagNumber() (int, error) { // sections with the same identity (e.g. inside mutually-exclusive `%if`/`%else` // branches), every such section is removed. Returns [ErrSectionNotFound] if no // matching section exists. -func (s *Spec) RemoveSection(sectionName, packageName string) error { +func (s *legacySpec) RemoveSection(sectionName, packageName string) error { slog.Debug("Removing section from spec", "section", sectionName, "package", packageName) if sectionName == "" { @@ -852,7 +856,7 @@ func (s *Spec) RemoveSection(sectionName, packageName string) error { // 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 { +func (s *legacySpec) RemoveSubpackage(packageName string) error { slog.Debug("Removing sub-package from spec", "package", packageName) if packageName == "" { @@ -893,7 +897,7 @@ type sectionLineRange struct { // 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( +func (s *legacySpec) collectSectionRanges( matches func(sectName, packageName string) bool, ) ([]sectionLineRange, error) { var ( @@ -953,8 +957,8 @@ func (s *Spec) collectSectionRanges( return ranges, err } -// conditionalPair represents a matched `%if`/`%endif` pair by their line numbers. -type conditionalPair struct { +// legacyConditionalPair represents a matched `%if`/`%endif` pair by their line numbers. +type legacyConditionalPair struct { ifLine int endifLine int } @@ -962,9 +966,9 @@ 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. -func collectConditionalPairs(rawLines []string) ([]conditionalPair, error) { +func collectConditionalPairs(rawLines []string) ([]legacyConditionalPair, error) { var ( - pairs []conditionalPair + pairs []legacyConditionalPair stack []int ) @@ -980,7 +984,7 @@ func collectConditionalPairs(rawLines []string) ([]conditionalPair, error) { ifLine := stack[len(stack)-1] stack = stack[:len(stack)-1] - pairs = append(pairs, conditionalPair{ifLine: ifLine, endifLine: lineNum}) + pairs = append(pairs, legacyConditionalPair{ifLine: ifLine, endifLine: lineNum}) } } @@ -1007,7 +1011,9 @@ func collectConditionalPairs(rawLines []string) ([]conditionalPair, error) { // 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) { +// +//nolint:lll +func balanceRange(sectionRange sectionLineRange, rawLines []string, pairs []legacyConditionalPair) (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). @@ -1092,7 +1098,7 @@ func validateNoContentAfter(startLine, endLine int, rawLines []string) error { func validateNoBranchDirectivesInExternalConditional( sectionRange sectionLineRange, rawLines []string, - pairs []conditionalPair, + pairs []legacyConditionalPair, ) error { for lineNum := sectionRange.start; lineNum < sectionRange.end; lineNum++ { if !isConditionalBranchDirective(rawLines[lineNum]) { @@ -1130,8 +1136,56 @@ func isBlankOrComment(line string) bool { // 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) { +func (s *legacySpec) removeRanges(ranges []sectionLineRange) { for i := len(ranges) - 1; i >= 0; i-- { s.RemoveLines(ranges[i].start, ranges[i].end) } } + +// GetTag returns the first matching tag in a package. +func (s *legacySpec) GetTag(packageName, tag string) (string, error) { + var value string + + found := false + + err := s.VisitTagsPackage(packageName, func(tagLine *TagLine, _ *Context) error { + if !found && strings.EqualFold(tagLine.Tag, tag) { + value, found = tagLine.Value, true + } + + return nil + }) + if err != nil { + return "", err + } + + if !found { + return "", fmt.Errorf("tag %#q not found in package %#q:\n%w", tag, packageName, ErrNoSuchTag) + } + + return value, nil +} + +// GetLastTag returns the last matching tag in a package. +func (s *legacySpec) GetLastTag(packageName, tag string) (string, error) { + var value string + + found := false + + err := s.VisitTagsPackage(packageName, func(tagLine *TagLine, _ *Context) error { + if strings.EqualFold(tagLine.Tag, tag) { + value, found = tagLine.Value, true + } + + return nil + }) + if err != nil { + return "", err + } + + if !found { + return "", fmt.Errorf("tag %#q not found in package %#q:\n%w", tag, packageName, ErrNoSuchTag) + } + + return value, nil +} diff --git a/internal/rpm/spec/spec.go b/internal/rpm/spec/legacy_spec.go similarity index 70% rename from internal/rpm/spec/spec.go rename to internal/rpm/spec/legacy_spec.go index 1ab04206b..7c3aaabc0 100644 --- a/internal/rpm/spec/spec.go +++ b/internal/rpm/spec/legacy_spec.go @@ -12,130 +12,10 @@ import ( "strings" ) -// sectionTypesByName is a table of known sections, mapping them to their types. This table must -// be kept in sync with new section types as they are added to the RPM spec format. -// -//nolint:gochecknoglobals // This is effectively a constant, but Go doesn't have const maps. -var sectionTypesByName = map[string]SectionType{ - "%package": PackageSection, - "%prep": ScriptSection, - "%conf": ScriptSection, - "%build": ScriptSection, - "%install": ScriptSection, - "%check": ScriptSection, - "%clean": ScriptSection, - "%generate_buildrequires": ScriptSection, - "%pre": ScriptSection, - "%post": ScriptSection, - "%preun": ScriptSection, - "%postun": ScriptSection, - "%pretrans": ScriptSection, - "%posttrans": ScriptSection, - "%preuntrans": ScriptSection, - "%postuntrans": ScriptSection, - "%verify": ScriptSection, - "%triggerin": ScriptSection, - "%triggerun": ScriptSection, - "%triggerprein": ScriptSection, - "%triggerpostun": ScriptSection, - "%filetriggerin": ScriptSection, - "%filetriggerun": ScriptSection, - "%filetriggerpostun": ScriptSection, - "%transfiletriggerin": ScriptSection, - "%transfiletriggerun": ScriptSection, - "%transfiletriggerpostun": ScriptSection, - "%description": RawSection, - "%files": FilesSection, - "%changelog": ChangelogSection, - "%patchlist": SourceFileListSection, - "%sourcelist": SourceFileListSection, -} - -// Spec encapsulates the contents of an RPM spec file. -type Spec struct { +type legacySpec struct { rawLines []string } -// Line represents a single line in an RPM spec file. -type Line struct { - // Text is the original physical text of the line. - Text string - // Parsed is the parsed representation of the line's contents. - Parsed ParsedLine -} - -// ParsedLineType represents the type of a parsed line. -type ParsedLineType string - -const ( - // SectionStart applies to lines that start a new section, e.g. "%description". - SectionStart ParsedLineType = "SectionStart" - // Tag applies to lines that define a tag, e.g. "Name: foo". - Tag ParsedLineType = "Tag" - // Raw applies to lines that are raw text, e.g. a line in a script section. - Raw ParsedLineType = "Raw" -) - -// ParsedLine is the interface that all parsed line types implement. -type ParsedLine interface { - // GetType returns the type of the parsed line. - GetType() ParsedLineType -} - -// SectionType represents the type of a section in an RPM spec file. -type SectionType string - -const ( - // PackageSection applies to sections that define a package, e.g. "%package -n foo". - PackageSection SectionType = "Package" - // ScriptSection applies to sections that contain scripts, e.g. "%build". - ScriptSection SectionType = "Script" - // RawSection applies to sections that contain raw content, e.g.: "%description". - RawSection SectionType = "Raw" - // ChangelogSection applies to the "%changelog" section. - ChangelogSection SectionType = "Changelog" - // FilesSection applies to a "%files" section. - FilesSection SectionType = "Files" - // SourceFileListSection applies to a section that lists source files, e.g.: "%sourcelist". - SourceFileListSection SectionType = "SourceFileList" -) - -// SectionStartLine represents a line that starts a new section in the spec, e.g.: "%build". -type SectionStartLine struct { - SectType SectionType - SectName string - Tokens []string -} - -// GetType returns the type of the parsed line. -func (*SectionStartLine) GetType() ParsedLineType { - return SectionStart -} - -// TagLine encapsulates the definition of a tag. -type TagLine struct { - // Tag is the name of the tag being defined. - Tag string - // Value is the value assigned to the tag. - Value string -} - -// GetType returns the type of the parsed line. -func (*TagLine) GetType() ParsedLineType { - return Tag -} - -// RawLine represents a line that is raw text. -type RawLine struct { - // Content is the raw line text. - Content string -} - -// GetType returns the type of the parsed line. -func (*RawLine) GetType() ParsedLineType { - return Raw -} - type parseState struct { currentSect SectionTarget } @@ -152,9 +32,9 @@ func newParseState() parseState { // 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) { +func openLegacySpec(reader io.Reader) (*legacySpec, error) { scanner := bufio.NewScanner(reader) - spec := &Spec{} + spec := &legacySpec{} // Read each line from the reader, parsing as we go. Store all parsed lines in the spec object. for scanner.Scan() { @@ -170,7 +50,7 @@ func OpenSpec(reader io.Reader) (*Spec, error) { } // Serialize writes the spec's contents to the provided writer. -func (s *Spec) Serialize(writer io.Writer) error { +func (s *legacySpec) Serialize(writer io.Writer) error { bufWriter := bufio.NewWriter(writer) for _, line := range s.rawLines { _, err := bufWriter.WriteString(line + "\n") @@ -188,22 +68,22 @@ func (s *Spec) Serialize(writer io.Writer) error { } // ReplaceLine replaces the line at the specified (0-indexed) line number with the provided replacement line. -func (s *Spec) ReplaceLine(lineNumber int, replacement string) { +func (s *legacySpec) ReplaceLine(lineNumber int, replacement string) { s.rawLines[lineNumber] = replacement } // RemoveLine removes the line at the specified (0-indexed) line number. -func (s *Spec) RemoveLine(lineNumber int) { +func (s *legacySpec) RemoveLine(lineNumber int) { s.rawLines = slices.Delete(s.rawLines, lineNumber, lineNumber+1) } // RemoveLines removes the lines in the specified (0-indexed) line number range [startLineNumber, endLineNumber). -func (s *Spec) RemoveLines(startLineNumber int, endLineNumber int) { +func (s *legacySpec) RemoveLines(startLineNumber int, endLineNumber int) { s.rawLines = slices.Delete(s.rawLines, startLineNumber, endLineNumber) } // InsertLinesAt inserts the provided lines just before the specified (0-indexed) line number. -func (s *Spec) InsertLinesAt(insertedLines []string, lineNumber int) { +func (s *legacySpec) InsertLinesAt(insertedLines []string, lineNumber int) { s.rawLines = slices.Insert(s.rawLines, lineNumber, insertedLines...) } @@ -230,14 +110,22 @@ type Context struct { nextLineNumToParse int // nextLineNumToVisit is the next (0-indexed) line number that will be visited. nextLineNumToVisit int - // spec is the spec being visited. - spec *Spec + // spec is the legacy spec being visited. + spec *legacySpec + // structuralLine is the structural line being visited, when applicable. + structuralLine *lineHandle } // 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) { + if ctx.structuralLine != nil { + ctx.structuralLine.InsertBefore(lines) + + return + } + ctx.spec.InsertLinesAt(lines, ctx.CurrentLineNum) // Account for the displacement from the inserted lines. We will parse the @@ -252,6 +140,12 @@ func (ctx *Context) InsertLinesBefore(lines []string) { // 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) { + if ctx.structuralLine != nil { + ctx.structuralLine.InsertAfter(lines) + + return + } + ctx.spec.InsertLinesAt(lines, ctx.CurrentLineNum+1) // Skip ahead past the newly inserted lines. @@ -262,6 +156,12 @@ func (ctx *Context) InsertLinesAfter(lines []string) { // 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() { + if ctx.structuralLine != nil { + ctx.structuralLine.Remove() + + return + } + ctx.spec.RemoveLine(ctx.CurrentLineNum) // Account for the removed line. We will reparse the new current line and revisit it. @@ -274,6 +174,12 @@ func (ctx *Context) RemoveLine() { // ReplaceLine replaces the line currently being visited with the provided replacement line, // updating the context accordingly. func (ctx *Context) ReplaceLine(replacement string) { + if ctx.structuralLine != nil { + ctx.structuralLine.Replace(replacement) + + return + } + ctx.spec.ReplaceLine(ctx.CurrentLineNum, replacement) // Account for the replaced line. We will reparse the current line, but not revisit it. @@ -311,17 +217,6 @@ const ( SpecEndTarget VisitTargetType = "SpecEnd" ) -// SectionTarget encapsulates information about the current section context. -type SectionTarget struct { - // SectName is the name of the section, e.g. "%description". - SectName string - // SectType is the type of the section. - SectType SectionType - // Package is the package this section applies to, if any. Left empty for - // the default package or sections that aren't package-specific. - Package string -} - // Visitor is the type of a visitor function that can be passed to [Spec.Visit]. type Visitor = func(ctx *Context) error @@ -337,7 +232,7 @@ type Visitor = func(ctx *Context) error // - Context mutation methods update these values to maintain correct traversal after modifications. // //nolint:funlen -func (s *Spec) Visit(visitor Visitor) error { +func (s *legacySpec) Visit(visitor Visitor) error { ctx := Context{ Target: VisitTarget{TargetType: SpecStartTarget}, CurrentSection: newParseState().currentSect, @@ -459,7 +354,7 @@ func parseSpecLine(physicalText string, state parseState) (ParsedLine, parseStat if sectionStartLine, ok := parsedLine.(*SectionStartLine); ok { state.currentSect.SectType = sectionStartLine.SectType state.currentSect.SectName = sectionStartLine.SectName - state.currentSect.Package = getPackageNameForSection(sectionStartLine.SectType, sectionStartLine.Tokens) + state.currentSect.Package = legacyGetPackageNameForSection(sectionStartLine.SectType, sectionStartLine.Tokens) } return parsedLine, state @@ -476,7 +371,7 @@ func newParsedLine(physicalText string, state parseState) ParsedLine { return parseLogicalLine(logicalLine, state) } -var tagRegex = regexp.MustCompile(`^\s*([^\s:]+):\s*(.*?)\s*$`) +var legacyTagRegex = regexp.MustCompile(`^\s*([^\s:]+):\s*(.*?)\s*$`) func parseLogicalLine(logicalLine string, state parseState) ParsedLine { tokens := strings.Fields(logicalLine) @@ -500,7 +395,7 @@ func parseLogicalLine(logicalLine string, state parseState) ParsedLine { if state.currentSect.SectType == PackageSection { const reSubmatchCount = 3 - matches := tagRegex.FindStringSubmatch(logicalLine) + matches := legacyTagRegex.FindStringSubmatch(logicalLine) if len(matches) == reSubmatchCount { return &TagLine{ Tag: matches[1], @@ -515,7 +410,7 @@ func parseLogicalLine(logicalLine string, state parseState) ParsedLine { } } -func getPackageNameForSection(sectionType SectionType, headerTokens []string) string { +func legacyGetPackageNameForSection(sectionType SectionType, headerTokens []string) string { switch sectionType { case SourceFileListSection: fallthrough @@ -528,7 +423,7 @@ func getPackageNameForSection(sectionType SectionType, headerTokens []string) st case FilesSection: fallthrough case ScriptSection: - return GetPackageNameFromSectionHeader(headerTokens) + return legacyGetPackageNameFromSectionHeader(headerTokens) default: return "" } @@ -539,7 +434,7 @@ func getPackageNameForSection(sectionType SectionType, headerTokens []string) st // For a line like "%package foo", it would return "foo" as well. Because this function // does not know the base name of the spec, it cannot take a suffix-only name and resolve // it to a full name. -func GetPackageNameFromSectionHeader(tokens []string) string { +func legacyGetPackageNameFromSectionHeader(tokens []string) string { fullName := "" nameSuffix := "" index := 1 // Skip the first token diff --git a/internal/rpm/spec/spec_test.go b/internal/rpm/spec/spec_test.go index 88167abfd..b8c79c5db 100644 --- a/internal/rpm/spec/spec_test.go +++ b/internal/rpm/spec/spec_test.go @@ -52,14 +52,7 @@ func TestGetPackageNameFromSectionHeader(t *testing.T) { func TestOpenSpec_EmptyInput(t *testing.T) { sf, err := spec.OpenSpec(strings.NewReader("")) 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) + assert.NotNil(t, sf) } func TestOpenSpec_BinaryContent(t *testing.T) { diff --git a/internal/rpm/spec/structural_edit.go b/internal/rpm/spec/structural_edit.go new file mode 100644 index 000000000..62fd61265 --- /dev/null +++ b/internal/rpm/spec/structural_edit.go @@ -0,0 +1,1042 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "errors" + "fmt" + "log/slog" + "regexp" + "slices" + "strconv" + "strings" + "time" + + "github.com/bmatcuk/doublestar/v4" +) + +// 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. +func (s *structuralSpec) SetTag(packageName string, tag string, value string) (err error) { + err = s.UpdateExistingTag(packageName, tag, value) + if err == nil { + return nil + } + + if errors.Is(err, ErrNoSuchTag) { + err = s.AddTag(packageName, tag, value) + } + + return err +} + +// UpdateExistingTag replaces every instance of the named tag in the given +// package with the provided value. If no such tag exists, it returns an error. +func (s *structuralSpec) 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 + + err = s.mutateTree(func(tree *specTree) error { + return tree.VisitAllLines(func(secName, secPkg string, line *lineHandle) error { + if secPkg != packageName || !isTagBearingSection(secName) { + return nil + } + + parsedTag, _, isTag := parseTagLine(line.Text) + if !isTag || strings.ToLower(parsedTag) != tagToCompareAgainst { + return nil + } + + line.Replace(fmt.Sprintf("%s: %s", tag, value)) + + updated = true + + return nil + }) + }) + if err != nil { + return err + } + + if !updated { + return fmt.Errorf("tag %#q not found in spec:\n%w", tag, ErrNoSuchTag) + } + + return nil +} + +// RemoveTag removes all instances of the given tag from the spec, under the specified +// package (or globally if `packageName` is empty). If the provided `value` is non-empty, +// then only tag instances whose values are as specified will be removed. This function +// returns an error if a tag matching those criteria did not exist in the given package. +func (s *structuralSpec) RemoveTag(packageName string, tag string, value string) (err error) { + slog.Debug("Removing tag from spec", "package", packageName, "tag", tag, "value", value) + + tagToCompareAgainst := strings.ToLower(tag) + + removed, err := s.RemoveTagsMatching(packageName, func(t, v string) bool { + if strings.ToLower(t) != tagToCompareAgainst { + return false + } + + if value != "" && !strings.EqualFold(v, value) { + return false + } + + return true + }) + if err != nil { + return err + } + + if removed == 0 { + return fmt.Errorf("tag %#q with value %#q not found in spec:\n%w", tag, value, ErrNoSuchTag) + } + + 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 *structuralSpec) VisitTags(visitor func(tagLine *TagLine, ctx *Context) error) error { + root, err := parseTree(s.rawLines) + if err != nil { + return fmt.Errorf("parsing spec tree:\n%w", err) + } + + tree := &specTree{root: root} + + err = tree.VisitAllLines(func(sectionName, packageName string, line *lineHandle) error { + if !isTagBearingSection(sectionName) { + return nil + } + + tag, value, isTag := parseTagLine(line.Text) + if !isTag { + return nil + } + + rawLine := line.Text + + return visitor(&TagLine{Tag: tag, Value: value}, &Context{ + Target: VisitTarget{ + TargetType: SectionLineTarget, + Line: &Line{Text: line.Text, Parsed: &TagLine{Tag: tag, Value: value}}, + }, + RawLine: &rawLine, + CurrentLineNum: line.lineNumber, + CurrentSection: SectionTarget{ + SectName: sectionName, + SectType: PackageSection, + Package: packageName, + }, + structuralLine: line, + }) + }) + if err != nil { + return err + } + + lines := serializeTree(root) + if _, err := parseTree(lines); err != nil { + return fmt.Errorf("validating mutated spec tree:\n%w", err) + } + + s.rawLines = lines + + return 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]. +func (s *structuralSpec) 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 + } + + return visitor(tagLine, ctx) + }) +} + +// 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 *structuralSpec) GetTag(packageName string, tag string) (string, error) { + var ( + foundValue string + found bool + ) + + err := s.inspectTree(func(tree *specTree) error { + foundValue, found = tree.GetTag(packageName, tag) + + return nil + }) + if err != nil { + return "", err + } + + if !found { + return "", fmt.Errorf("tag %#q not found in package %#q:\n%w", tag, packageName, ErrNoSuchTag) + } + + return foundValue, nil +} + +// GetLastTag returns the value of the final lexical instance of the named tag +// in the given package. Returns [ErrNoSuchTag] if the tag does not exist. +func (s *structuralSpec) GetLastTag(packageName string, tag string) (string, error) { + var ( + value string + found bool + ) + + err := s.inspectTree(func(tree *specTree) error { + value, found = tree.GetLastTag(packageName, tag) + + return nil + }) + if err != nil { + return "", err + } + + if !found { + return "", fmt.Errorf("tag %#q not found in package %#q:\n%w", tag, packageName, ErrNoSuchTag) + } + + return value, nil +} + +// RemoveTagsMatching removes all tags in the given package for which the provided matcher +// function returns true. The matcher receives the tag name and value as arguments. Returns +// the number of tags removed. If no matching tags were found, returns 0 and no error. +func (s *structuralSpec) RemoveTagsMatching(packageName string, matcher func(tag, value string) bool) (int, error) { + removed := 0 + + err := s.mutateTree(func(tree *specTree) error { + return tree.VisitAllLines(func(secName, secPkg string, line *lineHandle) error { + if secPkg != packageName || !isTagBearingSection(secName) { + return nil + } + + parsedTag, parsedValue, isTag := parseTagLine(line.Text) + if !isTag || !matcher(parsedTag, parsedValue) { + return nil + } + + line.Remove() + + removed++ + + return nil + }) + }) + + return removed, err +} + +// AddTag adds the given tag to the spec, under the specified package (or globally if +// `packageName` is empty). This function will indiscriminately add the tag and does not +// first check to see if any instances of this tag already exist in the indicated +// package. This is useful for tags that can appear multiple times, or in cases in which +// a determination has already been made that a singleton tag in question doesn't already exist. +// +// Note: When adding to a sub-package (non-empty packageName), the corresponding %package +// section must already exist in the spec; otherwise, an [ErrSectionNotFound] error is returned. +func (s *structuralSpec) 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 = packageSectionName + } + + return s.AppendLinesToSection(sectionName, packageName, []string{fmt.Sprintf("%s: %s", tag, value)}) +} + +// For example, "Source9999" returns "source", "Patch100" returns "patch", and +// "BuildRequires" returns "buildrequires". The result is always lowercased. + +// -1 for %endif, and 0 for everything else. Comments are ignored. +// +// The recognized conditional openers are: %if, %ifarch, %ifnarch, %ifos, %ifnos. + +// 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. + +// InsertTag inserts a tag into the spec, placing it after the last existing tag from the +// same "family" (e.g., Source9999 is placed after the last Source* tag). If no tags from +// the same family exist, the tag is placed after the last tag of any kind. If there are no +// tags at all, it falls back to [AddTag] behavior (appending to the section end). +// +// The tag family is determined by stripping trailing digits from the tag name +// (case-insensitive). For example, "Source0", "Source1", and "Source" all belong to the +// "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. +// +// Note: When inserting into a sub-package (non-empty packageName), the corresponding +// %package section must already exist in the spec; otherwise, an [ErrSectionNotFound] +// error is returned. +func (s *structuralSpec) InsertTag(packageName string, tag string, value string) error { + slog.Debug("Inserting tag to spec", "package", packageName, "tag", tag, "value", value) + + sectionName := "" + if packageName != "" { + sectionName = packageSectionName + } + + insertAfter, found, err := findLinearTagInsertPosition(s.rawLines, sectionName, packageName, structuralTagFamily(tag)) + if err != nil { + return err + } + + if !found { + return s.AddTag(packageName, tag, value) + } + + lines := slices.Clone(s.rawLines) + lines = append(lines, "") + copy(lines[insertAfter+2:], lines[insertAfter+1:]) + lines[insertAfter+1] = fmt.Sprintf("%s: %s", tag, value) + + if _, err := parseTree(lines); err != nil { + return fmt.Errorf("validating inserted tag:\n%w", err) + } + + s.rawLines = lines + + return nil +} + +// findLinearTagInsertPosition reproduces the legacy lexical tag ordering while +// ignoring directive-shaped macro bodies. +// +//nolint:cyclop,gocognit,nestif // One pass keeps macro, section, and conditional state synchronized. +func findLinearTagInsertPosition(lines []string, sectionName, packageName, family string) (int, bool, error) { + lastAny, lastFamily := -1, -1 + lastAnyConditional, lastFamilyConditional := -1, -1 + currentName, currentPackage := "", "" + sectionFound := sectionName == "" && packageName == "" + inMacroBody := false + macroParseState := macroState{} + + var conditionals []int + + for lineNum, line := range lines { + if inMacroBody { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroParseState) + + continue + } + + if _, isMacro := isMacroDefLine(line); isMacro { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroState{}) + + continue + } + + if isSectionHeaderLine(line) { + currentName, currentPackage = getSectionNameAndPackageFromHeader(line) + sectionFound = sectionFound || (currentName == sectionName && currentPackage == packageName) + } else if currentName == sectionName && currentPackage == packageName { + tag, _, isTag := parseTagLine(line) + if isTag { + lastAny = lineNum + + if len(conditionals) > 0 { + lastAnyConditional = conditionals[0] + } else { + lastAnyConditional = -1 + } + + if structuralTagFamily(tag) == family { + lastFamily = lineNum + lastFamilyConditional = lastAnyConditional + } + } + } + + switch structuralConditionalDepthChange(line) { + case 1: + conditionals = append(conditionals, lineNum) + case -1: + if len(conditionals) > 0 { + conditionals = conditionals[:len(conditionals)-1] + } + } + } + + if !sectionFound { + return 0, false, fmt.Errorf("section %#q (package=%#q) not found:\n%w", + sectionName, packageName, ErrSectionNotFound) + } + + insertAfter, conditionalStart := lastAny, lastAnyConditional + if lastFamily >= 0 { + insertAfter, conditionalStart = lastFamily, lastFamilyConditional + } + + if insertAfter < 0 { + return 0, false, nil + } + + if conditionalStart >= 0 { + insertAfter = matchingConditionalEndInSection(lines, conditionalStart, insertAfter, sectionName, packageName) + } + + return insertAfter, true, nil +} + +func matchingConditionalEndInSection(lines []string, start, fallback int, sectionName, packageName string) int { + depth := 0 + inMacroBody := false + macroParseState := macroState{} + + for lineNum := start; lineNum < len(lines); lineNum++ { + line := lines[lineNum] + if inMacroBody { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroParseState) + + continue + } + + if _, isMacro := isMacroDefLine(line); isMacro { + macroParseState, inMacroBody = macroBodyStateAfter(line, macroState{}) + + continue + } + + if lineNum > fallback && isSectionHeaderLine(line) { + name, pkg := getSectionNameAndPackageFromHeader(line) + if name != sectionName || pkg != packageName { + return fallback + } + } + + switch structuralConditionalDepthChange(line) { + case 1: + depth++ + case -1: + depth-- + if depth == 0 { + return lineNum + } + } + } + + return fallback +} + +// PrependLines prepends lines to the beginning of the spec without interpreting +// section structure. +func (s *structuralSpec) PrependLines(lines []string) { + slog.Debug("Prepending lines to spec file", "lines", lines) + s.rawLines = append(append([]string{}, lines...), s.rawLines...) +} + +// AppendLines appends lines to the end of the spec without interpreting +// section structure. +func (s *structuralSpec) 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 each matching section header (or at the top of the file in +// the global section). An error is returned if no matching section is found. +func (s *structuralSpec) PrependLinesToSection(sectionName, packageName string, lines []string) (err error) { + slog.Debug("Prepending lines to spec", "section", sectionName, "package", packageName, "lines", 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) + } + + for _, section := range sections { + section.PrependLines(lines) + } + + return nil + }) +} + +// AppendLinesToSection appends the given lines at the end of the specified section, placing +// them just after the current last line of each matching 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 *structuralSpec) AppendLinesToSection(sectionName, packageName string, lines []string) (err error) { + slog.Debug("Appending lines to spec", "section", sectionName, "package", packageName, "lines", lines) + + err = 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) + } + + for _, section := range sections { + section.AppendLines(lines) + } + + return nil + }) + if err == nil || !strings.Contains(err.Error(), "unmatched %if") { + return err + } + + return s.appendLinesThatCompleteConditional(sectionName, packageName, lines) +} + +// appendLinesThatCompleteConditional permits an append overlay to close an +// unmatched conditional introduced by an earlier overlay. The candidate must +// parse successfully, so it cannot preserve an otherwise malformed spec. +func (s *structuralSpec) appendLinesThatCompleteConditional(sectionName, packageName string, lines []string) error { + headers := findSectionHeaderLines(s.rawLines) + insertions := sectionAppendInsertionPositions(s.rawLines, headers, sectionName, packageName) + + if len(insertions) == 0 { + return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound) + } + + candidate := slices.Clone(s.rawLines) + for index := len(insertions) - 1; index >= 0; index-- { + candidate = slices.Insert(candidate, insertions[index], lines...) + } + + if _, err := parseTree(candidate); err != nil { + return fmt.Errorf("validating appended lines:\n%w", err) + } + + s.rawLines = candidate + + return nil +} + +func sectionAppendInsertionPositions(lines []string, headers []int, sectionName, packageName string) []int { + if sectionName == "" && packageName == "" { + if len(headers) == 0 { + return []int{len(lines)} + } + + return []int{headers[0]} + } + + insertions := make([]int, 0, len(headers)) + for index, header := range headers { + name, pkg := getSectionNameAndPackageFromHeader(lines[header]) + if name != sectionName || pkg != packageName { + continue + } + + end := len(lines) + if index+1 < len(headers) { + end = headers[index+1] + } + + insertions = append(insertions, end) + } + + return insertions +} + +// 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. +// +// Search-and-replace is deliberately line-oriented rather than structural: a +// sequence of overlays may temporarily leave conditional directives unbalanced. +// Every non-section-header physical line is eligible, including macro +// definitions and bodies plus conditional directives. +func (s *structuralSpec) SearchAndReplace(sectionName, packageName, regex, replacement string) (err error) { + slog.Debug("Searching and replacing in spec", + "section", sectionName, + "package", packageName, + "regex", regex, + "replacement", replacement, + ) + + // Compile the regex once. + compiledRegex, err := regexp.Compile(regex) + if err != nil { + return fmt.Errorf("failed to compile regex %#q:\n%w", regex, err) + } + + updatedLines := slices.Clone(s.rawLines) + updated := searchReplaceLines(updatedLines, sectionName, packageName, compiledRegex, replacement) + + if !updated { + return fmt.Errorf( + "pattern %#q not found (section=%#q, package=%#q):\n%w", + regex, sectionName, packageName, ErrPatternNotFound, + ) + } + + s.rawLines = updatedLines + + return nil +} + +// searchReplaceLines applies replacement to physical lines under the requested +// lexical section. Section headers remain structural delimiters and are not +// replaced, matching the historical section-content behavior. +func searchReplaceLines( + lines []string, + filterSection, filterPkg string, + compiledRegex *regexp.Regexp, + replacement string, +) bool { + updated := false + headers := findSectionHeaderLines(lines) + headerAt := make(map[int]bool, len(headers)) + + for _, index := range headers { + headerAt[index] = true + } + + state := searchReplaceState{} + + for index, line := range lines { + if headerAt[index] { + state.sectionName, state.packageName = getSectionNameAndPackageFromHeader(line) + + continue + } + + state.advanceBefore(line) + + if state.matchesFilter(filterSection, filterPkg) { + if newLine := compiledRegex.ReplaceAllLiteralString(line, replacement); newLine != line { + lines[index] = newLine + updated = true + } + } + + state.advanceAfter(line) + } + + return updated +} + +type searchReplaceContext struct{ name, pkg string } + +type searchReplaceState struct { + sectionName, packageName string + conditionalContexts []searchReplaceContext + inMacroBody bool + macroParseState macroState +} + +func (state *searchReplaceState) advanceBefore(line string) { + if state.inMacroBody { + return + } + + switch { + case structuralConditionalDepthChange(line) == 1: + state.conditionalContexts = append(state.conditionalContexts, searchReplaceContext{ + state.sectionName, state.packageName, + }) + case structuralIsConditionalBranchDirective(line), structuralConditionalDepthChange(line) == -1: + if len(state.conditionalContexts) > 0 { + context := state.conditionalContexts[len(state.conditionalContexts)-1] + state.sectionName, state.packageName = context.name, context.pkg + } + } +} + +func (state *searchReplaceState) matchesFilter(section, pkg string) bool { + return (section == "" || section == state.sectionName) && + (pkg == "" || pkg == state.packageName) +} + +func (state *searchReplaceState) advanceAfter(line string) { + if state.inMacroBody { + state.macroParseState, state.inMacroBody = macroBodyStateAfter(line, state.macroParseState) + + return + } + + if structuralConditionalDepthChange(line) == -1 && len(state.conditionalContexts) > 0 { + state.conditionalContexts = state.conditionalContexts[:len(state.conditionalContexts)-1] + } + + if _, isMacro := isMacroDefLine(line); isMacro { + state.macroParseState, state.inMacroBody = macroBodyStateAfter(line, macroState{}) + } +} + +// AddChangelogEntry adds a changelog entry to the spec's changelog section. An error is returned if +// no %changelog section exists in the spec. +// +//nolint:lll +func (s *structuralSpec) AddChangelogEntry(user, email, version, release string, time time.Time, details []string) (err error) { + slog.Debug("Adding changelog entry to spec", + "user", user, "email", email, "version", version, "release", release, "details", details) + + formattedDate := time.Format("Mon Jan 02 2006") + header := fmt.Sprintf("* %s %s <%s> - %s-%s", formattedDate, user, email, version, release) + + lines := []string{header} + for _, detail := range details { + lines = append(lines, "- "+detail) + } + + lines = append(lines, "") + + return s.mutateTree(func(tree *specTree) error { + sect := tree.Section("%changelog", "") + if sect == nil { + return errors.New("existing changelog section could not be found") + } + + sect.PrependLines(lines) + + return nil + }) +} + +// StructuralParsePatchTagNumber checks if the given tag name is a PatchN tag (case-insensitive) +// and returns the numeric suffix N. Returns -1, false if the tag is not a PatchN tag +// or the suffix is not a valid integer. + +// HasSection returns true if the spec contains a section with the given name. +// The comparison is exact (case-sensitive), consistent with [AppendLinesToSection]. +func (s *structuralSpec) HasSection(sectionName string) (bool, error) { + var found bool + + err := s.inspectTree(func(tree *specTree) error { + found = tree.HasSection(sectionName) + + return nil + }) + + return found, err +} + +// AddPatchEntry registers a patch in the spec, either by appending to an existing %patchlist +// section or by adding a new PatchN tag with the next available number. Returns an error +// if the spec cannot be examined or updated. +func (s *structuralSpec) AddPatchEntry(packageName, filename string) error { + slog.Debug("Adding patch entry to spec", "package", packageName, "filename", filename) + + hasPatchlist, err := s.HasSection("%patchlist") + if err != nil { + return fmt.Errorf("failed to check for %%patchlist section:\n%w", err) + } + + if hasPatchlist { + return s.AppendLinesToSection("%patchlist", "", []string{filename}) + } + + highest, err := s.GetHighestPatchTagNumber() + if err != nil { + return fmt.Errorf("failed to scan for existing patch tags:\n%w", err) + } + + return s.AddTag(packageName, fmt.Sprintf("Patch%d", highest+1), filename) +} + +// RemovePatchEntry removes all references to patches matching the given pattern from the spec. +// The pattern is a glob pattern (supporting doublestar syntax) matched against PatchN tag values +// and %patchlist entries across all packages. Returns an error if no references matched the pattern. +func (s *structuralSpec) RemovePatchEntry(pattern string) error { + slog.Debug("Removing patch entry from spec", "pattern", pattern) + + totalRemoved := 0 + + tagsRemoved, err := s.removePatchTagsMatching(pattern) + if err != nil { + return fmt.Errorf("failed to remove matching patch tags:\n%w", err) + } + + totalRemoved += tagsRemoved + + hasPatchlist, err := s.HasSection("%patchlist") + if err != nil { + return fmt.Errorf("failed to check for %%patchlist section:\n%w", err) + } + + if hasPatchlist { + patchlistRemoved, err := s.removePatchlistEntriesMatching(pattern) + if err != nil { + return fmt.Errorf("failed to remove matching patchlist entries:\n%w", err) + } + + totalRemoved += patchlistRemoved + } + + if totalRemoved == 0 { + return fmt.Errorf("no patches matching %#q found in spec", pattern) + } + + return nil +} + +// removePatchTagsMatching removes all PatchN tags across all packages whose values match the +// given glob pattern. Returns the number of tags removed. +func (s *structuralSpec) removePatchTagsMatching(pattern string) (int, error) { + removed := 0 + + err := s.mutateTree(func(tree *specTree) error { + return tree.VisitAllLines(func(secName, _ string, line *lineHandle) error { + if !isTagBearingSection(secName) { + return nil + } + + parsedTag, parsedValue, isTag := parseTagLine(line.Text) + if !isTag { + return nil + } + + if _, ok := StructuralParsePatchTagNumber(parsedTag); !ok { + return nil + } + + 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) + } + + if matched { + line.Remove() + + removed++ + } + + return nil + }) + }) + + return removed, err +} + +// removePatchlistEntriesMatching removes lines from the %patchlist section whose trimmed content +// matches the given glob pattern. Returns the number of entries removed. +func (s *structuralSpec) removePatchlistEntriesMatching(pattern string) (int, error) { + removed := 0 + + err := s.mutateTree(func(tree *specTree) error { + for _, section := range tree.Sections("%patchlist", "") { + err := section.VisitLines(func(line *lineHandle) error { + trimmed := strings.TrimSpace(line.Text) + if trimmed == "" { + return nil + } + + 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 { + line.Remove() + + removed++ + } + + return nil + }) + if err != nil { + return err + } + } + + return nil + }) + + return removed, err +} + +// GetHighestPatchTagNumber scans the spec for all PatchN tags (where N is a decimal number) +// across all packages and returns the highest N found. Unnumbered "Patch:" tags (no numeric +// suffix) are treated as auto-numbered starting from 0, consistent with RPM's behavior. +// Returns -1 if no numbered PatchN tags and no unnumbered "Patch:" tags are found. Tags with +// non-numeric suffixes (e.g., macro-based names like Patch%{n}) are silently skipped. +func (s *structuralSpec) GetHighestPatchTagNumber() (int, error) { + highest := -1 + unnumberedCount := 0 + + err := s.inspectTree(func(tree *specTree) error { + return tree.VisitAllLines(func(secName, _ string, line *lineHandle) error { + if !isTagBearingSection(secName) { + return nil + } + + parsedTag, _, isTag := parseTagLine(line.Text) + if !isTag { + return nil + } + + num, isPatchTag := StructuralParsePatchTagNumber(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. + if unnumberedCount > 0 && (unnumberedCount-1) > highest { + highest = unnumberedCount - 1 + } + + return highest, err +} + +// RemoveSection removes every section from the spec whose name and package qualifier +// match the supplied values, including each section's header line and all body lines. +// +// In valid RPM specs the `(sectionName, packageName)` pair is unique, so this is +// effectively a single-section removal. When a spec lexically contains multiple +// sections with the same identity (e.g. inside mutually-exclusive `%if`/`%else` +// branches), every such section is removed. Returns [ErrSectionNotFound] if no +// matching section exists. +func (s *structuralSpec) RemoveSection(sectionName, packageName string) error { + slog.Debug("Removing section from spec", "section", sectionName, "package", packageName) + + if sectionName == "" { + return errors.New("cannot remove the global/preamble section") + } + + 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 tree.RemoveSections(matches) + }) +} + +// RemoveSubpackage removes every section in the spec that is associated with the given +// sub-package name (i.e. every section whose package qualifier equals packageName). +// This includes the sub-package's own `%package` preamble section as well as any +// per-section directives that target it (e.g. `%description -n pkg`, `%files pkg`, +// `%post pkg`, etc.). +// +// Returns an error if packageName is empty or if the spec contains no sections +// associated with the given sub-package. +// +// packageName matching: RPM permits two forms for declaring sub-package sections — the +// suffix form (e.g. `%package devel`, which declares a sub-package named `-devel`) +// and the absolute form (e.g. `%package -n my-pkg`). Each section is matched against +// packageName using the form that appears on its header line; callers should pass +// whichever form the spec uses. Specs that mix both forms for the same sub-package +// (uncommon but legal) require a call per form. +// +// 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 *structuralSpec) RemoveSubpackage(packageName string) error { + slog.Debug("Removing sub-package from spec", "package", packageName) + + if packageName == "" { + return errors.New("cannot remove sub-package with empty name") + } + + 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 tree.RemoveSections(matches) + }) +} + +func structuralTagFamily(tag string) string { + lower := strings.ToLower(tag) + + // Strip trailing digits. + end := len(lower) + for end > 0 && lower[end-1] >= '0' && lower[end-1] <= '9' { + end-- + } + + // If the entire tag is digits, return the full lowered tag. + if end == 0 { + return lower + } + + return lower[:end] +} + +func structuralConditionalDepthChange(rawLine string) int { + trimmed := strings.TrimSpace(rawLine) + if strings.HasPrefix(trimmed, "#") { + return 0 + } + + token := strings.Fields(trimmed) + if len(token) == 0 { + return 0 + } + + lower := strings.ToLower(token[0]) + + switch lower { + case "%endif": + return -1 + case "%if", "%ifarch", "%ifnarch", "%ifos", "%ifnos": + return 1 + default: + return 0 + } +} + +func structuralIsConditionalBranchDirective(rawLine string) bool { + trimmed := strings.TrimSpace(rawLine) + if strings.HasPrefix(trimmed, "#") { + return false + } + + tokens := strings.Fields(trimmed) + if len(tokens) == 0 { + return false + } + + lower := strings.ToLower(tokens[0]) + + switch lower { + case elseDirective, "%elif", "%elifarch", "%elifnarch", "%elifos", "%elifnos": + return true + default: + return false + } +} + +func StructuralParsePatchTagNumber(tag string) (int, bool) { + suffix, found := strings.CutPrefix(strings.ToLower(tag), "patch") + if !found || suffix == "" { + return -1, false + } + + num, err := strconv.Atoi(suffix) + if err != nil { + return -1, false + } + + return num, true +} diff --git a/internal/rpm/spec/structural_spec.go b/internal/rpm/spec/structural_spec.go index a7bccbf03..ac1da2871 100644 --- a/internal/rpm/spec/structural_spec.go +++ b/internal/rpm/spec/structural_spec.go @@ -3,7 +3,284 @@ package spec -// structuralSpec encapsulates the raw contents used by structural operations. +import ( + "bufio" + "fmt" + "io" + "slices" + "strings" +) + +// sectionTypesByName is a table of known sections, mapping them to their types. This table must +// be kept in sync with new section types as they are added to the RPM spec format. +// +//nolint:gochecknoglobals // This is effectively a constant, but Go doesn't have const maps. +var sectionTypesByName = map[string]SectionType{ + "%package": PackageSection, + "%prep": ScriptSection, + "%conf": ScriptSection, + "%build": ScriptSection, + "%install": ScriptSection, + "%check": ScriptSection, + "%clean": ScriptSection, + "%generate_buildrequires": ScriptSection, + "%pre": ScriptSection, + "%post": ScriptSection, + "%preun": ScriptSection, + "%postun": ScriptSection, + "%pretrans": ScriptSection, + "%posttrans": ScriptSection, + "%preuntrans": ScriptSection, + "%postuntrans": ScriptSection, + "%verify": ScriptSection, + "%triggerin": ScriptSection, + "%triggerun": ScriptSection, + "%triggerprein": ScriptSection, + "%triggerpostun": ScriptSection, + "%filetriggerin": ScriptSection, + "%filetriggerun": ScriptSection, + "%filetriggerpostun": ScriptSection, + "%transfiletriggerin": ScriptSection, + "%transfiletriggerun": ScriptSection, + "%transfiletriggerpostun": ScriptSection, + "%description": RawSection, + "%files": FilesSection, + "%changelog": ChangelogSection, + "%patchlist": SourceFileListSection, + "%sourcelist": SourceFileListSection, +} + +// Spec encapsulates the contents of an RPM spec file. type structuralSpec struct { rawLines []string } + +// Line represents a single line in an RPM spec file. +type Line struct { + // Text is the original physical text of the line. + Text string + // Parsed is the parsed representation of the line's contents. + Parsed ParsedLine +} + +// ParsedLineType represents the type of a parsed line. +type ParsedLineType string + +const ( + // SectionStart applies to lines that start a new section, e.g. "%description". + SectionStart ParsedLineType = "SectionStart" + // Tag applies to lines that define a tag, e.g. "Name: foo". + Tag ParsedLineType = "Tag" + // Raw applies to lines that are raw text, e.g. a line in a script section. + Raw ParsedLineType = "Raw" +) + +// ParsedLine is the interface that all parsed line types implement. +type ParsedLine interface { + // GetType returns the type of the parsed line. + GetType() ParsedLineType +} + +// SectionType represents the type of a section in an RPM spec file. +type SectionType string + +const ( + // PackageSection applies to sections that define a package, e.g. "%package -n foo". + PackageSection SectionType = "Package" + // ScriptSection applies to sections that contain scripts, e.g. "%build". + ScriptSection SectionType = "Script" + // RawSection applies to sections that contain raw content, e.g.: "%description". + RawSection SectionType = "Raw" + // ChangelogSection applies to the "%changelog" section. + ChangelogSection SectionType = "Changelog" + // FilesSection applies to a "%files" section. + FilesSection SectionType = "Files" + // SourceFileListSection applies to a section that lists source files, e.g.: "%sourcelist". + SourceFileListSection SectionType = "SourceFileList" +) + +// SectionStartLine represents a line that starts a new section in the spec, e.g.: "%build". +type SectionStartLine struct { + SectType SectionType + SectName string + Tokens []string +} + +// GetType returns the type of the parsed line. +func (*SectionStartLine) GetType() ParsedLineType { + return SectionStart +} + +// TagLine encapsulates the definition of a tag. +type TagLine struct { + // Tag is the name of the tag being defined. + Tag string + // Value is the value assigned to the tag. + Value string +} + +// GetType returns the type of the parsed line. +func (*TagLine) GetType() ParsedLineType { + return Tag +} + +// RawLine represents a line that is raw text. +type RawLine struct { + // Content is the raw line text. + Content string +} + +// GetType returns the type of the parsed line. +func (*RawLine) GetType() ParsedLineType { + return Raw +} + +// 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 openStructuralSpec(reader io.Reader) (*structuralSpec, error) { + scanner := bufio.NewScanner(reader) + spec := &structuralSpec{} + + // Read each line from the reader, parsing as we go. Store all parsed lines in the spec object. + for scanner.Scan() { + spec.rawLines = append(spec.rawLines, scanner.Text()) + } + + // Check for scanner errors (e.g., I/O error or line too long for buffer). + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("failed to read spec:\n%w", err) + } + + return spec, nil +} + +// Serialize writes the spec's contents to the provided writer. +func (s *structuralSpec) Serialize(writer io.Writer) error { + bufWriter := bufio.NewWriter(writer) + for _, line := range s.rawLines { + _, err := bufWriter.WriteString(line + "\n") + if err != nil { + return fmt.Errorf("failed to write spec line: %w", err) + } + } + + err := bufWriter.Flush() + if err != nil { + return fmt.Errorf("failed to flush spec: %w", err) + } + + return nil +} + +// ReplaceLine replaces the line at the specified (0-indexed) line number with the provided replacement line. +func (s *structuralSpec) ReplaceLine(lineNumber int, replacement string) { + s.rawLines[lineNumber] = replacement +} + +// RemoveLine removes the line at the specified (0-indexed) line number. +func (s *structuralSpec) RemoveLine(lineNumber int) { + s.rawLines = slices.Delete(s.rawLines, lineNumber, lineNumber+1) +} + +// RemoveLines removes the lines in the specified (0-indexed) line number range [startLineNumber, endLineNumber). +func (s *structuralSpec) RemoveLines(startLineNumber int, endLineNumber int) { + s.rawLines = slices.Delete(s.rawLines, startLineNumber, endLineNumber) +} + +// InsertLinesAt inserts the provided lines just before the specified (0-indexed) line number. +func (s *structuralSpec) InsertLinesAt(insertedLines []string, lineNumber int) { + s.rawLines = slices.Insert(s.rawLines, lineNumber, insertedLines...) +} + +// Visit preserves the public visitor API. Structural edit operations use the +// tree API directly; visitor callbacks retain the established line semantics. +func (s *structuralSpec) Visit(visitor Visitor) error { + legacy := legacySpec{rawLines: slices.Clone(s.rawLines)} + if err := legacy.Visit(visitor); err != nil { + return err + } + + s.rawLines = legacy.rawLines + + return nil +} + +// SectionTarget encapsulates information about the current section context. +type SectionTarget struct { + // SectName is the name of the section, e.g. "%description". + SectName string + // SectType is the type of the section. + SectType SectionType + // Package is the package this section applies to, if any. Left empty for + // the default package or sections that aren't package-specific. + Package string +} + +func getPackageNameForSection(sectionType SectionType, headerTokens []string) string { + switch sectionType { + case SourceFileListSection: + fallthrough + case ChangelogSection: + return "" + case PackageSection: + fallthrough + case RawSection: + fallthrough + case FilesSection: + fallthrough + case ScriptSection: + return GetPackageNameFromSectionHeader(headerTokens) + default: + return "" + } +} + +// GetPackageNameFromSectionHeader extracts the package name from the tokens of a section +// header line. For example, for a line like "%package -n foo", it would return "foo". +// For a line like "%package foo", it would return "foo" as well. Because this function +// does not know the base name of the spec, it cannot take a suffix-only name and resolve +// it to a full name. +func GetPackageNameFromSectionHeader(tokens []string) string { + fullName := "" + nameSuffix := "" + index := 1 // Skip the first token + + for index < len(tokens) { + token := tokens[index] + + switch { + case token == "--": + // Trigger terminator: in %trigger* sections, `--` separates the + // owning sub-package from the trigger condition. Everything after + // `--` is the trigger condition, not the package name. + index = len(tokens) + case token == "-n": + // Absolute package name form: the next token is the full package name. + index++ + if index < len(tokens) { + fullName = tokens[index] + index++ + } + case token == "-f", token == "-p", token == "-l", token == "-P": + // Flags that consume the next token as their argument. + index += 2 + case strings.HasPrefix(token, "-"): + // Other flags (e.g. -q, -e, or unknown): skip the flag itself. + index++ + case nameSuffix == "": + nameSuffix = token + index++ + default: + index++ + } + } + + switch { + case fullName != "": + return fullName + case nameSuffix != "": + return nameSuffix + default: + return "" + } +} diff --git a/internal/rpm/spec/structural_tree_api.go b/internal/rpm/spec/structural_tree_api.go index fa9d104fa..55aac0831 100644 --- a/internal/rpm/spec/structural_tree_api.go +++ b/internal/rpm/spec/structural_tree_api.go @@ -6,22 +6,29 @@ package spec import ( "errors" "fmt" + "regexp" "strings" ) -// specTree is an opaque handle for a parsed spec structure. +// 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 refers to one section in a [specTree]. +// 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, applies mutate, and validates the resulting tree -// before replacing [structuralSpec.rawLines]. Errors leave the spec unchanged. +// 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 *structuralSpec) mutateTree(mutate func(*specTree) error) error { root, err := parseTree(s.rawLines) if err != nil { @@ -43,8 +50,9 @@ func (s *structuralSpec) mutateTree(mutate func(*specTree) error) error { return nil } -// inspectTree parses the spec and passes its structure to inspect without -// modifying [structuralSpec.rawLines]. +// 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 *structuralSpec) inspectTree(inspect func(*specTree) error) error { root, err := parseTree(s.rawLines) if err != nil { @@ -63,6 +71,54 @@ func (t *specTree) Section(name, pkg string) *sectionHandle { return nil } +// GetTag returns the first tag matching name in the requested package. +func (t *specTree) GetTag(pkg, name string) (string, bool) { + var ( + value string + found bool + ) + + _ = t.VisitAllLines(func(secName, secPkg string, line *lineHandle) error { + if found || secPkg != pkg || !isTagBearingSection(secName) { + return nil + } + + tag, tagValue, isTag := parseTagLine(line.Text) + if isTag && strings.EqualFold(tag, name) { + value = tagValue + found = true + } + + return nil + }) + + return value, found +} + +// GetLastTag returns the last lexical tag matching name in the requested package. +func (t *specTree) GetLastTag(pkg, name string) (string, bool) { + var ( + value string + found bool + ) + + _ = t.VisitAllLines(func(secName, secPkg string, line *lineHandle) error { + if secPkg != pkg || !isTagBearingSection(secName) { + return nil + } + + tag, tagValue, isTag := parseTagLine(line.Text) + if isTag && strings.EqualFold(tag, name) { + value = tagValue + found = true + } + + return nil + }) + + return value, found +} + // HasSection reports whether a section with name is present for any package. func (t *specTree) HasSection(name string) bool { found := false @@ -132,33 +188,236 @@ func (t *specTree) RemoveSections(handles []*sectionHandle) error { return nil } -// Name returns the section keyword. The preamble has an empty name. -func (h *sectionHandle) Name() string { - return h.block.Name +// --- 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, + }) } -// Package returns the section package qualifier. -func (h *sectionHandle) Package() string { - return h.block.Package +// 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...) } -// AppendLines appends lines to the section's content. -func (h *sectionHandle) AppendLines(lines []string) { - if len(lines) == 0 { - return +// --- 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 + lineNumber int + before []string + after []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 +} + +// InsertBefore queues lines immediately before this line. +func (lh *lineHandle) InsertBefore(lines []string) { + lh.before = append(lh.before, lines...) +} + +// InsertAfter queues lines immediately after this line. +func (lh *lineHandle) InsertAfter(lines []string) { + lh.after = append(lh.after, lines...) +} + +// 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 + + lineNumber := 0 + + visitErr := collectAndVisitLines(t.root, "", "", visit, &handles, &lineNumber) + + 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 + + lineNumber := 0 + + wrap := func(_, _ string, lh *lineHandle) error { return visit(lh) } + + visitErr := collectAndVisitLines(h.block, h.block.Name, h.block.Package, wrap, &handles, &lineNumber) + + flushLineMutations(handles) + + return visitErr +} + +// collectAndVisitLines walks blk, calls visit on every text-line, and records +// each handle for later mutation flushing. +// +//nolint:cyclop,gocognit // 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, + lineNumber *int, +) error { + switch blk.Kind { + case rootBlock: + for _, child := range blk.Children { + if err := collectAndVisitLines(child, secName, secPkg, visit, handles, lineNumber); err != nil { + return err + } + } + + case sectionBlock: + if blk.Name != "" { + *lineNumber++ + } + + for _, child := range blk.Children { + if err := collectAndVisitLines(child, blk.Name, blk.Package, visit, handles, lineNumber); err != nil { + return err + } + } + + case conditionalBlock: + *lineNumber++ + for _, child := range blk.Children { + if err := collectAndVisitLines(child, secName, secPkg, visit, handles, lineNumber); err != nil { + return err + } + } + + if blk.ElseDirective != "" { + *lineNumber++ + } + + for _, child := range blk.Else { + if err := collectAndVisitLines(child, secName, secPkg, visit, handles, lineNumber); err != nil { + return err + } + } + + if blk.Endif != "" { + *lineNumber++ + } + + case textBlock: + for i, line := range blk.Lines { + handle := &lineHandle{Text: line, block: blk, idx: i, lineNumber: *lineNumber} + *lineNumber++ + + *handles = append(*handles, handle) + + if err := visit(secName, secPkg, handle); err != nil { + return err + } + } + + case macroDefBlock: + // Macro definitions are not visited as content lines. + *lineNumber += len(blk.Lines) } - h.block.Children = append(h.block.Children, &block{Kind: textBlock, Lines: lines}) + return nil } -// PrependLines inserts lines immediately after the section header. -func (h *sectionHandle) PrependLines(lines []string) { - if len(lines) == 0 { - return +// 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] + + line := handle.block.Lines[handle.idx] + if handle.replaced { + line = handle.newText + } + + replacement := append([]string{}, handle.before...) + if !handle.removed { + replacement = append(replacement, line) + } + + replacement = append(replacement, handle.after...) + handle.block.Lines = append(handle.block.Lines[:handle.idx], + append(replacement, handle.block.Lines[handle.idx+1:]...)...) + } +} + +// 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 } - child := &block{Kind: textBlock, Lines: lines} - h.block.Children = append([]*block{child}, h.block.Children...) + 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 } func walkBlocks(blk *block, visit func(*block) bool) bool { @@ -227,19 +486,8 @@ func validateRemovalChildren(children []*block, removeSet map[*block]bool, prece continue } - if conditionalHasTextOrMacroContent(child) && containsSectionBlocks(child) { - if preceding != nil && removeSet[preceding] { - return fmt.Errorf("conditional block at %#q contains content belonging to the preceding section:\n%w", - child.Header, ErrConditionalSpansSections) - } - } - - if wouldEmptySectionWrapper(child, removeSet) && index+1 < len(children) { - next := children[index+1] - if next.Kind == conditionalBlock && !containsSectionBlocks(next) && conditionalHasTextOrMacroContent(next) { - return fmt.Errorf("content in conditional block at %#q would be orphaned after removing the preceding section:\n%w", - next.Header, ErrConditionalSpansSections) - } + if err := validateConditionalRemoval(child, children, index, removeSet, preceding); err != nil { + return err } if err := validateRemovalChildren(child.Children, removeSet, preceding); err != nil { @@ -254,6 +502,43 @@ func validateRemovalChildren(children []*block, removeSet map[*block]bool, prece return nil } +//nolint:cyclop // Conditional wrapper validation must examine each independent unsafe shape. +func validateConditionalRemoval( + child *block, + children []*block, + index int, + removeSet map[*block]bool, + preceding *block, +) error { + if preceding != nil && preceding.Name != "" && + containsBranchDirective(child) && containsRemovedSection(child, removeSet) { + return fmt.Errorf("conditional block at %#q contains a branch directive across a removed section:\n%w", + child.Header, ErrConditionalSpansSections) + } + + if conditionalHasTextOrMacroContent(child) && containsSectionBlocks(child) { + if preceding != nil && removeSet[preceding] { + return fmt.Errorf("conditional block at %#q contains content belonging to the preceding section:\n%w", + child.Header, ErrConditionalSpansSections) + } + } + + if wouldEmptySectionWrapper(child, removeSet) && containsBranchDirective(child) { + return fmt.Errorf("conditional block at %#q contains branches that would be removed with its sections:\n%w", + child.Header, ErrConditionalSpansSections) + } + + if wouldEmptySectionWrapper(child, removeSet) && index+1 < len(children) { + next := children[index+1] + if next.Kind == conditionalBlock && !containsSectionBlocks(next) && conditionalHasTextOrMacroContent(next) { + return fmt.Errorf("content in conditional block at %#q would be orphaned after removing the preceding section:\n%w", + next.Header, ErrConditionalSpansSections) + } + } + + return nil +} + func conditionalHasTextOrMacroContent(conditional *block) bool { return hasTextOrMacroContent(conditional.Children) || hasTextOrMacroContent(conditional.Else) } @@ -308,3 +593,43 @@ func hasRemainingSection(blocks []*block, removeSet map[*block]bool) bool { return false } + +func containsBranchDirective(blk *block) bool { + if blk.ElseDirective != "" || isConditionalBranchDirective(blk.Header) { + return true + } + + for _, child := range blk.Children { + if containsBranchDirective(child) { + return true + } + } + + for _, child := range blk.Else { + if containsBranchDirective(child) { + return true + } + } + + return false +} + +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 +} diff --git a/internal/rpm/spec/structural_tree_api_internal_test.go b/internal/rpm/spec/structural_tree_api_internal_test.go index db4759cac..28c34c48d 100644 --- a/internal/rpm/spec/structural_tree_api_internal_test.go +++ b/internal/rpm/spec/structural_tree_api_internal_test.go @@ -12,6 +12,101 @@ import ( "github.com/stretchr/testify/require" ) +func TestVisitAllLinesTracksPhysicalLineNumbersThroughConditionals(t *testing.T) { + tests := []struct { + name string + lines []string + expected []int + }{ + { + name: "one elif", + lines: []string{ + "%if 1", + "then", + "%elif 0", + "elif", + "%endif", + "after", + }, + expected: []int{1, 3, 5}, + }, + { + name: "multiple elif", + lines: []string{ + "%if 1", + "then", + "%elif 0", + "first elif", + "%elif 0", + "second elif", + "%endif", + "after", + }, + expected: []int{1, 3, 5, 7}, + }, + { + name: "elif and else", + lines: []string{ + "%if 1", + "then", + "%elif 0", + "elif", + "%else", + "else", + "%endif", + "after", + }, + expected: []int{1, 3, 5, 7}, + }, + { + name: "nested if within elif", + lines: []string{ + "%if 1", + "then", + "%elif 0", + "%if 1", + "nested", + "%endif", + "elif", + "%endif", + "after", + }, + expected: []int{1, 4, 6, 8}, + }, + { + name: "ordinary if and else", + lines: []string{ + "%if 1", + "then", + "%else", + "else", + "%endif", + "after", + }, + expected: []int{1, 3, 5}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.lines) + + var lineNumbers []int + + err := specification.inspectTree(func(tree *specTree) error { + return tree.VisitAllLines(func(_, _ string, line *lineHandle) error { + lineNumbers = append(lineNumbers, line.lineNumber) + + return nil + }) + }) + + require.NoError(t, err) + assert.Equal(t, test.expected, lineNumbers) + }) + } +} + func TestInspectTreeQueriesSectionsInDocumentOrder(t *testing.T) { specification := newTreeAPISpec([]string{ "Name: example", From ebb814591c526a5562e4aa5dc28ea32e0c339e6c Mon Sep 17 00:00:00 2001 From: Thien Trung Vuong Date: Wed, 2 Sep 2026 20:39:13 +0000 Subject: [PATCH 4/5] test(spec): add structural parser fixtures --- .../specs/comment-only-conditional.spec | 32 +++ .../rpm/spec/testdata/specs/elif-chain.spec | 39 +++ .../testdata/specs/elif-with-sections.spec | 52 ++++ .../testdata/specs/if-with-continuation.spec | 34 +++ .../testdata/specs/macro-conditional.spec | 44 ++++ .../testdata/specs/macro-continuation.spec | 33 +++ .../testdata/specs/macro-with-parameters.spec | 35 +++ .../testdata/specs/multi-package-mixed.spec | 69 +++++ .../spec/testdata/specs/nested-wrappers.spec | 43 +++ .../specs/script-section-tag-shaped.spec | 43 +++ .../testdata/specs/straddling-wrapper.spec | 30 +++ .../specs/subpackage-define-unreferenced.spec | 36 +++ internal/rpm/spec/testdata_test.go | 248 ++++++++++++++++++ .../rpm/spec/tree_fixture_internal_test.go | 105 ++++++++ 14 files changed, 843 insertions(+) create mode 100644 internal/rpm/spec/testdata/specs/comment-only-conditional.spec create mode 100644 internal/rpm/spec/testdata/specs/elif-chain.spec create mode 100644 internal/rpm/spec/testdata/specs/elif-with-sections.spec create mode 100644 internal/rpm/spec/testdata/specs/if-with-continuation.spec create mode 100644 internal/rpm/spec/testdata/specs/macro-conditional.spec create mode 100644 internal/rpm/spec/testdata/specs/macro-continuation.spec create mode 100644 internal/rpm/spec/testdata/specs/macro-with-parameters.spec create mode 100644 internal/rpm/spec/testdata/specs/multi-package-mixed.spec create mode 100644 internal/rpm/spec/testdata/specs/nested-wrappers.spec create mode 100644 internal/rpm/spec/testdata/specs/script-section-tag-shaped.spec create mode 100644 internal/rpm/spec/testdata/specs/straddling-wrapper.spec create mode 100644 internal/rpm/spec/testdata/specs/subpackage-define-unreferenced.spec create mode 100644 internal/rpm/spec/testdata_test.go create mode 100644 internal/rpm/spec/tree_fixture_internal_test.go 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-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..1cdc670ba --- /dev/null +++ b/internal/rpm/spec/testdata_test.go @@ -0,0 +1,248 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec_test + +import ( + "bytes" + "embed" + "io/fs" + "math/rand/v2" + "path" + "slices" + "strconv" + "strings" + "testing" + + "github.com/microsoft/azure-linux-dev-tools/internal/rpm/spec" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +//go:embed testdata/specs/*.spec +var fixtureFS embed.FS + +const fixtureDirectory = "testdata/specs" + +func fixtureNames(t *testing.T) []string { + t.Helper() + + entries, err := fs.ReadDir(fixtureFS, fixtureDirectory) + require.NoError(t, err) + + names := make([]string, 0, len(entries)) + for _, entry := range entries { + if !entry.IsDir() { + names = append(names, entry.Name()) + } + } + + slices.Sort(names) + + return names +} + +func fixture(t *testing.T, name string) []byte { + t.Helper() + + contents, err := fixtureFS.ReadFile(path.Join(fixtureDirectory, name)) + require.NoError(t, err) + + return contents +} + +func openFixture(t *testing.T, name string) *spec.Spec { + t.Helper() + + specification, err := spec.OpenSpec(bytes.NewReader(fixture(t, name)), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + return specification +} + +func serializeFixture(t *testing.T, specification *spec.Spec) string { + t.Helper() + + var contents bytes.Buffer + require.NoError(t, specification.Serialize(&contents)) + + return contents.String() +} + +func assertReparseable(t *testing.T, contents string) { + t.Helper() + + _, err := spec.OpenSpec(strings.NewReader(contents), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) +} + +func TestStructuralParserFixturesRoundTripByteForByte(t *testing.T) { + for _, name := range fixtureNames(t) { + t.Run(name, func(t *testing.T) { + contents := fixture(t, name) + specification, err := spec.OpenSpec(bytes.NewReader(contents), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + assert.Equal(t, string(contents), serializeFixture(t, specification)) + }) + } +} + +func TestStructuralParserFixtureEditsReparse(t *testing.T) { + tests := []struct { + name string + fixtureName string + edit func(*testing.T, *spec.Spec) + }{ + { + name: "insert tag after conditional source", + fixtureName: "macro-continuation.spec", + edit: func(t *testing.T, specification *spec.Spec) { + t.Helper() + + require.NoError(t, specification.InsertTag("", "Source9999", "fixture-marker")) + }, + }, + { + name: "append through nested wrapper", + fixtureName: "nested-wrappers.spec", + edit: func(t *testing.T, specification *spec.Spec) { + t.Helper() + + require.NoError(t, specification.AppendLinesToSection( + "%files", "devel", []string{"/usr/share/fixture-marker"}, + )) + }, + }, + { + name: "remove unreferenced subpackage macro", + fixtureName: "subpackage-define-unreferenced.spec", + edit: func(t *testing.T, specification *spec.Spec) { + t.Helper() + + require.NoError(t, specification.RemoveSubpackage("tools")) + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := openFixture(t, test.fixtureName) + + test.edit(t, specification) + assertReparseable(t, serializeFixture(t, specification)) + }) + } +} + +func TestStructuralParserFixturesHasSectionThroughWrappers(t *testing.T) { + tests := []struct { + fixture string + section string + want bool + }{ + {fixture: "straddling-wrapper.spec", section: "%install", want: true}, + {fixture: "straddling-wrapper.spec", section: "%check", want: true}, + {fixture: "nested-wrappers.spec", section: "%package", want: true}, + {fixture: "nested-wrappers.spec", section: "%files", want: true}, + {fixture: "elif-with-sections.spec", section: "%files", want: true}, + {fixture: "nested-wrappers.spec", section: "%post", want: false}, + } + + for _, test := range tests { + t.Run(test.fixture+"/"+test.section, func(t *testing.T) { + got, err := openFixture(t, test.fixture).HasSection(test.section) + require.NoError(t, err) + assert.Equal(t, test.want, got) + }) + } +} + +func TestStructuralParserFixtureAppendRespectsWrapperBoundary(t *testing.T) { + specification := openFixture(t, "straddling-wrapper.spec") + require.NoError(t, specification.AppendLinesToSection("%build", "", []string{"echo fixture-marker"})) + + contents := serializeFixture(t, specification) + assert.Less(t, strings.Index(contents, "echo fixture-marker"), strings.Index(contents, "%if 0%{?with_tests}")) + assertReparseable(t, contents) +} + +func TestStructuralParserFixtureScriptTagShapedLinesAreNotTags(t *testing.T) { + specification := openFixture(t, "script-section-tag-shaped.spec") + + _, err := specification.RemoveTagsMatching("", func(tag, _ string) bool { + return strings.EqualFold(tag, "Name") || strings.EqualFold(tag, "Version") + }) + require.NoError(t, err) + + contents := serializeFixture(t, specification) + for _, line := range []string{ + `echo "Name: not-a-tag-write"`, + `printf "Version: still-not-a-tag\n"`, + } { + assert.Contains(t, contents, line) + } +} + +func TestStructuralParserFixtureSearchAndReplaceCoversLineTypes(t *testing.T) { + specification := openFixture(t, "macro-conditional.spec") + require.NoError(t, specification.SearchAndReplace("", "", "kernel", "fixture-kernel")) + require.NoError(t, specification.SearchAndReplace("", "", "0%\\{\\?fedora\\}", "1")) + + contents := serializeFixture(t, specification) + assert.Contains(t, contents, "%define fixture-kernel_reqprovconf") + assert.Contains(t, contents, "Provides: fixture-kernel") + assert.Contains(t, contents, "%if 1") + assertReparseable(t, contents) +} + +func TestStructuralParserGDBShapedMacroBodyIsOpaqueKnownLimitation(t *testing.T) { + input := `%define gdb_python_configure \ +%if 0%{?with_python}\ +--with-python\ +%endif\ +%{nil} +` + + specification, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + + // Parser-only coverage: structural edits cannot target directives inside a + // macro body individually. Macro hoisting and symbolic macro edits follow + // in the issue #203 implementation. + assert.Equal(t, input, serializeFixture(t, specification)) +} + +func TestStructuralParserSyntheticRoundTripsAreDeterministic(t *testing.T) { + for seed := uint64(1); seed <= 32; seed++ { + t.Run("seed-"+strconv.FormatUint(seed, 10), func(t *testing.T) { + rng := rand.New(rand.NewPCG(seed, seed+1)) //nolint:gosec // Fixed test seeds. + input := syntheticSpec(rng, rng.IntN(4)+1) + + specification, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(spec.EditorStructural)) + require.NoError(t, err) + assert.Equal(t, input, serializeFixture(t, specification)) + }) + } +} + +func syntheticSpec(rng *rand.Rand, branches int) string { + var output strings.Builder + output.WriteString("Name: synthetic\n%global flags \\\n --seed=") + output.WriteRune(rune('0' + rng.IntN(10))) + output.WriteString("\n") + + for branch := range branches { + output.WriteString("%if ") + output.WriteRune(rune('0' + branch%2)) + output.WriteString("\n%package package") + output.WriteRune(rune('0' + branch)) + output.WriteString("\n%description package") + output.WriteRune(rune('0' + branch)) + output.WriteString("\nsynthetic\n%endif\n") + } + + output.WriteString("%build\necho synthetic\n%files\n/usr/bin/synthetic\n") + + return output.String() +} diff --git a/internal/rpm/spec/tree_fixture_internal_test.go b/internal/rpm/spec/tree_fixture_internal_test.go new file mode 100644 index 000000000..c7e1f9556 --- /dev/null +++ b/internal/rpm/spec/tree_fixture_internal_test.go @@ -0,0 +1,105 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "bufio" + "embed" + "math/rand/v2" + "path" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +//go:embed testdata/specs/*.spec +var treeFixtureFS embed.FS + +func TestStructuralFixtureTreesRoundTripByteForByte(t *testing.T) { + entries, err := treeFixtureFS.ReadDir("testdata/specs") + require.NoError(t, err) + + for _, entry := range entries { + if entry.IsDir() { + continue + } + + t.Run(entry.Name(), func(t *testing.T) { + lines := fixtureLines(t, entry.Name()) + tree, err := parseTree(lines) + require.NoError(t, err) + assert.Equal(t, lines, serializeTree(tree)) + }) + } +} + +func TestStructuralTreesReparseEditedFixtureAndSyntheticOutputs(t *testing.T) { + inputs := map[string][]string{ + "fixture": fixtureLines(t, "straddling-wrapper.spec"), + } + + for seed := uint64(1); seed <= 32; seed++ { + rng := rand.New(rand.NewPCG(seed, seed+1)) //nolint:gosec // Fixed test seeds. + inputs["synthetic-"+strconv.FormatUint(seed, 10)] = syntheticTreeLines(rng.IntN(4) + 1) + } + + for name, lines := range inputs { + t.Run(name, func(t *testing.T) { + tree, err := parseTree(lines) + require.NoError(t, err) + + sections := (&specTree{root: tree}).Sections("%build", "") + require.NotEmpty(t, sections) + + for _, section := range sections { + section.AppendLines([]string{"/usr/share/structural-marker"}) + } + + edited := serializeTree(tree) + reparsed, err := parseTree(edited) + require.NoError(t, err) + assert.Equal(t, edited, serializeTree(reparsed)) + }) + } +} + +func fixtureLines(t *testing.T, name string) []string { + t.Helper() + + contents, err := treeFixtureFS.ReadFile(path.Join("testdata/specs", name)) + require.NoError(t, err) + + var lines []string + + scanner := bufio.NewScanner(strings.NewReader(string(contents))) + for scanner.Scan() { + lines = append(lines, scanner.Text()) + } + + require.NoError(t, scanner.Err()) + + return lines +} + +func syntheticTreeLines(branches int) []string { + lines := []string{"Name: synthetic"} + for branch := range branches { + lines = append(lines, + "%if "+strconv.Itoa(branch%2), + "%package package"+strconv.Itoa(branch), + "%description package"+strconv.Itoa(branch), + "synthetic", + "%files", + "/usr/bin/synthetic", + "%endif", + ) + } + + lines = append(lines, "%build", "make") + + return lines +} From 4a1f4679a0080c372d710bfe73bdf25cac573901 Mon Sep 17 00:00:00 2001 From: Thien Trung Vuong Date: Wed, 2 Sep 2026 20:39:24 +0000 Subject: [PATCH 5/5] feat(spec): preserve referenced macros during removal --- internal/rpm/spec/structural_tree_api.go | 4 + .../specs/subpackage-define-referenced.spec | 39 + .../specs/subpackage-define-shadowed.spec | 33 + .../specs/subpackage-define-transitive.spec | 38 + internal/rpm/spec/testdata_test.go | 35 + internal/rpm/spec/tree_hoist.go | 857 ++++++++++++++ internal/rpm/spec/tree_hoist_internal_test.go | 1021 +++++++++++++++++ 7 files changed, 2027 insertions(+) create mode 100644 internal/rpm/spec/testdata/specs/subpackage-define-referenced.spec create mode 100644 internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec create mode 100644 internal/rpm/spec/testdata/specs/subpackage-define-transitive.spec create mode 100644 internal/rpm/spec/tree_hoist.go create mode 100644 internal/rpm/spec/tree_hoist_internal_test.go diff --git a/internal/rpm/spec/structural_tree_api.go b/internal/rpm/spec/structural_tree_api.go index 55aac0831..630d09c40 100644 --- a/internal/rpm/spec/structural_tree_api.go +++ b/internal/rpm/spec/structural_tree_api.go @@ -183,6 +183,10 @@ func (t *specTree) RemoveSections(handles []*sectionHandle) error { return err } + if err := t.hoistReferencedMacros(sections); err != nil { + return err + } + removeSections(t.root, sections) return nil 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..2eac029cb --- /dev/null +++ b/internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec @@ -0,0 +1,33 @@ +Name: subpackage-define-shadowed +Version: 1.0 +Release: 1 +Summary: Subpackage %%define overrides 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 hoisted when it is the exact effective +binding of the surviving %%install reference. + +%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..cdf0814d0 --- /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 the transitive macro-hoisting case: 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 +hoist BOTH macros so the survivor reference resolves. + +%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_test.go b/internal/rpm/spec/testdata_test.go index 1cdc670ba..576a4d20f 100644 --- a/internal/rpm/spec/testdata_test.go +++ b/internal/rpm/spec/testdata_test.go @@ -196,6 +196,41 @@ func TestStructuralParserFixtureSearchAndReplaceCoversLineTypes(t *testing.T) { assertReparseable(t, contents) } +func TestStructuralParserFixtureSubpackageMacroRemovalHoistsReferencedDefinitions(t *testing.T) { + for _, name := range []string{ + "subpackage-define-referenced.spec", + "subpackage-define-transitive.spec", + "subpackage-define-shadowed.spec", + } { + t.Run(name, func(t *testing.T) { + specification := openFixture(t, name) + require.NoError(t, specification.RemoveSubpackage( + map[string]string{ + "subpackage-define-referenced.spec": "tests", + "subpackage-define-transitive.spec": "tests", + "subpackage-define-shadowed.spec": "tools", + }[name], + )) + + contents := serializeFixture(t, specification) + assertReparseable(t, contents) + + switch name { + case "subpackage-define-referenced.spec": + assert.Contains(t, contents, "%define testsdir %{_libdir}/%{name}/tests-src") + assert.Less(t, strings.LastIndex(contents, "%define testsdir"), strings.Index(contents, "\n%install\n")) + case "subpackage-define-transitive.spec": + assert.Less(t, strings.Index(contents, "%define testroot"), strings.Index(contents, "%define testsdir")) + assert.Contains(t, contents, "%define testsdir %{testroot}/tests-src") + case "subpackage-define-shadowed.spec": + assert.Equal(t, 2, strings.Count(contents, "%global toolsdir")) + assert.Contains(t, contents, "tools-override") + assert.Less(t, strings.LastIndex(contents, "%global toolsdir"), strings.Index(contents, "\n%install\n")) + } + }) + } +} + func TestStructuralParserGDBShapedMacroBodyIsOpaqueKnownLimitation(t *testing.T) { input := `%define gdb_python_configure \ %if 0%{?with_python}\ diff --git a/internal/rpm/spec/tree_hoist.go b/internal/rpm/spec/tree_hoist.go new file mode 100644 index 000000000..e0f196f34 --- /dev/null +++ b/internal/rpm/spec/tree_hoist.go @@ -0,0 +1,857 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "errors" + "fmt" + "log/slog" + "regexp" + "slices" + "strings" + "unicode" +) + +// ErrUnsafeMacroHoist is returned when moving a macro declaration out of a +// removed section could change RPM macro evaluation. +var ErrUnsafeMacroHoist = errors.New("unsafe macro hoist") + +var undefineDirective = regexp.MustCompile(`(?i)^\s*%undefine\s+([[:alnum:]_.-]+)\b`) + +const macroDirectiveSubmatches = 2 + +type macroDefinition struct { + block *block + global bool + conditional bool + parametered bool + lua bool + removed bool + order int + refs map[string]bool + dynamicRefs []string +} + +type macroReference struct { + name string + order int +} + +type macroDynamicReference struct { + pattern string + order int +} + +type macroDependencyTraversal struct { + definition *macroDefinition + useOrder int +} + +type selectedMacroClosure struct { + definitions map[*macroDefinition]bool +} + +type macroFacts struct { + removed []*macroDefinition + all map[string][]*macroDefinition + references []macroReference + dynamicRefs []macroDynamicReference + undefined map[string]bool + rootStarts map[*block]int +} + +// hoistReferencedMacros preserves only the exact declarations used by surviving +// references. RPM macro evaluation is contextual, so any ambiguous relocation +// is rejected rather than guessed. +func (t *specTree) hoistReferencedMacros(removeSet map[*block]bool) error { + insertAt := -1 + + for index, child := range t.root.Children { + if containsRemovedSection(child, removeSet) { + insertAt = index + + break + } + } + + if insertAt < 0 { + return fmt.Errorf("finding removed root ancestor for macro hoist:\n%w", ErrUnsafeMacroHoist) + } + + facts := collectMacroFacts(t.root, removeSet) + if len(facts.removed) == 0 { + return nil + } + + closure, err := selectMacroClosure(facts) + if err != nil { + return err + } + + if len(closure.definitions) == 0 { + return validateDynamicMacroReferences(closure.definitions, facts) + } + + if err := validateMacroHoist(closure, facts, facts.rootStarts[t.root.Children[insertAt]]); err != nil { + return err + } + + hoisted := make([]*block, 0, len(closure.definitions)) + for _, definition := range facts.removed { + if !closure.definitions[definition] { + continue + } + + hoisted = append(hoisted, &block{ + Kind: macroDefBlock, + Header: definition.block.Header, + Name: definition.block.Name, + Lines: slices.Clone(definition.block.Lines), + }) + slog.Debug("Hoisting macro definition from removed section", "macro", definition.block.Name) + } + + t.root.Children = append(t.root.Children[:insertAt], append(hoisted, t.root.Children[insertAt:]...)...) + + return nil +} + +func selectMacroClosure(facts *macroFacts) (*selectedMacroClosure, error) { + closure := &selectedMacroClosure{ + definitions: make(map[*macroDefinition]bool), + } + selectedByName := make(map[string]*macroDefinition) + traversed := make(map[macroDependencyTraversal]bool) + + var visit func(string, int) error + + visit = func(name string, useOrder int) error { + definition := effectiveMacroDefinition(facts.all[name], useOrder) + if definition == nil { + return nil + } + + if definition.removed { + closure.definitions[definition] = true + } + + traversal := macroDependencyTraversal{definition: definition, useOrder: useOrder} + if traversed[traversal] { + return nil + } + + traversed[traversal] = true + + // A %global is expanded at its definition, while a %define remains + // lazy. Follow lazy dependencies at each surviving use so a removed + // definition they resolve to can be preserved. Traversal includes the + // use order because separate invocations can resolve dependencies to + // different declarations. + dependencyOrder := useOrder + if definition.global { + dependencyOrder = definition.order - 1 + } + + for dependency := range definition.refs { + if err := visit(dependency, dependencyOrder); err != nil { + return err + } + } + + return nil + } + + for _, reference := range facts.references { + definition := effectiveMacroDefinition(facts.all[reference.name], reference.order) + if definition != nil && definition.removed { + if existing := selectedByName[reference.name]; existing != nil && existing != definition { + return nil, fmt.Errorf("multiple removed declarations of macro %#q are effective at surviving references:\n%w", + reference.name, ErrUnsafeMacroHoist) + } + + selectedByName[reference.name] = definition + } + + if err := visit(reference.name, reference.order); err != nil { + return nil, err + } + } + + return closure, nil +} + +func effectiveMacroDefinition(definitions []*macroDefinition, order int) *macroDefinition { + var effective *macroDefinition + for _, definition := range definitions { + if definition.order <= order && (effective == nil || effective.order < definition.order) { + effective = definition + } + } + + return effective +} + +func validateMacroHoist(closure *selectedMacroClosure, facts *macroFacts, insertionOrder int) error { + if err := validateSelectedMacroHoists(closure.definitions, facts, insertionOrder); err != nil { + return err + } + + if err := validateConditionalMacroSelections(closure.definitions, facts); err != nil { + return err + } + + if err := validateLazyDependencyBindings(closure.definitions, facts, insertionOrder); err != nil { + return err + } + + if err := validateDynamicMacroReferences(closure.definitions, facts); err != nil { + return err + } + + return nil +} + +func validateSelectedMacroHoists(selected map[*macroDefinition]bool, facts *macroFacts, insertionOrder int) error { + for definition := range selected { + if definition.conditional || definition.parametered || definition.lua { + return fmt.Errorf("cannot safely hoist macro %#q from conditional, parameterized, or Lua scope:\n%w", + definition.block.Name, ErrUnsafeMacroHoist) + } + + if facts.undefined[definition.block.Name] { + return fmt.Errorf("cannot safely hoist macro %#q across '%%undefine':\n%w", + definition.block.Name, ErrUnsafeMacroHoist) + } + + for _, candidate := range facts.all[definition.block.Name] { + if !candidate.removed && + insertionOrder < candidate.order && candidate.order < definition.order { + return fmt.Errorf("cannot safely hoist macro %#q across surviving declaration:\n%w", + definition.block.Name, ErrUnsafeMacroHoist) + } + } + + if err := validateRelocation(definition, facts.references, insertionOrder); err != nil { + return err + } + + if err := validateGlobalDependency(definition, facts); err != nil { + return err + } + + if err := validateSelectedGlobalDynamicBindings(definition, facts); err != nil { + return err + } + } + + return nil +} + +func validateSelectedGlobalDynamicBindings(definition *macroDefinition, facts *macroFacts) error { + if !definition.global { + return nil + } + + for _, pattern := range definition.dynamicRefs { + for _, candidate := range facts.all { + for _, binding := range candidate { + if dynamicMacroNameMayReferTo(pattern, binding.block.Name) { + return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q with dynamic name %#q:\n%w", + definition.block.Name, pattern, ErrUnsafeMacroHoist) + } + } + } + } + + return nil +} + +//nolint:cyclop,funlen // Selected closure validation follows both reachability and dependency bindings. +func validateConditionalMacroSelections(selected map[*macroDefinition]bool, facts *macroFacts) error { + containsSelected := make(map[macroDependencyTraversal]bool) + searching := make(map[macroDependencyTraversal]bool) + relevant := make(map[macroDependencyTraversal]bool) + + var reachesSelected func(*macroDefinition, int) bool + + reachesSelected = func(definition *macroDefinition, useOrder int) bool { + if definition == nil { + return false + } + + traversal := macroDependencyTraversal{definition: definition, useOrder: useOrder} + if known, ok := containsSelected[traversal]; ok { + return known + } + + if searching[traversal] { + return selected[definition] + } + + searching[traversal] = true + found := selected[definition] + + dependencyOrder := useOrder + if definition.global { + dependencyOrder = definition.order - 1 + } + + for dependency := range definition.refs { + dependencyDefinition := effectiveMacroDefinition(facts.all[dependency], dependencyOrder) + found = found || reachesSelected(dependencyDefinition, dependencyOrder) + } + + delete(searching, traversal) + containsSelected[traversal] = found + + return found + } + + var markRelevant func(*macroDefinition, int) + + markRelevant = func(definition *macroDefinition, useOrder int) { + if definition == nil { + return + } + + traversal := macroDependencyTraversal{definition: definition, useOrder: useOrder} + if relevant[traversal] { + return + } + + relevant[traversal] = true + + dependencyOrder := useOrder + if definition.global { + dependencyOrder = definition.order - 1 + } + + for dependency := range definition.refs { + markRelevant(effectiveMacroDefinition(facts.all[dependency], dependencyOrder), dependencyOrder) + } + } + + for _, reference := range facts.references { + definition := effectiveMacroDefinition(facts.all[reference.name], reference.order) + if reachesSelected(definition, reference.order) { + markRelevant(definition, reference.order) + } + } + + for binding := range relevant { + for _, candidate := range facts.all[binding.definition.block.Name] { + if candidate.conditional && candidate.order <= binding.useOrder { + return fmt.Errorf("cannot safely choose macro %#q around conditional declarations:\n%w", + binding.definition.block.Name, ErrUnsafeMacroHoist) + } + } + } + + return nil +} + +func validateLazyDependencyBindings(selected map[*macroDefinition]bool, facts *macroFacts, insertionOrder int) error { + visited := make(map[macroDependencyTraversal]bool) + + var visit func(*macroDefinition, int) error + + visit = func(definition *macroDefinition, useOrder int) error { + if definition == nil || definition.global { + return nil + } + + traversal := macroDependencyTraversal{definition: definition, useOrder: useOrder} + if visited[traversal] { + return nil + } + + visited[traversal] = true + + for dependency := range definition.refs { + before := effectiveMacroDefinition(facts.all[dependency], useOrder) + + after := effectiveHoistedMacroDefinition(facts.all[dependency], selected, insertionOrder, useOrder) + if before != after { + return fmt.Errorf("cannot safely hoist dependency %#q used by lazy macro %#q:\n%w", + dependency, definition.block.Name, ErrUnsafeMacroHoist) + } + + if err := visit(before, useOrder); err != nil { + return err + } + } + + return nil + } + + for _, reference := range facts.references { + definition := effectiveMacroDefinition(facts.all[reference.name], reference.order) + if definition == nil || definition.global { + continue + } + + if err := visit(definition, reference.order); err != nil { + return err + } + } + + return nil +} + +func effectiveHoistedMacroDefinition( + definitions []*macroDefinition, + selected map[*macroDefinition]bool, + insertionOrder int, + useOrder int, +) *macroDefinition { + var effective *macroDefinition + + effectiveOrder := -1 + + for _, definition := range definitions { + order := definition.order + if definition.removed { + if !selected[definition] { + continue + } + + order = insertionOrder + } + + if order <= useOrder && order > effectiveOrder { + effective = definition + effectiveOrder = order + } + } + + return effective +} + +func validateDynamicMacroReferences(selected map[*macroDefinition]bool, facts *macroFacts) error { + dynamics := slices.Clone(facts.dynamicRefs) + visited := make(map[macroDependencyTraversal]bool) + + for definition := range selected { + for _, pattern := range definition.dynamicRefs { + dynamics = append(dynamics, macroDynamicReference{pattern: pattern, order: definition.order}) + } + } + + var visit func(*macroDefinition, int) + + visit = func(definition *macroDefinition, useOrder int) { + if definition == nil || definition.global { + return + } + + traversal := macroDependencyTraversal{definition: definition, useOrder: useOrder} + if visited[traversal] { + return + } + + visited[traversal] = true + + for _, pattern := range definition.dynamicRefs { + dynamics = append(dynamics, macroDynamicReference{pattern: pattern, order: useOrder}) + } + + for dependency := range definition.refs { + visit(effectiveMacroDefinition(facts.all[dependency], useOrder), useOrder) + } + } + + for _, reference := range facts.references { + visit(effectiveMacroDefinition(facts.all[reference.name], reference.order), reference.order) + } + + for _, dynamic := range dynamics { + for _, definition := range facts.removed { + if (definition.order <= dynamic.order || selected[definition]) && + dynamicMacroNameMayReferTo(dynamic.pattern, definition.block.Name) { + return fmt.Errorf("cannot safely resolve dynamic macro name %#q:\n%w", + dynamic.pattern, ErrUnsafeMacroHoist) + } + } + } + + return nil +} + +func dynamicMacroNameMayReferTo(pattern, name string) bool { + prefixEnd := strings.IndexByte(pattern, '%') + if prefixEnd < 0 { + return false + } + + suffixStart := strings.LastIndexByte(pattern, '}') + 1 + prefix, suffix := pattern[:prefixEnd], pattern[suffixStart:] + + return strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) +} + +func validateRelocation(definition *macroDefinition, references []macroReference, insertionOrder int) error { + if definition.order <= insertionOrder { + return nil + } + + for _, reference := range references { + if reference.name == definition.block.Name && + insertionOrder <= reference.order && reference.order < definition.order { + return fmt.Errorf("cannot safely hoist macro %#q across an earlier surviving reference:\n%w", + definition.block.Name, ErrUnsafeMacroHoist) + } + } + + return nil +} + +func validateGlobalDependency(definition *macroDefinition, facts *macroFacts) error { + if !definition.global { + return nil + } + + for dependency := range definition.refs { + if effectiveMacroDefinition(facts.all[dependency], definition.order-1) == nil { + for _, candidate := range facts.all[dependency] { + if candidate == definition { + return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q before dependency %#q:\n%w", + definition.block.Name, dependency, ErrUnsafeMacroHoist) + } + } + } + + for _, candidate := range facts.all[dependency] { + if candidate == definition { + continue + } + + if !candidate.removed || candidate.order >= definition.order { + return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q before dependency %#q:\n%w", + definition.block.Name, dependency, ErrUnsafeMacroHoist) + } + } + + if facts.undefined[dependency] { + return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q across '%%undefine' of %#q:\n%w", + definition.block.Name, dependency, ErrUnsafeMacroHoist) + } + } + + return nil +} + +//nolint:cyclop,funlen,gocognit // Macro facts require one pass over structural block types. +func collectMacroFacts(root *block, removeSet map[*block]bool) *macroFacts { + facts := ¯oFacts{ + all: make(map[string][]*macroDefinition), + undefined: make(map[string]bool), + rootStarts: make(map[*block]int), + } + order := 0 + + addReferences := func(content string, referenceOrder int) { + references, dynamics := macroReferenceDetails(content) + for name := range references { + facts.references = append(facts.references, macroReference{name: name, order: referenceOrder}) + } + + for _, pattern := range dynamics { + facts.dynamicRefs = append(facts.dynamicRefs, macroDynamicReference{pattern: pattern, order: referenceOrder}) + } + } + + var walk func(*block, bool, bool) + + walk = func(current *block, removed, conditional bool) { + removed = removed || removeSet[current] + conditional = conditional || current.Kind == conditionalBlock + + switch current.Kind { + case rootBlock: + for _, child := range current.Children { + facts.rootStarts[child] = order + walk(child, false, false) + } + + return + case sectionBlock: + if !removed { + addReferences(sectionHeaderArguments(current.Header, current.Name), order) + } + + order++ + case macroDefBlock: + references, dynamics := macroReferenceDetails(strings.Join(current.Lines, "\n")) + definition := ¯oDefinition{ + block: current, + global: strings.HasPrefix(strings.ToLower(strings.TrimSpace(current.Header)), "%global"), + conditional: conditional, + parametered: strings.Contains(strings.Fields(strings.TrimSpace(current.Header))[1], "("), + lua: strings.Contains(strings.Join(current.Lines, "\n"), "%{lua:"), + removed: removed, + order: order, + refs: references, + dynamicRefs: dynamics, + } + + facts.all[current.Name] = append(facts.all[current.Name], definition) + if removed { + facts.removed = append(facts.removed, definition) + } else if definition.global { + // A %define body is expanded only at an invocation, so recording + // its references here would treat them as real earlier uses. + addReferences(strings.Join(current.Lines, "\n"), order-1) + } + + order++ + case textBlock: + for _, line := range current.Lines { + if matches := undefineDirective.FindStringSubmatch(line); len(matches) == macroDirectiveSubmatches { + facts.undefined[matches[1]] = true + } + + if !removed { + addReferences(line, order) + } + + order++ + } + case conditionalBlock: + if !removed { + addReferences(current.Header, order) + addReferences(current.ElseDirective, order) + } + + order++ + } + + for _, child := range current.Children { + walk(child, removed, conditional) + } + + for _, child := range current.Else { + walk(child, removed, conditional) + } + } + + walk(root, false, false) + + return facts +} + +// sectionHeaderArguments excludes the section marker, which RPM does not expand, +// while retaining the raw argument text where macro references are meaningful. +func sectionHeaderArguments(header, name string) string { + header = strings.TrimLeftFunc(header, unicode.IsSpace) + if !strings.HasPrefix(strings.ToLower(header), strings.ToLower(name)) { + return "" + } + + return header[len(name):] +} + +func macroReferences(content string) map[string]bool { + refs := make(map[string]bool) + scanMacroReferences(content, refs, nil) + + return refs +} + +func macroReferenceDetails(content string) (map[string]bool, []string) { + refs := make(map[string]bool) + + var dynamics []string + scanMacroReferences(content, refs, &dynamics) + + return refs, dynamics +} + +func scanMacroReferences(content string, refs map[string]bool, dynamics *[]string) { + for index := 0; index < len(content); { + if content[index] != '%' { + index++ + + continue + } + + runEnd := index + for runEnd < len(content) && content[runEnd] == '%' { + runEnd++ + } + + if runEnd == len(content) { + index = runEnd + + continue + } + + if (runEnd-index)%2 == 0 { + index = runEnd + + continue + } + + if content[runEnd] == '{' && percentRunOpensBracedMacro(content, index) { + end, ok := bracedMacroEnd(content, runEnd+1) + if !ok { + index = runEnd + 1 + + continue + } + + addBracedMacroReference(content[runEnd+1:end], refs, dynamics) + index = end + 1 + + continue + } + + if macroNameStart(content[runEnd]) { + end := runEnd + 1 + for end < len(content) && macroNameCharacter(content[end]) { + end++ + } + + if !macroDirectiveName(content[runEnd:end]) { + refs[content[runEnd:end]] = true + } + + index = end + + continue + } + + index = runEnd + 1 + } +} + +func addBracedMacroReference(body string, refs map[string]bool, dynamics *[]string) { + name := strings.TrimLeft(body, "!?") + + fields := strings.Fields(name) + if len(fields) == 0 { + return + } + + addBracedMacroName(fields, refs, dynamics) + scanMacroReferences(body, refs, dynamics) +} + +func addBracedMacroName(fields []string, refs map[string]bool, dynamics *[]string) { + if macroTestDirectiveName(fields[0]) { + if len(fields) > 1 { + refs[fields[1]] = true + } + + return + } + + macroName := strings.Split(fields[0], ":")[0] + if strings.Contains(macroName, "%") { + if dynamics != nil { + *dynamics = append(*dynamics, macroName) + } + + return + } + + if !macroDirectiveName(macroName) { + refs[macroName] = true + } +} + +func bracedMacroEnd(content string, start int) (int, bool) { + depth := 1 + + for index := start; index < len(content); index++ { + if content[index] == '%' { + nextIndex, nested, ok := bracedPercentIndex(content, index) + if !ok { + return 0, false + } + + if nested { + depth++ + } + + index = nextIndex + + continue + } + + if content[index] == '}' { + depth-- + if depth == 0 { + return index, true + } + } + } + + return 0, false +} + +func bracedPercentIndex(content string, index int) (nextIndex int, nested bool, ok bool) { + runEnd := index + for runEnd < len(content) && content[runEnd] == '%' { + runEnd++ + } + + if runEnd == len(content) || content[runEnd] != '{' { + return runEnd - 1, false, true + } + + if percentRunOpensBracedMacro(content, index) { + return runEnd, true, true + } + + escapedEnd, ok := escapedBracedMacroEnd(content, runEnd+1) + + return escapedEnd, false, ok +} + +func escapedBracedMacroEnd(content string, start int) (int, bool) { + depth := 1 + + for index := start; index < len(content); index++ { + switch content[index] { + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return index, true + } + } + } + + return 0, false +} + +func macroNameStart(character byte) bool { + return character == '_' || (character >= 'a' && character <= 'z') || (character >= 'A' && character <= 'Z') +} + +func macroNameCharacter(character byte) bool { + return macroNameStart(character) || + (character >= '0' && character <= '9') || + character == '.' || character == '-' +} + +func macroDirectiveName(name string) bool { + switch strings.ToLower(name) { + case "define", "defined", "global", "undefine", "undefined", + "if", "else", "elif", "endif", "ifarch", "ifnarch", "ifos", "ifnos": + return true + } + + return false +} + +func macroTestDirectiveName(name string) bool { + switch strings.ToLower(name) { + case "defined", "undefined": + return true + } + + return false +} diff --git a/internal/rpm/spec/tree_hoist_internal_test.go b/internal/rpm/spec/tree_hoist_internal_test.go new file mode 100644 index 000000000..48d75d508 --- /dev/null +++ b/internal/rpm/spec/tree_hoist_internal_test.go @@ -0,0 +1,1021 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "bytes" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRemoveSectionsHoistsReferencedMacroClosure(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define root %{base}/tools", + "%define base /usr/lib", + "%define unused ignored", + "%description tools", + "%{root}", + "%install", + "install -d %{root}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + + assert.Equal(t, []string{ + "%define root %{base}/tools", + "%define base /usr/lib", + "%install", + "install -d %{root}", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsMacroReferencedBySurvivingSectionHeader(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define suffix tools", + "%description tools", + "tools", + "%package %{name}-%{suffix}", + "%description %{name}-%{suffix}", + "survives", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + + assert.Equal(t, []string{ + "%define suffix tools", + "%package %{name}-%{suffix}", + "%description %{name}-%{suffix}", + "survives", + }, specification.rawLines) +} + +func TestRemoveSectionsDoesNotTreatBuildSectionMarkerAsMacroReference(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define build(arg) %{arg}", + "%description tools", + "tools", + "%build", + "make", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%build", + "make", + }, specification.rawLines) +} + +func TestRemoveSectionsDoesNotTreatInstallSectionMarkerAsMacroReference(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define install /usr/bin/install", + "%description tools", + "tools", + "%install", + "install -d %{buildroot}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%install", + "install -d %{buildroot}", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsMacroReferencedByPackageHeaderArguments(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define name replacement", + "%define suffix extras", + "%description tools", + "tools", + "%package -n %{name}-%{suffix}", + "%description -n %{name}-%{suffix}", + "survives", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define name replacement", + "%define suffix extras", + "%package -n %{name}-%{suffix}", + "%description -n %{name}-%{suffix}", + "survives", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsMacroReferencedByFilesHeaderArguments(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define manifest files.list", + "%description tools", + "tools", + "%files -f %{manifest}", + "/usr/bin/tool", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define manifest files.list", + "%files -f %{manifest}", + "/usr/bin/tool", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsMacroReferencedByTriggerHeaderArguments(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define trigger_target other-package", + "%description tools", + "tools", + "%triggerin -- %{trigger_target}", + "echo triggered", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define trigger_target other-package", + "%triggerin -- %{trigger_target}", + "echo triggered", + }, specification.rawLines) +} + +func TestRemoveSectionsPreservesQemuIssue203Macro(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define qemu_target %{_arch}", + "%description tools", + "tools", + "%install", + "echo %{qemu_target}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define qemu_target %{_arch}", + "%install", + "echo %{qemu_target}", + }, specification.rawLines) +} + +func TestRemoveSectionsAtomicallyHoistsExpandMacroWithRawBraces(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package dates", + "%define date_corpus %{expand:", + "for date in 2024-02-29 2025-02-28; do", + ` if { test "${date#????-??-??}" = "$date"; }; then`, + " %if 0", + " printf '%s\\n' %{date}", + " %endif", + " fi", + "done", + "}", + "%description dates", + "dates", + "%install", + "printf '%s\\n' %{date_corpus}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("dates")) + })) + assert.Equal(t, []string{ + "%define date_corpus %{expand:", + "for date in 2024-02-29 2025-02-28; do", + ` if { test "${date#????-??-??}" = "$date"; }; then`, + " %if 0", + " printf '%s\\n' %{date}", + " %endif", + " fi", + "done", + "}", + "%install", + "printf '%s\\n' %{date_corpus}", + }, specification.rawLines) +} + +func TestRemoveSectionsIgnoresUnrelatedConditionalMacroDeclarations(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%if %{with kvm}", + "%define kvm_package qemu-kvm", + "%else", + "%define kvm_package qemu-system", + "%endif", + "%package tests", + "%define testsdir %{_libdir}/%{name}/tests-src", + "%description tests", + "tests", + "%install", + "install -d %{testsdir}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tests")) + })) + assert.Equal(t, []string{ + "%if %{with kvm}", + "%define kvm_package qemu-kvm", + "%else", + "%define kvm_package qemu-system", + "%endif", + "%define testsdir %{_libdir}/%{name}/tests-src", + "%install", + "install -d %{testsdir}", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsLaterEffectiveGlobalMacro(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%global toolsdir /usr/share/tools", + "%package tools", + "%global toolsdir %{_libdir}/tools", + "%description tools", + "tools", + "%install", + "install -d %{toolsdir}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + + assert.Equal(t, []string{ + "%global toolsdir /usr/share/tools", + "%global toolsdir %{_libdir}/tools", + "%install", + "install -d %{toolsdir}", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsPriorGlobalUsedByLaterGlobal(t *testing.T) { + tests := []struct { + name string + lines []string + expected []string + }{ + { + name: "later global survives", + lines: []string{ + "%package tools", + "%global foo old", + "%description tools", + "tools", + "%install", + "%global foo %{?foo}-new", + "echo %{foo}", + }, + expected: []string{ + "%global foo old", + "%install", + "%global foo %{?foo}-new", + "echo %{foo}", + }, + }, + { + name: "later global is selected", + lines: []string{ + "%package tools", + "%global foo old", + "%global foo %{?foo}-new", + "%description tools", + "tools", + "%install", + "echo %{foo}", + }, + expected: []string{ + "%global foo old", + "%global foo %{?foo}-new", + "%install", + "echo %{foo}", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.lines) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + + assert.Equal(t, test.expected, specification.rawLines) + }) + } +} + +func TestRemoveSectionsHoistCycleTerminates(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define one %{two}", + "%define two %{one}", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define one %{two}", + "%define two %{one}", + "%install", + "echo %{one}", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsLazyDependencyEffectiveAtSurvivingUse(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define root %{base}/tools", + "%define base /usr/lib", + "%description tools", + "tools", + "%install", + "install -d %{root}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define root %{base}/tools", + "%define base /usr/lib", + "%install", + "install -d %{root}", + }, specification.rawLines) +} + +func TestRemoveSectionsHoistsDependencyOfSurvivingLazyMacro(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%define root %{base}/tools", + "%package tools", + "%define base /usr/lib", + "%description tools", + "tools", + "%install", + "install -d %{root}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define root %{base}/tools", + "%define base /usr/lib", + "%install", + "install -d %{root}", + }, specification.rawLines) +} + +func TestRemoveSectionsRejectsAmbiguousDependencyOfSurvivingLazyMacro(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%define root %{base}/tools", + "%package tools", + "%define base /usr/lib", + "%description tools", + "tools", + "%install", + "install -d %{root}", + "%files tools", + "%define base /opt/lib", + "%check", + "install -d %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsDifferentRemovedLazyDependenciesAtSurvivingUses(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define root %{base}/tools", + "%define base /usr/lib", + "%description tools", + "tools", + "%install", + "install -d %{root}", + "%files tools", + "%define base /opt/lib", + "%check", + "install -d %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsRemovedLazyRootWithChangedDependencyBinding(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%define base old", + "%package tools", + "%define root %{base}", + "%description tools", + "tools", + "%install", + "echo %{root}", + "%files tools", + "%define base new", + "%check", + "echo %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsHoistsLazyDependencyUsedAfterRemovedDeclaration(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%description tools", + "tools", + "%package other", + "%define root %{base}/other", + "%description other", + "other", + "%files tools", + "%define base /usr/lib", + "%check", + "install -d %{root}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define base /usr/lib", + "%package other", + "%define root %{base}/other", + "%description other", + "other", + "%check", + "install -d %{root}", + }, specification.rawLines) +} + +func TestRemoveSectionsRejectsHoistingAcrossSurvivingSameNameDefinition(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%description tools", + "tools", + "%install", + "%define location /usr/lib", + "%files tools", + "%define location /opt/lib", + "%check", + "echo %{location}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsHoistsWithoutCrossingSameNameDefinition(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define location /opt/lib", + "%description tools", + "tools", + "%install", + "echo %{location}", + "%check", + "%define location /usr/lib", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define location /opt/lib", + "%install", + "echo %{location}", + "%check", + "%define location /usr/lib", + }, specification.rawLines) +} + +func TestRemoveSectionsRejectsUnsafeMacroHoistsWithoutMutation(t *testing.T) { + tests := []struct { + name string + lines []string + }{ + { + name: "eager global dependency declared later", + lines: []string{ + "%package tools", + "%global one %{two}", + "%define two value", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + }, + }, + { + name: "self-referential eager global", + lines: []string{ + "%package tools", + "%global one %{?one}", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + }, + }, + { + name: "eager global dependency has ambiguous surviving definition", + lines: []string{ + "%global toolsdir /usr/share/tools", + "%package tools", + "%global toolpath %{toolsdir}/bin", + "%description tools", + "%{toolpath}", + "%install", + "echo %{toolpath}", + }, + }, + { + name: "surviving undefine", + lines: []string{ + "%package tools", + "%define one value", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + "%undefine one", + }, + }, + { + name: "removed undefine", + lines: []string{ + "%package tools", + "%define one value", + "%undefine one", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + }, + }, + { + name: "parameterized declaration", + lines: []string{ + "%package tools", + "%define one(arg) %{arg}", + "%description tools", + "%one value", + "%install", + "echo %one value", + }, + }, + { + name: "conditional declaration", + lines: []string{ + "%package tools", + "%if 1", + "%define one value", + "%endif", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + }, + }, + { + name: "Lua declaration", + lines: []string{ + "%package tools", + "%global one %{lua:print('value')}", + "%description tools", + "%{one}", + "%install", + "echo %{one}", + }, + }, + { + name: "different removed declarations effective at surviving references", + lines: []string{ + "%package tools", + "%define one first", + "%description tools", + "tools", + "%install", + "echo %{one}", + "%files tools", + "%define one second", + "%check", + "echo %{one}", + }, + }, + { + name: "conditional peer declaration", + lines: []string{ + "%if 1", + "%define one conditional", + "%endif", + "%package tools", + "%define one removed", + "%description tools", + "tools", + "%install", + "echo %{one}", + }, + }, + { + name: "relocation crosses earlier surviving reference", + lines: []string{ + "%if 1", + "echo %{one}", + "%package tools", + "%define one value", + "%description tools", + "tools", + "%endif", + "%install", + "echo %{one}", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + specification := newTreeAPISpec(test.lines) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) + }) + } +} + +func TestRemoveSectionsHoistsMultilineMacroAndLogs(t *testing.T) { + var logs bytes.Buffer + + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + specification := newTreeAPISpec([]string{ + "%package tools", + "%define path /usr \\", + " /share/tools", + "%description tools", + "%{path}", + "%install", + "echo %{path}", + }) + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + + assert.Equal(t, []string{ + "%define path /usr \\", + " /share/tools", + "%install", + "echo %{path}", + }, specification.rawLines) + assert.Contains(t, logs.String(), "Hoisting macro definition from removed section") +} + +func TestMacroReferencesRecognizesDefinedAndUndefined(t *testing.T) { + refs := macroReferences("%{defined feature} %{undefined missing} %{?optional} %bare") + assert.Equal(t, map[string]bool{ + "feature": true, + "missing": true, + "optional": true, + "bare": true, + }, refs) +} + +func TestMacroReferencesRecognizesNestedAndArgumentReferences(t *testing.T) { + refs := macroReferences("%{expand:%{dep}} %{helper arg}") + assert.Equal(t, map[string]bool{ + "expand": true, + "dep": true, + "helper": true, + }, refs) +} + +func TestRemoveSectionsHoistsNestedAndArgumentMacroReferences(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define dep /usr/lib", + "%global expanded %{expand:%{dep}}", + "%define helper value", + "%description tools", + "tools", + "%install", + "echo %{expanded} %{helper arg}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + + assert.Equal(t, []string{ + "%define dep /usr/lib", + "%global expanded %{expand:%{dep}}", + "%define helper value", + "%install", + "echo %{expanded} %{helper arg}", + }, specification.rawLines) +} + +func TestRemoveSectionsRejectsLazyDependencyMovedBeforeEarlierBinding(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%if 1", + "%define root %{base}", + "%define base old", + "echo %{root}", + "%package tools", + "%define base new", + "%description tools", + "tools", + "%endif", + "%check", + "echo %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestValidateLazyDependencyBindingsChecksEveryInvocation(t *testing.T) { + root := ¯oDefinition{ + block: &block{Name: "root"}, + order: 1, + refs: map[string]bool{"base": true}, + } + oldBase := ¯oDefinition{block: &block{Name: "base"}, order: 2} + newBase := ¯oDefinition{block: &block{Name: "base"}, removed: true, order: 4} + facts := ¯oFacts{ + all: map[string][]*macroDefinition{ + "root": {root}, + "base": {oldBase, newBase}, + }, + references: []macroReference{ + {name: "root", order: 3}, + {name: "root", order: 5}, + }, + } + + err := validateLazyDependencyBindings(map[*macroDefinition]bool{newBase: true}, facts, 0) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) +} + +func TestRemoveSectionsRejectsConditionalDeclarationWithoutEvaluatingIt(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%if 0", + "%define base old", + "%endif", + "%define root %{base}", + "%package tools", + "%define base new", + "%description tools", + "tools", + "%install", + "echo %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsConditionalDependencyChosenAtSurvivingUse(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define root %{base}", + "%description tools", + "tools", + "%install", + "%if %{with alternate}", + "%define base first", + "%else", + "%define base second", + "%endif", + "%check", + "echo %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsSurvivingDynamicMacroNameMatchingRemovedDefinition(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%define suffix name", + "%package tools", + "%define dirname value", + "%description tools", + "tools", + "%install", + "echo %{dir%{suffix}}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsSurvivingLazyMacroDynamicReferenceMatchingRemovedDefinition(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%define suffix name", + "%define root %{dir%{suffix}}", + "%package tools", + "%define dirname value", + "%description tools", + "tools", + "%install", + "echo %{root}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsAllowsUnrelatedDynamicMacroName(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%define suffix name", + "%package tools", + "%define unrelated value", + "%description tools", + "tools", + "%install", + "echo %{dir%{suffix}}", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%define suffix name", + "%install", + "echo %{dir%{suffix}}", + }, specification.rawLines) +} + +func TestMacroReferencesHonorsPercentEscapes(t *testing.T) { + assert.Empty(t, macroReferences("%%{helper} %%helper %%%%{helper} %%%%helper")) + assert.Equal(t, map[string]bool{"helper": true}, + macroReferences("%%%{helper} %%%helper %%%%%{helper} %%%%%helper")) + assert.Equal(t, map[string]bool{"outer": true}, macroReferences("%{outer %%{helper}}")) +} + +func TestRemoveSectionsDoesNotHoistEscapedMacroReferences(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define helper value", + "%description tools", + "tools", + "%install", + "echo %%{helper} %%helper", + }) + + require.NoError(t, specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + })) + assert.Equal(t, []string{ + "%install", + "echo %%{helper} %%helper", + }, specification.rawLines) +} + +func TestRemoveSectionsRejectsSelectedSelfReferentialGlobal(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%global toolsdir %{toolsdir}", + "%description tools", + "tools", + "%install", + "echo %{toolsdir}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Contains(t, err.Error(), "toolsdir") + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsSelectedGlobalWithDynamicName(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define suffix name", + "%define dirname value", + "%global selected %{dir%{suffix}}", + "%description tools", + "tools", + "%install", + "echo %{selected}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Contains(t, err.Error(), "dir%{suffix}") + assert.Equal(t, before, specification.rawLines) +} + +func TestRemoveSectionsRejectsSelectedGlobalDynamicNameMatchingSurvivingDefinition(t *testing.T) { + specification := newTreeAPISpec([]string{ + "%package tools", + "%define suffix name", + "%global selected %{dir%{suffix}}", + "%description tools", + "tools", + "%install", + "%define dirname value", + "echo %{selected}", + }) + before := append([]string(nil), specification.rawLines...) + + err := specification.mutateTree(func(tree *specTree) error { + return tree.RemoveSections(tree.SectionsByPackage("tools")) + }) + + require.ErrorIs(t, err, ErrUnsafeMacroHoist) + assert.Contains(t, err.Error(), "dir%{suffix}") + assert.Equal(t, before, specification.rawLines) +}