-
Notifications
You must be signed in to change notification settings - Fork 62
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
[ML] Add a dev-tools/ci script to automate the Linux/macOS portion of the CI #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 6.x version of this will be slightly different - I added some notes to say where.
I'm proposing that 6.3+ CI will move to Infra, but CI for 6.2 and earlier will remain on the ML team CI.
# | ||
|
||
# Increment the version here when a new tools/3rd party components image is built | ||
FROM droberts195/ml-linux-build:3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 -> 2 on backport to 6.x
RUN \ | ||
wget --quiet -O - http://dev-www.libreoffice.org/src/cppunit-1.13.2.tar.gz | tar zxf - && \ | ||
cd cppunit-1.13.2 && \ | ||
./configure --prefix=/usr/local/gcc73 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
73 -> 62 on backport to 6.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: you could use something like
ENV GCC_BUILD=gcc733
...
./configure --prefix=/usr/local/${GCC_BUILD}
etc. to make it more reusable for different branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the versions are to be made into variables I'd prefer to do that in a separate change, as it affects more than the files changed in this PR.
The test image is based on the build image, so to minimise typing that environment variable would be set in the build image and reused in the test image. But the test image's Dockerfile
still needs to specify the correct version of the build image. If it doesn't then having the compiler version duplicated in the test image helps to detect that inconsistency as the test image then won't build. The whole setup process relies on getting the versions correct throughout.
There may be a better way to do it, but, like I said, I think if we decide it's worthwhile there should be a separate PR for it.
# | ||
|
||
# Increment the version here when a new unit test image is built | ||
FROM droberts195/ml-linux-test:2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 -> 1 on backport to 6.x
|
||
ACCOUNT=droberts195 | ||
REPOSITORY=ml-linux-test | ||
VERSION=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 -> 1 on backport to 6.x
To facilitate migration from ML team CI to Infra CI we need to make the steps triggered by Jenkins as simple as possible. This change adds a ci script that can be used for Linux/macOS. (A future change will add something similar for Windows.)
The idea is that Infra CI would run this on a Linux worker VM with Git, Docker and Java 8 installed. (The vast majority of dependencies needed to build and test ML are in the Docker images, so the requirements on the worker VM are simple.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dev-tools/docker_test.sh
Outdated
DOCKERFILE="$TOOLS_DIR/docker/${PLATFORM}_tester/Dockerfile" | ||
TEMP_TAG=`git rev-parse --short=14 HEAD`-$PLATFORM-$$ | ||
|
||
docker build -t $TEMP_TAG --build-arg SNAPSHOT=$SNAPSHOT -f "$DOCKERFILE" . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you considered using docker hub instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base images are on docker hub. For example, the Linux test base image gets uploaded here: https://github.com/elastic/ml-cpp/pull/29/files#diff-c9c89afce5ca3660c277ceb81fcff6d3R36
The build/test images are temporary and get deleted after the artifacts have been copied out of them. But the build/test images are created using the base image downloaded from docker hub as the starting point. We don't rebuild the tools every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I saw that, just wondering why we are still building some parts and not use docker hub. It seems only cppunit that is missing from the images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 3 images involved:
- build base image - on docker hub - contains the minimum set of components to build the software - used by release manager
- test base image - on docker hub - contains the minimum set of components to build and unit test the software - will be used by CI
- temporary image - we copy the source code from the repo into this such that there is an image that contains what's currently in the local repo
The alternative to (3) would be to directly use one of the images from docker hub and pull some version of the code from GitHub as part of the build process within the container. But that means you cannot try out a build of uncommitted changes within the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, got it now. The names confused me, there is a linux_test_image
and a linux_tester
image, I got the impression that the image that is used in CI is building the linux_test_image
but it is builder the later image, which is not re-building the image all the time (including cppunit).
Maybe a doc string above the DOCKERFILE
definition would help.
… the CI (#29) To facilitate migration from ML team CI to Infra CI we need to make the steps triggered by Jenkins as simple as possible. This change adds a ci script that can be used for Linux/macOS.
… the CI (#29) To facilitate migration from ML team CI to Infra CI we need to make the steps triggered by Jenkins as simple as possible. This change adds a ci script that can be used for Linux/macOS.
… the CI (#29) To facilitate migration from ML team CI to Infra CI we need to make the steps triggered by Jenkins as simple as possible. This change adds a ci script that can be used for Linux/macOS.
To facilitate migration from ML team CI to Infra CI we need to make the
steps triggered by Jenkins as simple as possible.
This change adds a ci script that can be used for Linux/macOS.
(#31 is the corresponding change for Windows.)