Move some setup out of the docker file to fix taskcluster problems. #10762

jgraham · 2018-05-02T12:15:48Z

This change is

foolip

LGTM % documentation for everything I don't understand :)

foolip · 2018-05-02T12:27:25Z

tools/docker/Dockerfile

-RUN mkdir -p /home/test/.fonts && \
-    cp web-platform-tests/fonts/Ahem.ttf ~/.fonts && \
-    fc-cache -f -v
+RUN mkdir -p /home/test/web-platform-tests


Can you add a comment explaining why this isn't just git clone https://github.com/w3c/web-platform-tests + WORKDIR /home/test/web-platform-tests?

And also comment that the reason for having a checkout as part of the image is to save on network traffic or speed things up?

Having the checkout as part of the image was the previous state, but now we don't. It probably did make things a little faster, but I imagine the gains would decline as the image gets older and on the other hand it does make it much slower to iterate on the docker image. In the future we could maybe consider a task to build a base image containing a checkout, updated e.g. weekly.

Oh, so the whole point of this now is to not run git fetch. I did not spot that, comment would be good :)

Have the comment say "the remote is added but not fetched, the right revision is fetched inside the container".

But... then I have to wonder why bother setting this up in the Dockerfile at all, a plain git clone inside the container would be fewer lines of code and easier to understand.

foolip · 2018-05-02T12:28:18Z

tools/docker/start.sh


 cd web-platform-tests
-git pull --depth=50
+git fetch origin -n -q --depth=50


Isn't the --depth=50 a bit pointless now? Presumably we'll be updating the image once in a while, and does it even work to do a shallow fetch from within a full clone?

foolip · 2018-05-02T12:28:42Z

tools/docker/start.sh


 cd web-platform-tests
-git pull --depth=50
+git fetch origin -n -q --depth=50
+git checkout -b master origin/master


Is there a separate step that checks out the right revision?

Yeah, although I have a followup to do this more obviously.

In particular: * Don't try to install Ahem into the docker image since this fails if Ahem ever changes * Do the souce checkout in the startup script. This should ensure that subsequent wpt commands work with the latest code, and fixes some of the intermittent errors we have seen.

foolip

Asking for more comments and wondering about --depth=50 :)

foolip · 2018-05-03T13:17:21Z

tools/docker/start.sh

-wget https://dl.google.com/linux/direct/$deb_archive
+if [[ $BROWSER == "chrome"* ]] || [[ "$BROWSER" == all ]]
+then
+    # Install Chome dev


Preexisting typo: Chome

foolip · 2018-05-03T13:19:06Z

tools/docker/start.sh

+
+cd ~/web-platform-tests
+git fetch ${REPO} ${BRANCH} -n -q --depth=50
+git checkout -b build ${REV}


Why create a build branch?

Why not? It avoids a useless warning about checking out a refless revision, avoids having to suppress that warning, and potentially makes things a little easier to work with for interactive debugging.

OK, so there's a warning to suppress :) If not for that, it'd just be more code.

foolip · 2018-05-03T13:20:18Z

tools/docker/start.sh

-# Install Chome dev
-deb_archive=google-chrome-unstable_current_amd64.deb
-wget https://dl.google.com/linux/direct/$deb_archive
+if [[ $BROWSER == "chrome"* ]] || [[ "$BROWSER" == all ]]


Why the asterix after "chrome"?

It makes it a prefix match. Otherwise chrome-dev isn't going to match :)

OK, but if "chrome" is given we'll still use Chrome Dev... I guess this doesn't matter unless we want to also run something other than Chrome Dev though.

foolip · 2018-05-03T13:22:37Z

tools/docker/start.sh


-cd web-platform-tests
-git pull --depth=50
+REPO=${1:-origin}


s/REPO/REMOTE/

foolip · 2018-05-03T13:23:50Z

tools/docker/start.sh

-git pull --depth=50
+REPO=${1:-origin}
+BRANCH=${2:-master}
+REV=${3:-master}


I think FETCH_HEAD or origin/master would be better here. Otherwise, when you get rid of the "build" branch as I've suggested, the checkout step will sometimes create a new branch called master and sometimes not.

foolip · 2018-05-03T13:27:58Z

tools/docker/Dockerfile

-RUN mkdir -p /home/test/.fonts && \
-    cp web-platform-tests/fonts/Ahem.ttf ~/.fonts && \
-    fc-cache -f -v
+RUN mkdir -p /home/test/web-platform-tests


Have the comment say "the remote is added but not fetched, the right revision is fetched inside the container".

