Add tooltool support to Windows builds

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: glandium, Assigned: mshal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 7 obsolete attachments)

2.36 KB, patch
bhearsum
: review+
mshal
: checked-in+
Details | Diff | Splinter Review
2.09 KB, patch
bhearsum
: review+
mshal
: checked-in+
Details | Diff | Splinter Review
5.14 KB, patch
bhearsum
: review+
mshal
: checked-in+
Details | Diff | Splinter Review
3.16 KB, patch
bhearsum
: review+
mshal
: checked-in+
Details | Diff | Splinter Review
1.09 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
8.10 KB, patch
bhearsum
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
1.57 KB, patch
bhearsum
: review+
mshal
: checked-in+
Details | Diff | Splinter Review
1.98 KB, patch
bhearsum
: review+
mshal
: checked-in+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
In order for sccache to be deployed on windows, we need windows builds to be using tooltool.

Mike, do you think you can look into this?
Flags: needinfo?(mshal)
(Assignee)

Comment 1

5 years ago
Sure thing.
Flags: needinfo?(mshal)
(Assignee)

Updated

5 years ago
Assignee: nobody → mshal
(Assignee)

Updated

5 years ago
Depends on: 980964
(Assignee)

Comment 2

5 years ago
Posted patch windows-tools (obsolete) — Splinter Review
I'm not sure if this is the best way to fix it, but retry.py can't call Popen on tooltool.py directly, so we need to prefix the command with python.exe. See also: http://stackoverflow.com/questions/912830/using-subprocess-to-run-python-script-on-windows
Attachment #8387656 - Flags: review?(bhearsum)
(Assignee)

Comment 3

5 years ago
Posted patch buildbotcustom.patch (obsolete) — Splinter Review
The patch to misc.py passes the tooltool_script argument through so we can specify that in the windows config.

The patch to factory.py is similar to the tools patch - Windows can't execute fetch_and_unpack.sh directly, so we prefix the command with sh.exe. If there's a better solution here just let me know!
Attachment #8387659 - Flags: review?(bhearsum)
(Assignee)

Comment 4

5 years ago
Still needed is the config.py patch to pass in tooltool_script and tooltool_manifest_src. The location of tooltool_script depends on bug 980964, and the manifest src needs to be checked into m-c after we get the actual setup.sh uploaded to tooltool.

glandium, did you have any additional requirements aside from 'rm -rf sccache; tar -Jxf sccache.tar.xz'? Also FYI, it seems the tar on the Windows machines doesn't support -J or --xz...
Flags: needinfo?(mh+mozilla)
Comment on attachment 8387656 [details] [diff] [review]
windows-tools

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

I think the right way to do this is to adjust the commands to include 'python' and 'bash'. We do this in a bunch of places for other things, eg:
https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.py#L499
https://github.com/mozilla/build-buildbotcustom/blob/master/process/factory.py#L703
Attachment #8387656 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 6

5 years ago
Posted patch windows-tools (obsolete) — Splinter Review
Specifying 'python .../tooltool.py' for the Windows config is a bit tricky since it gets passed into fetch_and_unpack.sh on the command-line. This moves the script to the end of the command-line, so we can slurp multiple arguments.
Attachment #8387656 - Attachment is obsolete: true
Attachment #8387953 - Flags: review?(bhearsum)
(Assignee)

Comment 7

5 years ago
Posted patch buildbotcustom.patch (obsolete) — Splinter Review
Add 'sh' to fetch_and_unpack.sh, and handle tooltool_script as a list.
Attachment #8387659 - Attachment is obsolete: true
Attachment #8387659 - Flags: review?(bhearsum)
Attachment #8387954 - Flags: review?(bhearsum)
(Assignee)

Comment 8

5 years ago
Posted patch buildbot-configs.patch (obsolete) — Splinter Review
Change buildbot-configs to use a list for tooltool_script. Note the Windows values aren't correct yet - we'll need the actual script path from bug 980964, and obviously a win64/releng.manifest (I was just testing with linux32/releng.manifest). I'm just posting this here now for reference.
Comment on attachment 8387953 [details] [diff] [review]
windows-tools

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

These look much better, thanks for removing the hacks :).

