Skip to content

Split embedded eic-shell heredocs into template files, assembled via Jinja2#27

Open
Copilot wants to merge 6 commits intomainfrom
copilot/reduce-duplication-in-install-script
Open

Split embedded eic-shell heredocs into template files, assembled via Jinja2#27
Copilot wants to merge 6 commits intomainfrom
copilot/reduce-duplication-in-install-script

Conversation

Copy link
Contributor

Copilot AI commented Mar 14, 2026

install.sh embedded two large heredocs (~180 lines total) for generating the Singularity and Docker eic-shell launchers inline, making the script bloated and the templates hard to maintain independently.

Changes

Template files

  • eic-shell.singularity — Singularity launcher extracted from install_singularity()
  • eic-shell.docker — Docker launcher extracted from install_docker(), including the macOS X11 block as natural shell code gated by a runtime $(uname -s) check

Both files are proper standalone shell scripts using ${VAR:=default} bash defaults for all install-time variables, so they can be run or linted directly (set the relevant env vars to override the defaults for testing). There are no ${EICS_*} pseudo-variables.

Launcher generation in install.sh

Install-time values are baked into the generated eic-shell script in two steps:

# 1. Write a preamble with actual install-time values (bash variable expansion)
cat > eic-shell << PREAMBLE
#!/bin/bash
ORGANIZATION="$ORGANIZATION"
CONTAINER="$CONTAINER"
INSTALLED_SINGULARITY="$SINGULARITY"
...
PREAMBLE
# 2. Append the template body (skip its own shebang)
tail -n +2 "$SCRIPT_DIR/eic-shell.singularity" >> eic-shell

Assembly for distribution

  • assemble.py — Uses jinja2.Environment(loader=FileSystemLoader(...)) to load each template file cleanly, then replaces the tail -n +2 "$SCRIPT_DIR/TEMPLATE" >> OUTPUT calls with equivalent inline heredocs, producing a self-contained install.sh suitable for curl … | bash distribution. A post-assembly validation raises an error if any $SCRIPT_DIR/ reference survives.

CI deploy workflow

  • .github/workflows/deploy.yml — Extended (from the existing workflow) to install jinja2 and run assemble.py before building the Jekyll site, so the assembled standalone install.sh is published to GitHub Pages on every push to main.
Original prompt

This section details on the original issue you should resolve

<issue_title>Reduce duplication in eic-shell script embedded install.sh</issue_title>
<issue_description>Is your feature request related to a problem? Please describe.
There is a significant amount of duplication here, but it is pre-existing.

Describe the solution you'd like
In order to not make duplication worse, we should split off the embedded eic-shell heredocs into separate files and read them in with envsubst. We should modify the CI workflow to publish a complete single install.sh at the current path.
</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

Copilot AI and others added 2 commits March 14, 2026 18:17
…I publish workflow

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Copilot AI changed the title [WIP] Reduce duplication in eic-shell script embedded install.sh Split embedded eic-shell heredocs into template files, use envsubst for substitution Mar 14, 2026
Copilot AI requested a review from wdconinc March 14, 2026 18:19
@wdconinc wdconinc marked this pull request as ready for review March 14, 2026 18:22
Copilot AI review requested due to automatic review settings March 14, 2026 18:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the install.sh launcher generation by moving the embedded Singularity/Docker eic-shell scripts into dedicated template files and using envsubst for substitution, with a CI workflow to publish a self-contained assembled installer for curl | bash usage.

Changes:

  • Extract eic-shell launchers into eic-shell.singularity and eic-shell.docker templates, generated via envsubst from install.sh.
  • Add assemble.py to inline templates back into a standalone install.sh for distribution.
  • Add a GitHub Pages publish workflow that assembles and deploys install.sh on pushes to main.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
install.sh Uses envsubst + $SCRIPT_DIR templates to generate the eic-shell launcher.
eic-shell.singularity New Singularity launcher template to be substituted at install time.
eic-shell.docker New Docker launcher template (including the macOS X11 logic) to be substituted at install time.
assemble.py Generates a standalone install.sh by replacing template reads with inline heredocs.
.github/workflows/publish.yml Builds and deploys the assembled install.sh to GitHub Pages.
.gitignore Ignores Python __pycache__/.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +79 to +83
if [ ! -z ${UPGRADE} ]; then
echo "Upgrading eic-shell..."
if [ -z "$DISABLE_CVMFS_USAGE" -a -d /cvmfs/singularity.opensciencegrid.org/${ORGANIZATION}/${CONTAINER}:${VERSION} ]; then
echo ""
echo "Note: You cannot manually update the container as you are using the CVMFS version."

export EIC_SHELL_PREFIX=${EICS_PREFIX}/local
export SINGULARITY_BINDPATH=${EICS_BINDPATH}
${SINGULARITY:-${EICS_SINGULARITY}} exec ${SINGULARITY_OPTIONS:-} ${SIF:-${EICS_SIF}} eic-shell $@
eic-shell.docker Outdated
Comment on lines +61 to +65
if [ ! -z ${UPGRADE} ]; then
echo "Upgrading eic-shell..."
docker pull ${EICS_IMG} || exit 1
echo "eic-shell upgrade successful"
exit 0
eic-shell.docker Outdated
dispnum=`ps -e |grep Xquartz | grep listen | grep -v xinit |awk '{print $5}'`
XSTUFF="-e DISPLAY=host.docker.internal${dispnum} -v /tmp/.X11-unix:/tmp/.X11-unix"
fi
docker run ${EICS_PLATFORM_FLAG} ${EICS_MOUNT} $XSTUFF -w=$PWD -it --rm -e EIC_SHELL_PREFIX=${EICS_PREFIX}/local ${EICS_IMG} eic-shell $@
install.sh Outdated
Comment on lines 206 to 207
envsubst '${EICS_ORGANIZATION} ${EICS_CONTAINER} ${EICS_TMPDIR} ${EICS_VERSION} ${EICS_PREFIX} ${EICS_DISABLE_CVMFS_USAGE} ${EICS_SINGULARITY} ${EICS_BINDPATH} ${EICS_SIF}' < "$SCRIPT_DIR/eic-shell.singularity" > eic-shell

