fix(patcher.mjs): merge process.env into the linter spawn env #868
Open
linzhp wants to merge 1 commit into
Open
fix(patcher.mjs): merge process.env into the linter spawn env #868linzhp wants to merge 1 commit into
process.env into the linter spawn env #868linzhp wants to merge 1 commit into
Conversation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

env: config.env || {}replaces the child's environment entirely — it does not extend the patcher's own env.config.envis whatever the rule passed viapatch_cfg_envinpatcher_action.bzl, which forclang_tidy_actionis just:i.e. no
PATH. The spawned linter therefore runs with an emptyPATH.Symptom
Any linter wrapper that relies on
PATHto resolve a binary breaks. The most visible case islint/clang_tidy_wrapper.bash, which usesmktemp,cat,eval,etc., and starts with
#!/usr/bin/env bash. With noPATH:/usr/bin/env bashmay fail to findbash.bashstarts,out_file=$(mktemp)returns empty, the redirection target becomes empty, and the wrapper can't write its capturedclang-tidyoutput.In
--fix/lint:fix=Truemode the failure is silent:JS_BINARY__SILENT_ON_SUCCESS=1swallows the wrapper's stderr, the patcher still runs the post-spawndiffstep (which finds no changes becauseclang-tidynever ran), and the output.patchfile ends up empty (0 bytes). To the caller it looks like "no fixesavailable."
Fix
Merge
process.envso anyPATH(and other env vars Bazel set for the action via--action_env, the default shell env, etc.) reach the linter, while stillletting
config.envoverride individual keys:This matches the behavior most callers expect —
config.envis treated as additions/overrides on top of the inherited environment, not as a hermetic replacement.Repro
Use
clang_tidyaspect with--@aspect_rules_lint//lint:fix=True --output_groups=rules_lint_patchon anycc_librarytarget. Before the fix, the produced.AspectRulesLintClangTidy.patchfile is 0 bytes and the wrapper's.outfile is also empty. After the fix, the wrapper actually invokesclang-tidy --fix, thesandbox source gets modified, and the patch contains a real unified diff.
A direct repro at the wrapper level: invoke
patcher.sh <patch_cfg.json>from the execroot withJS_BINARY__LOG_DEBUG=1. Before the fix, the spawned linter'sstderr (when reachable at all) shows shell errors complaining about missing commands; after the fix, you see normal
clang-tidyoutput.Notes
config.envto whatever they need —Object.assignlets explicit keys win over inheritedones.
JS_BINARY__*env vars are already expected to flow throughprocess.envfor thejs_binaryruntime.Test Plan
clang_tidyaspect in--fixmode produces a non-empty patch on a target with normal C++ compile flags (any target whose macro defines orcoptswouldotherwise fail the path-less wrapper).
config.envvalues still override conflicting keys inprocess.env(verified by a unit test or by setting an env key explicitly inpatch_cfg_env).config.env.Changes are visible to end-users: no