1302767 - remove TOOLTOOL_REPO from all tasks

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

We use tooltool in lots of ways, but in no case do we need to download a new copy of tooltool.py for every task -- it is a part of the docker image.

This bug involves seeking out and destroying everywhere TOOLTOOL_REPO is used, and then removing it from all task definitions.

Comment 1

[Steven Ellul]

After the repo is removed from the task definition will the tasks default to the .py in the docker image?

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Yes, exactly.

Comment 3

[Steven Ellul]

Attachment: patchForBug1302767.patch

I have removed most lines containing TOOLTOOL_REPO and removed it from lines where I thought the rest of the text was still needed.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8814960 [details] [diff] [review]
patchForBug1302767.patch

Review of attachment 8814960 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I see I misunderstood your question -- you'll need to make the necessary changes for each actual use of TOOLTOOL_REPO to avoid cloning the repo and instead use the built-in file.

That said, maybe it's (almost) never used?  In which case all of the places where we *set* the variable can be dropped, which is what you've just done.  We should also drop TOOLTOOL_REV at the same time.

When you've made the changes described below, put up a new patch and I will pus it to try.  Then you can look at any failing jobs to see if they failed because of your changes.  If not, then I think we're OK to proceed!

::: taskcluster/scripts/builder/install-packages.sh
@@ -8,3 @@
>  test -n "$TOOLTOOL_REV"
>  
> -tc-vcs checkout $gecko_dir/tooltool $TOOLTOOL_REPO $TOOLTOOL_REPO $TOOLTOOL_REV

This definitely won't work, as tc-vcs needs all of those arguments.  That said, I think this script is completely unused.  Just drop the changes to this file, and I'll delete it in a separate bug.

::: taskcluster/scripts/misc/build-clang32-windows.sh
@@ -5,5 @@
>  # This script is for building clang-cl on Windows.
>  
> -# Fetch our toolchain from tooltool.
> -wget -O tooltool.py ${TOOLTOOL_REPO}/raw/${TOOLTOOL_REV}/tooltool.py
> -chmod +x tooltool.py

This script uses `tooltool.py` later, so you'll need to change that to refer to the actual location of the file.  Since this is Windows, it doesn't use docker.  However, you can refer to the file directly at

  testing/docker/recipes/tooltool.py

(the same is true for build-clang64-windows.sh)

Comment 5

[Steven Ellul]

Attachment: PatchFile

Have dropped changes to taskcluster/scripts/builder/install-packages.sh
Removed all uses of TOOLTOOL_REV
Added filepath to tooltool.py

When is tooltool script run? As in how can I check for failing jobs?

Thanks

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

This looks good from a read through.  The way to test is to push to try and see what happens.  This requires some sign-up if you want to do it, which probably isn't worthwhile for you at the moment.  I'll push to try for you now :)

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b65615e2210c

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

That's the treeherder view for the try push.  Take a look, and look especially at failed jobs (red, purple, or orange).  Look at their logs and see if it was a tooltool-related failure.  I'll check to make sure all of the possibly-affected jobs got run.

Comment 9

[Steven Ellul]

(In reply to Dustin J. Mitchell [:dustin] from comment #8)
> That's the treeherder view for the try push.  Take a look, and look
> especially at failed jobs (red, purple, or orange).  Look at their logs and
> see if it was a tooltool-related failure.  I'll check to make sure all of
> the possibly-affected jobs got run.

It looks like I have not successfully refered to the location of tooltool.py
I'm assuming my syntax is incorrect, will look at that bit again.

Comment 10

[Steven Ellul]

I believe what I should have written for path to tooltool.py is

../../../testing/docker/recipes/tooltool.py

Unfortunately I can only reupload patch in two weeks as I am away, will do so then.

Comment 11

[Steven Ellul]

Attachment: patchFile.patch

Just occured to me to modify patch file instead of redo changes.

Have changed filepath to tooltool.py

When you get a chance can you please push again.

Thanks

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7302644f4ce6

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8819320 [details] [diff] [review]
patchFile.patch

This looks good to me, but we'll see what the try push says.

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1889225b5ab6

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

Still no luck:
 https://treeherder.mozilla.org/logviewer.html#?job_id=33037232&repo=try#L75

Comment 16

[Steven Ellul]

Attachment: patchFile

Stopped trying to work it out and instead copied syntax from different script

Have used 'build/src/testing/docker/recipes/tooltool.py' as path

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

I switched 'testing/docker' to 'taskcluster/docker' due to a patch landing yesterday that moved the former directory to the latter.

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c49eb2ee194

Comment 19

[Steven Ellul]

Ah okay, thanks for that.

Looks like the patch has finally passed on treeherder.

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

Awesome!  I'll get it landed.

Comment 21

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8821184 [details] [diff] [review]
patchFile

by the way, when requesting review, use "?" and then enter the bugzilla name of the person you want to do the review :)

Comment 22

[Dustin J. Mitchell [:dustin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef0e968b7d2feb1376a2e70bd0555d66278fb9d
Bug 1302767: remove TOOLTOOl_REPO from all tasks; r=dustin

Comment 23

[Dustin J. Mitchell [:dustin] (he/him)]

Nice work Steven!  This was a bit of a difficult bug because it involved making a lot of changes without having a lot of context for what each bit does.  But you did well!

I know this is class-related, but if you're interested in working on another bug please let me know and I can help you find one.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ef0e968b7d2