remove TOOLTOOL_REPO from all tasks

RESOLVED FIXED in mozilla53

Status

RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: dustin, Assigned: stevenellul, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

2 years ago
After the repo is removed from the task definition will the tasks default to the .py in the docker image?
Yes, exactly.
(Assignee)

Comment 3

2 years ago
Posted patch patchForBug1302767.patch (obsolete) — Splinter Review
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 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)
Assignee: nobody → stevenellul
Blocks: 1296148
(Assignee)

Comment 5

2 years ago
Posted patch PatchFile (obsolete) — Splinter Review
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)
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)
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

2 years 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

2 years 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

2 years ago
Posted patch patchFile.patch (obsolete) — Splinter Review
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
Attachment #8819320 - Flags: review?(dustin)
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+
(Assignee)

Comment 16

2 years ago
Posted patch patchFileSplinter Review
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

2 years ago
Attachment #8819320 - Attachment is obsolete: true
I switched 'testing/docker' to 'taskcluster/docker' due to a patch landing yesterday that moved the former directory to the latter.
(Assignee)

Comment 19

2 years ago
Ah okay, thanks for that.

Looks like the patch has finally passed on treeherder.
Awesome!  I'll get it landed.
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+
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ef0e968b7d2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a year ago
Product: TaskCluster → Firefox Build System

Updated

10 months ago
Blocks: 1461756
You need to log in before you can comment on or make changes to this bug.