Landing this will probably be tricky though. Any jobs that start prior to the reconfig will use the old buildbotcustom code. I don't think it's possible to find a time to land this that doesn't end up with a bunch of jobs getting a version of fetch_and_unpack.sh that doesn't have the order of args that they expect. You may want to consider renaming this script so you can land it without bustage. If you can find a way to support both invocations in one version of the script, that would work too. I'd be fine with something hacky there (eg, some hardcoded thing that uniquely identifies TT_BOOTSTRAP), as long as you remove it after landing.
Attachment #8387953 - Flags: review?(bhearsum) → review+
Attachment #8387954 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 10

5 years ago
This just adds the script as tooltool_wrapper.sh as discussed in IRC. We can then remove fetch_and_unpack.sh after the dust settles.
Attachment #8387953 - Attachment is obsolete: true
Attachment #8389350 - Flags: review?(bhearsum)
(Assignee)

Comment 11

5 years ago
This doesn't have the Windows config yet, it just converts tooltool_script to a list.
Attachment #8387956 - Attachment is obsolete: true
Attachment #8389351 - Flags: review?(bhearsum)
(Assignee)

Comment 12

5 years ago
Posted patch buildbotcustom.patch (obsolete) — Splinter Review
Attachment #8387954 - Attachment is obsolete: true
Attachment #8389352 - Flags: review?(bhearsum)
Attachment #8389350 - Flags: review?(bhearsum) → review+
Attachment #8389352 - Flags: review?(bhearsum) → review+
Attachment #8389351 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 14

5 years ago
Comment on attachment 8389351 [details] [diff] [review]
buildbot-configs.patch

buildbot-configs: https://hg.mozilla.org/build/buildbot-configs/rev/8c57b9d04ad6
Attachment #8389351 - Flags: checked-in+
(Assignee)

Comment 15

5 years ago
Comment on attachment 8389352 [details] [diff] [review]
buildbotcustom.patch

buildbotcustom: https://hg.mozilla.org/build/buildbotcustom/rev/9c95333cc33f
Attachment #8389352 - Flags: checked-in+
(Assignee)

Comment 16

5 years ago
Comment on attachment 8389351 [details] [diff] [review]
buildbot-configs.patch

Backed out since Linux apparently shouldn't be using /builds/tooltool.py, even with the mock environment. Still investigating.
Attachment #8389351 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Attachment #8389352 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Depends on: 983314
(Assignee)

Updated

5 years ago
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 17

5 years ago
Rebased buildbotcustom.patch, and made it work with l10n repack builds. (The buildbot-configs patch is the same as before)
Attachment #8389352 - Attachment is obsolete: true
Attachment #8394826 - Flags: review?(bhearsum)
(Assignee)

Comment 18

5 years ago
Posted patch tools-l10nSplinter Review
tools patch to handle multiple --tooltool-script arguments.
Attachment #8394828 - Flags: review?(bhearsum)
(Assignee)

Comment 19

5 years ago
Posted file setup.sh (obsolete) —
setup.sh script to be uploaded to tooltool for Windows to unpack sccache
Attachment #8394829 - Flags: review?(mh+mozilla)
Attachment #8394829 - Flags: review?(bhearsum)
(Assignee)

Comment 20

5 years ago
m-c patch to add the win64 releng.manifest for tooltool.
Attachment #8394830 - Flags: review?(bhearsum)
Attachment #8394828 - Attachment is patch: true
Comment on attachment 8394829 [details]
setup.sh

I'm not actually sure how this works...I think rail is probably a better reviewer for this.
Attachment #8394829 - Flags: review?(bhearsum) → review?(rail)
Attachment #8394830 - Flags: review?(bhearsum) → review+
Attachment #8394828 - Flags: review?(bhearsum) → review+
Comment on attachment 8394826 [details] [diff] [review]
buildbotcustom.patch

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

::: process/release.py
@@ +793,5 @@
>                  if pf.get('tooltool_manifest_src'):
>                      extra_args.extend(['--tooltool-manifest', pf.get('tooltool_manifest_src')])
>                  if pf.get('tooltool_script'):
> +                    for script in pf['tooltool_script']:
> +                        extra_args.extend(['--tooltool-script', script])

This looks OK. Can you confirm that this test run on your master was using this code? http://dev-master1.srv.releng.scl3.mozilla.com:8064/builders/release-mozilla-beta-linux_repack_1%2F6/builds/15

Updated

5 years ago
Attachment #8394829 - Flags: review?(rail) → review+
Attachment #8394826 - Flags: review?(bhearsum) → review+
(Reporter)

