macOS: wire tarball installs to run the real postinst + bundle node_modules - #3949
Draft
moflwi wants to merge 4 commits into
Draft
macOS: wire tarball installs to run the real postinst + bundle node_modules#3949moflwi wants to merge 4 commits into
moflwi wants to merge 4 commits into
Conversation
The Darwin branch previously only offered `brew tap ... && brew install`, but that tap does not exist yet, so INSTALL.sh could not actually install anything on a Mac. Fall back to the release tarball -- the same mechanism the CI smoke test exercises -- when brew is missing or the tap install fails, so the tested path is what users get by default. The usr subtree goes to /usr/local (since /usr is SIP-sealed), while the opt subtree keeps its normal location because /opt is not sealed and api.sh expects the SDK venv at /opt/opentelemetry_shell/venv.
INSTALL.sh, actions/instrument/shared/install.sh, and test_shell.yml each extracted the tarball and then hand-rolled a subset of setup (just the SDK venv) inline. That's a duplicate, out-of-sync copy of what meta/debian/postinst already does for deb/rpm/apk — and it skips postinst's deep node/python instrumentation setup entirely, since these paths never called it. build-brew now copies meta/debian/postinst into the tarball as a top-level file (sibling to usr/ and opt/, so the existing --strip-components=2/1 extractions of those two subtrees don't touch it). All three install sites extract just that file and run it with the same 'configure' argument dpkg already passes, instead of re-implementing a piece of it. This makes tarball installs benefit from any current or future postinst logic automatically (e.g. the deep node/python bundling once macOS build artifacts for those exist), and removes the drift risk of three inline copies falling out of sync with the real script. Verified with a throwaway fixture tarball (build-brew's exact tar -C src . layout, reproducing the postinst member) that extracting the top-level `postinst` file via `tar -xzf pkg -O postinst` and running it with `sh script configure` works with macOS's system bsdtar, both with and without the `./` prefix tar stores the entry under. Fixture removed after verifying. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Node deep-injection (agent.instrumentation.node.sh) was still a dead path on macOS: even with the $_otel_shell_home path fix on a sibling branch, there was nothing at that path to find, because build-brew never fetched node_modules.tar.xz the way the deb/rpm/apk jobs do. Adds merge-node-modules to build-brew's needs and downloads its artifact into src/usr/share/opentelemetry_shell/agent.instrumentation.node, same placement the deb job uses. No macOS-specific build is needed: the node tree was confirmed pure JS earlier (verified via a real npm install of all 446 packages during this branch's planning — no .node addons, no binding.gyp), so the single Linux-built artifact is portable as-is, same reasoning already applied to the Java agents. With postinst now bundled into the tarball too (previous commit), this closes the loop: a tarball install has both the archive to extract from and the script that knows how to extract it per-Node-version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks
Owner
|
Hello, thanks for contributing for the first time! |
Open
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR updates macOS tarball-based installation paths to invoke the project’s canonical postinst script (instead of duplicating partial setup logic) and extends the macOS tarball to include prebuilt node_modules.
Changes:
- Ship
meta/debian/postinstinside the macOS tarball and invoke it during tarball installs (INSTALL.sh, GitHub Action install script, and macOS CI). - Bundle Linux-built
node_modulesartifact into the macOS tarball build via workflow dependency updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| actions/instrument/shared/install.sh | Runs postinst configure after extracting tarball, replacing inline venv/pip setup. |
| INSTALL.sh | Adds macOS tarball fallback path that extracts files and runs postinst configure. |
| .github/workflows/test_shell.yml | Updates macOS CI installation flow to run postinst configure from tarball. |
| .github/workflows/build.yml | Makes build-brew depend on merge-node-modules, downloads node artifact, and adds top-level postinst into tarball. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+112
to
+115
| postinst_script="$(mktemp)" | ||
| tar -xzf "$package" -O postinst > "$postinst_script" | ||
| $wrapper sh "$postinst_script" configure | ||
| rm -f "$postinst_script" |
Comment on lines
+32
to
+35
| postinst_script="$(mktemp)" | ||
| tar -xzf "$tarball_file" -O postinst > "$postinst_script" | ||
| sudo sh "$postinst_script" configure | ||
| rm -f "$postinst_script" "$tarball_file" |
Comment on lines
+51
to
+54
| postinst_script="$(mktemp)" | ||
| tar -xzf opentelemetry-shell_*.tar.gz -O postinst > "$postinst_script" | ||
| sudo sh "$postinst_script" configure | ||
| rm -f "$postinst_script" |
Comment on lines
+53
to
+56
| postinst_script="$(mktemp)" | ||
| tar -xzf /tmp/package.tar.gz -O postinst > "$postinst_script" | ||
| sudo sh "$postinst_script" configure | ||
| rm -f "$postinst_script" /tmp/package.tar.gz |
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.
Summary
Two commits:
Depends on #3948 (postinst prefix-awareness, base branch) and #3946 (INSTALL.sh tarball fallback — this PR edits the same lines, so its one commit will also appear in this diff until #3946 merges).
Test plan
🤖 Generated with Claude Code