Addressing docker comments #668

bam241 · 2020-04-02T16:36:55Z

This address comments from #660 about Docker.

I added something from me as well, see if I can separate the housekeeping jobs from the others... and that way simply the travis CI file

bam241 · 2020-04-02T22:14:37Z

@kkiesling @pshriwise @gonuke this is ready for review.

pshriwise

Just one small comment from me. Nice cleanup of the housekeeping check!

pshriwise · 2020-04-03T14:29:32Z

docker/build_geant4.sh


 source ${docker_env}

 rm -rf ${geant4_build_dir}/bld ${geant4_install_dir}
 mkdir -p ${geant4_build_dir}/bld
 cd ${geant4_build_dir}
-wget http://geant4.cern.ch/support/source/geant4.${geant4_version}.tar.gz
+wget https://geant4.cern.ch/support/source/geant4.${geant4_version}.tar.gz --no-check-certificate


In #660 they mention verifying our file against a checksum. Where do we stand on adding that?

did not found any checksum for geant4 tarball :(

We could generate a checksum against the version we are pulling

make sense !

ljacobson64 · 2020-04-03T16:27:53Z

docker/Dockerfile

 # Get some pip packages
-RUN pip install cython sphinx
+RUN pip install cython


Why was sphinx moved out of this file?

It looks like anything having to do w/ housekeeping was moved to the housekeeping.sh file.

docker/Dockerfile

ljacobson64 · 2020-04-03T16:29:14Z

docker/build_dagmc.sh

@@ -13,7 +13,6 @@ fi
 rm -rf ${dagmc_build_dir}/bld ${dagmc_install_dir}
 mkdir -p ${dagmc_build_dir}/bld
 cd ${dagmc_build_dir}
-#git clone https://github.com/svalinn/DAGMC -b develop


The reason these lines were commented is that they would normally be part of a DAGMC build script, but in this specific case, we get the DAGMC source code by copying it into the docker image, not cloning it from github.

@ljacobson64 is there any reason to leave this line in as a comment w/ justification or can we just remove it all together? (see here)

If I understand correctly, there are there just because the normal install DAGMC procedure should include them. But in this case as we copy the DAGMC from the PR (using Travis) we don't need them.

If this is correct, I don't think they should be there:
Those scripts are dedicated for CI not to show people how to install DAGMC.
It is probably a good idea to have a version of them somewhere to facilitate DAGMC install but for me this is definitely not the place.

I agree w/ this assessment @bam241. I think the lines here should be removed and if we want a docker to help users, we need to host that elsewhere.

For maintainability purposes, I think it would be good to have some indication of why the source code already exists in this directory. (The current situation with just a commented git clone line is also not very helpful.) Perhaps a comment line that says

# DAGMC source code already exists in this directory because it was copied into # the docker image by travis.yml

I think I understand,

the line is left there to highlight that we don't need to copy DAGMC source, while we normally would in a standard installation process.
So the line is basically there for educational purposes...
Is that correct @ljacobson64 ?

I would definitely prefer to not mix purposes in those scripts. So if there are used for CI install and testing, I would remove the git clone line and add the comment at the head of the file.

ljacobson64 · 2020-04-03T16:33:05Z

docker/build_dagmc.sh

@@ -29,4 +28,3 @@ cmake ../src -DMOAB_DIR=${moab_install_dir} \
 make -j${jobs}
 make install
 cd
-#rm -rf ${dagmc_build_dir}


The dagmc_build_dir would normally be deleted after building DAGMC, but we need to keep it in this specific case in order to run the tests.

My same question above applies here too- do you think it should be left in as a comment w/ an explanation or removed?

@ljacobson64

agreed with @kkiesling if this is commented without explanation it should not be part of the build.
I'd be happy to add them back with a line of explanation.

when you mean run the tests, do you mean run the test locally ?
Because we run the test in CI without those...

I think this line can be deleted.

To answer @bam241's question: I do mean for running the tests in CI. The CI runs make test in the DAGMC build directory, so the DAGMC build directory must exist in order to run the tests. See here: https://github.com/bam241/DAGMC/blob/fix_docker/docker/travis.tests.sh#L28

ljacobson64 · 2020-04-03T16:34:41Z

.travis.yml

+    - env: UBUNTU_VERSION=18.04 COMPILER=clang HDF5_VERSION=1.10.4 MOAB_VERSION=develop
+      if: type != pull_request
+    - stage: housekeeping
+      env: UBUNTU_VERSION=16.04 COMPILER=gcc   HDF5_VERSION=1.10.4 MOAB_VERSION=5.1.0


The housekeeping run should use 18.04

oups this was not intended

docker/Dockerfile

kkiesling

I have no major comments. Thanks @bam241!

kkiesling · 2020-04-04T19:58:15Z

docker/Dockerfile

 # Get some pip packages
-RUN pip install cython sphinx
+RUN pip install cython


It looks like anything having to do w/ housekeeping was moved to the housekeeping.sh file.

bam241 · 2020-04-05T16:41:01Z

@ljacobson64 to avoid having the housekeeping stuff everywhere and only used for 1 job in ubuntu 18.04. I moved astyle and sphinx install in the housekeeping scripts.

gonuke · 2020-04-08T01:03:58Z

docker/travis.housekeeping.sh

@@ -18,6 +18,9 @@ if [ "${TRAVIS_REPO_SLUG}" == "svalinn/DAGMC" ] && \
  fi
 fi

+# install package for style check
+apt-get install astyle


Why are we moving this to here?

gonuke · 2020-04-08T01:07:37Z

docker/travis.tests.sh

@@ -19,7 +19,7 @@ cd ${dagmc_build_dir}/bld
 # If this is not a pull request, get files needed for regression tests
 if [ "${TRAVIS_PULL_REQUEST}" == "false" ]; then
  cd src/make_watertight/tests
-  wget ${MW_REG_TEST_MODELS_URL} -O mw_reg_test_files.tar.gz -o wget.out
+  wget ${MW_REG_TEST_MODELS_URL} -O mw_reg_test_files.tar.gz


The comment suggested streaming these to stderr and stdout - do we do this now? Or is that default behavior?

my understanding of the comment was the default behavior streams them to stderr

gonuke · 2020-04-08T01:09:10Z

I'm ambivalent on the housekeeping changes. They may happen fairly often, which means a lot of apt install and pip installing for stuff we could do once. Should we have different docker images? Would that help?

bam241 · 2020-04-08T15:03:38Z

I was thinking @ljacobson64 @kkiesling @pshriwise @gonuke
For separation of concern what do you think about:

creating a .travis_scripts folder containing all the scripts required for CI (exept the update_docker.sh one)
keep only the Dockerfile and the update_docker.sh macro in the docker folder
create a script folder, with a version of the install scripts dedicated to installation for users ?

ljacobson64 · 2020-04-08T15:04:48Z

That kind of sounds like overkill. I'd rather just delete those 2 lines we were discussing

bam241 · 2020-04-08T15:18:36Z

That kind of sounds like overkill. I'd rather just delete those 2 lines we were discussing

Yes but I feel like it will clarify the purpose of everything.
It is just that it does not seems obvious to me, that the scripts used to install and run the tests in CI would be located in the docker folder

ljacobson64 · 2020-04-08T15:22:24Z

Would it make more sense if the docker folder were renamed CI?

bam241 · 2020-04-10T04:46:48Z

@gonuke @kkiesling @ljacobson64 this should be ready for an around of review

bam241 · 2020-04-16T14:08:04Z

@ljacobson64 @kkiesling I think I am now satisfied with the way it looks.

cc @pshriwise @gonuke

gonuke · 2020-04-25T16:39:36Z

Should we test some of this by adding an automatic build to dockerhub?

ljacobson64 · 2020-04-25T19:28:43Z

How does that work?

gonuke · 2020-04-25T19:41:56Z

You point dockerhub at a Dockerfile in a github repo and everytime that repo changes, it rebuilds the image. We have deployed it for

So far they have all been for images whose configuration is 100% defined in the dockerfile, so I don't know how it will behave for this image that requires scripts...

bam241 · 2020-04-25T23:14:08Z

@gonuke this seems like a good idea, will take a look at this

gonuke · 2020-04-27T11:53:02Z

I have something running here: https://hub.docker.com/repository/docker/svalinn/dagmc-ci-ubuntu-18.04/builds

gonuke · 2020-04-27T15:25:11Z

The dockerhub build is taking a long time.... probably not surprising

gonuke · 2020-04-27T15:27:59Z

Looks like it might have timed out...

bam241 · 2020-04-27T21:40:23Z

@gonuek if we want to do the automated one, we will probably need 2 dockerfiles one for each ubuntu version

ljacobson64 · 2020-04-27T21:49:15Z

We could stop supporting 16.04 since it's 4 years old now

ljacobson64 · 2020-04-27T21:49:30Z

Although in that case we may as well start supporting 20.04...

bam241 · 2020-04-27T22:07:43Z

@gonuke I think we will have problem....
timeout limits is set to 2h
https://success.docker.com/article/what-are-the-current-resource-limits-placed-on-automated-builds

bam241 · 2020-04-27T22:11:13Z

I think the main problem is building geant4... that takes forever....

CI/docker.build_hdf5.sh

bam241 · 2020-05-15T14:47:35Z

@ljacobson64 @gonuke I this I addressed your last comments :)

