Skip to content

base: remove repetition of environment variables. #88

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

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Conversation

ericonr
Copy link
Member

@ericonr ericonr commented Nov 29, 2024

Switch to using shell files defining the version for each module, and explicitly sourcing them in the relevant scripts.

This change makes adding new modules easier (2 fewer places to make changes), as well as simplifying adding any new variables for existing modules (e.g. checksums).

Switch to using shell files defining the version for each module, and
explicitly sourcing them in the relevant scripts.

This change makes adding new modules easier (2 fewer places to make
changes), as well as simplifying adding any new variables for existing
modules (e.g. checksums).

Unfortunately, the Compose env_file attribute [1] is only relevant
during runtime [2], and can't affect ARGs, otherwise it would have been
used in order to further simplify this PR.

[1] https://docs.docker.com/reference/compose-file/services/#env_file
[2] https://stackoverflow.com/a/68831814
Use single COPY instruction for related files.
Copy link
Collaborator

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

It indeed simplifies those definitions. Thanks!

I'm not very comfortable with the way we are copying those scripts to the image, though. How about placing them all in a known location beforehand (possibly outside /opt)?

@ericonr
Copy link
Member Author

ericonr commented Dec 2, 2024

I'm not very comfortable with the way we are copying those scripts to the image, though. How about placing them all in a known location beforehand

Is the issue copying them to the middle of the epics tree, or copying them for each install script?

Copying all of them at once loses the benefit of doing it for each stage, which is not invalidating the cache for packages which are built at the end.

@henriquesimoes
Copy link
Collaborator

Is the issue copying them to the middle of the epics tree

Yep. We are not very much consistent with the way run them. I'd rather have them in a known filesystem location and execute them with their absolute path than relying on the fact that they have been copied side-by-side somewhere in the epics tree.

Maybe placing them somewhere in the PATH, e.g. /usr/local/bin, is a sensible approach to avoid relying on yet another variable which defines where they are.

@ericonr
Copy link
Member Author

ericonr commented Dec 3, 2024

What about a single /opt/epics-in-docker (or similar) directory where everything goes? Otherwise we still have patches in the middle of the tree, and putting patches in /usr/local/bin feels wrong.

@henriquesimoes
Copy link
Collaborator

Good point. I wasn't considering the patches. I'm okay with having them in /opt/epics-in-docker. ;)

@ericonr
Copy link
Member Author

ericonr commented Dec 3, 2024

Good point. I wasn't considering the patches. I'm okay with having them in /opt/epics-in-docker. ;)

Don't forget to add it to the cleanup !

These scripts and patches are only used during build time, so they can
be stored in a dedicated directory instead. This also makes the build
process more explicit, by not relying on being in the same directory as
other files.
Copy link
Collaborator

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

The tree in the resulting image is now clean. Thanks!

$EPICS_RELEASE_FILE could also be placed in $EPICS_IN_DOCKER, IMO. That's not a big deal, though.

Also switch to using $EPICS_RELEASE_PATH where relevant so they don't
have to care about the specific location.
@ericonr ericonr merged commit f72ee54 into main Dec 3, 2024
2 checks passed
@ericonr ericonr deleted the env-duplication branch December 3, 2024 21:01
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.

2 participants