feat: add structural tree parser for conditional-aware spec editing#215
Open
trungams wants to merge 9 commits into
Open
feat: add structural tree parser for conditional-aware spec editing#215trungams wants to merge 9 commits into
trungams wants to merge 9 commits into
Conversation
Adds the design RFC documenting the problem (straddling conditionals break naive line-range edits), the two evaluated approaches (enriched structure vs structural tree), and the chosen direction (tree model). Also documents known limitations in the overlays user guide: section-scoped operations cannot reach content past a wrapper %endif when the section header is inside the wrapper (workaround: spec-search-replace with anchored regex), and macro-generated sections are invisible to the static parser.
Two-pass parser that builds a block tree from raw spec lines: - Pass 1 (collectConditionalPairs in edit.go): collects conditional pairs, skipping %if/%endif inside %define/%global macro continuation bodies. Shell-level continuations still process %if as structural. - Pass 2 (buildBlockChildren): builds nested block tree, classifying conditionals as wrappers (contain section headers) vs content blocks. Block kinds: rootBlock, sectionBlock, conditionalBlock, textBlock, macroDefBlock. Preamble content is wrapped in an implicit sectionBlock with empty Name for uniform access via findSectionBlock. All tree types and helpers are kept unexported so the parsed representation remains an internal implementation detail. Includes serializeTree for lossless round-tripping, query helpers (findSectionBlock, findAllSectionBlocks, findAllSectionBlocksByPackage), mutation helpers (removeBlockFromParent), and validateSectionRemoval for detecting unsafe removal patterns (orphaned content). Validation checks: - Preceding-section: wrapper has text AND preceding section is removed. - Empty-wrapper + adjacent content: all sections removed from wrapper AND adjacent content conditional would be orphaned. Cross-branch asymmetry is intentionally allowed (removing from %if while %else retains sections is a valid overlay operation).
…tch of edit operations Add a tree interface in tree_api.go so edit.go callers no longer reach into the parse tree directly. specTree wraps the parsed *block root and exposes query helpers (Section, Sections, SectionsByPackage, RemoveSections); sectionHandle wraps a section block with append/prepend operations. Spec.mutateTree handles parse -> mutate -> serialize so call sites become 3-line orchestrations. Three edit.go operations now go through the interface as a starting point: AppendLinesToSection, RemoveSection, and RemoveSubpackage. Remaining Visit-based operations will migrate in follow-up commits.
Move HasSection off the Visit-based scan and onto the tree interface. Adds Spec.inspectTree (the read-only sibling of mutateTree) and specTree.HasSection, which walks the parsed tree (including sections inside conditional wrappers, matching previous Visit semantics).
Add line-level iteration to the tree interface (specTree.VisitAllLines / sectionHandle.VisitLines + lineHandle) plus a tag-aware insertion helper (sectionHandle.InsertTag), and use them to rewrite the tag-mutating operations on Spec. No behavior change for callers. Removes findInsertTagPosition, skipPastConditional, and insertTagScanResult.
trungams
force-pushed
the
tvuong/feat/spec-visit
branch
from
c4e7ac6 to
46a9ae0
Compare
Convert the last group of Visit-based mutators to mutateTree + section or line handles, then remove the Visit infrastructure entirely: Migrated operations: - PrependLinesToSection -> sectionHandle.PrependLines. - AddChangelogEntry -> sectionHandle.PrependLines on %changelog. - SearchAndReplace -> searchReplaceBlock, a recursive tree walker that visits ALL line types (text, macros, conditional headers, %endif) so patterns targeting %define/%global or %if lines are found correctly. Wrapper else-branch section filter is relaxed for loose content. - removePatchlistEntriesMatching -> sectionHandle.VisitLines on %patchlist. New API: - GetTag(packageName, tag) for read-only tag lookup via inspectTree. Removed (Visit infrastructure): - Visit, VisitTags, VisitTagsPackage from edit.go. - Context, VisitTarget, VisitTargetType, Visitor, InsertLinesBefore, InsertLinesAfter, RemoveLine, ReplaceLine from spec.go. - parseState, inContinuation, newParseState, parseSpecLine, newParsedLine, parseLogicalLine from spec.go. Dead code from line-range approach also removed: - sectionLineRange, collectSectionRanges, balanceRange, validateNoContentAfter, isBlankOrComment, removeRanges, etc. Migrated external caller: release.go GetReleaseValue now uses GetTag instead of VisitTagsPackage.
Stand up 13 hand-crafted minimized .spec fixtures under internal/rpm/spec/testdata/specs/ distilling real-world patterns. testdata_test.go drives fixtures through two tiers: - Tier 1: lossless round-trip (byte-for-byte identity). - Tier 3: per-pattern edit-operation regression tests. Synthetic generator composes primitives into random spec inputs (64 round-trip + 32 AddTag seeds). Also includes SearchAndReplace tests for macro definitions, conditional headers, and wrapper else-branch section filter.
When a %define/%global directive lives inside a %package subpackage block but the macro it declares is referenced by surviving content, RemoveSections now automatically hoists it to the root level. Two regression tests validate both the positive case (macro hoisted) and the negative case (unreferenced macro dropped cleanly).
…overlay Add a new PrependLinesToAllSections method that prepends lines to every section matching a given name and package, unlike PrependLinesToSection which targets only the first match. This is exposed as the new spec-prepend-all-lines overlay type. Update the check-disablement synthesizer to use the new overlay type so that specs with multiple %check sections (e.g., python-azure-mgmt-media, python-zmq) get exit 0 prepended to all of them.
trungams
force-pushed
the
tvuong/feat/spec-visit
branch
from
46a9ae0 to
56a4c63
Compare
Member
Author
|
The gdb render failure in e2e is expected as we'll need to update the overlay |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the flat Visit-based spec line walker with a structural tree parser for all spec editing operations. This makes conditional-boundary bugs structurally impossible and fixes several long-standing issues (#144, #193, #203).
Closes #214
Changes
New: tree parser (
tree.go)Two-pass parser that builds a
blocktree from raw spec lines:%if/%endifpairs, skipping pairs inside%define/%globalmacro continuation bodiesBlock kinds:
rootBlock,sectionBlock,conditionalBlock,textBlock,macroDefBlock. Lossless round-trip viaserializeTree.New: tree API (
tree_api.go)specTree/sectionHandletypes wrap the parsed tree.Spec.mutateTreehandles parse → mutate → serialize;Spec.inspectTreefor read-only access. The publicSpec.*API is unchanged for callers.New:
SearchAndReplacerewrite (searchReplaceBlock)Dedicated recursive walker that visits ALL line types (text, macros, conditional headers,
%endif). The oldVisitAllLines-based implementation skipped%define/%globaland conditional directive lines. Also relaxes the section filter for wrapper else-branch content.New: macro hoisting (
tree_hoist.go)RemoveSectionsnow automatically hoists%define/%globalmacros from removed sections when they're referenced by surviving content (#203). Scans for%{name},%{?name},%{!?name}, bare%name, and conditional headers like%if 0%{?with_name}.New:
GetTagandPrependLinesToAllSectionsGetTag(packageName, tag)— read-only tag lookup viainspectTree, replacesVisitTagsPackageusagePrependLinesToAllSections+spec-prepend-all-linesoverlay — prepends to every matching section (used by check-skip to disable all%checksections)New: testdata fixtures + synthetic stress
13 curated
.specfixtures distilling real-world patterns. Two test tiers (lossless round-trip + per-pattern edit-operation regression). Synthetic generator with 64+32 deterministic seeds.Removed: Visit infrastructure
Visit,VisitTags,VisitTagsPackage,Context,VisitTarget,VisitTargetType,Visitor,parseState,parseSpecLine,newParsedLine,parseLogicalLine— all deleted. The sole external caller (release.go) migrated toGetTag.Also removed:
sectionLineRange,collectSectionRanges,balanceRange,validateNoContentAfter,skipPastConditional,findInsertTagPosition,isBlankOrComment,removeRanges, and related helpers.Docs
docs/developer/rfc/docs/user/reference/config/overlays.mdValidation
Full render against Azure Linux 4.0 (~7,400 components):
Rendered output:
tvuong/4.0/feat/azldev-parsetreeKnown limitations
%ifwrapper with content past%endifis invisible to section-scoped overlays. Workaround:spec-search-replace. Affects 1 spec (gdb).%ghc_lib_subpackage,%fontpkg, etc. are invisible to the static parser.