Skip to content

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

Open
wdconinc with Copilot wants to merge 6 commits into
mainfrom
copilot/reduce-duplication-in-install-script
Open

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

Conversation

CopilotAI commented Mar 14, 2026

Copy link
Copy Markdown
Contributor

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/bashORGANIZATION="$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.

CopilotAIand 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>
CopilotAI changed the title [WIP] Reduce duplication in eic-shell script embedded install.shSplit embedded eic-shell heredocs into template files, use envsubst for substitutionMar 14, 2026
CopilotAI requested a review from wdconincMarch 14, 2026 18:19
@wdconinc
wdconinc marked this pull request as ready for review March 14, 2026 18:22
CopilotAI review requested due to automatic review settings March 14, 2026 18:22

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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
FileDescription
install.shUses envsubst + $SCRIPT_DIR templates to generate the eic-shell launcher.
eic-shell.singularityNew Singularity launcher template to be substituted at install time.
eic-shell.dockerNew Docker launcher template (including the macOS X11 logic) to be substituted at install time.
assemble.pyGenerates a standalone install.sh by replacing template reads with inline heredocs.
.github/workflows/publish.ymlBuilds and deploys the assembled install.sh to GitHub Pages.
.gitignoreIgnores 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 threadeic-shell.singularity Outdated
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."
Comment threadeic-shell.singularity Outdated

export EIC_SHELL_PREFIX=${EICS_PREFIX}/local
export SINGULARITY_BINDPATH=${EICS_BINDPATH}
${SINGULARITY:-${EICS_SINGULARITY}} exec ${SINGULARITY_OPTIONS:-} ${SIF:-${EICS_SIF}} eic-shell $@
Comment threadeic-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
Comment threadeic-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 $@
Comment threadinstall.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

Comment threadeic-shell.singularity Outdated
if [ ! -z ${DISABLE_CVMFS_USAGE} ]; then
FLAGS="${FLAGS} --no-cvmfs"
fi
curl -L https://github.com/eic/eic-shell/raw/main/install.sh \
Comment threadeic-shell.singularity
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)"
Comment threadeic-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 threadassemble.py
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,
)

Comment threadinstall.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

netlifyBot commented Mar 14, 2026

Copy link
Copy Markdown

Deploy Preview for kaleidoscopic-syrniki-0cfb8a failed.

NameLink
🔨 Latest commitc199be7
🔍 Latest deploy loghttps://app.netlify.com/projects/kaleidoscopic-syrniki-0cfb8a/deploys/69b5c7a968cc810008032e7b

Comment thread.github/workflows/publish.yml Outdated

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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>

@wdconincwdconinc left a comment

Copy link
Copy Markdown
Contributor

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>

CopilotAI commented Mar 14, 2026

Copy link
Copy Markdown
ContributorAuthor

@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.

CopilotAI changed the title Split embedded eic-shell heredocs into template files, use envsubst for substitutionSplit embedded eic-shell heredocs into template files, assembled via Jinja2Mar 14, 2026
CopilotAI requested a review from wdconincMarch 14, 2026 20:40
Sign up for freeto 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

@wdconinc