Closed Bug 980120 Opened 9 years ago Closed 9 years ago

Add tooltool support to Windows builds

Categories

(Release Engineering :: General, defect)

All
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: mshal)

References

Details

Attachments

(8 files, 7 obsolete files)

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
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)
Sure thing.
Flags: needinfo?(mshal)
Assignee: nobody → mshal
Depends on: 980964
Attached 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)
Attached 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)
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-
Attached 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)
Attached 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)
Attached 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+
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)
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)
Attached 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+
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+
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+
Attachment #8389352 - Flags: checked-in+
Depends on: 983314
Flags: needinfo?(mh+mozilla)
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)
Attached patch tools-l10nSplinter Review
tools patch to handle multiple --tooltool-script arguments.
Attachment #8394828 - Flags: review?(bhearsum)
Attached 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)
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
Attachment #8394829 - Flags: review?(rail) → review+
Attachment #8394826 - Flags: review?(bhearsum) → review+
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-
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+
Comment on attachment 8394829 [details]
setup.sh

Moving setup.sh to bug 980117
Attachment #8394829 - Attachment is obsolete: true
in production
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.
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)
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
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+
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
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)
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+
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.
I think we're in a state where we can say this is fixed.
Status: NEW → RESOLVED
Closed: 9 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
You need to log in before you can comment on or make changes to this bug.