From 2d0e821dea75686a6173c7b00b0fd6415d2af35c Mon Sep 17 00:00:00 2001 From: Nan Liu Date: Wed, 11 Nov 2026 00:05:42 +0000 Subject: [PATCH] feat(render): diagnose added patch application Check final specs for supported unconditional application of overlay-added patches and report selection uncertainty or likely strip mismatches. Expose patchWarnings in render JSON and add a strict warning policy, with regression tests and updated user and agent documentation. --- .../reference/cli/azldev_component_render.md | 8 + docs/user/reference/config/overlays.md | 167 +++ .../app/azldev/agentskill/agentskill_test.go | 14 + .../agentskill/content/overlays.md.tmpl | 50 +- .../content/withoutlockfile/overlays.md.tmpl | 50 +- internal/app/azldev/cmds/component/render.go | 141 +- .../render_patch_warnings_internal_test.go | 468 +++++++ .../app/azldev/cmds/component/render_test.go | 15 + .../azldev/core/sources/patch_application.go | 111 ++ .../patch_application_internal_test.go | 304 ++++ .../app/azldev/core/sources/sourceprep.go | 24 +- internal/rpm/spec/patch_application.go | 1146 +++++++++++++++ internal/rpm/spec/patch_application_test.go | 1248 +++++++++++++++++ ...napshots_component_render_help_1.snap.json | 3 + ...pshots_component_render_help_stderr_1.snap | 1 + ...pshots_component_render_help_stdout_1.snap | 79 ++ scenario/clismoke_test.go | 1 + scenario/component_render_test.go | 94 ++ 18 files changed, 3888 insertions(+), 36 deletions(-) create mode 100644 internal/app/azldev/cmds/component/render_patch_warnings_internal_test.go create mode 100644 internal/app/azldev/core/sources/patch_application.go create mode 100644 internal/app/azldev/core/sources/patch_application_internal_test.go create mode 100644 internal/rpm/spec/patch_application.go create mode 100644 internal/rpm/spec/patch_application_test.go create mode 100644 scenario/__snapshots__/TestSnapshots_component_render_help_1.snap.json create mode 100644 scenario/__snapshots__/TestSnapshots_component_render_help_stderr_1.snap create mode 100644 scenario/__snapshots__/TestSnapshots_component_render_help_stdout_1.snap diff --git a/docs/user/reference/cli/azldev_component_render.md b/docs/user/reference/cli/azldev_component_render.md index c7beff797..76eeee5d6 100644 --- a/docs/user/reference/cli/azldev_component_render.md +++ b/docs/user/reference/cli/azldev_component_render.md @@ -20,6 +20,13 @@ Unlike prepare-sources, render skips downloading source tarballs from the lookaside cache — only spec files, patches, scripts, and other git-tracked sidecar files are included. Multiple components can be rendered at once. +Added-patch application diagnostics are logged and included as patchWarnings +in -O json output. Missing unconditional patch selection, uncertain application, +and likely strip mismatches are warnings unless --patch-warnings=error promotes +them to component errors. Conditional branches and duplicate application are not checked. +Use --fail-on-error to exit nonzero on component failures. This is static +analysis, not proof a patch applies. + When rendering all components (-a), the --clean-stale flag prunes orphan rendered-spec directories (per-component dirs that no longer correspond to any component in the project config). Per-component dirs that ARE in config @@ -62,6 +69,7 @@ azldev component render [flags] -f, --force[=true|false] allow overwriting existing rendered component directories -h, --help[=true|false] help for render -o, --output-dir string output directory for rendered specs (overrides rendered-specs-dir from config) + --patch-warnings string added-patch warning policy: warn or error (error rejects affected components; use --fail-on-error for a nonzero exit) (default "warn") --rpmdev-bumpspec[=true|false] Use rpmdev-bumpspec instead of the legacy static release calculation --skip-lock-validation[=true|false] skip lock file consistency checks -s, --spec-path stringArray Spec path diff --git a/docs/user/reference/config/overlays.md b/docs/user/reference/config/overlays.md index f252e9d91..762badf7c 100644 --- a/docs/user/reference/config/overlays.md +++ b/docs/user/reference/config/overlays.md @@ -499,6 +499,173 @@ file = "fix-upstream-bug.patch" description = "Rename upstream patch for clarity" ``` +After all overlays have run, source preparation (including `component render`) checks +whether the final spec has a recognizable application command for each added patch. +It reports missing unconditional selection, uncertain application, and likely strip-level mismatches. +For example, a git-style patch to `a/lib/file.c` usually needs `-p1`. + +The legacy and experimental structural parsers use the same read-only analysis. +It recognizes `%patch`, `%autopatch`, `%autosetup`, and straightforward forms of +the following standard wrappers: + +| Wrapper | Patch application | +|---------|-------------------| +| `%forgeautosetup` | Like `%autosetup`, applies declared patches unless `-N` is supplied; honors `-p` and `-S`. | +| `%goprep` with an explicit `-p` | Uses `%autosetup` to apply declared patches at the requested strip level, for example `%goprep -p1`. | +| Bare `%goprep` | Only prepares sources; does not automatically apply declared patches. | + +**Recognized unconditional selections take precedence over unrelated opaque flow.** +An unconditional `%patch -P2202 -p1`, selected `%autopatch -p1 2202`, or bulk +`%autopatch -p1` / `%autosetup -p1` accounts for the added patch when its selection +and application options are understood. Ordinary preamble macros, unrelated macro +definitions, and surrounding commands such as `cp` or `autoreconf` do not invalidate +that selection. Directory-name macros such as `-n %{name}-%{version}` are accepted +without expanding them; selection, strip, and backend options must be literal. + +Conditional branches and their side effects are not evaluated. Their commands +cannot establish unconditional application, and conditional helper redefinitions +or directory changes do not affect later analysis. Duplicate application is not +detected, even among understood commands. These are scope limitations, not proof +that conditional behavior or duplicate application is harmless. + +Each candidate retains relevant preceding unconditional directory/control-flow +uncertainty and inline helper redefinitions. Later commands do not retroactively +invalidate it. A candidate with unsupported setup, backend, arguments, or likely +strip mismatch remains unverified unless another supported candidate accounts for +the patch without a detected problem. Use the build to verify actual application. + +Only the added patch's declaration is checked. An unknown automatic or macro-based +number can be accepted by an unrestricted bulk application command; an individual +selection or limited range requires a known number. Conditional or conflicting +declarations of the added patch remain uncertain. Unrelated declarations with +unknown numbers do not invalidate a known explicit number. +Automatic numbers for bare `Patch:` tags and `%patchlist` entries follow the highest +assigned patch number, not the most recent explicit tag. For example, `Patch10:` +followed by `Patch1:` leaves the next automatic number at 11. Conditional or +opaque declarations can make subsequent automatic numbers uncertain; a later +explicit tag does not by itself remove that uncertainty. + +**Limitation: loaded macro files are not inspected.** The analyzer ignores external +macro loads, whether azldev-generated or user-supplied, without reading or verifying +their contents. It assumes standard patch-macro behavior; definitions hidden in +those files, including configured `build.defines` overrides of `%setup`, +`%autosetup`, `%autopatch`, their wrappers, or `__scm`, are not modeled. +Later overlays that change or remove loaded files are not checked either. These cases can cause +missed diagnostics, false unused-patch errors, or misleading strip warnings; +strict mode does not remove this limitation. Unconditional relevant macro definitions and +`%undefine` directives preceding an invocation are tracked across sections, including +between `%description` and `%prep`. This includes patch helpers +`__patch`, `__apply_patch`, `__scm_apply_patch`, and `_default_patch_flags`. Affected invocations and +unresolved command arguments produce uncertainty diagnostics without evaluating +macros. The corresponding `__scm_apply_` helper is checked for each supported +backend. Bulk application checks `__apply_patch`; source-setup wrappers also check +`__scm_setup_` for their selected backend. Direct `%patch` commands do +not depend on those bulk/setup helpers, and `%autopatch` does not initialize the +backend. Unrelated backend helpers do not trigger warnings. +Git-backed setup and application also check `__git`; patch-backed commands do not. +Invoking a locally redefined setup/application macro or relevant dependency makes +subsequent application uncertain: the replacement can emit arbitrary shell, +including an early `exit`. Setup dependencies are invoked even when setup uses +`-N` to disable patch application; disabled application dependencies do not +affect subsequent commands. Bulk application helpers are not treated as invoked +when no declared patch can enter the selection; the bulk macro itself is still invoked. +Defining an unrelated or uninvoked helper has no such effect, and an earlier +sound selection remains valid. +Shell context visible beside an expansion is still checked. +Macro-generated definitions and conditional effects are outside the analysis. +`_default_patch_fuzz` alone does not trigger a +warning: actual hunk applicability is outside the analyzer's scope. +Path checks skip unified- and context-diff hunk contents, even when changed source +lines or range markers resemble `---` or `+++` file headers. +Shell function definitions, including `name () {` +syntax, make commands in their bodies unverified rather than treating them as +unconditional application. +Execution checks also recognize leading shell negation (`!`), variable assignments, and `command` / +`builtin` prefixes before directory changes or shell-exit commands; lookup-only +`command -v` / `-V` forms do not execute their operands. Shell continuation is +tracked from active, uncommented trailing escapes, not backslashes in comments +or escaped backslash pairs. +Review affected specs manually and use a build or `%prep` execution to verify +their actual behavior. + +When no supported unconditional application is found, the analyzer warns rather +than declaring the patch definitely unused. This also applies to an excluding +manual/range command and setup-only flows such as `%setup`, `%autosetup -N`, and +`%goprep` without `-p`. The whole `%prep` is scanned: +an excluding range or disabled setup does not hide a later application command. +One ordinary unconditional `%prep` is supported; conditional, repeated, and augmented +sections such as `%prep -a` or `%prep -p` warn as unsupported preparation rather than +recommending a potentially duplicate application command. An unrelated conditional +closed before `%prep` does not make it conditional. Missing selection, ambiguous +declarations, unsupported preparation, and likely strip mismatches are warnings +by default. +Other source-preparation callers continue to log all findings as warnings. + +Preparation does not execute `%prep` or check the extracted source tree, and render +does not download source tarballs. An empty warning list is +**not proof that a patch applies or a CVE is fixed**. +`patch-add` still only copies and declares the patch; it does not add application +commands or change strip levels. Use a spec overlay to add or adjust the command +where necessary. A later overlay can resolve the warning. If a later overlay removes +the patch's declaration, that patch is no longer checked. + +#### Machine-readable diagnostics and strict rendering + +`component render -O json` includes a `patchWarnings` array on each affected +component. It is omitted when there are no added-patch diagnostics. Each entry +contains the patch `filename` and `patchNumber`, a human-readable `reason`, and a +`command` when a relevant application command exists. Unknown numbering is +reported as `patchNumber: -1`; zero is reported explicitly. The `reason` explains the +specific cause and is human-readable, not a stable machine code. A diagnostic does +not necessarily mean the patch is never applied. + +```json +[ + { + "component": "mypackage", + "outputDir": "specs/m/mypackage", + "status": "ok", + "changed": true, + "patchWarnings": [ + { + "filename": "fix-build-flags.patch", + "patchNumber": 0, + "reason": "no recognized command outside conditional branches selects this patch in '%prep'; verify the preparation flow before adding an application command" + } + ] + } +] +``` + +For a CI gate, use: + +```bash +azldev component render -a --patch-warnings=error --fail-on-error -O json +``` + +The default policy is `--patch-warnings=warn`. With `error`, warnings are logged +as errors and reject their components too: + +| Finding | Default policy | Strict policy | +|---------|----------------|---------------| +| No recognized unconditional selection, including an excluding manual/range flow | Warning; component can render | Component error | +| Setup-only flow with no applying command | Warning; component can render | Component error | +| Uncertain application or likely strip mismatch | Warning; component can render | Component error | + +Rejected components get `status: "error"` before RPM processing or publication. +Normal renders use the existing `RENDER_FAILED` marker behavior; unaffected components +can still render successfully. JSON results retain the diagnostics under `patchWarnings`, +including findings treated as errors. Existing overwrite/`--force` safeguards still apply. +The default policy does not reject a component solely for an added-patch diagnostic. + +As with ordinary component failures, the command can exit zero unless +`--fail-on-error` is set. Command-level validation/setup failures remain nonzero. +`--check-only` remains a freshness check: it can pass for a rejected component whose +existing output is already exactly the expected failure marker, but output drift +returns nonzero. It never writes rendered output and cannot be combined with +`--fail-on-error`. The patch-warning policy applies only to render and does not +change `prepare-sources` or build's warning behavior. + ### Removing a Patch The `patch-remove` overlay removes patch references from the spec (`PatchN` tags and/or diff --git a/internal/app/azldev/agentskill/agentskill_test.go b/internal/app/azldev/agentskill/agentskill_test.go index e53c985a8..cb5ece88d 100644 --- a/internal/app/azldev/agentskill/agentskill_test.go +++ b/internal/app/azldev/agentskill/agentskill_test.go @@ -89,6 +89,20 @@ func TestImageSkillDependencyGuidance(t *testing.T) { } } +func TestOverlaySkillPatchDiagnostics(t *testing.T) { + for _, withoutLockfile := range []bool{false, true} { + doc, err := agentskill.NewCatalog(withoutLockfile).SkillDocument("azldev-overlays", testParams()) + require.NoError(t, err) + assert.Contains(t, doc, "warn, rather than declaring a patch definitely unused") + assert.Contains(t, doc, "Recognized unconditional selections take precedence") + assert.Contains(t, doc, "Conditional branches and their side effects are skipped") + assert.Contains(t, doc, "Duplicate application\n is not detected") + assert.Contains(t, doc, "Loaded macro files are not inspected") + assert.Contains(t, doc, "`%prep -a` or `%prep -p`") + assert.Contains(t, doc, "--patch-warnings=error --fail-on-error") + } +} + func TestOverlayMetadataSkillDocument(t *testing.T) { params := testParams() params.Bindings = agentskill.Bindings{RenderedSpecsDir: "build/specs"} diff --git a/internal/app/azldev/agentskill/content/overlays.md.tmpl b/internal/app/azldev/agentskill/content/overlays.md.tmpl index 23b8b1121..579232226 100644 --- a/internal/app/azldev/agentskill/content/overlays.md.tmpl +++ b/internal/app/azldev/agentskill/content/overlays.md.tmpl @@ -128,7 +128,55 @@ the config loads, so a missing field fails fast rather than at apply time. references. Keep the spec `Name` aligned with the component instead. - To add a real `.patch` file (rather than an inline edit), use `patch-add`; it copies the `source` into the component sources and registers a `PatchN` tag or `%patchlist` - entry. + entry. It does not add application commands or change strip levels. After all + overlays, source preparation warns if an added patch's application cannot be + verified or its paths suggest a strip-level mismatch. Fix the final `%prep` with + a spec overlay; warnings do not prove that the patch applies. + Recognized unconditional selections take precedence over unrelated opaque flow. + An unconditional `%patch -P2202 -p1`, selected `%autopatch -p1 2202`, or understood + bulk `%autopatch -p1` / `%autosetup -p1` accounts for the added patch. Ordinary + preamble macros and unrelated surrounding commands do not invalidate selection. + Directory-name macros such as `-n %{name}-%{version}` are accepted without expansion; + selection, strip, and backend options must be literal. + Conditional branches and their side effects are skipped. Duplicate application + is not detected, even among understood commands. Known preceding unconditional + directory/control-flow changes and relevant helper redefinitions still affect a + candidate; later commands do not retroactively invalidate it. Another supported + candidate without a detected problem can establish selection. + Standard `%forgeautosetup` applies patches unless `-N`; `%goprep` applies them + only when `-p` is explicitly supplied. Custom layouts, source blocks, local + redefinitions, and Go vendor removal can remain unverified. + Relevant unconditional inline definitions and `%undefine` directives preceding + a command are tracked. This includes `__patch`, + `__scm_apply_patch`, and `_default_patch_flags`. Affected invocations and unresolved command arguments + stay unverified without macro evaluation. Disabled application ignores patch-helper + overrides, but not overridden setup dependencies. The corresponding + `__scm_apply_` helper is checked for each supported backend. Visible commands + beside an expansion are still checked. Macro-generated definitions and conditional + effects are not modeled. `_default_patch_fuzz` + alone does not warn because actual hunk applicability is outside scope. + Loaded macro files are not inspected or verified, whether azldev-generated or + user-supplied, even if later overlays change or remove them. Hidden patch-macro + and `__scm` overrides are unsupported; + assuming standard behavior can miss diagnostics or cause false unused-patch errors + and misleading strip warnings, including in strict mode. Review these cases manually. + Render exposes per-component `patchWarnings` with `-O json`: `filename`, + `patchNumber` (-1 when unknown; zero is explicit), optional `command`, and + human-readable `reason` (not a stable machine code). + Both parsers warn, rather than declaring a patch definitely unused, when no supported + unconditional application is found, including excluding manual/range commands and + setup-only flows. An unknown patch number can be selected by unrestricted bulk + application; numbered selections and limited ranges require a known number. + The whole prep is scanned; disabled setups and excluding + ranges do not hide later application commands. Conditional, repeated, and augmented + prep sections (`%prep -a` or `%prep -p`) warn as unsupported; do not blindly add + another application command. Uncertain application and likely strip mismatches + remain warnings. Use `component render --patch-warnings=error --fail-on-error` + to promote warnings to component errors and exit nonzero on component failures. + Without `--fail-on-error`, component errors can still exit zero. + JSON diagnostics are still emitted. `--check-only` checks output freshness, not + component success, and can pass when a failure marker is already current. + This does not prove that a CVE is fixed. ## Document intent with `metadata` diff --git a/internal/app/azldev/agentskill/content/withoutlockfile/overlays.md.tmpl b/internal/app/azldev/agentskill/content/withoutlockfile/overlays.md.tmpl index 2235eb559..74f398087 100644 --- a/internal/app/azldev/agentskill/content/withoutlockfile/overlays.md.tmpl +++ b/internal/app/azldev/agentskill/content/withoutlockfile/overlays.md.tmpl @@ -128,7 +128,55 @@ the config loads, so a missing field fails fast rather than at apply time. references. Keep the spec `Name` aligned with the component instead. - To add a real `.patch` file (rather than an inline edit), use `patch-add`; it copies the `source` into the component sources and registers a `PatchN` tag or `%patchlist` - entry. + entry. It does not add application commands or change strip levels. After all + overlays, source preparation warns if an added patch's application cannot be + verified or its paths suggest a strip-level mismatch. Fix the final `%prep` with + a spec overlay; warnings do not prove that the patch applies. + Recognized unconditional selections take precedence over unrelated opaque flow. + An unconditional `%patch -P2202 -p1`, selected `%autopatch -p1 2202`, or understood + bulk `%autopatch -p1` / `%autosetup -p1` accounts for the added patch. Ordinary + preamble macros and unrelated surrounding commands do not invalidate selection. + Directory-name macros such as `-n %{name}-%{version}` are accepted without expansion; + selection, strip, and backend options must be literal. + Conditional branches and their side effects are skipped. Duplicate application + is not detected, even among understood commands. Known preceding unconditional + directory/control-flow changes and relevant helper redefinitions still affect a + candidate; later commands do not retroactively invalidate it. Another supported + candidate without a detected problem can establish selection. + Standard `%forgeautosetup` applies patches unless `-N`; `%goprep` applies them + only when `-p` is explicitly supplied. Custom layouts, source blocks, local + redefinitions, and Go vendor removal can remain unverified. + Relevant unconditional inline definitions and `%undefine` directives preceding + a command are tracked. This includes `__patch`, + `__scm_apply_patch`, and `_default_patch_flags`. Affected invocations and unresolved command arguments + stay unverified without macro evaluation. Disabled application ignores patch-helper + overrides, but not overridden setup dependencies. The corresponding + `__scm_apply_` helper is checked for each supported backend. Visible commands + beside an expansion are still checked. Macro-generated definitions and conditional + effects are not modeled. `_default_patch_fuzz` + alone does not warn because actual hunk applicability is outside scope. + Loaded macro files are not inspected or verified, whether azldev-generated or + user-supplied, even if later overlays change or remove them. Hidden patch-macro + and `__scm` overrides are unsupported; + assuming standard behavior can miss diagnostics or cause false unused-patch errors + and misleading strip warnings, including in strict mode. Review these cases manually. + Render exposes per-component `patchWarnings` with `-O json`: `filename`, + `patchNumber` (-1 when unknown; zero is explicit), optional `command`, and + human-readable `reason` (not a stable machine code). + Both parsers warn, rather than declaring a patch definitely unused, when no supported + unconditional application is found, including excluding manual/range commands and + setup-only flows. An unknown patch number can be selected by unrestricted bulk + application; numbered selections and limited ranges require a known number. + The whole prep is scanned; disabled setups and excluding + ranges do not hide later application commands. Conditional, repeated, and augmented + prep sections (`%prep -a` or `%prep -p`) warn as unsupported; do not blindly add + another application command. Uncertain application and likely strip mismatches + remain warnings. Use `component render --patch-warnings=error --fail-on-error` + to promote warnings to component errors and exit nonzero on component failures. + Without `--fail-on-error`, component errors can still exit zero. + JSON diagnostics are still emitted. `--check-only` checks output freshness, not + component success, and can pass when a failure marker is already current. + This does not prove that a CVE is fixed. ## Document intent with `metadata` diff --git a/internal/app/azldev/cmds/component/render.go b/internal/app/azldev/cmds/component/render.go index afe1e95cc..b69adb8ab 100644 --- a/internal/app/azldev/cmds/component/render.go +++ b/internal/app/azldev/cmds/component/render.go @@ -39,6 +39,7 @@ type RenderOptions struct { CleanStale bool CheckOnly bool RPMDevBumpspec bool + PatchWarnings string } func renderOnAppInit(app *azldev.App, parentCmd *cobra.Command) { @@ -68,6 +69,13 @@ Unlike prepare-sources, render skips downloading source tarballs from the lookaside cache — only spec files, patches, scripts, and other git-tracked sidecar files are included. Multiple components can be rendered at once. +Added-patch application diagnostics are logged and included as patchWarnings +in -O json output. Missing unconditional patch selection, uncertain application, +and likely strip mismatches are warnings unless --patch-warnings=error promotes +them to component errors. Conditional branches and duplicate application are not checked. +Use --fail-on-error to exit nonzero on component failures. This is static +analysis, not proof a patch applies. + When rendering all components (-a), the --clean-stale flag prunes orphan rendered-spec directories (per-component dirs that no longer correspond to any component in the project config). Per-component dirs that ARE in config @@ -102,7 +110,12 @@ valid with -a.`, } addComponentFilterOptions(cmd, &options.ComponentFilter, newCmdOptions(opts...)) + addRenderFlags(cmd, &options) + + return cmd +} +func addRenderFlags(cmd *cobra.Command, options *RenderOptions) { cmd.Flags().StringVarP(&options.OutputDir, "output-dir", "o", "", "output directory for rendered specs (overrides rendered-specs-dir from config)") _ = cmd.MarkFlagDirname("output-dir") @@ -110,6 +123,14 @@ valid with -a.`, cmd.Flags().BoolVar(&options.FailOnError, "fail-on-error", false, "exit with error if any component fails to render (useful for CI)") + cmd.Flags().StringVar(&options.PatchWarnings, "patch-warnings", patchWarningsWarn, + "added-patch warning policy: warn or error "+ + "(error rejects affected components; use --fail-on-error for a nonzero exit)") + _ = cmd.RegisterFlagCompletionFunc("patch-warnings", + func(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) { + return []string{patchWarningsWarn, patchWarningsError}, cobra.ShellCompDirectiveNoFileComp + }) + cmd.Flags().BoolVarP(&options.Force, "force", "f", false, "allow overwriting existing rendered component directories") cmd.Flags().BoolVar(&options.CleanStale, "clean-stale", false, @@ -129,17 +150,16 @@ valid with -a.`, // muddled (CI would fail on stale failures even when on-disk markers // already record them) and forcing a choice keeps the contract crisp. cmd.MarkFlagsMutuallyExclusive("fail-on-error", "check-only") - - return cmd } // RenderResult holds the result of rendering a single component. type RenderResult struct { - Component string `json:"component" table:"Component"` - OutputDir string `json:"outputDir" table:"Output"` - Status string `json:"status" table:"Status"` - Error string `json:"error,omitempty" table:"Error,omitempty"` - Changed bool `json:"changed" table:"Changed"` + Component string `json:"component" table:"Component"` + OutputDir string `json:"outputDir" table:"Output"` + Status string `json:"status" table:"Status"` + Error string `json:"error,omitempty" table:"Error,omitempty"` + Changed bool `json:"changed" table:"Changed"` + PatchWarnings []spec.PatchApplicationDiagnostic `json:"patchWarnings,omitempty" table:"-"` } // Render status constants. @@ -147,6 +167,8 @@ const ( renderStatusOK = "ok" renderStatusError = "error" renderStatusCancelled = "cancelled" + patchWarningsWarn = "warn" + patchWarningsError = "error" ) // RenderComponents renders the post-overlay spec and sidecar files for each @@ -155,6 +177,10 @@ const ( // 2. Batch mock processing (rpmautospec + spectool in a single chroot call) // 3. Parallel finishing (filter files, remove .git, copy output) func RenderComponents(env *azldev.Env, options *RenderOptions) ([]*RenderResult, error) { + if err := validatePatchWarningPolicy(options.PatchWarnings); err != nil { + return nil, err + } + if err := resolveAndValidateOutputDir(env, options); err != nil { return nil, err } @@ -252,11 +278,26 @@ func RenderComponents(env *azldev.Env, options *RenderOptions) ([]*RenderResult, // Sort results alphabetically for consistent output. sortRenderResults(results) + return results, checkRenderOutcome(env.FS(), options, componentList, results) +} + +func checkRenderOutcome( + fs opctx.FS, options *RenderOptions, comps []components.Component, results []*RenderResult, +) error { if options.CheckOnly { - return results, checkOnlyRenderResult(env.FS(), options, componentList, results) + return checkOnlyRenderResult(fs, options, comps, results) } - return results, checkRenderErrors(results, options.FailOnError) + return checkRenderErrors(results, options.FailOnError) +} + +func validatePatchWarningPolicy(policy string) error { + switch policy { + case "", patchWarningsWarn, patchWarningsError: + return nil + default: + return fmt.Errorf("invalid '--patch-warnings' value %#q; expected 'warn' or 'error'", policy) + } } // checkOnlyRenderResult inspects results from a --check-only run and returns @@ -370,6 +411,7 @@ type preparedComponent struct { comp components.Component specFilename string // e.g., "curl.spec" compOutputDir string // validated output path computed in phase 1 + patchWarnings []spec.PatchApplicationDiagnostic } // prepResult pairs a prepared component (on success) or a render result (on error). @@ -414,7 +456,7 @@ func parallelPrepare( //nolint:contextcheck // env carries the ctx return prepareOneComponent( workerEnv, mockProcessor, comp, stagingDir, options.OutputDir, - options.specEditorMode(), options.RPMDevBumpspec) + options.specEditorMode(), options.RPMDevBumpspec, options.PatchWarnings) }, ) @@ -448,6 +490,30 @@ func parallelPrepare( return prepared } +func applyPatchDiagnosticPolicy( + ctx context.Context, componentName string, diagnostics []spec.PatchApplicationDiagnostic, policy string, +) error { + rejected := false + + for _, diagnostic := range diagnostics { + level := slog.LevelWarn + if diagnostic.BaseSeverity == spec.PatchApplicationError || policy == patchWarningsError { + level = slog.LevelError + rejected = true + } + + slog.Log(ctx, level, "Added patch application diagnostic", + "component", componentName, "patch", diagnostic.Filename, + "patchNumber", diagnostic.PatchNumber, "command", diagnostic.Command, "reason", diagnostic.Reason) + } + + if rejected { + return errors.New("added-patch application rejected; see 'patchWarnings' diagnostics") + } + + return nil +} + // prepareOneComponent validates the output path for a single component and // prepares its sources. Returns a [prepResult] carrying either a successful // preparedComponent or a [RenderResult] describing the error. @@ -463,6 +529,7 @@ func prepareOneComponent( outputDir string, specEditor spec.EditorMode, rpmDevBumpspec bool, + patchWarnings string, ) prepResult { componentName := comp.GetName() @@ -478,16 +545,25 @@ func prepareOneComponent( } prep, err := prepareComponentSources(env, mockProcessor, comp, stagingDir, specEditor, rpmDevBumpspec) + if prep != nil { + err = errors.Join(err, applyPatchDiagnosticPolicy(env, componentName, prep.patchWarnings, patchWarnings)) + } + if err != nil { slog.Error("Failed to prepare component sources", "component", componentName, "error", err) - return prepResult{result: &RenderResult{ + result := &RenderResult{ Component: componentName, OutputDir: compOutputDir, Status: renderStatusError, Error: err.Error(), - }} + } + if prep != nil { + result.PatchWarnings = prep.patchWarnings + } + + return prepResult{result: result} } prep.compOutputDir = compOutputDir @@ -540,6 +616,11 @@ func prepareComponentSources( sources.WithUpstreamProvenance(sources.FedoraDistTag(distro.Ref.Name, distro.Version.ReleaseVer)), sources.WithMockProcessor(mockProcessor), ) + prepared := &preparedComponent{comp: comp} + + preparerOpts = append(preparerOpts, sources.WithPatchDiagnostics(func(diagnostics []spec.PatchApplicationDiagnostic) { + prepared.patchWarnings = diagnostics + })) if rpmDevBumpspec { preparerOpts = append(preparerOpts, sources.WithRPMDevBumpspec(env, env.WorkDir(), "")) } @@ -553,19 +634,18 @@ func prepareComponentSources( } if prepErr := preparer.PrepareSources(env, comp, componentDir, true /*applyOverlays*/); prepErr != nil { - return nil, fmt.Errorf("preparing sources for %#q:\n%w", componentName, prepErr) + return prepared, fmt.Errorf("preparing sources for %#q:\n%w", componentName, prepErr) } // Find the spec file so we can pass the filename to mock. specPath, specErr := findSpecFile(env.FS(), componentDir, componentName) if specErr != nil { - return nil, fmt.Errorf("finding spec file for %#q:\n%w", componentName, specErr) + return prepared, fmt.Errorf("finding spec file for %#q:\n%w", componentName, specErr) } - return &preparedComponent{ - comp: comp, - specFilename: filepath.Base(specPath), - }, nil + prepared.specFilename = filepath.Base(specPath) + + return prepared, nil } // ────────────────────────────────────────────────────────────────────────────── @@ -661,10 +741,11 @@ func parallelFinish( case result.Cancelled: // Worker never started — ctx ended before parmap reached it. results[prep.index] = &RenderResult{ - Component: prep.comp.GetName(), - OutputDir: prep.compOutputDir, - Status: renderStatusCancelled, - Error: "context cancelled", + Component: prep.comp.GetName(), + OutputDir: prep.compOutputDir, + Status: renderStatusCancelled, + Error: "context cancelled", + PatchWarnings: prep.patchWarnings, } default: results[prep.index] = result.Value @@ -691,17 +772,19 @@ func finishOneComponent( // a Ctrl-C while the worker pool is draining. if workerEnv.Err() != nil { return &RenderResult{ - Component: componentName, - OutputDir: compOutputDir, - Status: renderStatusCancelled, - Error: "context cancelled", + Component: componentName, + OutputDir: compOutputDir, + Status: renderStatusCancelled, + Error: "context cancelled", + PatchWarnings: prep.patchWarnings, } } result := &RenderResult{ - Component: componentName, - OutputDir: compOutputDir, - Status: renderStatusOK, + Component: componentName, + OutputDir: compOutputDir, + Status: renderStatusOK, + PatchWarnings: prep.patchWarnings, } drifted, err := finishComponentRender(env, prep, mockResultMap, stagingDir, allowOverwrite, checkOnly) diff --git a/internal/app/azldev/cmds/component/render_patch_warnings_internal_test.go b/internal/app/azldev/cmds/component/render_patch_warnings_internal_test.go new file mode 100644 index 000000000..cddbbea51 --- /dev/null +++ b/internal/app/azldev/cmds/component/render_patch_warnings_internal_test.go @@ -0,0 +1,468 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package component + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io/fs" + "log/slog" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev" + "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/components" + "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/components/components_testutils" + "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/sources" + "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/testutils" + "github.com/microsoft/azure-linux-dev-tools/internal/global/opctx" + "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/rpm/mock" + "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" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" +) + +func TestRenderPatchWarningsJSONAndPolicy(t *testing.T) { + tests := []struct { + name string + args []string + prep string + resolved bool + mockError string + wantStatus string + wantError bool + wantFinding bool + }{ + { + name: "default warns for setup-only flow", prep: "%setup -q", wantStatus: renderStatusOK, + wantFinding: true, + }, + { + name: "setup-only warning does not fail with fail-on-error", args: []string{"--fail-on-error"}, + prep: "%setup -q", wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "default warns for excluded patch", prep: "%setup -q\n%patch -P1 -p1", + wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "excluded patch warning does not fail with fail-on-error", args: []string{"--fail-on-error"}, + prep: "%setup -q\n%patch -P1 -p1", wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "default warns for opaque macro", prep: "%setup -q\n%apply_fixes", + wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "explicit warn with fail-on-error still publishes", args: []string{"--patch-warnings=warn", "--fail-on-error"}, + prep: "%setup -q\n./prepare.sh", wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "strip mismatch warns by default", prep: "%autosetup -p0", + wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "strict rejects without command failure", args: []string{"--patch-warnings=error"}, + prep: "%setup -q", wantStatus: renderStatusError, wantFinding: true, + }, + { + name: "strict with fail-on-error", args: []string{"--patch-warnings=error", "--fail-on-error"}, + prep: "%setup -q", wantStatus: renderStatusError, wantError: true, wantFinding: true, + }, + { + name: "strict rejects uncertain application", args: []string{"--patch-warnings=error"}, + prep: "%autosetup -p%{strip}", wantStatus: renderStatusError, + wantFinding: true, + }, + { + name: "strict uncertainty fails with fail-on-error", args: []string{"--patch-warnings=error", "--fail-on-error"}, + prep: "%autosetup -p%{strip}", wantStatus: renderStatusError, wantError: true, + wantFinding: true, + }, + { + name: "strict rejects strip mismatch", args: []string{"--patch-warnings=error"}, + prep: "%autosetup -p0", wantStatus: renderStatusError, + wantFinding: true, + }, + { + name: "strict mismatch fails with fail-on-error", args: []string{"--patch-warnings=error", "--fail-on-error"}, + prep: "%autosetup -p0", wantStatus: renderStatusError, wantError: true, + wantFinding: true, + }, + { + name: "mismatch alone does not fail with fail-on-error", args: []string{"--fail-on-error"}, + prep: "%autosetup -p0", wantStatus: renderStatusOK, wantFinding: true, + }, + { + name: "strict allows resolved final overlay", args: []string{"--patch-warnings=error"}, + prep: "%setup -q", resolved: true, wantStatus: renderStatusOK, + }, + { + name: "strict recognizes forge", args: []string{"--patch-warnings=error"}, + prep: "%forgeautosetup -p1", wantStatus: renderStatusOK, + }, + { + name: "strict accepts explicit match despite opaque flow", + args: []string{"--patch-warnings=error", "--fail-on-error"}, + prep: strings.Join([]string{ + "%if %{with compat}", "%setup -q -n %{compat_source}", + "%autopatch -m%{compat}00 -M%{compat}99 -p1", "%endif", + "%autosetup -N -n %{main_source}", "%autopatch -M%{limit} -p1", "%patch -P0 -p1", + }, "\n"), + wantStatus: renderStatusOK, + }, + { + name: "strict accepts bulk match despite opaque flow", + args: []string{"--patch-warnings=error", "--fail-on-error"}, + prep: "%{gpgverify} --data=%{SOURCE0}\n%autosetup -n %{name}-%{version} -p1\nautoreconf -i", + wantStatus: renderStatusOK, + }, + { + name: "strict rejects excluded patch", args: []string{"--patch-warnings=error", "--fail-on-error"}, + prep: "%setup -q\n%patch -P1 -p1", wantStatus: renderStatusError, wantError: true, wantFinding: true, + }, + { + name: "strict recognizes go", args: []string{"--patch-warnings=error"}, + prep: "%goprep -p1", wantStatus: renderStatusOK, + }, + { + name: "mock failure retains warnings", prep: "%setup -q\n%apply_fixes", mockError: "mock failed", + wantStatus: renderStatusError, wantFinding: true, + }, + } + + for _, editor := range []string{specEditorLegacy, specEditorExperimental} { + t.Run(editor, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + addPatchWarningRenderComponent(t, testEnv, "test-patch", test.prep, test.resolved) + stubPatchWarningRenderMock(t, testEnv, test.mockError) + + var logs bytes.Buffer + + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + var report strings.Builder + + testEnv.Env.SetDefaultReportFormat(azldev.ReportFormatJSON) + testEnv.Env.SetReportFile(&report) + + cmd := newComponentCmd(&azldev.App{}) + cmd.SetArgs(append([]string{ + "render", "test-patch", "-o", "/rendered", "--spec-editor", editor, + }, test.args...)) + + err := cmd.ExecuteContext(testEnv.Env) + if test.wantError { + require.ErrorContains(t, err, "component(s) failed to render") + } else { + require.NoError(t, err) + } + + var results []RenderResult + require.NoError(t, json.Unmarshal([]byte(report.String()), &results), + "JSON results must still be emitted on strict failure") + require.Len(t, results, 1) + result := results[0] + assert.Equal(t, test.wantStatus, result.Status) + assert.Equal(t, "test-patch", result.Component) + + if !test.wantFinding { + assert.Empty(t, result.PatchWarnings) + assert.NotContains(t, report.String(), `"patchWarnings"`) + } else { + require.Len(t, result.PatchWarnings, 1) + diagnostic := result.PatchWarnings[0] + assert.Equal(t, "fix.patch", diagnostic.Filename) + assert.Equal(t, 0, diagnostic.PatchNumber) + + if strings.HasPrefix(test.prep, "%setup -q") { + assert.Empty(t, diagnostic.Command, "there is no recognized application command") + } else { + assert.Equal(t, test.prep, diagnostic.Command) + } + + assert.NotEmpty(t, diagnostic.Reason) + assert.Contains(t, report.String(), `"patchNumber": 0`) + assert.NotContains(t, report.String(), `"line":`) + assert.NotContains(t, report.String(), `"spec":`) + assert.NotContains(t, report.String(), `"code":`) + assert.Equal(t, 1, strings.Count(logs.String(), "Added patch application diagnostic")) + + level := "level=WARN" + if test.wantStatus == renderStatusError && test.mockError == "" { + level = "level=ERROR" + } + + assert.Contains(t, logs.String(), level+` msg="Added patch application diagnostic"`) + } + + specPath := filepath.Join(result.OutputDir, "test-patch.spec") + if test.wantStatus == renderStatusOK { + content, readErr := fileutils.ReadFile(testEnv.TestFS, specPath) + require.NoError(t, readErr) + assert.Contains(t, string(content), "\n"+test.prep+"\n") + assert.Contains(t, string(content), "fix.patch") + + if !test.resolved { + assert.Equal(t, strings.Count(test.prep, "%patch -P0"), + strings.Count(string(content), "%patch -P0"), + "analysis must not insert an application command") + } + } else { + exists, existsErr := fileutils.Exists(testEnv.TestFS, specPath) + require.NoError(t, existsErr) + assert.False(t, exists, "strict mode must not publish an unverified spec") + exists, existsErr = fileutils.Exists(testEnv.TestFS, + filepath.Join(result.OutputDir, renderErrorMarkerFile)) + require.NoError(t, existsErr) + assert.True(t, exists, "rejected components use the existing failure marker") + } + }) + } + }) + } +} + +func TestRenderPatchWarningsStrictCheckOnlyWithoutDrift(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + addPatchWarningRenderComponent(t, testEnv, "test-patch", "%setup -q", false) + + marker := "/rendered/t/test-patch/" + renderErrorMarkerFile + require.NoError(t, fileutils.WriteFile(testEnv.TestFS, marker, + []byte(renderErrorMarkerContent), fileperms.PublicFile)) + + results, err := RenderComponents(testEnv.Env, &RenderOptions{ + OutputDir: "/rendered", OutputDirExplicit: true, CheckOnly: true, PatchWarnings: patchWarningsError, + ComponentFilter: components.ComponentFilter{ComponentNamePatterns: []string{"test-patch"}}, + }) + require.NoError(t, err) + require.Len(t, results, 1) + assert.False(t, results[0].Changed, "a current failure marker satisfies check-only") + assert.Equal(t, renderStatusError, results[0].Status) + require.Len(t, results[0].PatchWarnings, 1) + + content, readErr := fileutils.ReadFile(testEnv.TestFS, marker) + require.NoError(t, readErr) + assert.Equal(t, renderErrorMarkerContent, string(content), "check-only must not write output") + assert.Empty(t, testEnv.CommandsExecuted, "strict rejection must happen before batch mock processing") +} + +func TestRenderPatchWarningsStrictWithoutAdditions(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + addPatchWarningRenderComponent(t, testEnv, "test-patch", "%setup -q", false) + config := testEnv.Config.Components["test-patch"] + config.Overlays = nil + testEnv.Config.Components["test-patch"] = config + stubPatchWarningRenderMock(t, testEnv, "") + + results, err := RenderComponents(testEnv.Env, &RenderOptions{ + OutputDir: "/rendered", OutputDirExplicit: true, PatchWarnings: patchWarningsError, + ComponentFilter: components.ComponentFilter{IncludeAllComponents: true}, + }) + require.NoError(t, err) + require.Len(t, results, 1) + assert.Equal(t, renderStatusOK, results[0].Status) + assert.Empty(t, results[0].PatchWarnings) +} + +func TestRenderPatchWarningsCheckOnlyDrift(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + addPatchWarningRenderComponent(t, testEnv, "test-patch", "%autosetup -p%{strip}", false) + + results, err := RenderComponents(testEnv.Env, &RenderOptions{ + OutputDir: "/rendered", OutputDirExplicit: true, CheckOnly: true, PatchWarnings: patchWarningsError, + ComponentFilter: components.ComponentFilter{ComponentNamePatterns: []string{"test-patch"}}, + }) + require.ErrorContains(t, err, "rendered output is stale") + require.Len(t, results, 1) + assert.Equal(t, renderStatusError, results[0].Status) + assert.True(t, results[0].Changed) + + exists, err := fileutils.Exists(testEnv.TestFS, "/rendered") + require.NoError(t, err) + assert.False(t, exists, "check-only must not create output or failure markers") + assert.Empty(t, testEnv.CommandsExecuted, "reject before batch RPM processing") +} + +func TestRenderPatchWarningsMixedComponents(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + addPatchWarningRenderComponent(t, testEnv, "warning", "%setup -q", false) + addPatchWarningRenderComponent(t, testEnv, "applied", "%forgeautosetup -p1", false) + stubPatchWarningRenderMock(t, testEnv, "") + + results, err := RenderComponents(testEnv.Env, &RenderOptions{ + OutputDir: "/rendered", OutputDirExplicit: true, PatchWarnings: patchWarningsError, + ComponentFilter: components.ComponentFilter{IncludeAllComponents: true}, + }) + require.NoError(t, err) + require.Len(t, results, 2) + assert.Equal(t, "applied", results[0].Component) + assert.Equal(t, renderStatusOK, results[0].Status) + assert.Empty(t, results[0].PatchWarnings) + assert.Equal(t, "warning", results[1].Component) + assert.Equal(t, renderStatusError, results[1].Status) + require.Len(t, results[1].PatchWarnings, 1) + assert.Contains(t, results[1].PatchWarnings[0].Reason, "no recognized command") +} + +func TestRenderPatchWarningsSurvivePreparationError(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + addPatchWarningRenderComponent(t, testEnv, "test-patch", "%setup -q", false) + + original := newSourcePreparer + newSourcePreparer = func( + manager sourceproviders.SourceManager, fileSystem opctx.FS, events opctx.EventListener, + dryRunnable opctx.DryRunnable, options ...sources.PreparerOption, + ) (sources.SourcePreparer, error) { + preparer, err := original(manager, fileSystem, events, dryRunnable, options...) + if err != nil { + return nil, err + } + + return &patchWarningFailingPreparer{SourcePreparer: preparer}, nil + } + + t.Cleanup(func() { newSourcePreparer = original }) + + results, err := RenderComponents(testEnv.Env, &RenderOptions{ + OutputDir: "/rendered", OutputDirExplicit: true, PatchWarnings: patchWarningsError, + ComponentFilter: components.ComponentFilter{IncludeAllComponents: true}, + }) + require.NoError(t, err) + require.Len(t, results, 1) + assert.Equal(t, renderStatusError, results[0].Status) + assert.Contains(t, results[0].Error, "failure after overlay analysis") + require.Len(t, results[0].PatchWarnings, 1) + assert.Contains(t, results[0].PatchWarnings[0].Reason, "no recognized command") +} + +type patchWarningFailingPreparer struct { + sources.SourcePreparer +} + +func (p *patchWarningFailingPreparer) PrepareSources( + ctx context.Context, comp components.Component, outputDir string, applyOverlays bool, +) error { + if err := p.SourcePreparer.PrepareSources(ctx, comp, outputDir, applyOverlays); err != nil { + return err + } + + return errors.New("failure after overlay analysis") +} + +func TestRenderPatchWarningsCancelledFinish(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + comp := components_testutils.NewMockComponent(gomock.NewController(t)) + comp.EXPECT().GetName().AnyTimes().Return("cancelled") + + warnings := []spec.PatchApplicationDiagnostic{{Filename: "fix.patch", Reason: "application cannot be verified"}} + prepared := &preparedComponent{comp: comp, compOutputDir: "/rendered/c/cancelled", patchWarnings: warnings} + cancelledEnv, cancel := testEnv.Env.WithCancel() + cancel() + + result := finishOneComponent(cancelledEnv, testEnv.Env, prepared, nil, "/staging", false, false) + assert.Equal(t, renderStatusCancelled, result.Status) + assert.Equal(t, warnings, result.PatchWarnings) + + results := make([]*RenderResult, 1) + parallelFinish(cancelledEnv, []*preparedComponent{prepared}, nil, results, "/staging", false, false) + require.NotNil(t, results[0]) + assert.Equal(t, renderStatusCancelled, results[0].Status) + assert.Equal(t, warnings, results[0].PatchWarnings) +} + +func addPatchWarningRenderComponent(t *testing.T, env *testutils.TestEnv, name, prep string, resolved bool) { + t.Helper() + + specPath := filepath.Join("/project", "specs", name, name+".spec") + lines := []string{ + "Name: " + name, "Version: 1", "Release: 1", "Summary: patch fixture", + "%description", "fixture", "%prep", prep, "%build", "make", + } + require.NoError(t, fileutils.WriteFile(env.TestFS, specPath, + []byte(strings.Join(lines, "\n")+"\n"), fileperms.PublicFile)) + require.NoError(t, fileutils.WriteFile(env.TestFS, "/project/fix.patch", + []byte("--- a/lib/file.c\n+++ b/lib/file.c\n"), fileperms.PublicFile)) + + overlays := []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlayAddPatch, Source: "/project/fix.patch", + }} + if resolved { + overlays = append(overlays, projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayAppendSpecLines, SectionName: "%prep", + Lines: []string{"%patch -P0 -p1"}, + }) + } + + env.Config.Components[name] = projectconfig.ComponentConfig{ + Name: name, Spec: projectconfig.SpecSource{SourceType: projectconfig.SpecSourceTypeLocal, Path: specPath}, + Build: projectconfig.ComponentBuildConfig{With: []string{"tests"}}, + Overlays: overlays, + } +} + +func stubPatchWarningRenderMock(t *testing.T, env *testutils.TestEnv, mockError string) { + t.Helper() + + env.CmdFactory.RegisterCommandInSearchPath(mock.MockBinary) + env.CmdFactory.RunHandler = func(cmd *exec.Cmd) error { + if !strings.Contains(strings.Join(cmd.Args, " "), "render_process.py") { + return nil + } + + return afero.Walk(env.TestFS, env.Env.WorkDir(), func(path string, info fs.FileInfo, walkErr error) error { + if walkErr != nil { + return walkErr + } + + if info.IsDir() || info.Name() != "inputs.json" { + return nil + } + + content, err := fileutils.ReadFile(env.TestFS, path) + if err != nil { + return err + } + + var inputs []struct { + Name string `json:"name"` + } + if err := json.Unmarshal(content, &inputs); err != nil { + return err + } + + var results []map[string]any + for _, input := range inputs { + result := map[string]any{"name": input.Name, "specFiles": "Patch0: fix.patch", "error": nil} + if mockError != "" { + result["error"] = mockError + } + + results = append(results, result) + } + + output, err := json.Marshal(results) + if err != nil { + return err + } + + return fileutils.WriteFile(env.TestFS, filepath.Join(filepath.Dir(path), "results.json"), + output, fileperms.PublicFile) + }) + } +} diff --git a/internal/app/azldev/cmds/component/render_test.go b/internal/app/azldev/cmds/component/render_test.go index 00a03def8..143971748 100644 --- a/internal/app/azldev/cmds/component/render_test.go +++ b/internal/app/azldev/cmds/component/render_test.go @@ -37,6 +37,11 @@ func TestNewRenderCmd_Flags(t *testing.T) { require.NotNil(t, failOnErrorFlag, "fail-on-error flag should be registered") assert.Equal(t, "false", failOnErrorFlag.DefValue) + patchWarningsFlag := cmd.Flags().Lookup("patch-warnings") + require.NotNil(t, patchWarningsFlag) + assert.Equal(t, "warn", patchWarningsFlag.DefValue) + assert.Contains(t, patchWarningsFlag.Usage, "use --fail-on-error for a nonzero exit") + forceFlag := cmd.Flags().Lookup("force") require.NotNil(t, forceFlag, "force flag should be registered") assert.Equal(t, "false", forceFlag.DefValue) @@ -57,6 +62,16 @@ func TestNewRenderCmd_Flags(t *testing.T) { assert.Nil(t, componentcmds.NewDiffSourcesCmd().Flags().Lookup("rpmdev-bumpspec")) } +func TestRenderCmd_InvalidPatchWarningPolicy(t *testing.T) { + testEnv := testutils.NewTestEnv(t) + cmd := componentcmds.NewRenderCmd() + cmd.SetArgs([]string{"--patch-warnings", "ignore", "-o", "SPECS", "-a"}) + + err := cmd.ExecuteContext(testEnv.Env) + require.ErrorContains(t, err, "invalid '--patch-warnings' value") + assert.Empty(t, testEnv.CommandsExecuted, "validate the policy before preparing sources") +} + func TestRenderCmd_NoComponents(t *testing.T) { testEnv := testutils.NewTestEnv(t) diff --git a/internal/app/azldev/core/sources/patch_application.go b/internal/app/azldev/core/sources/patch_application.go new file mode 100644 index 000000000..9ab1b1d4f --- /dev/null +++ b/internal/app/azldev/core/sources/patch_application.go @@ -0,0 +1,111 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package sources + +import ( + "bytes" + "errors" + "fmt" + "io" + "io/fs" + "log/slog" + + "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/fileutils" +) + +func (p *sourcePreparerImpl) inspectAddedPatchApplications( + componentName, sourcesDir, specPath string, overlays []projectconfig.ComponentOverlay, +) error { + additions := addedPatchOverlays(overlays) + if len(additions) == 0 { + return nil + } + + specContent, err := fileutils.ReadFile(p.fs, specPath) + if err != nil { + return fmt.Errorf("reading final spec %#q for patch diagnostics:\n%w", specPath, err) + } + + opened, err := spec.OpenSpec(bytes.NewReader(specContent), spec.WithEditor(p.specEditor)) + if err != nil { + return fmt.Errorf("opening final spec %#q for patch diagnostics:\n%w", specPath, err) + } + + destFS, err := newDestFS(p.fs, sourcesDir) + if err != nil { + return fmt.Errorf("opening sources directory for patch diagnostics:\n%w", err) + } + + defer func() { + if closer, ok := destFS.(io.Closer); ok { + _ = closer.Close() + } + }() + + patches := make([]spec.AddedPatch, 0, len(additions)) + for _, overlay := range additions { + name := overlay.EffectiveSourceName() + + content, readErr := fileutils.ReadFile(destFS, name) + if errors.Is(readErr, fs.ErrNotExist) && p.dryRunnable.DryRun() { + // Dry-run file copies leave only the original overlay input available. + content, readErr = fileutils.ReadFile(p.fs, overlay.Source) + } + + if readErr != nil && !errors.Is(readErr, fs.ErrNotExist) { + return fmt.Errorf("reading final patch %#q for diagnostics:\n%w", name, readErr) + } + + patches = append(patches, spec.AddedPatch{Filename: name, Content: content, FileMissing: readErr != nil}) + } + + diagnostics, err := opened.AnalyzePatchApplications(patches) + if err != nil { + return fmt.Errorf("checking added patch application in %#q:\n%w", specPath, err) + } + + p.reportPatchDiagnostics(componentName, specPath, diagnostics) + + return nil +} + +func (p *sourcePreparerImpl) reportPatchDiagnostics( + componentName, specPath string, diagnostics []spec.PatchApplicationDiagnostic, +) { + if p.patchDiagnostics != nil { + p.patchDiagnostics(diagnostics) + + return + } + + for _, diagnostic := range diagnostics { + slog.Warn("Added patch application could not be verified", + "component", componentName, "patch", diagnostic.Filename, + "patchNumber", diagnostic.PatchNumber, "spec", specPath, + "command", diagnostic.Command, + "reason", diagnostic.Reason) + } +} + +func addedPatchOverlays(overlays []projectconfig.ComponentOverlay) []projectconfig.ComponentOverlay { + var additions []projectconfig.ComponentOverlay + + seen := make(map[string]bool) + + for _, overlay := range overlays { + if overlay.Type != projectconfig.ComponentOverlayAddPatch { + continue + } + + name := overlay.EffectiveSourceName() + if !seen[name] { + additions = append(additions, overlay) + seen[name] = true + } + } + + return additions +} diff --git a/internal/app/azldev/core/sources/patch_application_internal_test.go b/internal/app/azldev/core/sources/patch_application_internal_test.go new file mode 100644 index 000000000..ded62323c --- /dev/null +++ b/internal/app/azldev/core/sources/patch_application_internal_test.go @@ -0,0 +1,304 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package sources + +import ( + "bytes" + "log/slog" + "path/filepath" + "strings" + "testing" + + "github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/components/components_testutils" + "github.com/microsoft/azure-linux-dev-tools/internal/global/testctx" + "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/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" +) + +func TestAddedPatchWarningsAfterAllOverlays(t *testing.T) { + tests := []struct { + name string + prep []string + later []projectconfig.ComponentOverlay + wantWarning string + }{ + {name: "missing application", prep: []string{"%setup -q"}, wantWarning: "no recognized command"}, + {name: "matching bulk application", prep: []string{"%autosetup -p1"}}, + {name: "matching forge wrapper", prep: []string{"%forgeautosetup -p1"}}, + {name: "matching go wrapper", prep: []string{"%goprep -p1"}}, + {name: "go setup without application", prep: []string{"%goprep"}, wantWarning: "no recognized command"}, + {name: "bare bulk nested patch", prep: []string{"%autosetup"}, wantWarning: "omits '-p'"}, + { + name: "later overlay adds application", prep: []string{"%setup -q"}, + later: []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlayAppendSpecLines, SectionName: "%prep", + Lines: []string{"%patch -P0 -p1"}, + }}, + }, + { + name: "later overlay fixes strip", prep: []string{"%autosetup"}, + later: []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlaySearchAndReplaceInSpec, SectionName: "%prep", + Regex: "^%autosetup$", Replacement: "%autosetup -p1", + }}, + }, + { + name: "later overlay removes patch", prep: []string{"%setup -q"}, + later: []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlayRemovePatch, Filename: "fix.patch", + }}, + }, + { + name: "later overlay removes file but leaves declaration", prep: []string{"%autosetup -p1"}, + later: []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlayRemoveFile, Filename: "fix.patch", + }}, + wantWarning: "final file is missing", + }, + { + name: "later overlay changes final patch content", prep: []string{"%autosetup"}, + later: []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlaySearchAndReplaceInFile, Filename: "fix.patch", + Regex: "[ab]/lib/", Replacement: "", + }}, + }, + { + name: "explicit strip conflict", prep: []string{"%autosetup -p2"}, + wantWarning: "uses '-p2'", + }, + { + name: "multiple directories", prep: []string{"%setup -q", "pushd lib", "%autopatch -p1", "popd"}, + wantWarning: "working directories", + }, + { + name: "conditional application", prep: []string{"%setup -q", "%ifarch x86_64", "%autopatch -p1", "%endif"}, + wantWarning: "conditional", + }, + } + for _, mode := range []spec.EditorMode{spec.EditorLegacy, spec.EditorStructural} { + t.Run(string(mode), func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := testctx.NewCtx() + require.NoError(t, ctx.FS().MkdirAll("/sources", fileperms.PublicDir)) + + lines := append([]string{"Name: test", "%prep"}, test.prep...) + lines = append(lines, "%build", "make") + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/sources/test.spec", + []byte(strings.Join(lines, "\n")+"\n"), fileperms.PublicFile)) + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/fix.patch", + []byte("--- a/lib/file.c\n+++ b/lib/file.c\n"), fileperms.PublicFile)) + + overlays := []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlayAddPatch, Source: "/fix.patch", + }} + overlays = append(overlays, test.later...) + component := components_testutils.NewMockComponent(gomock.NewController(t)) + component.EXPECT().GetName().AnyTimes().Return("test") + component.EXPECT().GetConfig().AnyTimes().Return(&projectconfig.ComponentConfig{Overlays: overlays}) + + preparer := &sourcePreparerImpl{ + fs: ctx.FS(), dryRunnable: ctx, eventListener: ctx, specEditor: mode, + } + + var ( + diagnostics []spec.PatchApplicationDiagnostic + reports int + ) + + WithPatchDiagnostics(func(final []spec.PatchApplicationDiagnostic) { + diagnostics = final + reports++ + })(preparer) + + var logs bytes.Buffer + + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + _, err := preparer.applyOverlays(component, "/sources", "") + require.NoError(t, err, "diagnostic warnings must not fail preparation") + assert.Equal(t, 1, reports, "report only the final overlay state") + + if test.wantWarning == "" { + assert.Empty(t, diagnostics) + assert.NotContains(t, logs.String(), "Added patch application") + } else { + require.Len(t, diagnostics, 1) + assert.Equal(t, "fix.patch", diagnostics[0].Filename) + assert.Equal(t, 0, diagnostics[0].PatchNumber) + assert.Contains(t, diagnostics[0].Reason, test.wantWarning) + } + + assert.Empty(t, logs.String(), "the diagnostic sink owns reporting") + }) + } + }) + } +} + +func TestPatchDiagnosticsOnlyInspectPatchAddOverlays(t *testing.T) { + ctx := testctx.NewCtx() + preparer := &sourcePreparerImpl{fs: ctx.FS(), dryRunnable: ctx} + require.NoError(t, preparer.inspectAddedPatchApplications("test", "/missing", "/missing/test.spec", + []projectconfig.ComponentOverlay{{Type: projectconfig.ComponentOverlayAddSpecTag, Tag: "Name", Value: "test"}})) +} + +func TestPatchDiagnosticsGeneratedMacroFiles(t *testing.T) { + tests := []struct { + name string + prep string + defines map[string]string + change bool + remove bool + reason string + wantError bool + }{ + { + name: "patch-named component retains literal numbering", prep: "%setup -q", + reason: "no recognized command", + }, + {name: "forge wrapper still recognized", prep: "%forgeautosetup -p1"}, + { + name: "configured wrapper redefinition is not inspected", prep: "%forgeautosetup -p1", + defines: map[string]string{"forgeautosetup": "echo skipped"}, + }, + { + name: "configured setup redefinition remains setup-only warning", prep: "%setup -q", + defines: map[string]string{"setup": "%autopatch -p1"}, + reason: "no recognized command", + }, + { + name: "configured backend is not inspected", prep: "%forgeautosetup", + defines: map[string]string{"__scm": "git"}, reason: "omits '-p'", + }, + {name: "changed generated file is not inspected", prep: "%forgeautosetup -p1", change: true}, + {name: "removed generated file is not inspected", prep: "%forgeautosetup -p1", remove: true}, + } + for _, mode := range []spec.EditorMode{spec.EditorLegacy, spec.EditorStructural} { + t.Run(string(mode), func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := testctx.NewCtx() + lines := []string{"Name: test-patch", "%prep", test.prep, "%build", "make"} + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/sources/test-patch.spec", + []byte(strings.Join(lines, "\n")+"\n"), fileperms.PublicFile)) + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/fix.patch", + []byte("--- a/lib/file.c\n+++ b/lib/file.c\n"), fileperms.PublicFile)) + + config := &projectconfig.ComponentConfig{ + Build: projectconfig.ComponentBuildConfig{With: []string{"patches"}, Defines: test.defines}, + Overlays: []projectconfig.ComponentOverlay{{ + Type: projectconfig.ComponentOverlayAddPatch, Source: "/fix.patch", + }}, + } + if test.change { + config.Overlays = append(config.Overlays, projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlaySearchAndReplaceInFile, + Filename: "test-patch.azl.macros", Regex: "_with_patches", Replacement: "_with_modified", + }) + } + + if test.remove { + config.Overlays = append(config.Overlays, projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayRemoveFile, Filename: "test-patch.azl.macros", + }) + } + + comp := components_testutils.NewMockComponent(gomock.NewController(t)) + comp.EXPECT().GetName().AnyTimes().Return("test-patch") + comp.EXPECT().GetConfig().AnyTimes().Return(config) + + var diagnostics []spec.PatchApplicationDiagnostic + + preparer := &sourcePreparerImpl{ + fs: ctx.FS(), dryRunnable: ctx, eventListener: ctx, specEditor: mode, + } + WithPatchDiagnostics(func(final []spec.PatchApplicationDiagnostic) { diagnostics = final })(preparer) + macroPath, err := preparer.writeMacrosFile(ctx, comp, "/sources") + require.NoError(t, err) + _, err = preparer.applyOverlays(comp, "/sources", filepath.Base(macroPath)) + require.NoError(t, err) + + if test.reason == "" { + assert.Empty(t, diagnostics) + } else { + require.Len(t, diagnostics, 1) + assert.Contains(t, diagnostics[0].Reason, test.reason) + assert.Equal(t, 0, diagnostics[0].PatchNumber) + assert.Equal(t, test.wantError, diagnostics[0].BaseSeverity == spec.PatchApplicationError) + } + }) + } + }) + } +} + +func TestPatchDiagnosticsReadErrors(t *testing.T) { + ctx := testctx.NewCtx() + preparer := &sourcePreparerImpl{fs: ctx.FS(), dryRunnable: ctx, specEditor: spec.EditorLegacy} + err := preparer.inspectAddedPatchApplications("test", "/sources", "/sources/test.spec", + []projectconfig.ComponentOverlay{{Type: projectconfig.ComponentOverlayAddPatch, Source: "/fix.patch"}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "reading final spec") +} + +func TestPatchDiagnosticsDryRunUsesOriginalPatch(t *testing.T) { + ctx := testctx.NewCtx() + ctx.DryRunValue = true + require.NoError(t, ctx.FS().MkdirAll("/sources", fileperms.PublicDir)) + + lines := []string{"Name: test", "Patch7: renamed.patch", "%prep", "%autosetup", ""} + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/sources/test.spec", + []byte(strings.Join(lines, "\n")), fileperms.PublicFile)) + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/fix.patch", + []byte("--- a/lib/file.c\n+++ b/lib/file.c\n"), fileperms.PublicFile)) + + preparer := &sourcePreparerImpl{fs: ctx.FS(), dryRunnable: ctx, specEditor: spec.EditorLegacy} + + var logs bytes.Buffer + + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + overlay := projectconfig.ComponentOverlay{ + Type: projectconfig.ComponentOverlayAddPatch, Source: "/fix.patch", Filename: "renamed.patch", + } + require.NoError(t, preparer.inspectAddedPatchApplications("test", "/sources", "/sources/test.spec", + []projectconfig.ComponentOverlay{overlay, overlay})) + assert.Equal(t, 1, strings.Count(logs.String(), "Added patch application")) + assert.Contains(t, logs.String(), "patch=renamed.patch") + assert.Contains(t, logs.String(), "patchNumber=7") + assert.NotContains(t, logs.String(), "line=") + assert.Contains(t, logs.String(), "command=%autosetup") + assert.Contains(t, logs.String(), "omits '-p'") + assert.NotContains(t, logs.String(), "cannot be read") +} + +func TestPatchDiagnosticsWithoutSinkRetainWarningBehavior(t *testing.T) { + ctx := testctx.NewCtx() + lines := []string{"Name: test", "Patch0: fix.patch", "%prep", "%setup -q"} + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/sources/test.spec", + []byte(strings.Join(lines, "\n")+"\n"), fileperms.PublicFile)) + require.NoError(t, fileutils.WriteFile(ctx.FS(), "/sources/fix.patch", nil, fileperms.PublicFile)) + preparer := &sourcePreparerImpl{fs: ctx.FS(), dryRunnable: ctx} + + var logs bytes.Buffer + + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, nil))) + t.Cleanup(func() { slog.SetDefault(previous) }) + require.NoError(t, preparer.inspectAddedPatchApplications("test", "/sources", "/sources/test.spec", + []projectconfig.ComponentOverlay{{Type: projectconfig.ComponentOverlayAddPatch, Source: "/fix.patch"}})) + assert.Equal(t, 1, strings.Count(logs.String(), "Added patch application")) + assert.Contains(t, logs.String(), "level=WARN") + assert.NotContains(t, logs.String(), "level=ERROR") +} diff --git a/internal/app/azldev/core/sources/sourceprep.go b/internal/app/azldev/core/sources/sourceprep.go index 4edeb30b6..58ef9de06 100644 --- a/internal/app/azldev/core/sources/sourceprep.go +++ b/internal/app/azldev/core/sources/sourceprep.go @@ -127,6 +127,13 @@ func WithSpecEditor(mode spec.EditorMode) PreparerOption { return func(p *sourcePreparerImpl) { p.specEditor = mode } } +// WithPatchDiagnostics reports added-patch diagnostics synchronously after all +// overlays. The sink owns logging and policy; without a sink, findings are logged +// as warnings. Use a separate sink for each preparer when preparing components concurrently. +func WithPatchDiagnostics(sink func([]spec.PatchApplicationDiagnostic)) PreparerOption { + return func(p *sourcePreparerImpl) { p.patchDiagnostics = sink } +} + // WithSkipLookaside returns a [PreparerOption] that skips all lookaside cache // downloads during source preparation. This includes both explicit source file // downloads ([SourceManager.FetchFiles]) and lookaside extraction during @@ -184,11 +191,12 @@ 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 + sourceManager sourceproviders.SourceManager + specEditor spec.EditorMode + fs opctx.FS + eventListener opctx.EventListener + dryRunnable opctx.DryRunnable + patchDiagnostics func([]spec.PatchApplicationDiagnostic) // withGitRepo, when true, enables dist-git creation by preserving the // upstream .git directory and generating synthetic commit history. @@ -432,6 +440,12 @@ func (p *sourcePreparerImpl) applyOverlays( return nil, fmt.Errorf("failed to apply overlays for component %#q:\n%w", component.GetName(), err) } + if err := p.inspectAddedPatchApplications( + component.GetName(), sourcesDirPath, absSpecPath, allOverlays, + ); err != nil { + return nil, err + } + return repackedArchives, nil } diff --git a/internal/rpm/spec/patch_application.go b/internal/rpm/spec/patch_application.go new file mode 100644 index 000000000..d95756c6a --- /dev/null +++ b/internal/rpm/spec/patch_application.go @@ -0,0 +1,1146 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec + +import ( + "fmt" + "slices" + "strconv" + "strings" + + "github.com/kballard/go-shellquote" +) + +// AddedPatch is an overlay-added patch, with its final contents. +type AddedPatch struct { + Filename string + Content []byte + FileMissing bool +} + +// PatchApplicationSeverity is the severity before render policy is applied. +type PatchApplicationSeverity uint8 + +const ( + // PatchApplicationWarning describes uncertainty or a likely path mismatch. + PatchApplicationWarning PatchApplicationSeverity = iota + // PatchApplicationError describes an unselected patch in an understood application flow. + PatchApplicationError +) + +// PatchApplicationDiagnostic describes a problem with an overlay-added patch. +// [PatchApplicationDiagnostic.PatchNumber] is -1 when numbering is unknown. +type PatchApplicationDiagnostic struct { + Filename string `json:"filename"` + PatchNumber int `json:"patchNumber"` + Command string `json:"command,omitempty"` + Reason string `json:"reason"` + BaseSeverity PatchApplicationSeverity `json:"-"` +} + +const ( + prepCommand = "%prep" + setupCommand = "%setup" + patchCommand = "%patch" + autosetupCommand = "%autosetup" + autopatchCommand = "%autopatch" + forgeautosetupCommand = "%forgeautosetup" + goprepCommand = "%goprep" + patchlistCommand = "%patchlist" + patchBackend = "patch" + gitBackend = "git" +) + +type patchDeclaration struct { + filename string + number int + conditional bool + numberUnknown bool +} + +type patchConsumer struct { + command string + macro string + numbers []int + minimum int + maximum int + strip int + backend string + disabled bool + unresolved string + selectionUncertain bool + removesVendor bool +} + +// Only unconditional application is analyzed. Execution uncertainty is captured +// on each consumer so later commands cannot invalidate an earlier selection. +type patchApplicationScanner struct { + // declarations holds patch tags and patchlist entries in document order. + declarations []patchDeclaration + // consumers holds unconditional setup/application commands with their captured uncertainty. + consumers []patchConsumer + // section is the current section keyword; empty denotes the main preamble. + section string + // depth tracks RPM conditional nesting; negative values indicate unmatched closing directives. + depth int + // nextNumber is one above the highest assigned number for a bare Patch tag or patchlist entry. + nextNumber int + // nextNumberUncertain marks automatic numbering affected by conditional or opaque declarations. + nextNumberUncertain bool + // prepCount counts encountered prep headers to detect missing or repeated preparation sections. + prepCount int + // flow explains opaque or conditional preparation when no usable selection is found. + flow string + // execution remembers shell/directory hazards affecting current and subsequent consumers. + execution string + // prep records structural or hook uncertainty that prevents accepting any preparation candidate. + prep string + // backend is the inherited application backend for subsequent setup and bulk commands. + backend string + // redefined tracks unconditional local macro changes seen before the current invocation. + redefined map[string]bool + // macro tracks delimiters and language state while consuming a macro body or expansion. + macro macroState + // macroBody indicates that a definition or expansion continues onto another physical line. + macroBody bool + // expansion distinguishes an expansion being collected from a definition whose body is skipped. + expansion bool + // expansionText buffers expansion lines until the complete expression can be inspected. + expansionText strings.Builder + // continuation remembers a trailing shell backslash for detecting continued application commands. + continuation bool +} + +// AnalyzePatchApplications inspects only added patches still declared in the +// final spec. Both editors share this lexical visitor. It does not evaluate +// conditional branches, detect duplicate application, evaluate macros, or inspect +// loaded macro files; standard RPM macro behavior is assumed. +func (s *Spec) AnalyzePatchApplications(patches []AddedPatch) ([]PatchApplicationDiagnostic, error) { + scanner := patchApplicationScanner{backend: patchBackend, redefined: make(map[string]bool)} + + if err := s.Visit(func(ctx *Context) error { + if ctx.RawLine != nil { + scanner.scanLine(*ctx.RawLine) + } + + return nil + }); err != nil { + return nil, fmt.Errorf("reading spec for patch application analysis:\n%w", err) + } + + if scanner.depth != 0 || scanner.macroBody { + scanner.prep = "unbalanced conditional structure or unterminated macro body makes application uncertain" + } + + var diagnostics []PatchApplicationDiagnostic + + for _, patch := range patches { + if diagnostic := scanner.check(patch); diagnostic != nil { + diagnostics = append(diagnostics, *diagnostic) + } + } + + return diagnostics, nil +} + +// scanLine routes a physical line to macro, conditional, section, declaration, +// or preparation handling without trusting the editor's section classification. +func (s *patchApplicationScanner) scanLine(raw string) { + line := strings.TrimSpace(raw) + if s.scanMacro(raw, line) { + return + } + + if line == "" || strings.HasPrefix(line, "#") { + if s.section == prepCommand && s.depth == 0 { + s.continuation = false + } + + return + } + + change := conditionalDepthChange(line) + if change != 0 || isConditionalBranchDirective(line) { + s.depth += change + if s.depth < 0 { + s.prep = "unbalanced conditional structure makes application uncertain" + } + + return + } + + token := strings.Fields(line)[0] + if _, known := sectionTypesByName[token]; known { + s.startSection(line, token) + + return + } + + switch { + case isTagBearingSection(s.section): + s.scanDeclaration(line) + case s.section == patchlistCommand: + s.addDeclaration(line, s.nextNumber, true) + case s.section == prepCommand: + s.scanPrep(raw, line, token) + } +} + +// startSection updates the section and resets command continuation, marking +// repeated, conditional, or augmented preparation sections as unsupported. +func (s *patchApplicationScanner) startSection(line, token string) { + if token == prepCommand { + s.prepCount++ + if s.prepCount != 1 || s.depth != 0 || line != prepCommand { + s.prep = "multiple, conditional, or nonstandard preparation sections cannot be resolved" + } + } + + s.section = token + s.continuation = false +} + +// scanMacro consumes definitions and expansions, keeping multiline macro bodies +// out of ordinary command parsing. It reports whether the line was consumed. +func (s *patchApplicationScanner) scanMacro(raw, line string) bool { + if s.macroBody { + s.macro, s.macroBody = macroBodyStateAfter(raw, s.macro) + if s.expansion { + // A shell backslash after a closed expansion is not a macro boundary. + s.macroBody = s.macro.open() + s.expansionText.WriteString(raw) + s.expansionText.WriteByte('\n') + + if !s.macroBody { + s.noteExpansion(s.expansionText.String()) + s.expansionText.Reset() + } + + s.scanContinuation(raw) + } + + return true + } + + name, definition := isMacroDefLine(line) + + fields := strings.Fields(line) + if len(fields) >= 2 && fields[0] == "%undefine" { + s.noteDefinition(fields[1]) + + return true + } + + state := macroStateAfter(raw, macroState{}) + if !definition && !strings.HasPrefix(line, "%{") && !state.open() { + return false + } + + s.expansion = !definition + + s.macro, s.macroBody = macroBodyStateAfter(raw, macroState{}) + if definition { + s.noteDefinition(name) + } else { + s.macroBody = s.macro.open() + s.expansionText.WriteString(raw) + s.expansionText.WriteByte('\n') + + if !s.macroBody { + s.noteExpansion(s.expansionText.String()) + s.expansionText.Reset() + } + + s.scanContinuation(raw) + } + + return true +} + +// noteDefinition records unconditional local macro changes that may affect later +// consumers; preparation hooks make the overall preparation flow unsupported. +func (s *patchApplicationScanner) noteDefinition(name string) { + if s.depth != 0 { + return + } + + s.redefined["%"+name] = true + if strings.HasPrefix(name, "__spec_prep_") { + s.prep = "custom preparation hooks cannot be resolved" + } +} + +// noteExpansion records numbering and fallback flow uncertainty without expanding +// macros, while checking visible prep shell context and ignoring loaded contents. +func (s *patchApplicationScanner) noteExpansion(line string) { + if s.depth != 0 { + return + } + + if s.section == prepCommand { + s.noteExecutionContext(withoutPatchExpansions(line, "%{")) + } + + // Loaded file contents are deliberately outside this lexical analysis. + // Other expansions are opaque, including conditional macro definitions. + line = withoutPatchExpansions(line, "%{load:") + if line == "" { + return + } + + if (isTagBearingSection(s.section) || s.section == patchlistCommand) && + strings.Contains(strings.ToLower(line), "patch") { + s.nextNumberUncertain = true + } + + if isTagBearingSection(s.section) || s.section == patchlistCommand || s.section == prepCommand { + s.flow = "macro-generated preparation flow cannot be resolved" + } +} + +// withoutPatchExpansions removes braced expansions matching opener, including +// nested contents, and returns the surrounding text without evaluating it. +func withoutPatchExpansions(line, opener string) string { + var visible strings.Builder + + for { + start := strings.Index(line, opener) + if start < 0 { + visible.WriteString(line) + + return strings.TrimSpace(visible.String()) + } + + visible.WriteString(line[:start]) + + state := macroState{} + + end := start + for end < len(line) { + closing := strings.IndexByte(line[end:], '}') + if closing < 0 { + end = len(line) + + break + } + + next := end + closing + 1 + state = macroStateAfter(line[end:next], state) + end = next + + if !state.open() { + break + } + } + + line = line[end:] + } +} + +// scanDeclaration reads a patch tag's filename and explicit or automatic number, +// retaining the filename even when a macro-based tag number cannot be resolved. +func (s *patchApplicationScanner) scanDeclaration(line string) { + tag, value, ok := parseTagLine(line) + if !ok || !strings.HasPrefix(strings.ToLower(tag), "patch") { + return + } + + number, numbered := ParsePatchTagNumber(tag) + switch { + case strings.EqualFold(tag, "patch"): + number = s.nextNumber + case !numbered: + s.declarations = append(s.declarations, patchDeclaration{ + filename: value, number: -1, conditional: s.depth != 0, numberUnknown: true, + }) + s.nextNumberUncertain = true + + return + } + + s.addDeclaration(value, number, strings.EqualFold(tag, "patch")) +} + +// addDeclaration stores a patch entry and advances automatic numbering, carrying +// uncertainty from conditional entries into subsequent implicit numbers. +func (s *patchApplicationScanner) addDeclaration(filename string, number int, implicit bool) { + declaration := patchDeclaration{ + filename: filename, number: number, conditional: s.depth != 0, + numberUnknown: implicit && s.nextNumberUncertain, + } + s.declarations = append(s.declarations, declaration) + s.nextNumber = max(s.nextNumber, number+1) + s.nextNumberUncertain = s.nextNumberUncertain || declaration.conditional +} + +// scanPrep skips conditional commands and collects unconditional consumers with +// their current backend, local redefinitions, continuation, and execution hazards. +func (s *patchApplicationScanner) scanPrep(raw, line, token string) { + if s.depth != 0 { + s.flow = "conditional patch application cannot be verified for every build configuration" + + return + } + + s.noteExecutionContext(raw) + + continued := s.scanContinuation(raw) + + if token != setupCommand && !isPatchConsumer(token) { + s.flow = "unrecognized preparation commands may apply patches" + + return + } + + consumer := parsePatchConsumer(line, s.backend) + if continued { + consumer.unresolved = "continued patch application commands cannot be resolved" + } + + if token == setupCommand || isPatchSetup(token) { + s.backend = consumer.backend + } + + if s.consumerRedefined(consumer) { + s.execution = "locally redefined patch application macro makes subsequent patch application uncertain" + } + + if s.setupRedefined(consumer) { + s.execution = "locally redefined source setup macro makes subsequent patch application uncertain" + } + + if s.execution != "" { + consumer.unresolved = s.execution + } + + s.consumers = append(s.consumers, consumer) +} + +// scanContinuation updates unconditional shell continuation from lexical escape +// state and reports whether this line continues from or into another line. +func (s *patchApplicationScanner) scanContinuation(raw string) bool { + previous := s.continuation + if s.depth == 0 { + _, _, s.continuation = patchShellContext(raw) + } + + return previous || s.continuation +} + +// noteExecutionContext remembers directory changes and shell control flow that +// make the current and subsequent consumers uncertain, without executing shell. +func (s *patchApplicationScanner) noteExecutionContext(line string) { + visible, operators, continuation := patchShellContext(line) + + // Physical-line continuations are tracked separately on the actual command. + if continuation { + visible = strings.TrimSuffix(visible, "\\") + } + + fields, err := shellquote.Split(visible) + if operators || err != nil { + s.execution = "shell control flow or here-document makes patch application uncertain" + + return + } + + if len(fields) == 0 { + return + } + + switch patchShellCommand(fields) { + case "cd", "pushd", "popd": + s.execution = "patch application uses multiple working directories; the target directory cannot be verified" + case "if", "for", "while", "until", "case", "then", "else", "elif", "fi", "done", "esac", + "function", "{", "(", "exit", "return", "exec", "break", "continue": + s.execution = "shell control flow makes patch application uncertain" + } +} + +// patchShellCommand finds the executed command after negation, assignments, and +// command/builtin prefixes. Lookup-only forms do not execute their operands. +func patchShellCommand(fields []string) string { + if len(fields) != 0 && fields[0] == "!" { + fields = fields[1:] + } + + for len(fields) != 0 { + if patchShellAssignment(fields[0]) { + fields = fields[1:] + + continue + } + + if fields[0] != "command" && fields[0] != "builtin" { + return fields[0] + } + + fields = fields[1:] + for len(fields) != 0 && strings.HasPrefix(fields[0], "-") { + option := fields[0] + fields = fields[1:] + + if option == "--" { + break + } + + if option != "-p" { + return "" + } + } + } + + return "" +} + +// patchShellAssignment recognizes portable shell variable-assignment prefixes, +// without treating arguments containing '=' as commands to execute. +func patchShellAssignment(word string) bool { + name, _, assignment := strings.Cut(word, "=") + + return assignment && name != "" && (name[0] < '0' || name[0] > '9') && + strings.Trim(name, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_") == "" +} + +// patchShellContext only distinguishes literal shell operators from quoted or +// commented text and active trailing escapes. It does not execute commands. +func patchShellContext(line string) (visible string, operators, continuation bool) { + var quote rune + + escaped, wordStart := false, true + + for index, char := range line { + switch { + case escaped: + escaped = false + wordStart = false + case quote == '\'': + if char == '\'' { + quote = 0 + } + case char == '\\': + escaped = true + case quote == '"': + if char == '"' { + quote = 0 + } + case strings.ContainsRune("'\"", char): + quote = char + wordStart = false + case char == '#' && wordStart: + return line[:index], operators, false + case strings.ContainsRune(";&|()", char): + operators = true + wordStart = true + case char == '<' && strings.HasPrefix(line[index:], "<<"): + operators = true + default: + wordStart = strings.ContainsRune(" \t\n", char) + } + } + + return line, operators, escaped +} + +// isPatchSetup identifies supported wrappers that combine source setup with +// optional patch application, rather than plain source extraction. +func isPatchSetup(token string) bool { + return token == autosetupCommand || token == forgeautosetupCommand || token == goprepCommand +} + +// isPatchConsumer recognizes supported application macros, including legacy +// '%patchN' forms with a literal non-negative number. +func isPatchConsumer(token string) bool { + if isPatchSetup(token) || token == autopatchCommand || token == patchCommand { + return true + } + + if !strings.HasPrefix(token, patchCommand) { + return false + } + + number, err := strconv.Atoi(strings.TrimPrefix(token, patchCommand)) + + return err == nil && number >= 0 +} + +// setupRedefined checks invoked source-setup dependencies even when patch +// application is disabled, since their shell effects can reach later commands. +func (s *patchApplicationScanner) setupRedefined(consumer patchConsumer) bool { + if consumer.macro != setupCommand && !isPatchSetup(consumer.macro) { + return false + } + + dependencies := []string{consumer.macro} + + if isPatchSetup(consumer.macro) { + dependencies = append(dependencies, setupCommand, autosetupCommand, "%__scm", "%__scm_setup_"+consumer.backend) + if consumer.backend == gitBackend { + dependencies = append(dependencies, "%__git") + } + } + + if consumer.macro == goprepCommand { + dependencies = append(dependencies, "%gomkdir") + } + + return slices.ContainsFunc(dependencies, func(name string) bool { return s.redefined[name] }) +} + +// consumerRedefined checks invoked setup/application dependencies for local +// changes whose shell effects can reach both this and subsequent consumers. +func (s *patchApplicationScanner) consumerRedefined(consumer patchConsumer) bool { + if s.setupRedefined(consumer) { + return true + } + + dependencies := []string{consumer.macro} + + if strings.HasPrefix(consumer.macro, patchCommand) && consumer.macro != patchCommand { + dependencies = append(dependencies, patchCommand) + } + + applies := !consumer.disabled + + if isPatchSetup(consumer.macro) || consumer.macro == autopatchCommand { + if !consumer.disabled { + dependencies = append(dependencies, autopatchCommand) + } + + // Bulk helpers are not invoked when no declaration can enter the selection. + applies = applies && slices.ContainsFunc(s.declarations, func(declaration patchDeclaration) bool { + return declaration.numberUnknown || consumer.selectionUncertain || consumer.selects(declaration.number) + }) + if applies { + dependencies = append(dependencies, "%__scm", "%__apply_patch", "%__scm_apply_"+consumer.backend) + if consumer.backend == gitBackend { + dependencies = append(dependencies, "%__git") + } + } + } + + if applies && consumer.backend != gitBackend { + dependencies = append(dependencies, "%__patch", "%_default_patch_flags") + } + + return slices.ContainsFunc(dependencies, func(name string) bool { return s.redefined[name] }) +} + +// parsePatchConsumer turns a recognized command into selection and application +// settings, applying command-specific defaults and retaining unsupported behavior. +func parsePatchConsumer(line, backend string) patchConsumer { + consumer := patchConsumer{command: line, minimum: 0, maximum: -1, strip: -1, backend: backend} + + visible, _, _ := patchShellContext(line) + + tokens, err := shellquote.Split(visible) + if err != nil { + consumer.unresolved = fmt.Sprintf("cannot parse patch command arguments: %s", err) + + return consumer + } + + consumer.macro = tokens[0] + consumer.removesVendor = consumer.macro == goprepCommand + + consumer.disabled = consumer.macro == setupCommand + if !isPatchSetup(consumer.macro) && consumer.macro != autopatchCommand && consumer.macro != setupCommand { + consumer.backend = patchBackend + if consumer.macro != patchCommand { + number, _ := strconv.Atoi(strings.TrimPrefix(consumer.macro, patchCommand)) + consumer.numbers = []int{number} + } + } + + consumer.parseArguments(tokens) + + if consumer.macro == patchCommand && len(consumer.numbers) == 0 { + consumer.numbers = []int{0} + } + + if consumer.macro == goprepCommand { + consumer.disabled = consumer.strip < 0 + } + + if consumer.macro != autopatchCommand && len(consumer.numbers) > 1 { + consumer.unresolved = "multiple individual patch selections cannot be resolved" + consumer.selectionUncertain = true + } + + if !knownPatchBackend(consumer.backend) { + consumer.unresolved = "custom patch backend cannot be resolved" + } + + return consumer +} + +// parseArguments dispatches positional selections, grouped flags, and options +// with attached or separate values, recording unsupported or incomplete syntax. +func (c *patchConsumer) parseArguments(tokens []string) { + values, flags := patchConsumerOptions(c.macro) + + for index := 1; index < len(tokens); index++ { + token := tokens[index] + if strings.HasPrefix(token, "#") { + break + } + + if !strings.HasPrefix(token, "-") { + c.parseSelection(token) + + continue + } + + if len(token) < 2 || !strings.ContainsRune(values, rune(token[1])) { + if len(token) < 2 || strings.Trim(token[1:], flags) != "" { + c.unresolved = "unsupported patch command option" + c.selectionUncertain = true + + break + } + + c.applyFlags(token[1:]) + + continue + } + + option, value := token[1], token[2:] + if value == "" { + index++ + if index == len(tokens) { + c.unresolved = "patch command option is missing its value" + c.selectionUncertain = true + + break + } + + value = tokens[index] + } + + c.applyOption(option, value) + } +} + +// parseSelection accepts literal non-negative positional patch numbers, rejecting +// positional setup arguments and selections whose number cannot be determined. +func (c *patchConsumer) parseSelection(token string) { + if c.macro == setupCommand || isPatchSetup(c.macro) { + c.unresolved = "unexpected positional setup arguments cannot be resolved" + c.selectionUncertain = true + + return + } + + number, err := strconv.Atoi(token) + if err != nil || number < 0 { + c.unresolved = "patch selection is not a literal non-negative number" + c.selectionUncertain = true + + return + } + + c.numbers = append(c.numbers, number) +} + +// patchConsumerOptions lists the supported value-taking options and valueless +// flags for each command; membership does not imply every setup mode is modeled. +func patchConsumerOptions(command string) (valueOptions, flagOptions string) { + switch command { + case setupCommand: + return "abn", "cCDTqv" + case autosetupCommand: + return "abnSp", "cCDTvN" + case forgeautosetupCommand: + return "zSp", "vN" + case goprepCommand: + return "zibsSp", "akervA" + case autopatchCommand: + return "pmM", "qv" + default: + return "pPbBzF", "EZ" + } +} + +// applyFlags handles disabled automatic patching and goprep's vendor/layout +// behavior; other accepted flags do not change the analyzed selection. +func (c *patchConsumer) applyFlags(flags string) { + if c.macro == autosetupCommand || c.macro == forgeautosetupCommand { + c.disabled = c.disabled || strings.Contains(flags, "N") + } + + if c.macro == goprepCommand { + if strings.ContainsAny(flags, "ae") { + c.unresolved = "multi-source or no-extraction goprep cannot be resolved" + } + + if strings.Contains(flags, "k") { + c.removesVendor = false + } + } +} + +// applyOption records literal selection, strip, and backend settings. Braced +// directory-name macros remain opaque; unsupported values retain uncertainty. +func (c *patchConsumer) applyOption(option byte, value string) { + if option == 'n' { + value = withoutPatchExpansions(value, "%{") + } + + if strings.ContainsAny(value, "%$;|&<>\\`") { + c.unresolved = "macro-valued or shell patch command options cannot be resolved" + c.selectionUncertain = c.selectionUncertain || strings.ContainsRune("PmM", rune(option)) + + return + } + + if c.customSetupOption(option) { + c.unresolved = "custom or multi-source setup cannot be resolved" + + return + } + + if option == 'S' { + c.backend = value + + return + } + + if !strings.ContainsRune("pPmM", rune(option)) { + return + } + + number, err := strconv.Atoi(value) + if err != nil || number < 0 { + c.unresolved = "macro-valued or invalid patch command options cannot be resolved" + c.selectionUncertain = c.selectionUncertain || strings.ContainsRune("PmM", rune(option)) + + return + } + + switch option { + case 'p': + c.strip = number + case 'P': + c.numbers = append(c.numbers, number) + case 'm': + c.minimum = number + case 'M': + c.maximum = number + } +} + +// customSetupOption identifies additional-source and wrapper-layout options whose +// effect on the target source tree is outside this analysis. +func (c *patchConsumer) customSetupOption(option byte) bool { + switch c.macro { + case forgeautosetupCommand: + return option == 'z' + case goprepCommand: + return strings.ContainsRune("zibs", rune(option)) + case setupCommand, autosetupCommand: + return strings.ContainsRune("ab", rune(option)) + default: + return false + } +} + +// selects checks an enabled, understood explicit list or inclusive range. An +// unknown patch number can match only an unrestricted bulk selection. +func (c *patchConsumer) selects(number int) bool { + if c.disabled || c.selectionUncertain { + return false + } + + if len(c.numbers) != 0 { + return slices.Contains(c.numbers, number) + } + + if number < 0 { + return c.minimum == 0 && c.maximum < 0 + } + + return number >= c.minimum && (c.maximum < 0 || number <= c.maximum) +} + +// check validates one added patch's remaining declarations and file presence +// before checking consumers; a removed declaration needs no diagnostic. +func (s *patchApplicationScanner) check(patch AddedPatch) *PatchApplicationDiagnostic { + var declarations []patchDeclaration + for _, declaration := range s.declarations { + if declaration.filename == patch.Filename { + declarations = append(declarations, declaration) + } + } + + if len(declarations) == 0 { + return nil + } + + diagnostic := &PatchApplicationDiagnostic{Filename: patch.Filename, PatchNumber: declarations[0].number} + if declarations[0].numberUnknown { + diagnostic.PatchNumber = -1 + } + + switch { + case patch.FileMissing: + diagnostic.Reason = "the patch is still declared, but its final file is missing" + case len(declarations) != 1 || declarations[0].conditional: + diagnostic.Reason = "multiple or conditional patch declarations make application uncertain" + default: + diagnostic.Reason = s.declarationConflict(patch.Filename, diagnostic.PatchNumber) + } + + if diagnostic.Reason != "" { + return diagnostic + } + + return s.checkConsumers(patch, diagnostic) +} + +// declarationConflict reports another filename sharing the added patch's known +// number, without treating unrelated unknown numbers as proven conflicts. +func (s *patchApplicationScanner) declarationConflict(filename string, number int) string { + for _, other := range s.declarations { + if other.filename == filename { + continue + } + + if !other.numberUnknown && number >= 0 && other.number == number { + return "the patch number is also used by another declaration" + } + } + + return "" +} + +// matchingConsumers gathers selecting commands, including candidates with +// application hazards, and fallback uncertainty when selection cannot be proven. +func (s *patchApplicationScanner) matchingConsumers( + number int, +) (selected []patchConsumer, unresolved patchConsumer) { + unresolved.unresolved = s.flow + + for _, consumer := range s.consumers { + if consumer.disabled { + continue + } + + if consumer.selects(number) { + selected = append(selected, consumer) + } else if consumer.unresolved != "" && consumer.selectionUncertain { + unresolved = consumer + } + } + + if len(selected) == 0 && s.execution != "" { + unresolved = patchConsumer{unresolved: s.execution} + } + + return selected, unresolved +} + +// checkConsumers accepts any selecting candidate without a detected problem, +// unless preparation is unsupported; otherwise it returns a targeted warning. +// Multiple selections are alternatives here, not duplicate-application findings. +func (s *patchApplicationScanner) checkConsumers( + patch AddedPatch, diagnostic *PatchApplicationDiagnostic, +) *PatchApplicationDiagnostic { + selected, unresolved := s.matchingConsumers(diagnostic.PatchNumber) + diagnostic.Command = unresolved.command + + switch { + case s.prep != "": + diagnostic.Reason = s.prep + case len(selected) != 0: + for _, consumer := range selected { + reason := consumer.unresolved + if reason == "" { + reason = likelyPatchStripMismatch(patch.Content, consumer) + } + + if reason == "" { + return nil + } + + if diagnostic.Reason == "" { + diagnostic.Command, diagnostic.Reason = consumer.command, reason + } + } + case s.prepCount == 0: + diagnostic.Reason = "no explicit preparation section was found; patch application cannot be verified" + case diagnostic.PatchNumber < 0: + diagnostic.Reason = "patch numbering is unknown; no unconditional command selecting all patches was found" + case unresolved.unresolved != "": + diagnostic.Reason = "no recognized command outside conditional branches selects this patch; " + unresolved.unresolved + default: + diagnostic.Reason = "no recognized command outside conditional branches selects this patch in '%prep'; " + + "verify the preparation flow before adding an application command" + } + + if diagnostic.Reason == "" { + return nil + } + + return diagnostic +} + +// knownPatchBackend identifies backends whose standard behavior is modeled. +func knownPatchBackend(backend string) bool { + return backend == patchBackend || backend == gitBackend || backend == "gendiff" +} + +// likelyPatchStripMismatch inspects diff header paths using the consumer's strip +// level and vendor policy; it neither validates diff contents nor applies hunks. +func likelyPatchStripMismatch(content []byte, consumer patchConsumer) string { + strip := consumer.strip + if strip < 0 && consumer.backend == gitBackend { + strip = 1 + } + + oldRemaining, newRemaining := 0, 0 + contextHunk := false + lines := strings.Split(string(content), "\n") + + for index, line := range lines { + if strings.HasPrefix(line, "***************") { + contextHunk = true + + continue + } + + if contextHunk { + if !patchFileBoundary(lines, index) { + continue + } + + contextHunk = false + } + + if strings.HasPrefix(line, "@@ ") { + oldRemaining, newRemaining = patchHunkLineCounts(line) + + continue + } + + if oldRemaining != 0 || newRemaining != 0 { + oldRemaining, newRemaining = advancePatchHunk(line, oldRemaining, newRemaining) + + continue + } + + if !strings.HasPrefix(line, "--- ") && !strings.HasPrefix(line, "+++ ") { + continue + } + + if reason := checkPatchHeaderPath(line[4:], strip, consumer.removesVendor); reason != "" { + return reason + } + } + + return "" +} + +// advancePatchHunk consumes a unified-diff body line from its remaining old/new +// counts, leaving metadata such as no-newline markers out of those counts. +func advancePatchHunk(line string, oldCount, newCount int) (oldRemaining, newRemaining int) { + switch { + case strings.HasPrefix(line, " "): + return oldCount - 1, newCount - 1 + case strings.HasPrefix(line, "-"): + return oldCount - 1, newCount + case strings.HasPrefix(line, "+"): + return oldCount, newCount - 1 + default: + return oldCount, newCount + } +} + +// patchFileBoundary recognizes the next file after a context hunk, distinguishing +// context range markers from file headers and allowing a following unified diff. +func patchFileBoundary(lines []string, index int) bool { + line := lines[index] + + return strings.HasPrefix(line, "diff ") || strings.HasPrefix(line, "Index: ") || + (strings.HasPrefix(line, "*** ") && !strings.HasSuffix(line, " ****")) || + (strings.HasPrefix(line, "--- ") && index+1 < len(lines) && strings.HasPrefix(lines[index+1], "+++ ")) +} + +// patchHunkLineCounts reads unified-diff range lengths so hunk contents cannot +// be mistaken for file headers. Malformed ranges remain opaque until the next hunk. +func patchHunkLineCounts(line string) (oldCount, newCount int) { + fields := strings.Fields(line) + if len(fields) < 4 || fields[0] != "@@" || fields[3] != "@@" { + return -1, -1 + } + + counts := [2]int{1, 1} + + for index, prefix := range []string{"-", "+"} { + rangeText, found := strings.CutPrefix(fields[index+1], prefix) + if !found { + return -1, -1 + } + + start, count, explicit := strings.Cut(rangeText, ",") + + startNumber, err := strconv.Atoi(start) + if err != nil || startNumber < 0 { + return -1, -1 + } + + if explicit { + counts[index], err = strconv.Atoi(count) + if err != nil || counts[index] < 0 { + return -1, -1 + } + } + } + + return counts[0], counts[1] +} + +// checkPatchHeaderPath decodes one diff-header filename and reports likely path +// stripping or vendor-removal problems, ignoring timestamps and '/dev/null'. +func checkPatchHeaderPath(header string, strip int, removesVendor bool) string { + filename, _, _ := strings.Cut(header, "\t") + if strings.HasPrefix(filename, "\"") { + unquoted, err := strconv.Unquote(filename) + if err != nil { + return "quoted patch paths cannot be resolved for strip verification" + } + + filename = unquoted + } + + if filename == "/dev/null" { + return "" + } + + gitPrefix := strings.HasPrefix(filename, "a/") || strings.HasPrefix(filename, "b/") + if gitPrefix && strip >= 0 && strip != 1 { + return fmt.Sprintf("patch paths appear to need '-p1', but the command uses '-p%d'; application may fail", strip) + } + + if strip > strings.Count(filename, "/") { + return fmt.Sprintf("the command uses '-p%d', which removes the entire patch path; application may fail", strip) + } + + if strip < 0 && strings.Contains(filename, "/") && (!gitPrefix || strings.Count(filename, "/") > 1) { + return "patch names subdirectory files, but the command omits '-p'; application may fail" + } + + if removesVendor && targetsTopLevelVendor(filename, strip) { + return "goprep removes vendor after applying patches unless '-k' is used" + } + + return "" +} + +// targetsTopLevelVendor checks whether stripping leaves 'vendor' as the first +// path component, where goprep's default vendor cleanup would affect it. +func targetsTopLevelVendor(filename string, strip int) bool { + parts := strings.Split(filename, "/") + + return strip >= 0 && strip < len(parts) && parts[strip] == "vendor" +} diff --git a/internal/rpm/spec/patch_application_test.go b/internal/rpm/spec/patch_application_test.go new file mode 100644 index 000000000..ca8dcc19c --- /dev/null +++ b/internal/rpm/spec/patch_application_test.go @@ -0,0 +1,1248 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +package spec_test + +import ( + "bytes" + "encoding/json" + "slices" + "strconv" + "strings" + "testing" + + "github.com/microsoft/azure-linux-dev-tools/internal/rpm/spec" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type patchApplicationTestCase struct { + name string + declarations []string + prep []string + content []string + number int + reason string + noPrep bool +} + +func TestAnalyzePatchApplications(t *testing.T) { + tests := []patchApplicationTestCase{ + {name: "setup only warns", prep: []string{"%setup -q"}, reason: "no recognized command"}, + {name: "empty prep warns", reason: "no recognized command"}, + {name: "no prep warns", noPrep: true, reason: "no explicit"}, + {name: "disabled setup warns", prep: []string{"%autosetup -N -p1"}, reason: "no recognized command"}, + {name: "disabled forge warns", prep: []string{"%forgeautosetup -N"}, reason: "no recognized command"}, + {name: "go setup only warns", prep: []string{"%goprep"}, reason: "no recognized command"}, + {name: "autosetup", prep: []string{"%autosetup -p1"}}, + {name: "forge", prep: []string{"%forgeautosetup -p1"}}, + {name: "go", prep: []string{"%goprep -p1"}}, + {name: "disabled then bulk", prep: []string{"%autosetup -N", "%autopatch -p1"}}, + {name: "manual", prep: []string{"%setup -q", "%patch -P0 -p1 -b .backup"}}, + {name: "legacy", prep: []string{"%patch0 -p1"}}, + {name: "positional", prep: []string{"%patch 0 -p1"}}, + {name: "implicit zero", prep: []string{"%patch -p1"}}, + {name: "literal selection with shell comment", prep: []string{"%patch0 -p1 # backport; see issue"}}, + {name: "literal selection with quote inside comment", prep: []string{"%patch0 -p1 # author's backport; see issue"}}, + {name: "selected autopatch", prep: []string{"%autopatch -p1 0 5"}}, + {name: "selected ignores range", prep: []string{"%autopatch -p1 -m5 0"}}, + {name: "inclusive range", prep: []string{"%autopatch -m0 -M0 -p1"}}, + {name: "range excludes", prep: []string{"%autopatch -m1 -p1"}, reason: "no recognized"}, + { + name: "upper range excludes", prep: []string{"%autopatch -M0 -p1"}, declarations: []string{"Patch1: fix.patch"}, + number: 1, reason: "no recognized", + }, + {name: "individual excludes", prep: []string{"%patch -P1 -p1"}, reason: "no recognized"}, + {name: "positional excludes", prep: []string{"%patch 1 -p1"}, reason: "no recognized"}, + {name: "legacy excludes", prep: []string{"%patch1 -p1"}, reason: "no recognized"}, + { + name: "implicit zero excludes", declarations: []string{"Patch1: fix.patch"}, + prep: []string{"%patch -p1"}, number: 1, reason: "no recognized", + }, + {name: "range excludes then individual matches", prep: []string{"%autopatch -m1 -p1", "%patch -P0 -p1"}}, + { + name: "different individual commands", declarations: []string{"Patch0: old.patch", "Patch1: fix.patch"}, + prep: []string{"%patch0 -p1", "%patch1 -p1"}, number: 1, + }, + {name: "duplicate individual ignored", prep: []string{"%patch0 -p1", "%patch -P0 -p1"}}, + {name: "bulk and individual duplicate ignored", prep: []string{"%autosetup -p1", "%patch0 -p1"}}, + {name: "macro range", prep: []string{"%autopatch -M%{limit} -p1"}, reason: "macro-valued"}, + {name: "macro strip", prep: []string{"%patch -P0 -p%{strip}"}, reason: "macro-valued"}, + {name: "macro named setup", prep: []string{"%autosetup -n %{name}-%{version} -p1"}}, + {name: "macro setup then explicit", prep: []string{"%autosetup -N -n %{name}", "%patch0 -p1"}}, + {name: "python blivet", prep: []string{ + "%autosetup -n %{realname}-%{realversion} -N", "%autosetup -n %{realname}-%{realversion} -b1 -p1", + }, reason: "multi-source"}, + {name: "multi-source setup", prep: []string{"%autosetup -b1 -p1"}, reason: "multi-source"}, + {name: "multiple setups", prep: []string{"%setup -q", "%autosetup -p1"}}, + } + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationDeclarations(t *testing.T) { + tests := []patchApplicationTestCase{ + {name: "conditional application", prep: []string{"%ifarch x86_64", "%patch0 -p1", "%endif"}, reason: "conditional"}, + { + name: "closed conditional before prep", + declarations: []string{"%ifarch x86_64", "Summary: test", "%endif", "Patch0: fix.patch"}, + prep: []string{"%patch0 -p1"}, + }, + { + name: "conditional prep", declarations: []string{"Patch0: fix.patch", "%ifarch x86_64"}, + prep: []string{"%patch0 -p1", "%endif"}, reason: "conditional", + }, + {name: "repeated prep", prep: []string{"%prep", "%patch0 -p1"}, reason: "multiple"}, + {name: "augmented prep", prep: []string{"%prep -a", "%patch0 -p1"}, reason: "nonstandard"}, + { + name: "uv unsupported prep does not advise duplicate application", + declarations: []string{"BuildSystem: pyproject", "Patch0: fix.patch"}, prep: []string{"%prep -a", "echo extra"}, + reason: "nonstandard", + }, + {name: "unbalanced conditional", prep: []string{"%ifarch x86_64", "%patch0 -p1"}, reason: "unbalanced"}, + { + name: "conditional declaration", declarations: []string{"%ifarch x86_64", "Patch0: fix.patch", "%endif"}, + prep: []string{"%patch0 -p1"}, reason: "conditional", + }, + { + name: "duplicate declaration number", declarations: []string{"Patch0: old.patch", "Patch0: fix.patch"}, + prep: []string{"%patch0 -p1"}, reason: "also used", + }, + { + name: "duplicate declaration filename", declarations: []string{"Patch0: fix.patch", "Patch1: fix.patch"}, + prep: []string{"%autopatch -p1"}, reason: "multiple", + }, + { + name: "unnumbered declaration", declarations: []string{"Patch4: old.patch", "Patch: fix.patch"}, + prep: []string{"%patch5 -p1"}, number: 5, + }, + { + name: "lower explicit number does not reset bare numbering", + declarations: []string{"Patch10: old.patch", "Patch1: other.patch", "Patch: fix.patch"}, + prep: []string{"%patch -P11 -p1"}, + }, + { + name: "lower explicit number does not reset patchlist numbering", + declarations: []string{"Patch10: old.patch", "Patch1: other.patch", "%patchlist", "fix.patch"}, + prep: []string{"%patch -P11 -p1"}, + }, + { + name: "lower explicit number cannot select reset automatic number", + declarations: []string{"Patch10: old.patch", "Patch1: other.patch", "%patchlist", "fix.patch"}, + prep: []string{"%patch -P2 -p1"}, number: 11, reason: "no recognized", + }, + { + name: "automatic entries continue above highest assigned number", + declarations: []string{"Patch10: old.patch", "Patch1: other.patch", "Patch: auto.patch", "%patchlist", "fix.patch"}, + prep: []string{"%patch -P12 -p1"}, + }, + { + name: "lower explicit number preserves conditional numbering uncertainty", + declarations: []string{"%ifarch x86_64", "Patch10: old.patch", "%endif", "Patch1: other.patch", "Patch: fix.patch"}, + prep: []string{"%patch -P2 -p1"}, number: -1, reason: "numbering is unknown", + }, + { + name: "lower explicit number preserves conditional patchlist uncertainty", + declarations: []string{ + "%ifarch x86_64", "Patch10: old.patch", "%endif", "Patch1: other.patch", "%patchlist", "fix.patch", + }, + prep: []string{"%patch -P11 -p1"}, number: -1, reason: "numbering is unknown", + }, + { + name: "explicit number preserves opaque numbering uncertainty", + declarations: []string{"Patch%{n}: old.patch", "Patch1: other.patch", "Patch: fix.patch"}, + prep: []string{"%patch -P2 -p1"}, number: -1, reason: "numbering is unknown", + }, + { + name: "conditional declaration makes later bare number uncertain", + declarations: []string{"%ifarch x86_64", "Patch: old.patch", "%endif", "Patch: fix.patch"}, + prep: []string{"%patch1 -p1"}, number: -1, reason: "numbering is unknown", + }, + { + name: "conditional declaration makes later patchlist number uncertain", + declarations: []string{"%ifarch x86_64", "Patch: old.patch", "%endif", "%patchlist", "fix.patch"}, + prep: []string{"%patch1 -p1"}, number: -1, reason: "numbering is unknown", + }, + { + name: "fixed declaration stays identifiable after conditional declarations", + declarations: []string{"%ifarch x86_64", "Patch: old.patch", "%endif", "Patch10: fix.patch"}, + prep: []string{"%patch10 -p1"}, number: 10, + }, + { + name: "unknown automatic number may collide with fixed target", + declarations: []string{ + "%ifarch x86_64", "Patch4: conditional.patch", "%endif", "Patch: auto.patch", "Patch0: fix.patch", + }, + prep: []string{"%patch0 -p1"}, + }, + { + name: "patchlist", declarations: []string{"Patch4: old.patch", "%patchlist", "# comment", "fix.patch"}, + prep: []string{"%patch5 -p1"}, number: 5, + }, + { + name: "macro numbering", declarations: []string{"Patch%{n}: old.patch", "Patch0: fix.patch"}, + prep: []string{"%patch0 -p1"}, + }, + { + name: "generated declaration", + declarations: []string{"%{lua:", "print('Patch4: old.patch')", "}", "Patch0: fix.patch"}, + prep: []string{"%patch0 -p1"}, + }, + } + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationLexicalBoundaries(t *testing.T) { + tests := []patchApplicationTestCase{ + {name: "opaque prep", prep: []string{"./prepare.sh"}, reason: "unrecognized"}, + { + name: "opaque manual patch loop", + prep: []string{"for p in %patches; do", "patch -p1 < $p", "done"}, reason: "shell", + }, + {name: "conditional expansion", prep: []string{"%{?apply:%patch0 -p1}"}, reason: "macro-generated"}, + {name: "inline patch call", prep: []string{"echo prepare; %patch0 -p1"}, reason: "shell control flow"}, + {name: "commented call", prep: []string{"# %patch0 -p1"}, reason: "no recognized"}, + { + name: "macro definition not a call", + prep: []string{"%define unused() \\", "%patch0 -p1", "%setup -q"}, reason: "no recognized", + }, + {name: "macro body skipped", prep: []string{"%define unused() %{expand:", "%patch0 -p2", "}", "%patch0 -p1"}}, + { + name: "inline expansion body is not an explicit call", + prep: []string{"echo %{expand:", "%patch0 -p1", "}"}, reason: "macro-generated", + }, + { + name: "inline nested expansion body is not an explicit call", + prep: []string{"echo %{?custom:%{expand:", "%patch0 -p1", "}}"}, reason: "macro-generated", + }, + { + name: "inline Lua expansion body is not an explicit call", + prep: []string{"echo %{lua:", "print('expanded')", "%patch0 -p1", "}"}, reason: "macro-generated", + }, + {name: "unterminated macro", prep: []string{"%define unused() %{expand:"}, reason: "unterminated"}, + { + name: "loaded macros ignored", declarations: []string{"%{load:external.macros}", "Patch0: fix.patch"}, + prep: []string{"%patch0 -p1"}, + }, + { + name: "multiline load boundary", declarations: []string{"%{load:", "external.macros", "}", "Patch0: fix.patch"}, + prep: []string{"%autopatch -p1"}, + }, + { + name: "load filename mentions patch", declarations: []string{"%{load:pkg-patch.macros}", "Patch0: fix.patch"}, + prep: []string{"%autopatch -p1"}, + }, + { + name: "load with nested macro filename", declarations: []string{"%{load:%{name}.macros}", "Patch0: fix.patch"}, + prep: []string{"%autopatch -p1"}, + }, + { + name: "visible expansion after load", + prep: []string{"%{load:external.macros}%{apply_fixes}"}, reason: "macro-generated", + }, + { + name: "inline override", declarations: []string{"%define patch() echo skipped", "Patch0: fix.patch"}, + prep: []string{"%patch -P0 -p1"}, reason: "redefined", + }, + { + name: "inline undefine", declarations: []string{"%undefine autopatch", "Patch0: fix.patch"}, + prep: []string{"%autopatch -p1"}, reason: "redefined", + }, + { + name: "conditional override", + declarations: []string{"%ifarch x86_64", "%define patch() echo skipped", "%endif", "Patch0: fix.patch"}, + prep: []string{"%patch -P0 -p1"}, + }, + { + name: "expansion override", declarations: []string{"%{?custom:%global __patch /bin/true}", "Patch0: fix.patch"}, + prep: []string{"%patch -P0 -p1"}, + }, + { + name: "custom backend definition", declarations: []string{"%global __scm git", "Patch0: fix.patch"}, + prep: []string{"%autosetup -p1"}, reason: "redefined", + }, + { + name: "patch helper override", declarations: []string{"%global __patch /bin/true", "Patch0: fix.patch"}, + prep: []string{"%patch -P0 -p1"}, reason: "redefined", + }, + { + name: "direct patch ignores bulk helper", + declarations: []string{"%global __scm_apply_patch /bin/true", "Patch0: fix.patch"}, + prep: []string{"%patch -P0 -p1"}, + }, + { + name: "git ignores patch helper", declarations: []string{"%global __patch /bin/true", "Patch0: fix.patch"}, + prep: []string{"%autosetup -S git -p1"}, + }, + { + name: "prep hook", declarations: []string{"%global __spec_prep_pre apply_fixes", "Patch0: fix.patch"}, + prep: []string{"%patch0 -p1"}, reason: "hooks", + }, + { + name: "macro setup override affects wrapper", declarations: []string{"%undefine autosetup", "Patch0: fix.patch"}, + prep: []string{"%forgeautosetup -p1"}, reason: "redefined", + }, + } + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationHelperDependencies(t *testing.T) { + tests := []patchApplicationTestCase{ + { + name: "bulk apply helper override", + prep: []string{"%global __apply_patch(qp:m:) echo skipped", "%autopatch -p1"}, reason: "redefined", + }, + { + name: "autosetup apply helper override", + prep: []string{"%undefine __apply_patch", "%autosetup -p1"}, reason: "redefined", + }, + { + name: "git bulk apply helper override", + prep: []string{"%undefine __apply_patch", "%autosetup -S git -p1"}, reason: "redefined", + }, + { + name: "forge apply helper override", + prep: []string{"%undefine __apply_patch", "%forgeautosetup -p1"}, reason: "redefined", + }, + { + name: "go apply helper override", + prep: []string{"%undefine __apply_patch", "%goprep -p1"}, reason: "redefined", + }, + { + name: "manual patch ignores bulk apply helper", + prep: []string{"%undefine __apply_patch", "%patch -P0 -p1"}, + }, + { + name: "patch backend setup helper override", + prep: []string{"%global __scm_setup_patch exit 0", "%autosetup -p1"}, reason: "redefined", + }, + { + name: "git backend setup helper override", + prep: []string{"%undefine __scm_setup_git", "%autosetup -S git -p1"}, reason: "redefined", + }, + { + name: "forge backend setup helper override", + prep: []string{"%undefine __scm_setup_patch", "%forgeautosetup -p1"}, reason: "redefined", + }, + { + name: "go backend setup helper override", + prep: []string{"%undefine __scm_setup_patch", "%goprep -p1"}, reason: "redefined", + }, + { + name: "manual patch ignores backend setup helper", + prep: []string{"%undefine __scm_setup_patch", "%patch0 -p1"}, + }, + { + name: "autopatch ignores backend setup helper", + prep: []string{"%undefine __scm_setup_patch", "%autopatch -p1"}, + }, + { + name: "patch backend ignores git setup helper", + prep: []string{"%undefine __scm_setup_git", "%autosetup -p1"}, + }, + { + name: "git backend ignores patch setup helper", + prep: []string{"%undefine __scm_setup_patch", "%autosetup -S git -p1"}, + }, + { + name: "unrelated backend apply helper ignored", + prep: []string{"%undefine __scm_apply_git", "%autosetup -p1"}, + }, + { + name: "unrelated helper ignored", + prep: []string{"%undefine __unrelated", "%autosetup -p1"}, + }, + { + name: "conditional bulk helper override ignored", + prep: []string{"%ifarch x86_64", "%undefine __apply_patch", "%endif", "%autopatch -p1"}, + }, + { + name: "conditional setup helper override ignored", + prep: []string{"%ifarch x86_64", "%undefine __scm_setup_patch", "%endif", "%autosetup -p1"}, + }, + { + name: "later bulk helper override does not invalidate selection", + prep: []string{"%autopatch -p1", "%undefine __apply_patch", "%autopatch -p1"}, + }, + { + name: "later setup helper override does not invalidate selection", + prep: []string{"%autosetup -p1", "%undefine __scm_setup_patch", "%autosetup -p1"}, + }, + { + name: "manual candidate remains uncertain after invoked bulk helper override", + prep: []string{"%undefine __apply_patch", "%autopatch -p1", "%patch0 -p1"}, reason: "redefined", + }, + } + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationHelperExecution(t *testing.T) { + tests := []patchApplicationTestCase{ + { + name: "bulk helper can terminate later application of a different patch", + prep: []string{"%global __apply_patch(qp:m:) exit 0", "%autopatch -p1 1", "%patch -P0 -p1"}, + reason: "redefined", + }, + { + name: "backend apply helper can terminate later application", + prep: []string{"%global __scm_apply_patch exit 0", "%autopatch -p1 1", "%patch0 -p1"}, + reason: "redefined", + }, + { + name: "patch executable override can terminate later application", + prep: []string{"%global __patch exit 0", "%patch1 -p1", "%patch0 -p1"}, + reason: "redefined", + }, + { + name: "patch flags override can terminate later application", + prep: []string{"%global _default_patch_flags ; exit 0 #", "%patch1 -p1", "%patch0 -p1"}, + reason: "redefined", + }, + { + name: "bulk macro override can terminate later application", + prep: []string{"%define autopatch() exit 0", "%autopatch -p1 1", "%patch0 -p1"}, + reason: "redefined", + }, + { + name: "manual macro override can terminate later bulk application", + prep: []string{"%define patch() exit 0", "%patch -P1 -p1", "%autopatch -p1 0"}, + reason: "redefined", + }, + { + name: "earlier sound manual candidate remains valid", + prep: []string{"%patch0 -p1", "%global __apply_patch(qp:m:) exit 0", "%autopatch -p1 1"}, + }, + { + name: "earlier sound bulk candidate remains valid", + prep: []string{"%autopatch -p1 0", "%global __apply_patch(qp:m:) exit 0", "%autopatch -p1 1"}, + }, + { + name: "uninvoked bulk helper does not affect manual candidates", + prep: []string{"%global __apply_patch(qp:m:) exit 0", "%patch1 -p1", "%patch0 -p1"}, + }, + { + name: "uninvoked backend helper does not affect manual candidates", + prep: []string{"%global __scm_apply_patch exit 0", "%patch1 -p1", "%patch0 -p1"}, + }, + { + name: "disabled application helper does not affect later manual candidate", + prep: []string{"%global __apply_patch(qp:m:) exit 0", "%autosetup -N", "%patch0 -p1"}, + }, + { + name: "disabled application macro does not affect later manual candidate", + prep: []string{"%define autopatch() exit 0", "%autosetup -N", "%patch0 -p1"}, + }, + { + name: "unrelated backend helper does not affect later manual candidate", + prep: []string{"%global __scm_apply_git exit 0", "%autopatch -p1 1", "%patch0 -p1"}, + }, + { + name: "empty bulk selection does not invoke application helpers", + prep: []string{"%global __apply_patch(qp:m:) exit 0", "%autopatch -p1 -m2", "%patch0 -p1"}, + }, + { + name: "empty bulk selection does not invoke patch executable", + prep: []string{"%global __patch exit 0", "%autopatch -p1 -m2", "%autosetup -S git -p1"}, + }, + { + name: "empty bulk selection still invokes overridden bulk macro", + prep: []string{"%define autopatch() exit 0", "%autopatch -p1 -m2", "%patch0 -p1"}, + reason: "redefined", + }, + { + name: "conditional helper invocation does not affect later manual candidate", + prep: []string{ + "%global __apply_patch(qp:m:) exit 0", "%ifarch x86_64", "%autopatch -p1 1", "%endif", "%patch0 -p1", + }, + }, + { + name: "conditional helper definition does not affect later candidates", + prep: []string{ + "%ifarch x86_64", "%global __apply_patch(qp:m:) exit 0", "%endif", "%autopatch -p1 1", "%patch0 -p1", + }, + }, + } + for index := range tests { + tests[index].declarations = []string{"Patch0: fix.patch", "Patch1: other.patch"} + } + + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationSetupExecution(t *testing.T) { + tests := []patchApplicationTestCase{ + { + name: "disabled setup helper can terminate later manual application", + prep: []string{ + "%global __scm_setup_patch(q) exit 0", "%autosetup -c -T -N", "%patch -P0 -p1", + }, + reason: "redefined source setup", + }, + { + name: "disabled setup helper can terminate later bulk application", + prep: []string{"%undefine __scm_setup_patch", "%autosetup -N", "%autopatch -p1"}, + reason: "redefined source setup", + }, + { + name: "plain setup override affects later application", + prep: []string{"%define setup() exit 0", "%setup -q", "%patch0 -p1"}, + reason: "redefined source setup", + }, + { + name: "autosetup override affects later application", + prep: []string{"%define autosetup() exit 0", "%autosetup -N", "%patch0 -p1"}, + reason: "redefined source setup", + }, + { + name: "forge setup override affects later application", + prep: []string{"%undefine __scm_setup_patch", "%forgeautosetup -N", "%patch0 -p1"}, + reason: "redefined source setup", + }, + { + name: "go setup override affects later application", + prep: []string{"%undefine gomkdir", "%goprep", "%patch0 -p1"}, + reason: "redefined source setup", + }, + { + name: "later setup cannot restore known execution", + prep: []string{"%undefine __scm_setup_patch", "%autosetup -N", "%setup -q", "%patch0 -p1"}, + reason: "redefined source setup", + }, + { + name: "earlier manual selection remains valid", + prep: []string{"%patch0 -p1", "%undefine __scm_setup_patch", "%autosetup -N", "%patch0 -p1"}, + }, + { + name: "uninvoked setup helper override is irrelevant", + prep: []string{"%undefine __scm_setup_patch", "%setup -q", "%patch0 -p1"}, + }, + { + name: "unrelated backend setup helper override is irrelevant", + prep: []string{"%undefine __scm_setup_git", "%autosetup -N", "%patch0 -p1"}, + }, + { + name: "disabled bulk helper override does not affect setup", + prep: []string{"%undefine __apply_patch", "%autosetup -N", "%patch0 -p1"}, + }, + { + name: "disabled backend apply override does not affect setup", + prep: []string{"%undefine __scm_apply_patch", "%autosetup -N", "%patch0 -p1"}, + }, + { + name: "conditional setup invocation does not affect later application", + prep: []string{ + "%undefine __scm_setup_patch", "%ifarch x86_64", "%autosetup -N", "%endif", "%patch0 -p1", + }, + }, + { + name: "conditional setup override does not affect later application", + prep: []string{ + "%ifarch x86_64", "%undefine __scm_setup_patch", "%endif", "%autosetup -N", "%patch0 -p1", + }, + }, + } + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationGitHelperDependencies(t *testing.T) { + tests := []patchApplicationTestCase{ + { + name: "git executable override affects autosetup", + prep: []string{"%global __git /bin/true", "%autosetup -c -T -S git -p1"}, reason: "redefined", + }, + { + name: "git executable override affects inherited bulk application", + prep: []string{"%autosetup -N -S git", "%global __git /bin/true", "%autopatch -p1"}, + reason: "redefined", + }, + { + name: "git executable override affects disabled source setup", + prep: []string{"%global __git /bin/true", "%autosetup -N -S git", "%patch0 -p1"}, + reason: "redefined source setup", + }, + { + name: "git executable override affects forge", + prep: []string{"%undefine __git", "%forgeautosetup -S git -p1"}, reason: "redefined", + }, + { + name: "git executable override does not affect patch autosetup", + prep: []string{"%global __git /bin/true", "%autosetup -p1"}, + }, + { + name: "git executable override does not affect patch autopatch", + prep: []string{"%global __git /bin/true", "%autopatch -p1"}, + }, + { + name: "git executable override does not affect plain setup", + prep: []string{"%global __git /bin/true", "%setup -q", "%patch0 -p1"}, + }, + { + name: "manual patch ignores inherited git backend", + prep: []string{"%autosetup -N -S git", "%global __git /bin/true", "%patch0 -p1"}, + }, + { + name: "conditional git executable override ignored", + prep: []string{"%ifarch x86_64", "%undefine __git", "%endif", "%autosetup -S git -p1"}, + }, + { + name: "earlier git selection remains valid", + prep: []string{"%autosetup -S git -p1", "%undefine __git", "%autosetup -S git -p1"}, + }, + { + name: "manual candidate remains uncertain after invoked git application override", + prep: []string{"%autosetup -N -S git", "%undefine __git", "%autopatch -p1", "%patch0 -p1"}, + reason: "redefined", + }, + } + runPatchApplicationTests(t, tests) +} + +func TestAnalyzePatchApplicationCommandsAndPaths(t *testing.T) { + tests := []patchApplicationTestCase{ + {name: "unknown option", prep: []string{"%patch -P0 --unknown"}, reason: "unsupported"}, + {name: "missing option value", prep: []string{"%patch -P"}, reason: "missing its value"}, + {name: "negative number", prep: []string{"%patch -P-1"}, reason: "invalid"}, + {name: "conflicting individual selections", prep: []string{"%patch1 -P2 -p1"}, reason: "multiple individual"}, + {name: "malformed quote", prep: []string{"%patch -P0 -p'1"}, reason: "shell control"}, + {name: "shell option value", prep: []string{"%patch -P0 -b`run`"}, reason: "shell"}, + {name: "continued command", prep: []string{"%patch -P0 \\", "-p1"}, reason: "continued"}, + {name: "unknown backend", prep: []string{"%autosetup -S custom"}, reason: "backend"}, + {name: "git default strip", prep: []string{"%autosetup -S git"}}, + {name: "backend carries to autopatch", prep: []string{"%autosetup -N -S git", "%autopatch"}}, + {name: "strip zero mismatch", prep: []string{"%patch0 -p0"}, reason: "uses '-p0'"}, + {name: "strip two mismatch", prep: []string{"%patch0 -p2"}, reason: "uses '-p2'"}, + {name: "omitted strip nested path", prep: []string{"%patch0"}, reason: "omits '-p'"}, + {name: "omitted strip root git path", prep: []string{"%patch0"}, content: []string{"--- a/file.c", "+++ b/file.c"}}, + { + name: "strip entire path", prep: []string{"%patch0 -p1"}, + content: []string{"--- file.c", "+++ file.c"}, reason: "entire", + }, + { + name: "plain paths strip zero", prep: []string{"%patch0 -p0"}, + content: []string{"--- lib/file.c", "+++ lib/file.c"}, + }, + { + name: "quoted path", prep: []string{"%patch0 -p1"}, + content: []string{`--- "a/lib/my file.c"`, `+++ "b/lib/my file.c"`}, + }, + {name: "new file", prep: []string{"%patch0 -p1"}, content: []string{"--- /dev/null", "+++ b/lib/file.c"}}, + { + name: "go vendor removed", prep: []string{"%goprep -p1"}, + content: []string{"--- a/vendor/lib/file.c", "+++ b/vendor/lib/file.c"}, + reason: "removes vendor", + }, + { + name: "go vendor kept", prep: []string{"%goprep -k -p1"}, + content: []string{"--- a/vendor/lib/file.c", "+++ b/vendor/lib/file.c"}, + }, + } + + runPatchApplicationTests(t, tests) +} + +func runPatchApplicationTests(t *testing.T, tests []patchApplicationTestCase) { + t.Helper() + + for _, mode := range []spec.EditorMode{spec.EditorLegacy, spec.EditorStructural} { + t.Run(string(mode), func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + declarations := test.declarations + if declarations == nil { + declarations = []string{"Patch0: fix.patch"} + } + + lines := slices.Concat([]string{"Name: test"}, declarations) + if !test.noPrep { + lines = append(lines, "%prep") + } + + lines = slices.Concat(lines, test.prep, []string{"%build", "Patch99: fix.patch", "%{?unused:echo patch}", "make"}) + input := strings.Join(lines, "\n") + "\n" + opened, err := spec.OpenSpec(strings.NewReader(input), spec.WithEditor(mode)) + require.NoError(t, err) + + content := test.content + if content == nil { + content = []string{"--- a/lib/file.c", "+++ b/lib/file.c"} + } + + diagnostics, err := opened.AnalyzePatchApplications([]spec.AddedPatch{{ + Filename: "fix.patch", Content: []byte(strings.Join(content, "\n") + "\n"), + }}) + require.NoError(t, err) + + if test.reason == "" { + assert.Empty(t, diagnostics) + } else { + require.Len(t, diagnostics, 1) + assert.Equal(t, "fix.patch", diagnostics[0].Filename) + assert.Equal(t, test.number, diagnostics[0].PatchNumber) + assert.Contains(t, diagnostics[0].Reason, test.reason) + assert.Equal(t, spec.PatchApplicationWarning, diagnostics[0].BaseSeverity) + } + + var serialized bytes.Buffer + require.NoError(t, opened.Serialize(&serialized)) + assert.Equal(t, input, serialized.String(), "analysis must be read-only") + }) + } + }) + } +} + +func TestAnalyzePatchApplicationsExplicitMatch(t *testing.T) { + llvm := []string{ + "%{?compat:%{expand:echo prepare}} \\", + "%if %{with bundle_compat_lib}", + "%setup -T -b3000 -n %{compat_source}", + "%autopatch -M499 -p1", + "%autopatch -m%{compat_maj_ver}00 -M%{compat_maj_ver}99 -p1", + "%endif", + "%autosetup -N -T -b0 -n %{main_source}", + "%autopatch -M499 -p1", + "%autopatch -m%{maj_ver}00 -M%{maj_ver}99 -p1", + } + + tests := []struct { + name string + prep []string + warn bool + }{ + {name: "LLVM safe explicit", prep: append(slices.Clone(llvm), "%patch -p1 -P2202")}, + {name: "LLVM without explicit", prep: llvm, warn: true}, + {name: "LLVM different patch", prep: append(slices.Clone(llvm), "%patch -p1 -P22020"), warn: true}, + { + name: "LLVM conditional explicit", + prep: slices.Concat(llvm, []string{"%ifarch x86_64", "%patch -p1 -P2202", "%endif"}), warn: true, + }, + {name: "LLVM unsafe strip", prep: append(slices.Clone(llvm), "%patch -p0 -P2202"), warn: true}, + {name: "explicit before opaque range", prep: []string{"%patch -P2202 -p1", "%autopatch -M%{limit} -p1"}}, + { + name: "quoted operators are not shell context", + prep: []string{`echo 'prepare; cd lib' "&& exit 0"`, "%patch2202 -p1"}, + }, + { + name: "escaped operators are not shell context", + prep: []string{`echo prepare\;`, "%patch2202 -p1"}, + }, + { + name: "commented operators are not shell context", + prep: []string{"echo prepare # cd lib; exit 0 '", "%patch2202 -p1"}, + }, + {name: "explicit after opaque range", prep: []string{"%autopatch -M%{limit} -p1", "%patch -P2202 -p1"}}, + {name: "explicit selected autopatch", prep: []string{"%autopatch -M%{limit}", "%autopatch -p1 2202"}}, + {name: "legacy selection", prep: []string{"%autopatch -M%{limit}", "%patch2202 -p1"}}, + {name: "positional selection", prep: []string{"%autopatch -M%{limit}", "%patch 2202 -p1"}}, + {name: "bulk overrides opaque selection", prep: []string{"%autopatch -M%{limit}", "%autopatch -p1"}}, + { + name: "multiple disabled setups do not veto explicit selection", + prep: []string{"%setup -q", "%autosetup -N", "%patch -P2202 -p1"}, + }, + { + name: "conditional block before safe command", + prep: []string{"%ifarch x86_64", "%patch0 -p1", "%endif", "%patch2202 -p1"}, + }, + { + name: "conditional override ignored", + prep: []string{"%ifarch x86_64", "%undefine patch", "%endif", "%patch -P2202 -p1"}, + }, + {name: "working directory remains uncertain", prep: []string{"cd lib", "%patch -P2202 -p1"}, warn: true}, + { + name: "directory in chained shell command remains uncertain", + prep: []string{"echo prepare; cd lib", "%patch2202 -p1"}, warn: true, + }, + { + name: "exit in chained shell command remains uncertain", + prep: []string{"echo prepare; exit 0", "%patch2202 -p1"}, warn: true, + }, + { + name: "directory with multiline expansion remains uncertain", + prep: []string{"cd %{expand:", "lib", "}", "%patch2202 -p1"}, warn: true, + }, + { + name: "directory after closed expansion remains uncertain", + prep: []string{"%{echo prepare}; cd lib", "%patch2202 -p1"}, warn: true, + }, + { + name: "conditional directory ignored", + prep: []string{"%ifarch x86_64", "cd lib", "%endif", "%patch -P2202 -p1"}, + }, + { + name: "conditional shell context ignored", + prep: []string{"%ifarch x86_64", "if test -d lib; then", "%endif", "%patch -P2202 -p1"}, + }, + { + name: "conditional here document ignored", + prep: []string{"%ifarch x86_64", "cat <