gonuke

Thanks @bam241

bam241 · 2020-05-27T16:15:36Z

@ljacobson64 do you have any other comments ?

CI/update_docker.sh

ljacobson64 · 2020-05-27T19:35:50Z

Has the docker image on dockerhub already been updated? I'm curious about how the tests are passing if it hasn't. Unless the new docker image is the same as the old one despite having a new Dockerfile.

bam241 · 2020-05-27T19:38:05Z

Yes the docker container have been updated/pushed after last dockerfile change.

gonuke

Thank @bam241 - LGTM

bam241 changed the title ~~[WIP] Addressing docker comments~~ Addressing docker comments Apr 2, 2020

pshriwise reviewed Apr 3, 2020

View reviewed changes

ljacobson64 requested changes Apr 3, 2020

View reviewed changes

kkiesling approved these changes Apr 4, 2020

View reviewed changes

bam241 added the PLEASE_REVIEW label Apr 7, 2020

gonuke reviewed Apr 8, 2020

View reviewed changes

bam241 force-pushed the fix_docker branch from 525d1f9 to 224b854 Compare April 10, 2020 15:28

gonuke mentioned this pull request Apr 27, 2020

Shared library/executable testing in CI and update to the Geant4 RPATH #674

Merged

bam241 added 13 commits May 14, 2020 14:43

