Skip to content

z10_spack_environment.sh: don't set DETECTOR*#228

Merged
veprbl merged 3 commits intomasterfrom
pr/z10_spack_environment_unset_detector
Apr 8, 2026
Merged

z10_spack_environment.sh: don't set DETECTOR*#228
veprbl merged 3 commits intomasterfrom
pr/z10_spack_environment_unset_detector

Conversation

@veprbl
Copy link
Copy Markdown
Member

@veprbl veprbl commented Apr 6, 2026

As discussed in eic/eic-spack#856, the /etc/profile.d/z10_spack_environment.sh sets DETECTOR_PATH and DETECTOR_CONFIG to a wrong value when multiple detectors are present. In container, this is better handled by the /etc/profile.d/z21_epic_main.sh.

Copilot AI review requested due to automatic review settings April 6, 2026 21:28
Copy link
Copy Markdown
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

Note

Copilot was unable to run its full agentic suite in this review.

Updates the container’s Spack environment initialization to avoid exporting incorrect detector variables when multiple detectors are installed, deferring detector selection to z21_epic_main.sh.

Changes:

  • Removes export DETECTOR* lines from /etc/profile.d/z10_spack_environment.sh after generating it from spack env activate.
  • Adds inline documentation explaining why detector selection is handled elsewhere in the container.

Comment thread containers/eic/Dockerfile Outdated
@veprbl
Copy link
Copy Markdown
Member Author

veprbl commented Apr 7, 2026

@wdconinc What do you think about this? I wouldn't want to ship 26.04.0 with 25.12.0 geometry being loaded by default silently.

@wdconinc
Copy link
Copy Markdown
Contributor

wdconinc commented Apr 7, 2026

@wdconinc What do you think about this? I wouldn't want to ship 26.04.0 with 25.12.0 geometry being loaded by default silently.

These changes look fine (if hacky) to me. I don't have a better approach (one way would be to have a single most recent epic end up in the default view, but I don't know how we make sure that's the latest one either).

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

This comment was marked as resolved.

@veprbl veprbl merged commit 65012f2 into master Apr 8, 2026
31 of 33 checks passed
veprbl added a commit to eic/epic that referenced this pull request Apr 16, 2026
While testing the eic/containers#228 I
discovered that the thisepic.sh does not work in container
initialization due to it being a bash script. This converts it to a
POSIX shell script. setup.sh logic is moved to a separate file, and it
now reports that it's really an error.

---------

Co-authored-by: aider (anthropic/claude-sonnet-4-20250514) <aider@aider.chat>
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.

3 participants