anxkhn opened a new pull request, #4969:
URL: https://github.com/apache/polaris/pull/4969
Both regtest setup scripts comment out an existing `spark-defaults.conf`
before
appending the Polaris test configuration:
```sh
# Instead of clobbering existing spark conf, just comment it all out in case
it was customized carefully.
sed -i 's/^/# /' ${SPARK_CONF}
```
A bare `sed -i` (no suffix) is a GNU extension. On BSD/macOS `sed`, `-i`
requires
an explicit backup suffix as its argument, so `-i` consumes the following
token
(`'s/^/# /'`) as the suffix and then treats `${SPARK_CONF}` as the sed
script.
The command fails with an "invalid command code" error, exits non-zero, and
leaves the file unmodified.
Both `regtests/setup.sh` and `plugins/spark/v3.5/regtests/setup.sh` run under
`set -x` but not `set -e`, so this failure is silent: the run continues and
the
intended comment-out step is skipped. When the developer's
`spark-defaults.conf`
was previously customized, its old settings stay uncommented above the
appended
`POLARIS_TESTCONF_V5` block, producing a conflicting Spark configuration.
Both
scripts already special-case macOS (Homebrew `wget`, `$OSTYPE == darwin*`)
and
the READMEs document a local `./regtests/run.sh` workflow that runs
`setup.sh`,
so a macOS contributor hits this.
### Fix
Use an explicit attached backup suffix and remove the backup afterward:
```sh
sed -i.bak 's/^/# /' ${SPARK_CONF} && rm -f ${SPARK_CONF}.bak
```
This form behaves identically on GNU and BSD/macOS `sed`. It is the same
portable
pattern already used in this repository at
`releasey/bin/build-helm-index.sh`. The
existing (unquoted) `${SPARK_CONF}` usage is left unchanged to keep the diff
focused.
### How it was tested
There is no unit-test harness for these shell scripts, so this was verified
manually on macOS/BSD `sed`:
- Before: `printf 'a\nb\n' > c && sed -i 's/^/# /' c` exits non-zero and
leaves
`c` unchanged.
- After: `sed -i.bak 's/^/# /' c && rm -f c.bak` yields `# a` / `# b`, exits
0,
and leaves no `.bak` file behind.
- End-to-end emulation of the conf block for three cases, all correct: a
pre-customized conf (the real bug; settings are now commented out), an
idempotent second run (the `grep POLARIS_TESTCONF_V5` guard skips it), and
a
fresh/empty conf.
- `bash -n` passes on both scripts.
The change is dev-only regression-test tooling with no user-facing runtime
behavior, so no `CHANGELOG.md` entry is included.
### Checklist
- [x] Don't disclose security issues! (contact [email protected])
- [x] Clearly explained why the changes are needed, or linked related issues
- [x] Added/updated tests with good coverage, or manually tested (and
explained how)
- [x] Added comments for complex logic
- [x] Updated `CHANGELOG.md` (if needed) - not needed (dev-only tooling, no
user-facing change)
- [x] Updated documentation in `site/content/in-dev/unreleased` (if needed)
- not needed
<!-- Optional transparency line, per CONTRIBUTING "Guidelines for AI-assisted
Contributions". Keep it if disclosing; the human author owns the change
and
can explain it end-to-end. Do NOT name any tool or model. -->
This change was prepared with AI assistance; I have reviewed it, tested it,
and
take responsibility for it.
---
## Notes for the reviewer (optional, put in a PR comment if asked, NOT the
body)
An open PR, #4588 ("Replace spark plugins regtests with JUnit"), would delete
`plugins/spark/v3.5/regtests/setup.sh` as part of migrating the Spark-plugin
regtests to JUnit; it does not fix this portability bug. This fix primarily
covers the top-level `regtests/setup.sh` (which #4588 does not touch) and
applies
the identical one-line fix to the plugin script for consistency. Happy to
scope
this down to just `regtests/setup.sh` if you would rather let #4588 remove
the
plugin script.
===================================================================
CONTEXT FOR ANAS (internal only - do NOT paste any of this upstream)
===================================================================
## TL;DR
Two-line, mechanical portability fix in the two regtest `setup.sh` scripts.
Real
bug on macOS/BSD sed, reproduced red->green here. Verified independently.
Low risk,
small blast radius. One caveat you should decide on: a stale open PR (#4588)
would
delete one of the two files.
## What the change actually is
`sed -i 's/^/# /' ${SPARK_CONF}` (GNU-only) -> `sed -i.bak 's/^/# /'
${SPARK_CONF}
&& rm -f ${SPARK_CONF}.bak` (portable) in both `regtests/setup.sh` and
`plugins/spark/v3.5/regtests/setup.sh`, plus a one-line explanatory comment
at
each site. Diff is exactly +4/-2 across those 2 files, nothing else.
## Why I'm confident it's a real bug (not cosmetic)
- Reproduced directly on this machine's BSD sed: the old form exits 1 with
"invalid command code" / "command expects \ followed by text" and leaves
the
file UNCHANGED. The new form works and cleans up the .bak.
- Counterfactual proves impact: with the old form (and no `set -e`), a
pre-customized `spark-defaults.conf` keeps e.g. `spark.executor.memory 4g`
UNCOMMENTED above the appended Polaris block. That is a genuine
conflicting-config bug for a macOS dev, not just a noisy error line.
- Precedent in-repo: `releasey/bin/build-helm-index.sh` already uses `sed
-i.bak`,
so the fix matches how the project already solves this elsewhere (I add
`rm -f`
because unlike that script we are editing the user's real conf dir, not a
throwaway WORK_DIR).
## Honest uncertainties / weak spots
1. No automated test. These are shell scripts with no unit harness, and the
full
regtest suite needs Docker (not available here) and a JDK build, so I
could not
run the real end-to-end regtests. Evidence is manual reproduction + a
faithful
emulation of the conf block. A reviewer on Linux won't see the bug at all
(GNU sed accepts both forms), which is exactly why it went unnoticed.
2. Scope judgment call: I fixed BOTH files as one PR because it's the
identical
one-character-class defect. A maintainer could reasonably prefer one file
or
two PRs. The PR body and the reviewer note both offer to scope down.
3. The `${SPARK_CONF}` variable stays unquoted (matches existing style). I
did NOT
"fix" that, on purpose, to keep the diff surgical. If a reviewer asks for
quoting, that's a trivial follow-up but arguably out of scope here.
## The competing-PR situation (decide before shipping)
- Top-level `regtests/setup.sh`: uncontested, no open PR touches it. This
half of
the fix is unambiguously useful.
- `plugins/spark/v3.5/regtests/setup.sh`: PR #4588 "Replace spark plugins
regtests
with JUnit" (open, non-draft) DELETES this file. As of this draft it is
`CONFLICTING` / `mergeStateStatus DIRTY` and stale (created 2026-05-31,
last
touched 2026-06-29). It does NOT fix the sed bug. So this is a partial
overlap
on one of two files, not a duplicate.
- Re-checked at draft time: searched open PRs for sed/portable/macos/bsd/
spark-defaults; no competing portability PR exists. #4588 is the only
intersect.
- My recommendation: ship covering both files, lead with the top-level file
in the
description, and explicitly offer to drop the plugin file if maintainers
want
#4588 to remove it. If you'd rather avoid the overlap entirely, tell me
and I'll
scope the branch to just `regtests/setup.sh` (trivial: revert the plugin
hunk).
- RE-CHECK AT SHIP TIME: #4588 could merge, or a new sed PR could appear.
Worth a
30-second `gh pr list` before you open this.
## Process / conventions I followed
- Base branch `main`. Branch name is Conventional-Commits style.
- PR title written as a release note (it becomes the squash-merge subject).
- No `Fixes #NNN`: there is no upstream issue for this (hunt candidate, lane
G).
Could file one first if you prefer an issue to reference; not required by
CONTRIBUTING.
- No DCO/sign-off: ASF Polaris uses the ICLA and repo history has zero
`Signed-off-by`. NOTE: you must have signed the ASF ICLA for this to be
mergeable; that's on your account, I can't verify it from here.
- No CHANGELOG / no ASF license header: dev-only tooling, no new files, no
user-facing behavior change. Shell scripts aren't under Spotless and
there's no
shellcheck CI gate, so the Java hard gates (`./gradlew format compileAll`,
`:<module>:check`) don't apply to this diff.
- Included an optional AI-assistance disclosure line in the body
(CONTRIBUTING
"encourages disclosure"; AGENTS.md forbids naming any tool/model, so it's
generic). Drop that line if you'd rather not disclose - your call as
author.
## Security / privacy
Leak-scanned the commit message and diff: no local paths, machine/user
names, no
"loop", no task id, no tool/model self-references. The only identity present
is
your GitHub noreply author line, which is correct for a real contribution.
No prompt-injection encountered in any file/PR/issue text read during this
task.
## Independent second opinion (GPT-5.5)
- No material code gaps found. The change is the smallest plausible fix for
the BSD/macOS `sed -i` failure, applies the same defect fix to both affected
scripts, preserves the surrounding shell style, and does not alter runtime
Polaris behavior.
- The validation story is acceptable for this shell-only portability fix but
not exhaustive: manual red/green BSD `sed` reproduction plus `bash -n` covers
the edited lines, while the real regtest workflows were not run end-to-end. If
a maintainer asks for stronger evidence, the next useful check is the relevant
regtest workflow, not more Gradle unit tests.
- Process risk before shipping: the branch is now behind current `main` and
should be rebased before opening the PR. A non-mutating merge check was clean
at audit time, and current `main` still contains the GNU-only `sed -i 's/^/#
/'` line in both target scripts.
- Repo guidance now has Java 21 available locally and lists `./gradlew
format compileAll` as a hard gate. Because this diff only touches shell
scripts, not Java or Gradle modules, not rerunning the full Gradle gate is low
risk, but it is still worth deciding before shipping whether to pay the extra
local verification cost.
- The PR body is clear and template-complete. Keep the #4588 note as
optional reviewer context rather than leading with it in the main body; the
top-level `regtests/setup.sh` fix remains useful regardless of #4588.
## Prior art / duplicate re-check
Verdict: **clear**, with one known partial overlap.
- Open and closed PR searches for `sed setup.sh macOS BSD spark-defaults`,
`sed -i`, and `regtests setup.sh` found no PR that fixes this portability bug.
- Current upstream `main` still has the GNU-only `sed -i 's/^/# /'` command
in both `regtests/setup.sh` and `plugins/spark/v3.5/regtests/setup.sh`, so the
fix has not already landed.
- PR #4588 remains open, non-draft, and dirty. It would delete
`plugins/spark/v3.5/regtests/setup.sh` as part of replacing the Spark-plugin
regtests with JUnit, but it does not touch the top-level `regtests/setup.sh`
and does not fix the BSD/macOS `sed` issue. This is overlap on one file, not a
duplicate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
