-
Notifications
You must be signed in to change notification settings - Fork 215
validate setup universal #116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
validate setup universal #116
Conversation
11e8bea to
a6fc380
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11e8bead77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
verify.sh
Outdated
| CODEX_ENV_SWIFT_VERSION=${SWIFT[i]:-${SWIFT[-1]}} \ | ||
| CODEX_ENV_RUBY_VERSION=${RUBY[i]:-${RUBY[-1]}} \ | ||
| CODEX_ENV_PHP_VERSION=${PHP[i]:-${PHP[-1]}} \ | ||
| CODEX_ENV_JAVA_VERSION=${JAVA[i]:-${JAVA[-1]}} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard empty JAVA_VERSIONS before array access
Because the script runs with set -u, accessing JAVA[i]/JAVA[-1] will throw “unbound variable/bad array subscript” when JAVA_VERSIONS is empty. In this Dockerfile, only AMD_JAVA_VERSIONS/ARM_JAVA_VERSIONS are defined, so JAVA_VERSIONS is not exported into the verify step and read -ra JAVA produces an empty array. That makes the first iteration of this loop fail and the image build will stop at verification. Consider defaulting JAVA_VERSIONS or skipping this lookup when the array is empty.
Useful? React with 👍 / 👎.
dfd1c1a to
7b14486
Compare
7b14486 to
28d3507
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec801a1cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for ((i=max-1; i>=0; i--)); do | ||
| CODEX_ENV_PYTHON_VERSION=${PYTHON[i]:-${PYTHON[0]}} \ | ||
| CODEX_ENV_NODE_VERSION=${NODE[i]:-${NODE[0]}} \ | ||
| CODEX_ENV_RUST_VERSION=${RUST[i]:-${RUST[0]}} \ | ||
| CODEX_ENV_GO_VERSION=${GO[i]:-${GO[0]}} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore Node default after version loop
Because the loop runs from max-1 down to 0, the last setup_universal.sh invocation uses NODE[0] as the version. With NODE_VERSIONS="18 20 22" in the Dockerfile, that means the final pass sets Node to 18. setup_universal.sh persists this via nvm alias default (setup_universal.sh:27-33), so the built image ends up defaulting to Node 18 instead of the intended NODE_VERSION=22 (Dockerfile:158-182). This changes the runtime version users see and makes the verification output reflect the wrong default. Consider iterating forward, reversing NODE_VERSIONS, or explicitly restoring the desired default after the loop.
Useful? React with 👍 / 👎.
8044837 to
763a518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 763a518407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dockerfile
Outdated
| curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --profile minimal --default-toolchain none \ | ||
| && . "$HOME/.cargo/env" \ | ||
| && echo 'source $HOME/.cargo/env' >> /etc/profile \ | ||
| && rustup toolchain install $RUST_VERSIONS --profile minimal --component rustfmt --component clippy \ | ||
| && rustup default ${RUST_VERSIONS%% *} | ||
|
|
||
| && rustup toolchain install $RUST_VERSIONS --profile minimal --component rustfmt --component clippy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configure a default Rust toolchain before verification
This step initializes rustup with --default-toolchain none but no longer sets a default toolchain afterward. setup_universal.sh then does rustc --version to read the current toolchain before calling rustup default, and with no default configured rustup exits with “no default toolchain configured,” causing the verify loop (and any run with CODEX_ENV_RUST_VERSION set) to abort under set -e. Re‑introduce a rustup default here or make setup_universal.sh tolerate the no‑default case before querying rustc.
Useful? React with 👍 / 👎.
763a518 to
da11f5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da11f5d1d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "# Ruby: ${CODEX_ENV_RUBY_VERSION} (default: ${current})" | ||
| if [ "${current}" != "${CODEX_ENV_RUBY_VERSION}" ]; then | ||
| mise use --global "ruby@${CODEX_ENV_RUBY_VERSION}" | ||
| ruby --version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore a Ruby default before probing
ruby -v
Because the Dockerfile no longer sets a global Ruby (mise use --global was removed), setup_universal.sh now runs ruby -v with no default configured when it is invoked by verify.sh during the image build. The ruby shim exits non‑zero in that state, and with set -e the script aborts before it can call mise use --global, causing the verification step (and build) to fail. Consider restoring a default Ruby in the install step or making the current‑version probe resilient to an unset default (e.g., mise current ruby || true).
Useful? React with 👍 / 👎.
da11f5d to
4f0437a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f0437aa83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RUN --mount=type=cache,target=/root/.cache/mise \ | ||
| for v in $GO_VERSIONS; do mise install "go@${v}"; done \ | ||
| && mise use --global "go@${GO_VERSIONS%% *}" \ | ||
| && mise use --global "golangci-lint@${GOLANG_CI_LINT_VERSION}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set a default Go toolchain before verification
The Go install block now only installs versions via mise but never selects a global Go version (Dockerfile:266-268). verify.sh exports CODEX_ENV_GO_VERSION and runs /opt/codex/setup_universal.sh, which starts by running go version to read the current default (setup_universal.sh:56-58). When no global Go version is set, the mise shim exits non‑zero, and because both scripts run under set -e, the verification step will fail before it can call mise use --global for the requested version. Restoring a default Go version during install or making setup_universal.sh handle a missing default avoids the build failure.
Useful? React with 👍 / 👎.
| RUN --mount=type=cache,target=/root/.cache/mise \ | ||
| JAVA_VERSIONS="$( [ "$TARGETARCH" = "arm64" ] && echo "$ARM_JAVA_VERSIONS" || echo "$AMD_JAVA_VERSIONS" )" \ | ||
| && for v in $JAVA_VERSIONS; do mise install "java@${v}"; done \ | ||
| && mise use --global "java@${JAVA_VERSIONS%% *}" \ | ||
| && mise use --global "gradle@${GRADLE_VERSION}" \ | ||
| && mise use --global "maven@${MAVEN_VERSION}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configure a default Java toolchain before setup
This Java install step no longer sets a global Java version (Dockerfile:202-206). During verification, verify.sh sets CODEX_ENV_JAVA_VERSION and invokes setup_universal.sh, which first calls java -version to determine the current default (setup_universal.sh:84-86). Without a global toolchain, the mise shim exits non‑zero and set -e aborts the verification/build before it can run mise use --global. Restoring a default Java version (or making the current‑version probe tolerant of an unset default) prevents the build from failing.
Useful? React with 👍 / 👎.
2605392 to
3f1544b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f1544b12c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dockerfile
Outdated
| JAVA_VERSIONS="$( [ "$TARGETARCH" = "arm64" ] && echo "$ARM_JAVA_VERSIONS" || echo "$AMD_JAVA_VERSIONS" )" \ | ||
| && for v in $JAVA_VERSIONS; do mise install "java@${v}"; done \ | ||
| && mise use --global "java@${JAVA_VERSIONS%% *}" \ | ||
| && mise use --global "java@${JAVA_VERSIONS[0]}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use POSIX-safe expansion for default version
The RUN instructions in the Dockerfile execute under /bin/sh (no SHELL override), and ${JAVA_VERSIONS[0]} is not valid in POSIX sh; it yields a “bad substitution” error and aborts the build. Since JAVA_VERSIONS is a scalar string (not a bash array), this should remain ${JAVA_VERSIONS%% *} or be split in a bash shell. The same array-index syntax is also used in the Swift/Rust/Ruby/Go blocks, so those RUN steps will fail for the same reason.
Useful? React with 👍 / 👎.
71174d9 to
50c6128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50c612852d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ "${current}" != "v${CODEX_ENV_NODE_VERSION}" ]; then | ||
| nvm alias default "${CODEX_ENV_NODE_VERSION}" | ||
| nvm use "${CODEX_ENV_NODE_VERSION}" | ||
| nvm use --save "${CODEX_ENV_NODE_VERSION}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid persisting .nvmrc in setup_universal
This switches to nvm use --save, and nvm --help documents that --save “writes the specified version to .nvmrc.” Because entrypoint.sh runs setup_universal.sh in the caller’s working directory, this will create/overwrite a .nvmrc file in the mounted repo (or fail if the mount is read‑only). Under set -e, a write error will abort container startup; even when it succeeds it mutates user code unexpectedly. Consider dropping --save or changing to a neutral working directory before calling nvm use.
Useful? React with 👍 / 👎.
50c6128 to
80d1168
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80d1168935
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ENV PYENV_ROOT=/root/.pyenv | ||
| ENV PATH=$PYENV_ROOT/bin:$PATH | ||
| RUN git -c advice.detachedHead=0 clone --branch "$PYENV_VERSION" --depth 1 https://github.com/pyenv/pyenv.git "$PYENV_ROOT" \ | ||
| RUN git -c advice.detachedHead=0 clone --depth 1 https://github.com/pyenv/pyenv.git "$PYENV_ROOT" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pin pyenv checkout to keep builds reproducible
This clone no longer pins a pyenv release; git clone without --branch tracks whatever HEAD is on the default branch at build time. That makes the image build non‑reproducible and can break later when upstream changes or removes python-build definitions needed by pyenv install $PYTHON_VERSIONS. Consider restoring a specific tag/commit pin (as before) so builds are stable across time.
Useful? React with 👍 / 👎.
80d1168 to
5ccde2d
Compare
|
@codex why does this dockerfile build fail with |
|
Codex couldn't complete this request. Try again later. |
|
@codex why does this dockerfile build fail with |
|
Codex couldn't complete this request. Try again later. |
5ccde2d to
1013054
Compare
1013054 to
f01af4b
Compare
No description provided.