Comment 24

5 years ago
Comment on attachment 8394829 [details]
setup.sh

try 7z x -so sccache.tar.xz | tar -xf -

But this patch should go in bug 980117 anyways.
Attachment #8394829 - Flags: review?(mh+mozilla) → feedback-
(Assignee)

Comment 25

5 years ago
Comment on attachment 8394826 [details] [diff] [review]
buildbotcustom.patch

buildbotcustom: https://hg.mozilla.org/build/buildbotcustom/rev/f7ad8167f2fc
Attachment #8394826 - Flags: checked-in+
(Assignee)

Comment 26

5 years ago
Comment on attachment 8389351 [details] [diff] [review]
buildbot-configs.patch

buildbot-configs: https://hg.mozilla.org/build/buildbot-configs/rev/81a2764b35cb
Attachment #8389351 - Flags: checked-in+
(Assignee)

Comment 27

5 years ago
Comment on attachment 8394829 [details]
setup.sh

Moving setup.sh to bug 980117
Attachment #8394829 - Attachment is obsolete: true
in production
(Reporter)

Comment 29

5 years ago
As far as i can tell, tooltool doesn't run on windows builds yet. I guess there is still a piece needed in buildbot config.
(Reporter)

Comment 30

5 years ago
Presumably, this should work without adding files in the tree. I tested removing the releng.manifest file on linux on try, and the tooltool step says:

browser/config/tooltool-manifests/linux32/releng.manifest doesn't exist, skipping...
program finished with exit code 0

And the build continues as expected.

Without this, we can't test windows tooltool on try.
Attachment #8397762 - Flags: review?(bhearsum)
(Reporter)

Comment 31

5 years ago
Please land this for me once it's reviewed, and put in production asap. Thanks.
Comment on attachment 8397762 [details] [diff] [review]
Add tooltool manifests for Windows builds

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

This will go in with the next reconfig - either later today on tomorrow.
Attachment #8397762 - Flags: review?(bhearsum)
Attachment #8397762 - Flags: review+
Attachment #8397762 - Flags: checked-in+
in production
(Assignee)

Comment 34

5 years ago
Sorry, I thought the tooltool_manifest_src wouldn't work until it was in tree. In any case, for the Windows manifests, we'll need tooltool_script to pass in 'python'.
Attachment #8397905 - Flags: review?(bhearsum)
Attachment #8397905 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 35

5 years ago
Comment on attachment 8397905 [details] [diff] [review]
buildbot-configs-tooltool-script.patch

https://hg.mozilla.org/build/buildbot-configs/rev/b472d70340d2
Attachment #8397905 - Flags: checked-in+
in production
(Assignee)

Comment 37

5 years ago
I should have added the python tooltool.py lines in b2g_config.py and thunderbird_config.py as well.

Having the tooltool_manifest_src without tooltool_script works for now because those tooltool_manifest_src files don't exist in the tree yet, so tooltool_wrapper.sh helpfully avoids trying to run the default tooltool_script.
Attachment #8398194 - Flags: review?(bhearsum)
(Reporter)

Comment 38

5 years ago
Comment on attachment 8389350 [details] [diff] [review]
windows-tools.patch

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

::: lib/python/build/l10n.py
@@ +72,5 @@
>  
>      run_cmd(["mkdir", "-p", "l10n"])
>  
>      if tooltoolManifest:
> +        cmd = ['scripts/scripts/tooltool/tooltool_wrapper.sh',

Missing 'sh' here. This broke l10n repacks:
http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/29.0b3-candidates/build1/logs/release-mozilla-beta-win32_repack_1-bm86-build1-build8.txt.gz
Attachment #8398194 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 40

5 years ago
Comment on attachment 8398194 [details] [diff] [review]
buildbot-configs-b2g-thunderbird.patch

https://hg.mozilla.org/build/buildbot-configs/rev/ebccd9e955bb
Attachment #8398194 - Flags: checked-in+
in production.
(Reporter)

Comment 42

5 years ago
I think we're in a state where we can say this is fixed.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
We needed a followup fix for release automation builds:
  http://hg.mozilla.org/build/buildbotcustom/rev/5ac1a6ec2688

The windows 31.0b1 build failed when it tried to run /tools/tooltool.py. This wasn't a problem in 30.0 or older because there was no manifest file in the repo, but one merged this time round.
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.