dummy modif to travis.yml to trigger travis

c1764a1

allow empty commit in travis

20c557c

cleaning .travis.yml

21c78bf

revert some change

4740141

fixing dockerfile

402ceb6

re-ordering travis jobs

7b52232

naming the first test jobs

046bc7a

messing up some formating to have housekeeping fail

94f41c4

revert change in DagMC.cpp

163a515

renaming and moving

d819e6e

readding push form docker_update

f9fd66a

changing script_dir value to CI

273f518

fix type on build_dagmc script name

90dc84e

bam241 force-pushed the fix_docker branch from e83628a to 90dc84e Compare May 14, 2020 19:43

ljacobson64 reviewed May 15, 2020

View reviewed changes

CI/docker.build_hdf5.sh Outdated Show resolved Hide resolved

shuffling stuff around

e5a1b46

updating install script to new path

e8a5e60

bam241 force-pushed the fix_docker branch 2 times, most recently from 3453826 to e8a5e60 Compare May 15, 2020 16:30

gonuke approved these changes May 17, 2020

View reviewed changes

add and .vscode in gitignore

e0f98dd

ljacobson64 reviewed May 27, 2020

View reviewed changes

CI/update_docker.sh Show resolved Hide resolved

ljacobson64 approved these changes May 27, 2020

View reviewed changes

gonuke approved these changes May 28, 2020

View reviewed changes

gonuke merged commit 5735430 into svalinn:develop May 28, 2020

bam241 deleted the fix_docker branch November 28, 2024 14:39

Addressing docker comments #668

Addressing docker comments #668

Conversation

bam241 commented Apr 2, 2020

bam241 commented Apr 2, 2020

pshriwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bam241 Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bam241 Apr 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkiesling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bam241 commented Apr 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bam241 Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

gonuke commented Apr 8, 2020

bam241 commented Apr 8, 2020 • edited Loading

ljacobson64 commented Apr 8, 2020

bam241 commented Apr 8, 2020

ljacobson64 commented Apr 8, 2020

bam241 commented Apr 10, 2020

bam241 commented Apr 16, 2020

gonuke commented Apr 25, 2020

ljacobson64 commented Apr 25, 2020

gonuke commented Apr 25, 2020

bam241 commented Apr 25, 2020

gonuke commented Apr 27, 2020

gonuke commented Apr 27, 2020

gonuke commented Apr 27, 2020

bam241 commented Apr 27, 2020

ljacobson64 commented Apr 27, 2020

ljacobson64 commented Apr 27, 2020

bam241 commented Apr 27, 2020

bam241 commented Apr 27, 2020

bam241 commented May 15, 2020

gonuke left a comment

Choose a reason for hiding this comment

bam241 commented May 27, 2020

ljacobson64 commented May 27, 2020

bam241 commented May 27, 2020

gonuke left a comment

Choose a reason for hiding this comment

bam241 Apr 6, 2020 •

edited

Loading

bam241 Apr 5, 2020 •

edited

Loading

bam241 Apr 8, 2020 •

edited

Loading

bam241 commented Apr 8, 2020 •

edited

Loading