bundle sandbox helper binaries in main zip, for winget.#9707
bundle sandbox helper binaries in main zip, for winget.#9707iceweasel-oai merged 1 commit intomainfrom
Conversation
| # Fall back to the single-binary zip if the helpers are missing | ||
| # to avoid breaking releases. |
There was a problem hiding this comment.
I don't follow: are we talking about what happens on our CI builder machine? Shouldn't that be deterministic? I'm confused what "Fall back" situation we are designing for here.
There was a problem hiding this comment.
this is me not wanting to break the release, and if it fails to find the binaries (like I'm constructing the filepaths wrong) falling back to what currently happens (each binary in its own zip file)
There was a problem hiding this comment.
I plan to remove this once I'm sure this is working.
| fi | ||
| rm -rf "$bundle_dir" | ||
| else | ||
| (cd "$dest" && 7z a "${base}.zip" "$base") |
There was a problem hiding this comment.
So this step should just be run for "codex-windows-sandbox-setup-${{ matrix.target }}.exe", is that right?
If so, maybe we should make an elif for that right here and then a final else that raises an exception that $base does not match one of the expected patterns.
There was a problem hiding this comment.
the bundling part of it should happen in the loop for "codex-${{ matrix.target }}.exe"
all other binaries should be as they are today, just the binary by itself in a zip file
There was a problem hiding this comment.
Oh, it is both of these:
codex-windows-sandbox-setup-${{ matrix.target }}.exe
codex-windows-command-runner-${{ matrix.target }}.exe
Does each of these need to be in its own .zip? Do we encourage users to download these individually?
It feels like we should favor the big codex.zip now that it will include everything?
There was a problem hiding this comment.
yeah, both of those are being included in codex.zip with this change. That is what winget will use
There was a problem hiding this comment.
The existing build script for the npm package appears to rely on a .zst being available for both codex-windows-sandbox-setup and codex-command-runner:
codex/codex-cli/scripts/install_native_deps.py
Lines 334 to 337 in c6ded0a
so I guess we still need to publish both of these until we update that script.
| echo "warning: missing sandbox binaries; falling back to single-binary zip" | ||
| echo "warning: expected $runner_src and $setup_src" | ||
| (cd "$dest" && 7z a "${base}.zip" "$base") |
There was a problem hiding this comment.
Why shouldn't this break the release build? Hasn't something gone horribly wrong if this is true?
There was a problem hiding this comment.
I mostly don't want to break the build while I am testing/iterating on this. Like making sure I don't have a typo or something. Once it's working, I would revert any fallbacks and then the build would break if something was missing
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
We can land this as-is for now, but as a follow-up, I would like to see dependent build scripts rely on only these artifacts (or the .zip or .tar.gz versions) for Windows:
codex-x86_64-pc-windows-msvc.exe.zst
codex-aarch64-pc-windows-msvc.exe.zst
and then extract codex-windows-sandbox-setup and codex-command-runner from within the archive, as necessary.
We should then stop publishing all of the individual codex-windows-sandbox-setup and codex-command-runner entries from our GitHub release to make things easier to reason about.
2 participants
Winget uses the main codex.exe value as its target.
The elevated sandbox requires these two binaries to live next to codex.exe