remove TOOLTOOL_REPO from all tasks

RESOLVED FIXED in mozilla53

Status

Taskcluster
Task Configuration
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: dustin, Assigned: Steven Ellul, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 months ago
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.
(Assignee)

Comment 1

5 months ago
After the repo is removed from the task definition will the tasks default to the .py in the docker image?
(Reporter)

Comment 2

5 months ago
Yes, exactly.
(Assignee)

Comment 3

5 months ago
Created attachment 8814960 [details] [diff] [review]
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.
(Reporter)

Comment 4

5 months ago
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)
(Reporter)

Updated

5 months ago
Assignee: nobody → stevenellul
(Reporter)

Updated

5 months ago
Blocks: 1296148
(Assignee)

Comment 5

5 months ago
Created attachment 8817797 [details] [diff] [review]
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
Attachment #8814960 - Attachment is obsolete: true
Flags: needinfo?(dustin)
(Reporter)

Comment 6

5 months ago
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 :)
Flags: needinfo?(dustin)
(Reporter)

Comment 7

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b65615e2210c
(Reporter)

Comment 8

5 months ago
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.
(Assignee)

Comment 9

4 months ago
(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.
(Assignee)

Comment 10

4 months ago
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.
(Assignee)

Comment 11

4 months ago
Created attachment 8819320 [details] [diff] [review]
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
Attachment #8817797 - Attachment is obsolete: true
(Reporter)

Updated

4 months ago
Attachment #8819320 - Flags: review?(dustin)
(Reporter)

Comment 12

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7302644f4ce6
(Reporter)

Comment 13

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

This looks good to me, but we'll see what the try push says.
Attachment #8819320 - Flags: review?(dustin) → review+
(Reporter)

Comment 14

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1889225b5ab6
(Reporter)

Comment 15

4 months ago
Still no luck:
 https://treeherder.mozilla.org/logviewer.html#?job_id=33037232&repo=try#L75
(Assignee)

Comment 16

4 months ago
Created attachment 8821184 [details] [diff] [review]
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
Attachment #8821184 - Flags: review+
(Assignee)

Updated

4 months ago
Attachment #8819320 - Attachment is obsolete: true
(Reporter)

Comment 17

4 months ago
I switched 'testing/docker' to 'taskcluster/docker' due to a patch landing yesterday that moved the former directory to the latter.
(Reporter)

Comment 18

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c49eb2ee194
(Assignee)

Comment 19

4 months ago
Ah okay, thanks for that.

Looks like the patch has finally passed on treeherder.
(Reporter)

Comment 20

4 months ago
Awesome!  I'll get it landed.
(Reporter)

Comment 21

4 months ago
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 :)
Attachment #8821184 - Flags: review+ → review+
(Reporter)

Comment 22

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

Comment 23

4 months ago
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ef0e968b7d2
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.