WIP: Automate Update Site verification#1397
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CI step that runs a new Eclipse upgrade test script which downloads/extracts Eclipse, installs a stable and an RC plugin via the Eclipse director, performs log/conflict checks, writes a structured report, and always uploads logs and the report as an artifact. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GitHubActions as "GitHub Actions (CI)"
participant Script as "releng/update-site-tests/test-update.sh"
participant Eclipse as "Eclipse (download / director / headless)"
participant ArtifactStore as "actions/upload-artifact"
Developer->>GitHubActions: push / open PR
GitHubActions->>Script: run test-update.sh (env: LOGDIR, REPORT_FILE)
Script->>Eclipse: download & extract Eclipse
Script->>Eclipse: install stable plugin (director) -> stable-install.log
Script->>Eclipse: install RC plugin (director) -> rc-install.log
Script->>Script: scan logs, list installed roots, create report.txt
Script->>Eclipse: headless startup check -> headless.log
Script->>GitHubActions: exit and place REPORT_FILE
GitHubActions->>ArtifactStore: upload logs + report (always)
ArtifactStore-->>GitHubActions: artifact stored
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 135-148: The script launches Eclipse with -vmargs
-Djava.awt.headless=false which will fail on headless CI; change the invocation
to either run under Xvfb (wrap "$ECLIPSE_HOME/eclipse" with xvfb-run) or set
-Djava.awt.headless=true, and replace the fragile sleep+kill logic that uses PID
and sleep 20 with a timeout utility (e.g., use timeout to limit launch duration)
and add a robust startup check (verify $PID is running and/or scan
"$LOGDIR/eclipse-launch.log" for a successful startup marker) so the test
reliably times out or confirms a running process instead of blindly killing
after 20s.
- Around line 1-2: Add `set -o pipefail` after the existing `set -e` in
test-update.sh to ensure pipe failures aren't masked, then update the two piped
commands guarded by `PIPESTATUS` (the two places currently checking
PIPESTATUS[0], referenced in the comment as lines with PIPESTATUS checks) so
they won't be prematurely aborted: either temporarily disable `set -e` around
each pipeline or append `|| true` to the pipeline and continue to use the
existing `PIPESTATUS` checks to detect failures; ensure you apply the same
pattern to the RC install pipeline as well so both pipelines reliably surface
errors.
- Around line 111-115: Change the broad grep that searches for "conflict" in
$LOGDIR/*.log to first ensure there are log files (e.g., test if any
"$LOGDIR"/*.log exist and skip check if none), then grep only for p2-specific
error patterns (use grep -iqE -- 'Cannot satisfy|conflicting
requirements|Conflicts detected|Resolution failed' "$LOGDIR"/*.log) so you avoid
benign matches; if grep returns success, print the failure message and exit 1.
Ensure you reference LOGDIR and replace the existing grep -iq "conflict"
"$LOGDIR"/*.log block with this guarded, pattern-specific check.
🧹 Nitpick comments (4)
releng/update-site-tests/test-update.sh (3)
8-18: Hardcoded URLs and versions limit the stated goal of testing against multiple IDE/Eclipse versions.The PR objective is to verify compatibility "against previous Espressif-IDE versions and recent Eclipse CDT releases," but the script pins a single Eclipse release (
2025-09) and a single stable plugin version (v4.0.0). Consider parameterizing these as script arguments or environment variables so the CI matrix (or future callers) can iterate over multiple version combinations.Example
-ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz" -STABLE_ZIP_URL="https://dl.espressif.com/dl/idf-eclipse-plugin/updates/com.espressif.idf.update-v4.0.0.zip" -RC_REPO="https://dl.espressif.com/dl/idf-eclipse-plugin/updates/latest/" +ECLIPSE_URL="${ECLIPSE_URL:?Please set ECLIPSE_URL}" +STABLE_ZIP_URL="${STABLE_ZIP_URL:?Please set STABLE_ZIP_URL}" +RC_REPO="${RC_REPO:?Please set RC_REPO}"
27-27: Dead assignment —ECLIPSE_HOMEis overwritten on line 36.Line 27 sets
ECLIPSE_HOME="$WORKDIR/eclipse", but line 36 immediately reassigns it viafind. Remove the line 27 assignment to avoid confusion.
33-36: No integrity verification of downloaded artifacts.
wgetdownloads over HTTPS but there's no checksum verification for the Eclipse tarball or the plugin zip. A corrupted or tampered download would produce confusing failures downstream. Consider adding SHA-256 checks..github/workflows/ci.yml (1)
49-55: Consider whether update site test failures should block the entire CI pipeline.The test downloads external resources (
ftp.osuosl.org,dl.espressif.com) that are outside your control. If either host is temporarily unavailable, the build will fail. Consider addingcontinue-on-error: trueif this test is intended as informational, or at minimum add retry logic in the shell script for thewgetcalls.
| #!/bin/bash | ||
| set -e |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add set -o pipefail to avoid silently masking pipe failures.
set -e alone does not propagate failures from the left-hand side of a pipe. The PIPESTATUS[0] checks on lines 73 and 100 partially compensate, but any unexpected pipe failure (e.g., tee itself failing) will still be masked. Adding pipefail makes the intent explicit and catches additional failure modes. Note: with pipefail enabled, set -e will abort on a non-zero eclipse exit before reaching the PIPESTATUS check, so you'd need to temporarily disable set -e or use || true around the pipe and rely solely on PIPESTATUS.
Suggested approach
#!/bin/bash
-set -e
+set -euo pipefailThen guard the piped commands so the PIPESTATUS check still works:
-"$ECLIPSE_HOME/eclipse" \
+set +e
+"$ECLIPSE_HOME/eclipse" \
...
| tee "$LOGDIR/stable-install.log"
STABLE_EXIT=${PIPESTATUS[0]}
+set -e
if [ $STABLE_EXIT -ne 0 ]; thenApply the same pattern around the RC install pipe.
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` around lines 1 - 2, Add `set -o
pipefail` after the existing `set -e` in test-update.sh to ensure pipe failures
aren't masked, then update the two piped commands guarded by `PIPESTATUS` (the
two places currently checking PIPESTATUS[0], referenced in the comment as lines
with PIPESTATUS checks) so they won't be prematurely aborted: either temporarily
disable `set -e` around each pipeline or append `|| true` to the pipeline and
continue to use the existing `PIPESTATUS` checks to detect failures; ensure you
apply the same pattern to the RC install pipeline as well so both pipelines
reliably surface errors.
| echo "Checking logs for conflicts..." | ||
| if grep -iq "conflict" "$LOGDIR"/*.log; then | ||
| echo "❌ Conflict detected in installation logs!" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
grep "conflict" is overly broad and may produce false positives or false negatives.
The word "conflict" could appear in benign log messages (e.g., "No conflicts found", class names containing "conflict"). Consider grepping for p2-specific conflict markers or error patterns instead. Also, if no .log files exist the glob will fail under set -e.
Suggestion
-if grep -iq "conflict" "$LOGDIR"/*.log; then
+shopt -s nullglob
+if grep -iqP '(?<!no )conflicts?\b' "$LOGDIR"/*.log; then
echo "❌ Conflict detected in installation logs!"
exit 1
fiOr better, grep for the specific p2 error messages that indicate real conflicts.
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` around lines 111 - 115, Change the
broad grep that searches for "conflict" in $LOGDIR/*.log to first ensure there
are log files (e.g., test if any "$LOGDIR"/*.log exist and skip check if none),
then grep only for p2-specific error patterns (use grep -iqE -- 'Cannot
satisfy|conflicting requirements|Conflicts detected|Resolution failed'
"$LOGDIR"/*.log) so you avoid benign matches; if grep returns success, print the
failure message and exit 1. Ensure you reference LOGDIR and replace the existing
grep -iq "conflict" "$LOGDIR"/*.log block with this guarded, pattern-specific
check.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 117-124: The -listInstalledRoots invocation in test-update.sh is
currently restricted to -profile SDKProfile so it only reports the RC install;
update the script to list the default profile (where the stable plugin was
installed) as well — either run the org.eclipse.equinox.p2.director
-listInstalledRoots command once without -profile to capture the default profile
or run it twice (once with -profile SDKProfile and once without) and write both
outputs (e.g., to installed-roots-default.txt and installed-roots-sdk.txt) so
the report reflects both stable and RC plugins.
- Line 50: STABLE_REPO is hardcoded to
"file://$WORKDIR/stable-repo/artifacts/update" which breaks if the zip layout
changes; modify test-update.sh to discover the extracted subdirectory
dynamically by searching under "$WORKDIR/stable-repo" for a unique marker file
(e.g., content.jar or artifacts.jar) and set STABLE_REPO to the file:// URL of
the directory that contains that marker. In other words, replace the fixed
STABLE_REPO assignment with logic that uses WORKDIR and a find/lookup of
content.jar or artifacts.jar, assigns the containing directory to STABLE_REPO,
and emits a clear error if no marker is found.
- Around line 56-100: The stable and RC installs are using different p2 profiles
so the RC run is not upgrading the stable install; make both invocations target
the same p2 profile. Update the stable-install block (the
"$ECLIPSE_HOME/eclipse" call that uses "$STABLE_REPO") to include the same flags
used in the RC block: -profile SDKProfile -destination "$ECLIPSE_HOME"
-bundlepool "$WORKDIR/p2" (so FEATURE_ID is installed into the same
profile/bundlepool), or alternatively remove -profile
SDKProfile/-bundlepool/-destination from the RC block if you prefer default
profile behavior — but ensure both invocations use identical
profile/destination/bundlepool settings and remove any stray/duplicate mentions
of -profile SDKProfile elsewhere so the test actually performs an in-place
upgrade.
- Around line 36-37: The current assignment to ECLIPSE_HOME via the find command
can yield an empty string; guard against that by checking ECLIPSE_HOME after the
find (the variable set by ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d
-name "eclipse*" | head -n1")) and if empty, print a clear error message
(including WORKDIR context) and exit non‑zero; otherwise continue and echo the
path as now. Ensure subsequent references use the validated $ECLIPSE_HOME.
🧹 Nitpick comments (2)
releng/update-site-tests/test-update.sh (2)
8-15: Hardcoded URLs will silently break when versions rotate.
ECLIPSE_URLandSTABLE_ZIP_URLpoint to specific release versions. When these URLs are retired or updated, the script fails with no clear indication of why. Consider parameterizing these (via environment variables with defaults) so CI can override them, and add a check after eachwgetthat verifies the downloaded file is valid (e.g., non-zero size or correct content type).Proposed approach
-ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz" -STABLE_ZIP_URL="https://dl.espressif.com/dl/idf-eclipse-plugin/updates/com.espressif.idf.update-v4.0.0.zip" +ECLIPSE_URL="${ECLIPSE_URL:-https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz}" +STABLE_ZIP_URL="${STABLE_ZIP_URL:-https://dl.espressif.com/dl/idf-eclipse-plugin/updates/com.espressif.idf.update-v4.0.0.zip}"
149-163: Report always says "Conflict check: OK" — consider making it dynamic.Line 160 unconditionally writes
"Conflict check: OK". While this is technically correct today (the script exits on conflict), it's brittle if the flow is ever changed to be non-fatal. A small improvement:- echo "Conflict check: OK" + echo "Conflict check: ${CONFLICT_STATUS:-OK}"This is a minor maintainability nit.
| ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d -name "eclipse*" | head -n1) | ||
| echo "Eclipse installed at: $ECLIPSE_HOME" |
There was a problem hiding this comment.
Guard against an empty ECLIPSE_HOME from find.
If the tarball extracts to an unexpected directory name (or extraction fails silently after set -e is bypassed somehow), find returns nothing and ECLIPSE_HOME becomes an empty string. Subsequent uses like "$ECLIPSE_HOME/eclipse" would resolve to /eclipse, which is dangerous.
Proposed fix
ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d -name "eclipse*" | head -n1)
+if [ -z "$ECLIPSE_HOME" ] || [ ! -x "$ECLIPSE_HOME/eclipse" ]; then
+ echo "❌ Eclipse extraction failed or eclipse binary not found"
+ exit 1
+fi
echo "Eclipse installed at: $ECLIPSE_HOME"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d -name "eclipse*" | head -n1) | |
| echo "Eclipse installed at: $ECLIPSE_HOME" | |
| ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d -name "eclipse*" | head -n1) | |
| if [ -z "$ECLIPSE_HOME" ] || [ ! -x "$ECLIPSE_HOME/eclipse" ]; then | |
| echo "❌ Eclipse extraction failed or eclipse binary not found" | |
| exit 1 | |
| fi | |
| echo "Eclipse installed at: $ECLIPSE_HOME" |
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` around lines 36 - 37, The current
assignment to ECLIPSE_HOME via the find command can yield an empty string; guard
against that by checking ECLIPSE_HOME after the find (the variable set by
ECLIPSE_HOME=$(find "$WORKDIR" -maxdepth 1 -type d -name "eclipse*" | head
-n1")) and if empty, print a clear error message (including WORKDIR context) and
exit non‑zero; otherwise continue and echo the path as now. Ensure subsequent
references use the validated $ECLIPSE_HOME.
| mkdir -p "$WORKDIR/stable-repo" | ||
| unzip -q "$WORKDIR/stable.zip" -d "$WORKDIR/stable-repo" | ||
|
|
||
| STABLE_REPO="file://$WORKDIR/stable-repo/artifacts/update" |
There was a problem hiding this comment.
Hardcoded zip internal path artifacts/update is fragile.
STABLE_REPO assumes the zip extracts into artifacts/update/. If the zip structure changes across plugin versions, this silently breaks. Consider discovering the path dynamically (e.g., by locating content.jar or artifacts.jar inside the extracted directory).
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` at line 50, STABLE_REPO is hardcoded
to "file://$WORKDIR/stable-repo/artifacts/update" which breaks if the zip layout
changes; modify test-update.sh to discover the extracted subdirectory
dynamically by searching under "$WORKDIR/stable-repo" for a unique marker file
(e.g., content.jar or artifacts.jar) and set STABLE_REPO to the file:// URL of
the directory that contains that marker. In other words, replace the fixed
STABLE_REPO assignment with logic that uses WORKDIR and a find/lookup of
content.jar or artifacts.jar, assigns the containing directory to STABLE_REPO,
and emits a clear error if no marker is found.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 137-150: The unconditional "✅ Eclipse launched successfully"
should be gated by an actual check: after launching "$ECLIPSE_HOME/eclipse" to
background and capturing PID, wait and then inspect the process exit status
and/or the "$LOGDIR/eclipse-launch.log" contents for a known startup marker
before printing success; treat kill returning 137/143 as expected and consider
them not-a-success for immediate crash, e.g., check `kill -0 $PID` or `ps` to
see if the PID is still running and/or grep the log for a startup line, and only
echo the success message when those checks confirm Eclipse started (otherwise
print an error and exit non‑zero).
🧹 Nitpick comments (2)
releng/update-site-tests/test-update.sh (2)
28-28: Dead assignment —ECLIPSE_HOMEis overwritten on line 37.This initial assignment is never used because
findon line 37 replaces it unconditionally. Remove it to avoid confusion about which value is authoritative.-ECLIPSE_HOME="$WORKDIR/eclipse"
53-58: Capture the pre-install roots to a file for the report.The output of
listInstalledRootshere goes to stdout only (lost once CI scrollback is trimmed). Redirect to a log file so it can be compared with the post-install listing in step 6.echo "Listing installed roots before stable:" "$ECLIPSE_HOME/eclipse" \ -nosplash \ -application org.eclipse.equinox.p2.director \ -listInstalledRoots \ - -consoleLog + -consoleLog \ + | tee "$LOGDIR/installed-roots-before.txt"
| echo "Launching Eclipse once..." | ||
| "$ECLIPSE_HOME/eclipse" \ | ||
| -nosplash \ | ||
| -clean \ | ||
| -data "$WORKDIR/workspace" \ | ||
| -configuration "$WORKDIR/configuration" \ | ||
| -consoleLog \ | ||
| -vmargs -Djava.awt.headless=false \ | ||
| > "$LOGDIR/eclipse-launch.log" 2>&1 & | ||
|
|
||
| PID=$! | ||
| sleep 20 | ||
| kill $PID || true | ||
| echo "✅ Eclipse launched successfully" |
There was a problem hiding this comment.
Headless launch issues already flagged; additionally, the success message on line 150 is unconditional.
The prior review covered headless=false and the fragile sleep/kill pattern. On top of that: "✅ Eclipse launched successfully" is printed regardless of whether Eclipse actually started or crashed instantly. Check the process exit status (or at least verify the log isn't empty / contains a startup marker) before declaring success.
PID=$!
sleep 20
kill $PID || true
-echo "✅ Eclipse launched successfully"
+wait $PID 2>/dev/null
+LAUNCH_EXIT=$?
+if [ $LAUNCH_EXIT -ne 0 ] && [ $LAUNCH_EXIT -ne 137 ] && [ $LAUNCH_EXIT -ne 143 ]; then
+ echo "❌ Eclipse launch failed with exit code $LAUNCH_EXIT"
+ exit 1
+fi
+echo "✅ Eclipse launched successfully"Exit codes 137 (SIGKILL) and 143 (SIGTERM) are expected from the kill.
🤖 Prompt for AI Agents
In `@releng/update-site-tests/test-update.sh` around lines 137 - 150, The
unconditional "✅ Eclipse launched successfully" should be gated by an actual
check: after launching "$ECLIPSE_HOME/eclipse" to background and capturing PID,
wait and then inspect the process exit status and/or the
"$LOGDIR/eclipse-launch.log" contents for a known startup marker before printing
success; treat kill returning 137/143 as expected and consider them
not-a-success for immediate crash, e.g., check `kill -0 $PID` or `ps` to see if
the PID is still running and/or grep the log for a startup line, and only echo
the success message when those checks confirm Eclipse started (otherwise print
an error and exit non‑zero).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@releng/update-site-tests/test-update.sh`:
- Around line 5-8: The cleanup block uses $WORKDIR and $LOGDIR before their
defaults are set, causing rm/mkdir to run with empty paths; move the default
assignments (the parameter expansions ${WORKDIR:-...} and ${LOGDIR:-...}) so
they occur before the cleanup block or alternatively move the entire cleanup
block after the configuration section where WORKDIR and LOGDIR are set; ensure
only one mkdir -p is kept (remove the redundant second mkdir -p) and update
references to WORKDIR and LOGDIR accordingly.
🧹 Nitpick comments (4)
releng/update-site-tests/test-update.sh (4)
162-179: Report generation looks correct but always says "Conflict check: OK".Because the script exits on line 121 if a conflict is detected, line 172 will only ever print "OK". This is technically accurate but means the report is never generated in the failure case. Consider trapping on exit or deferring the conflict-check exit so the report is always produced (it could be useful for debugging failed runs, especially since CI uploads it as an artifact).
Sketch: generate report even on failure using a trap
# Near the top of the script, after REPORT is defined: generate_report() { echo "Generating report..." { echo "Espressif IDE Upgrade Test Report" echo "================================" echo "" echo "Stable install exit code: ${STABLE_EXIT:-N/A}" echo "RC install exit code: ${RC_EXIT:-N/A}" echo "" echo "Installed Roots:" cat "$LOGDIR/installed-roots.txt" 2>/dev/null || echo "(not available)" echo "" echo "Conflict check: ${CONFLICT_STATUS:-UNKNOWN}" echo "" echo "Logs directory: $LOGDIR" } > "$REPORT" } trap generate_report EXITThen set
CONFLICT_STATUS="OK"orCONFLICT_STATUS="FAILED"accordingly before exiting.
14-15: Hardcoded Eclipse version2025-09will become stale.The PR objective is to test compatibility with "recent Eclipse CDT releases." Consider parameterizing
ECLIPSE_URL(or at least the release identifier) so the CI workflow can matrix-test against multiple Eclipse versions without editing the script.Sketch
-ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/2025-09/R/eclipse-cpp-2025-09-R-linux-gtk-x86_64.tar.gz" +ECLIPSE_VERSION="${ECLIPSE_VERSION:-2025-09}" +ECLIPSE_URL="https://ftp.osuosl.org/pub/eclipse/technology/epp/downloads/release/${ECLIPSE_VERSION}/R/eclipse-cpp-${ECLIPSE_VERSION}-R-linux-gtk-x86_64.tar.gz"
17-18: Hardcoded stable plugin versionv4.0.0will become stale.Same concern — consider parameterizing
STABLE_ZIP_URLor at least the version component so it can be overridden from CI or updated easily.
58-63: Pre-installlistInstalledRootsoutput is not captured.The output of this invocation goes to stdout (interleaved with
set -xtrace output) and is not written to a file. If the intent is to compare before/after, capture it to a log file for the report.Proposed fix
echo "Listing installed roots before stable:" "$ECLIPSE_HOME/eclipse" \ -nosplash \ -application org.eclipse.equinox.p2.director \ -listInstalledRoots \ - -consoleLog + -consoleLog \ + > "$LOGDIR/installed-roots-before.txt" +cat "$LOGDIR/installed-roots-before.txt"
AndriiFilippov
changed the title
2 participants
Description
Automate compatibility checks for update site with previous Espressif-IDE versions & recent Eclipse CDT releases
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Tests
Chores