But... then I have to wonder why bother setting this up in the Dockerfile at all, a plain git clone inside the container would be fewer lines of code and easier to understand.

foolip · 2018-05-03T13:32:20Z

tools/docker/start.sh

+BROWSER=${4:-all}
+
+cd ~/web-platform-tests
+git fetch ${REPO} ${BRANCH} -n -q --depth=50


The --depth=50 still seems like a bit of a liability, making it impossible to restart a failed job if it failed more than 50 commits ago, and if we merge in other repos (like csswg-test) that might be just the previous commit.

How much time does it save to use --depth=50?

An alternative, which seems to work, is to make Taskcluster depend on the merge_pr_* tags. Then we could do git fetch origin merge_pr_10791 --depth=1 here.

At minimum, a comment explaining why --depth=50 is a good idea would be appreciated :)

It takes like 7 minutes longer on my machine to do a full clone.

I'm not sure that we want to make it only work on tagged revisions, because that makes testing it harder. I could probably arrange to keep pulling more commits until rev-parse on the target succeeds, but I think we should avoid that until we have a way to retry builds since getting that right seems error-prone.

Yeah, let's call it a "later" problem.

Also optimise things so we don't download Chrome unless actually required.

foolip

Hope it works now :)

foolip · 2018-05-03T21:20:04Z

tools/docker/start.sh

-git pull --depth=50
+if [[ ! `git rev-parse --verify -q ${REV}` ]];
+then
+    # But if for some reason the commit under test isn't in that range, we give in and


Nice fix, thanks!

foolip · 2018-05-03T21:21:07Z

tools/docker/start.sh

+cd ~
+
+# Initially we just fetch 50 commits in order to save several minutes of fetching
+git clone ${REMOTE} --single-branch --branch ${BRANCH} --no-checkout -q --depth=50 web-platform-tests


Optional nit: could call the directory wpt since it will be called that eventually, and shorter paths are nice in logs.

jgraham requested review from gsnedders and foolip May 2, 2018 12:15

wpt-pr-bot added the infra label May 2, 2018

Ms2ger approved these changes May 2, 2018

View reviewed changes

jgraham force-pushed the tc_less_dockerfile branch from 2b4119e to 0dbc13e Compare May 2, 2018 12:23

foolip approved these changes May 2, 2018

View reviewed changes

jgraham added 2 commits May 3, 2018 13:43

Update to latest docker image

b12a96a

jgraham force-pushed the tc_less_dockerfile branch from 0dbc13e to d955567 Compare May 3, 2018 13:13

foolip approved these changes May 3, 2018

View reviewed changes

jgraham added 2 commits May 3, 2018 18:12

Check out the required commit directly in start.sh.

cff7642

Also optimise things so we don't download Chrome unless actually required.

Update taskcluster tasks

c33d9e2

jgraham force-pushed the tc_less_dockerfile branch from d955567 to c33d9e2 Compare May 3, 2018 17:33

foolip approved these changes May 3, 2018

View reviewed changes

jgraham merged commit 843547c into master May 3, 2018

foolip deleted the tc_less_dockerfile branch May 7, 2018 09:07

foolip mentioned this pull request May 7, 2018

Taskcluster master runs fail very often #10653

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some setup out of the docker file to fix taskcluster problems. #10762

Move some setup out of the docker file to fix taskcluster problems. #10762

jgraham commented May 2, 2018 •

edited by wpt-reviewable-bot

Loading

foolip left a comment

foolip May 2, 2018

jgraham May 2, 2018

foolip May 2, 2018

foolip May 3, 2018

foolip May 2, 2018

foolip May 2, 2018

jgraham May 2, 2018

foolip left a comment

foolip May 3, 2018

foolip May 3, 2018

jgraham May 3, 2018

foolip May 3, 2018

foolip May 3, 2018

jgraham May 3, 2018

foolip May 3, 2018

foolip May 3, 2018

foolip May 3, 2018

foolip May 3, 2018

foolip May 3, 2018

jgraham May 3, 2018

foolip May 3, 2018

foolip left a comment

foolip May 3, 2018

foolip May 3, 2018

Move some setup out of the docker file to fix taskcluster problems. #10762

Move some setup out of the docker file to fix taskcluster problems. #10762

Conversation

jgraham commented May 2, 2018 • edited by wpt-reviewable-bot Loading

foolip 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

foolip 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

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

foolip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham commented May 2, 2018 •

edited by wpt-reviewable-bot

Loading