Conversation
jedwards4b
left a comment
There was a problem hiding this comment.
There is a variable user_home which it seems like should replace the hardcoded
/home/cime throughout - but then you are also handling a case of root running so I'm a little lost as to the intent.
There was a problem hiding this comment.
Pull request overview
Updates the project’s container image and CI entrypoint behavior to improve dependency handling (notably Perl), move inputdata download into the entrypoint logic, and adjust GitHub Actions to explicitly source the container entrypoint.
Changes:
- Switch Docker base image reference to
quay.io/condaforge/miniforge3and adjust OS-level package installs (Perl + gosu). - Refactor
docker/entrypoint.shto handle HOME/.cime setup in CI/root scenarios and add helper functions (e.g., inputdata download, cprnc build). - Update CI workflow steps to manually source
/entrypoint.shand remove old.cimecopy logic (plus a temporary debug listing).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
docker/entrypoint.sh |
Adds CI/root HOME handling, .cime relocation, and new helper functions; changes environment setup behavior. |
docker/cime.yaml |
Adds editor dependency and enables HDF5 OpenMPI build in the conda env spec. |
docker/Dockerfile |
Updates base image source, installs Perl/gosu via apt, and runs inputdata download via sourced entrypoint during build. |
.github/workflows/testing.yml |
Sources /entrypoint.sh in jobs and removes legacy .cime copy; includes a temporary debug ls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docker/Dockerfile
Outdated
| COPY .cime .cime | ||
| COPY entrypoint.sh /entrypoint.sh | ||
|
|
||
| RUN bash -c "SKIP_ENTRYPOINT=true . /entrypoint.sh && download_input_data" |
There was a problem hiding this comment.
The Docker build step calls download_input_data, but the image no longer installs wget (or curl) via apt and it’s not guaranteed to be available in the base conda env. This can make the build fail when wget is missing. Install wget/curl in the image layer before this RUN step, or invoke the downloader via conda run -n <env> / an explicit path to the env’s wget.
There was a problem hiding this comment.
Added wget/curl/subversion to dockerfile.
docker/entrypoint.sh
Outdated
| # Clobber PATH so system perl is used instead of conda perl | ||
| echo "export PATH=/usr/bin:\$PATH" |
There was a problem hiding this comment.
The generated .bashrc prepends /usr/bin to PATH after conda activation. This can cause system tools (e.g., python/cmake/mpirun) to be picked over the conda environment versions, breaking builds/tests. If the intent is only to force system Perl, use a more targeted approach (e.g., set PERL=/usr/bin/perl, hash -p /usr/bin/perl perl, or adjust only perl resolution) rather than reordering the entire PATH.
| # Clobber PATH so system perl is used instead of conda perl | |
| echo "export PATH=/usr/bin:\$PATH" | |
| # Ensure system perl is used instead of conda perl without changing PATH order | |
| echo "hash -p /usr/bin/perl perl" |
There was a problem hiding this comment.
Used both hash and PERL variable.
docker/entrypoint.sh
Outdated
| export USER=`id -nu` | ||
| export LOGNAME="${USER}" | ||
| export USER_ID="${USER_ID:-1000}" |
There was a problem hiding this comment.
Use $(...) instead of legacy backticks for command substitution (easier to read/nest, less error-prone).
.github/workflows/testing.yml
Outdated
| # TODO remove just for testing | ||
| ls -la "${HOME}" | ||
|
|
There was a problem hiding this comment.
Remove the temporary debug ls -la "${HOME}" step (and the TODO) before merging; it adds noise to CI logs and can inadvertently expose environment details in logs.
| # TODO remove just for testing | |
| ls -la "${HOME}" |
|
You can preview documentation at https://esmci.github.io/cime/branch/fix-containers/html/index.html |
| - hdf5@1.12.3+cxx+fortran+hl+mpi+shared %gcc@13.3.0 | ||
| - netcdf-c@4.9.2+mpi %gcc@13.3.0 | ||
| - netcdf-fortran@4.6.1 %gcc@13.3.0 | ||
| - parallel-netcdf@1.12.3 %gcc@13.3.0 |
|
@jedwards4b @billsacks I don't think the CESM failure is caused by this PR, probably related to this change. If it is I'll just merge this PR. Thanks! |
|
@jasonb5 - Thanks - I agree that the CESM failure is due to a CICE change - we just saw that in our recent testing as well without this CIME change. |
Description
Checklist