if [ ! -z ${DISABLE_CVMFS_USAGE} ]; then
FLAGS="${FLAGS} --no-cvmfs"
fi
curl -L https://github.com/eic/eic-shell/raw/main/install.sh \
Comment on lines +14 to +18
echo " -u,--upgrade Upgrade the container to the latest version"
echo " -n,--no-cvmfs Disable check for local CVMFS when updating. (D: enabled)"
echo " -o,--organization Organization (D: $ORGANIZATION) (requires cvmfs)"
echo " -c,--container Container family (D: $CONTAINER) (requires cvmfs)"
echo " -v,--version Version to install (D: $VERSION) (requires cvmfs)"
eic-shell.docker Outdated
Comment on lines +68 to +72
if [ "$(uname -s)" = 'Darwin' ] && [ ! ${NOX} ]; then
nolisten=`defaults find nolisten_tcp | grep nolisten | head -n 1 | awk '{print $3}'|cut -b 1 `
[[ $nolisten -ne 0 ]] && echo "For X support: In XQuartz settings --> Security --> enable \"Allow connections from network clients\" and restart (should be only once)."
xhost +localhost
dispnum=`ps -e |grep Xquartz | grep listen | grep -v xinit |awk '{print $5}'`
Comment on lines +58 to +64
install_sh = re.sub(
r'(?P<indent>[ \t]*)envsubst \'(?P<variables>[^\']+)\''
r' < "\$SCRIPT_DIR/(?P<template>[^"]+)" > (?P<output>\S+)',
inline_template,
install_sh,
)

install.sh Outdated
export EICS_ORGANIZATION="$ORGANIZATION" EICS_CONTAINER="$CONTAINER" EICS_TMPDIR="$TMPDIR" \
EICS_VERSION="$VERSION" EICS_PREFIX="$PREFIX" EICS_DISABLE_CVMFS_USAGE="${DISABLE_CVMFS_USAGE}" \
EICS_SINGULARITY="$SINGULARITY" EICS_BINDPATH="$BINDPATH" EICS_SIF="$SIF"
envsubst '${EICS_ORGANIZATION} ${EICS_CONTAINER} ${EICS_TMPDIR} ${EICS_VERSION} ${EICS_PREFIX} ${EICS_DISABLE_CVMFS_USAGE} ${EICS_SINGULARITY} ${EICS_BINDPATH} ${EICS_SIF}' < "$SCRIPT_DIR/eic-shell.singularity" > eic-shell
@netlify
Copy link

netlify bot commented Mar 14, 2026

Deploy Preview for kaleidoscopic-syrniki-0cfb8a failed.

Name Link
🔨 Latest commit c199be7
🔍 Latest deploy log https://app.netlify.com/projects/kaleidoscopic-syrniki-0cfb8a/deploys/69b5c7a968cc810008032e7b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Target branch calls this file deploy.yml. Instead of creating a new file, integrate into the target branch's deploy.yml workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 42c57e1 — integrated the "Assemble standalone install.sh" step into the existing deploy.yml and removed publish.yml.

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Copilot AI requested a review from wdconinc March 14, 2026 20:18
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot I don't like the envsubst replacement in assembly.py; it's hacky. Consider if a templating engine like jinja2 would be better here. The target is two goals:

  • a way to run the install.sh script locally for testing,
  • a way to provide a combined install.sh script that does not depend on external files.

The goals would be best accomplished by allowing the embedded shell scripts to be treated as shell scripts in the repository, not as embedded heredocs (the main motivation for this change).

Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 14, 2026

@copilot I don't like the envsubst replacement in assembly.py; it's hacky. Consider if a templating engine like jinja2 would be better here. The target is two goals:

  • a way to run the install.sh scri...

Done in c199be7. Here's what changed:

Template files are now proper shell scripts${EICS_*} pseudo-variables are gone. Both eic-shell.singularity and eic-shell.docker now use ${VAR:=default} bash defaults at the top, so they can be run or linted as standalone scripts (just set the relevant env vars for a real test). The #!/bin/bash shebang is preserved.

install.sh — no more envsubst — the export EICS_* … envsubst … < "$SCRIPT_DIR/template" blocks are replaced by:

  1. An unquoted heredoc (cat > eic-shell << PREAMBLE) that writes the shebang and bakes in all install-time values via normal bash variable expansion.
  2. tail -n +2 "$SCRIPT_DIR/template" >> eic-shell which appends the template body (skipping its own shebang since the preamble already provides one).

assemble.py — Jinja2 FileSystemLoader — template files are loaded via jinja2.Environment(loader=FileSystemLoader(...)). The regex is much simpler: it matches the unambiguous tail -n +2 "$SCRIPT_DIR/FILE" >> OUTPUT lines and replaces them with quoted heredocs. A post-assembly check raises an error if any $SCRIPT_DIR/ reference survives. deploy.yml gains a pip install jinja2 step.

Copilot AI changed the title Split embedded eic-shell heredocs into template files, use envsubst for substitution Split embedded eic-shell heredocs into template files, assembled via Jinja2 Mar 14, 2026
Copilot AI requested a review from wdconinc March 14, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce duplication in eic-shell script embedded install.sh

3 participants