svalinn / DAGMC

Direct Accelerated Geometry Monte Carlo Toolkit
https://svalinn.github.io/DAGMC
Other
103 stars 65 forks source link

Putting install scripts into dockerfile #822

Closed shimwell closed 1 year ago

shimwell commented 2 years ago

work in progress, not ready for review

Description

As mentioned in project https://github.com/svalinn/DAGMC/projects/6 it has been proposed to move the install scripts into the dockerfile.

Motivation and Context

This simplifies the passing of environments to cmake and other commands as they are all visible in one file and no longer creating in one script to be used in another.

Changes

What kind of change does this PR introduce? (Bug fix, feature, documentation update...) refactored dockerfile to include more ARGs ENVs and install commands.

Behavior

What is the current behavior? What is the new behavior? old = dockerfile copies in sh scripts and runs them, new = dockerfile runs install commands directly

gonuke commented 1 year ago

Sorry @shimwell , but I'm just noticing that none of the incremental stages are using any of the prior dockerfiles at all. You are building from scratch at each stage (and thus wasting a lot of CI resources :) ). Alas, I think we lost this without noticing in #813

gonuke commented 1 year ago

Prior to #813, we had different dockerfiles for each stage, and each of those files used a different container that had already been pushed to GHCR. With a single dockerfile, we have stages that build on each other. At each step, we aim to build a particular stage, but with no caching, the CI runner has no idea that we've already built that stage. As a result, every step is rebuilding GEANT, HDF5, etc... including the step where you test the DAGMC install.

gonuke commented 1 year ago

The right way to fix this is probably introduce all the right caching, etc, but perhaps the multistage-docker-build-action is what we need to simplify this.

shimwell commented 1 year ago

Good spot yes, I can start implementing the multistage-docker-build-action on this open PR

gonuke commented 1 year ago

I think this has been replaced by #863 - do you want to close this one @shimwell ?

shimwell commented 1 year ago

Closing in favour of #863