Closed Bug 1391427 Opened 7 years ago Closed 7 years ago

Repackage upstream rust releases on taskcluster

Categories

(Taskcluster Graveyard :: Docker Images, enhancement)

Product:
Taskcluster Graveyard
Component:
Docker Images
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla58

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(4 files, 10 obsolete files)

Bug 1391427 - Port the repack_rust script to taskcluster.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1391427 - Package upstream rust in taskcluster.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1391427 - Execute python toolchain scripts with mach.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1391427 - Add script 'arguments' key to toolchain tasks.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
In taskcluster/docker/rust-build we have scripts to repackage upstream rust releases for specific gecko targets and upload them to tooltool. Now that builds are using taskcluster, migrate to exporting the repacks as taskcluster artifacts instead.
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

Glandium, am I on the right track here?

The toolchain transform seems to expect each task to produce only one artifact. Should I run it once for each host type, and combine all the possible targets into just two artifacts?

Is there a way to pass the desired Rust version from the yaml task description? I guess you have different wrapper scripts for each target and version, but it would be nice to share this and just add a key for the Rust packaging.
Attachment #8906179 - Flags: feedback?(mh+mozilla)
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

This seems reasonable to me.

My understanding is it basically automates what you are doing today except it produces a TC artifact instead of uploading to tooltool. So within that context, it is strictly better. If we want to improve how that's done, that could be deferred to a follow-up bug IMO.

Of course, we'll need to hook up the toolchain dependencies to everywhere that is installing Rust via tooltool. Shouldn't be too much extra work.
Attachment #8906179 - Flags: feedback+
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review183636

::: commit-message-53c6f:5
(Diff revision 1)
> +wrapper since we cannot pass options from the task-
> +describing yaml file.

of course you can. Through environment variables. Search for "env:" in the yaml files under taskcluster/ci/toolchain
The downside is that environment variables are not taken into account for the task hash, so if you only change the environment variable, the job will not run and your change will be a noop. That's probably something that should change.

That being said, you can also put the version number in the task name (and set a toolchain-alias without the version number), and that will have you covered.
> Should I run it once for each host type

How big is a linux rust package with linux, android, and osx targets compared to one for each?
Attachment #8906179 - Flags: feedback?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #6)

> That being said, you can also put the version number in the task name (and
> set a toolchain-alias without the version number), and that will have you
> covered.

Yes. We probably want to do that for clarity on the base-toolchains tasks, or when one variant is on a different Rust version from another. However, I think it's more clear for updates if I add an `arguments` key which is passed to the script and use that to set the version from the yml file instead of needing wrapper scripts or going through the environment.

> How big is a linux rust package with linux, android, and osx targets compared to one for each?

The compiler supports code generation for all architectures (and currently uses the platform linker) so the only addition we need for cross targets is a copy of the standard library. Those are about 22 MB for Android targets, and ~70 MB for desktop. So a package with all linux and cross targets we currently support is 309 MB vs 104 MB for one targeting just the host architecture.

If size is a concern we could package the standard libraries separately, as Nathan's script originally did. We seem to save some space by compressing them together, and I expect some time in doing a single download and unpack, but probably it has more to do with how often the same builder cache switches between cross- and host-targeted tasks.

I think it's less fiddly have everything in one place, so I've been going with that for now.
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review183636

> of course you can. Through environment variables. Search for "env:" in the yaml files under taskcluster/ci/toolchain

I did this through adding an `arguments` key instead. Let me know what you think.
Attachment #8906179 - Flags: review?(mh+mozilla)
Attachment #8907358 - Flags: review?(mh+mozilla)
Comment on attachment 8907358 [details]
Bug 1391427 - Remove rust from linux tooltool manifests.

https://reviewboard.mozilla.org/r/179024/#review184490

::: browser/config/tooltool-manifests/linux64/releng.manifest
(Diff revision 3)
>      "filename": "gtk3.tar.xz",
>      "setup": "setup.sh",
>      "unpack": true
> -  },
> -  {
> -    "version": "rustc 1.19.0 (0ade33941 2017-07-17) repack",

We may not be ready for this one, since `static-analysis-linux64-st-an/opt` is failing on try, but that doesn't need to block the rest of the patches.
Comment on attachment 8907358 [details]
Bug 1391427 - Remove rust from linux tooltool manifests.

https://reviewboard.mozilla.org/r/179024/#review184490

> We may not be ready for this one, since `static-analysis-linux64-st-an/opt` is failing on try, but that doesn't need to block the rest of the patches.

This is passing `try -b do -p all` with additional changes. There are probably other issues, but ready for review.
Attachment #8906179 - Flags: review?(mh+mozilla)
Attachment #8907361 - Flags: review?(mh+mozilla)
Attachment #8907362 - Flags: review?(mh+mozilla)
Attachment #8907358 - Flags: review?(mh+mozilla)
Attachment #8907677 - Flags: review?(mh+mozilla)
Attachment #8907739 - Flags: review?(mh+mozilla)
Attachment #8907740 - Flags: review?(mh+mozilla)
Attachment #8907741 - Flags: review?(mh+mozilla)
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review184852

You should do the same modification to the part that handles windows workers.

::: taskcluster/taskgraph/transforms/job/toolchain.py:145
(Diff revision 4)
> -            run['script'])
> +                run['script'])
> -    ]
> +        )
> +
> +    args = run.get('arguments')
> +    if args:
> +        worker['command'][-1] += ' ' + args

you'll need some shell quoting here (from mozbuild.shellutil import quote as shell_quote)
Attachment #8907353 - Flags: review?(mh+mozilla)
Comment on attachment 8907354 [details]
Bug 1391427 - Add script 'arguments' key to toolchain tasks.

https://reviewboard.mozilla.org/r/179016/#review184856

::: taskcluster/taskgraph/transforms/job/toolchain.py:38
(Diff revision 4)
> +    # Python scripts are invoked with `mach python` so vendored libraries
> +    # are available.
>      Required('script'): basestring,
>  
> +    # Arguments to pass to the script.
> +    Optional('arguments'): basestring,

This should be in the previous patch.

::: taskcluster/taskgraph/transforms/job/toolchain.py:73
(Diff revision 4)
>      tooltool_manifest = taskdesc['worker']['env'].get('TOOLTOOL_MANIFEST')
>      if tooltool_manifest:
>          files.append(tooltool_manifest)
>  
> -    digest = hash_paths(GECKO, files)
> +    # Accumulate dependency hashes for index generation.
> +    data = [hash_paths(GECKO, files)]

This is not related.
Attachment #8907354 - Flags: review?(mh+mozilla)
Comment on attachment 8907354 [details]
Bug 1391427 - Add script 'arguments' key to toolchain tasks.

https://reviewboard.mozilla.org/r/179016/#review184856

> This is not related.

Err,  I missed the arguments being added.
Comment on attachment 8906178 [details]
Bug 1391427 - Port the repack_rust script to taskcluster.

https://reviewboard.mozilla.org/r/177932/#review184860

This is a lot of code. I wish rustup could do most of that work, but afaict, it's not really helpful for this :(

::: taskcluster/scripts/misc/repack_rust.py:36
(Diff revision 5)
> +def sha256sum():
> +    '''Return the command for verifying SHA-2 256-bit checksums.'''
> +    if sys.platform.startswith('darwin'):
> +        return 'shasum'
> +    else:
> +        return 'sha256sum'

you could compute a sha256 while downloading, with hashlib.sha256().

::: taskcluster/scripts/misc/repack_rust.py:140
(Diff revision 5)
> +Mve696B5tlHyc1KxjHR6w9GRsh4=
> +=5FXw
> +-----END PGP PUBLIC KEY BLOCK-----
> +'''
> +    gpg = subprocess.Popen(['gpg', '--import'], stdin=subprocess.PIPE)
> +    gpg.communicate(key)

you should gpg.wait() both processes.

::: taskcluster/scripts/misc/repack_rust.py:172
(Diff revision 5)
> +    install_cmd = [os.path.join(basename, 'install.sh')]
> +    install_cmd += ['--prefix=' + os.path.abspath(target)]
> +    install_cmd += ['--disable-ldconfig']
> +    subprocess.check_call(install_cmd)
> +    log('Cleaning %s...' % basename)
> +    subprocess.check_call(['rm', '-rf', basename])

shutil.rmtree

::: taskcluster/scripts/misc/repack_rust.py:263
(Diff revision 5)
> +    upload_dir = os.environ.get('UPLOAD_DIR')
> +    if upload_dir:
> +        # Create the upload directory if it doesn't exist.
> +        try:
> +            log('Creating upload directory in %s...' % os.path.abspath(upload_dir))
> +            os.mkdir(upload_dir)

os.makedirs
Attachment #8906178 - Flags: review?(mh+mozilla)
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review184866

::: taskcluster/ci/toolchain/linux.yml:188
(Diff revision 5)
>      run:
>          using: toolchain-script
>          script: build-libdmg-hfsplus.sh
>          toolchain-artifact: public/build/dmg.tar.xz
>  
> +linux64-rust:

please version this and add a toolchain-alias.
Comment on attachment 8907355 [details]
Experiment repacking rust-win64 on a win2012 host.

https://reviewboard.mozilla.org/r/179018/#review184868
Attachment #8907355 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review184870

::: taskcluster/ci/toolchain/linux.yml:208
(Diff revision 5)
> +        script: repack_rust.py
> +        arguments: >
> +          --channel 1.19.0
> +          --host linux64
> +          --target linux64 --target linux32
> +        toolchain-artifact: public/build/rustc-x86_64-unknown-linux-gnu-repack.tar.xz

The artifact should be rustc.tar.xz to actually replace the one from tooltool. (and it should unpack to a rustc/ directory)
Comment on attachment 8907356 [details]
Bug 1391427 - Link toolchain rust to linux sccache build.

https://reviewboard.mozilla.org/r/179020/#review184872

::: taskcluster/ci/toolchain/linux.yml:229
(Diff revision 4)
>          resources:
>              - 'taskcluster/scripts/misc/tooltool-download.sh'
>          toolchain-artifact: public/build/sccache2.tar.xz
>      toolchains:
>          - linux64-clang-3.9
> +        - linux64-rust

Maybe this should use an explicit version, and we'd keep an old version around to build sccache and avoid rebuilding sccache every time we update rust.
Attachment #8907356 - Flags: review?(mh+mozilla)
Comment on attachment 8907357 [details]
Bug 1391427 - Link toolchain rust to spidermonkey tasks.

https://reviewboard.mozilla.org/r/179022/#review184874
Attachment #8907357 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907361 [details]
Bug 1391427 - Link toolchain rust to static-analysis tasks.

https://reviewboard.mozilla.org/r/179028/#review184876
Attachment #8907361 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907362 [details]
Bug 1391427 - Link toolchain rust to valgrind tasks.

https://reviewboard.mozilla.org/r/179030/#review184878

All these "Link toolchain rust to $foo tasks" would have been faster to review as one single patch. With separate patches, most of the time reviewing is spent clicking around in mozreview.
Attachment #8907362 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907358 [details]
Bug 1391427 - Remove rust from linux tooltool manifests.

https://reviewboard.mozilla.org/r/179024/#review184884
Attachment #8907358 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907355 [details]
Experiment repacking rust-win64 on a win2012 host.

https://reviewboard.mozilla.org/r/179018/#review184886

::: taskcluster/ci/build/linux.yml:183
(Diff revision 4)
>          tooltool-downloads: public
>          need-xvfb: true
>      toolchains:
>          - linux64-clang-3.9
>          - linux64-gcc-4.9
>          - linux64-sccache

you missed this one (and it should use an explicit version)

::: taskcluster/ci/build/linux.yml:213
(Diff revision 4)
>          tooltool-downloads: public
>          need-xvfb: true
>      toolchains:
>          - linux64-clang-3.9
>          - linux64-gcc-4.9
>          - linux64-sccache

and this one.

I wish -p all really did all.
Attachment #8907355 - Flags: review+
Comment on attachment 8907677 [details]
Bug 1391427 - Use toolchain rust for macOS builds.

https://reviewboard.mozilla.org/r/179344/#review184888
Attachment #8907677 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907739 [details]
Bug 1391427 - Use toolchain rust builds for windows tasks.

https://reviewboard.mozilla.org/r/179412/#review184892

::: taskcluster/ci/toolchain/windows.yml:112
(Diff revision 3)
> +        max-run-time: 7200
> +        env:
> +          UPLOAD_DIR: artifacts
> +    run:
> +        using: toolchain-script
> +        script: repack_rust.py

I don't know how this can work without changes to windows_toolchain in job/toolchain.py.

::: taskcluster/ci/toolchain/windows.yml:115
(Diff revision 3)
> +    run:
> +        using: toolchain-script
> +        script: repack_rust.py
> +        arguments: >
> +          --channel 1.19.0
> +          --host win64

We should be able to use a single --host win64 --target win32 --target win64.

::: taskcluster/ci/toolchain/windows.yml:117
(Diff revision 3)
> +        script: repack_rust.py
> +        arguments: >
> +          --channel 1.19.0
> +          --host win64
> +          --target win64
> +        toolchain-artifact: public/build/rustc-x86_64-pc-windows-msvc-repack.tar.bz2

rustc.tar.bz2
Attachment #8907739 - Flags: review?(mh+mozilla)
Comment on attachment 8907740 [details]
Bug 1391427 - Use toolchain rust builds for android tasks.

https://reviewboard.mozilla.org/r/179414/#review184894

::: taskcluster/ci/toolchain/linux.yml:212
(Diff revision 3)
>            --target linux64 --target linux32
>            --target x86_64-apple-darwin
>          toolchain-artifact: public/build/rustc-x86_64-unknown-linux-gnu-repack.tar.xz
>  
> +linux64-rust-android:
> +    description: "rust minimum supported version repack"

weird description

::: taskcluster/ci/toolchain/linux.yml:232
(Diff revision 3)
> +          --target armv7-linux-androideabi
> +          --target aarch64-linux-android
> +          --target i686-linux-android

why a separate package for this, but not for osx?

::: taskcluster/ci/toolchain/linux.yml:235
(Diff revision 3)
> +          --host linux64
> +          --target linux64
> +          --target armv7-linux-androideabi
> +          --target aarch64-linux-android
> +          --target i686-linux-android
> +        toolchain-artifact: public/build/rustc-x86_64-unknown-linux-gnu-android-cross-repack.tar.xz

rustc.tar.xz
Attachment #8907740 - Flags: review?(mh+mozilla)
Comment on attachment 8907741 [details]
Bug 1391427 - Use toolchain rust for min version check.

https://reviewboard.mozilla.org/r/179416/#review184900

::: taskcluster/ci/build/linux.yml:214
(Diff revision 3)
>          tooltool-downloads: public
>          need-xvfb: true
>      toolchains:
>          - linux64-clang-3.9
>          - linux64-gcc-4.9
> +        - linux64-rust-min

would be better if it actually used the version number.

::: taskcluster/ci/toolchain/linux.yml:255
(Diff revision 3)
> +          --host linux64
> +          --target linux64

In fact, it would be better if it was exactly the same arguments as the non minimum version. Why? because then it doesn't need to be rebuilt at all (because it will match previous builds of that version).

::: taskcluster/ci/toolchain/linux.yml:257
(Diff revision 3)
> +        script: repack_rust.py
> +        arguments: >
> +          --channel 1.19.0
> +          --host linux64
> +          --target linux64
> +        toolchain-artifact: public/build/rustc-x86_64-unknown-linux-gnu-repack.tar.xz

rustc.tar.xz

I realized I haven't said why: there are multiple reasons:
- when you still have the rustc package in the tooltool manifest, it's not overridden if the file name is different.
- the contract is that $foo.tar.$ext extracts to $foo (like with tooltool), and mozconfigs expect rustc/ ; technically, rustc-....tar.xz should extract to rustc-....
- since the tooltool manifest is not overridden and since both rustc.tar.xz and rustc-....tar.xz extract to rustc/, when the tooltool manifest still has the rustc package, you'll randomly get either the tooltool package or the toolchain artifact in rustc/, depending how the .tar.xz file names are ordered in the corresponding python hashtable.
Attachment #8907741 - Flags: review?(mh+mozilla)
Comment on attachment 8907741 [details]
Bug 1391427 - Use toolchain rust for min version check.

https://reviewboard.mozilla.org/r/179416/#review186176
Attachment #8907356 - Attachment is obsolete: true
Attachment #8907357 - Attachment is obsolete: true
Attachment #8907361 - Attachment is obsolete: true
Attachment #8907362 - Attachment is obsolete: true
Attachment #8907358 - Attachment is obsolete: true
Attachment #8907677 - Attachment is obsolete: true
Attachment #8907739 - Attachment is obsolete: true
Attachment #8907740 - Attachment is obsolete: true
Attachment #8907741 - Attachment is obsolete: true
Attachment #8907355 - Attachment is obsolete: true
Attachment #8907355 - Flags: review?(mh+mozilla)
Comment on attachment 8906178 [details]
Bug 1391427 - Port the repack_rust script to taskcluster.

https://reviewboard.mozilla.org/r/177932/#review186072

::: taskcluster/scripts/misc/repack_rust.py:36
(Diff revision 5)
> +def sha256sum():
> +    '''Return the command for verifying SHA-2 256-bit checksums.'''
> +    if sys.platform.startswith('darwin'):
> +        return 'shasum'
> +    else:
> +        return 'sha256sum'

Yes, and this makes it easier to add validation against the manifest, which I've been wanting to do for a while.

I added something simple. Parsing the sha256sum file format adds more code than it saves, unfortunately.

::: taskcluster/scripts/misc/repack_rust.py:140
(Diff revision 5)
> +Mve696B5tlHyc1KxjHR6w9GRsh4=
> +=5FXw
> +-----END PGP PUBLIC KEY BLOCK-----
> +'''
> +    gpg = subprocess.Popen(['gpg', '--import'], stdin=subprocess.PIPE)
> +    gpg.communicate(key)

Is this so we can check the return value? The documentation says `.communicate()` waits for the process to terminate.

I've added an `if gpg.wait(): raise CalledProcessError` clause to each invocation.

::: taskcluster/scripts/misc/repack_rust.py:172
(Diff revision 5)
> +    install_cmd = [os.path.join(basename, 'install.sh')]
> +    install_cmd += ['--prefix=' + os.path.abspath(target)]
> +    install_cmd += ['--disable-ldconfig']
> +    subprocess.check_call(install_cmd)
> +    log('Cleaning %s...' % basename)
> +    subprocess.check_call(['rm', '-rf', basename])

Ok.

::: taskcluster/scripts/misc/repack_rust.py:263
(Diff revision 5)
> +    upload_dir = os.environ.get('UPLOAD_DIR')
> +    if upload_dir:
> +        # Create the upload directory if it doesn't exist.
> +        try:
> +            log('Creating upload directory in %s...' % os.path.abspath(upload_dir))
> +            os.mkdir(upload_dir)

Thanks. Still requires the `except` clause, sadly.
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review186248
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review186250

::: taskcluster/ci/toolchain/linux.yml:238
(Diff revision 8)
> +           '--channel', '1.19.0',
> +           '--suffix', 'macos-cross',
> +           '--host', 'linux64',
> +           '--target', 'linux64',
> +           '--target', 'x86_64-apple-darwin',
> +        ]

Per IRC discussion, I made the arguments key a sequence type, and then run each individual argument string through `mozbuild.shellutil.quote` when constructing the final command line.

I still think this is a poor design. Requiring yml sequence separators and quoting on entry just in case someone in the future wants to pass an argument which needs quoting on the command line seems silly when the data ends up serialized into a string in the generated task anyway. I think it's simpler to make this a free-form string to be appended to the command line in the style of the `CFLAGS` environment variable, and just rely on shell quoting within the argument string when necessary. Just as transparent but the common case is easier to read.
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review186944
Attachment #8907353 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review186946

::: taskcluster/taskgraph/transforms/job/toolchain.py:195
(Diff revision 6)
> +
>      bash = r'c:\mozilla-build\msys\bin\bash'
>      worker['command'] = [
>          ' '.join(hg_command),
>          # do something intelligent.
> -        r'{} -c ./build/src/taskcluster/scripts/misc/{}'.format(bash, run['script'])
> +        r'{} -c {} ./build/src/taskcluster/scripts/misc/{}'.format(

Actually, I doubt this works. `bash -c mach python foo` is not going to call `mach` with the arguments `python` and `foo`. `bash -c "mach python foo"` would, but you're entering a world of quoting pain.

Fortunately, all those taskcluster scripts should work without -c (as in `bash $script` should work), and calling `bash mach python foo` should work too.
Attachment #8907353 - Flags: review+
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review186958

::: taskcluster/taskgraph/transforms/job/toolchain.py:134
(Diff revision 6)
> +            'cd /builds/worker && ./workspace/build/src/mach python'
> +            ' workspace/build/src/taskcluster/scripts/misc/{}'.format(
> +                run['script'])

Come to think of it, the repetition is not very nice. The same wrapper thing as windows would probably be nicer.
Comment on attachment 8907354 [details]
Bug 1391427 - Add script 'arguments' key to toolchain tasks.

https://reviewboard.mozilla.org/r/179016/#review186962
Attachment #8907354 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8906178 [details]
Bug 1391427 - Port the repack_rust script to taskcluster.

https://reviewboard.mozilla.org/r/177932/#review186970

::: taskcluster/scripts/misc/repack_rust.py:149
(Diff revision 7)
> +'''
> +    gpg = subprocess.Popen(['gpg', '--import'], stdin=subprocess.PIPE)
> +    gpg.communicate(key)
> +    if gpg.wait():
> +        log('Error importing signing key!')
> +        raise subprocess.CalledProcessError

you're raising a class, you should raise an instance.

::: taskcluster/scripts/misc/repack_rust.py:156
(Diff revision 7)
> +                           '--edit-key', keyid],
> +                           stdin=subprocess.PIPE)
> +    gpg.communicate('trust\n5\ny\n')
> +    if gpg.wait():
> +        log('Error marking signing key as trusted!')
> +        raise subprocess.CalledProcessError

likewise

::: taskcluster/scripts/misc/repack_rust.py:161
(Diff revision 7)
> +        raise subprocess.CalledProcessError
> +
> +
> +def verify_sha(filename, sha):
> +    '''Verify that the checksum file matches the given sha digest.'''
> +    sha_filename = filename + '.sha256'

Note, that's actually rather pointless. Gpg validation, in practice, already does that. Except the gpg signatures from rust-lang.org are using a sha1 digest... (filed https://github.com/rust-lang/rust/issues/44714)

::: taskcluster/scripts/misc/repack_rust.py:365
(Diff revision 7)
> +    if name in platforms.keys():
> +        # Return the matching platform string.
> +        return platforms[name]
> +    else:
> +        # No match; just pass through the unknown name.
> +        return name

return platforms.get(name, name)
Attachment #8906178 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review186978

::: taskcluster/ci/toolchain/linux.yml:232
(Diff revision 8)
> +        arguments: [
> +           '--channel', '1.19.0',
> +           '--suffix', 'macos-cross',
> +           '--host', 'linux64',
> +           '--target', 'linux64',
> +           '--target', 'x86_64-apple-darwin',
> +        ]

Please either use this style for the other jobs too, or the other style here.

::: taskcluster/ci/toolchain/linux.yml:236
(Diff revision 8)
> +           '--target', 'linux64',
> +           '--target', 'x86_64-apple-darwin',

The mix of full triplets vs. taskcluster build platforms is not very helpful imho. This should be all one or the other.

::: taskcluster/ci/toolchain/windows.yml:115
(Diff revision 8)
> +          --host, win64,
> +          --target, win64,

as per previous review, we shouldn't need more than one windows rust package: --host win64 --target win64 --target win32.
Attachment #8906179 - Flags: review?(mh+mozilla)
Blocks: 1401647
Blocks: 1401789
Comment on attachment 8906178 [details]
Bug 1391427 - Port the repack_rust script to taskcluster.

https://reviewboard.mozilla.org/r/177932/#review186970

> you're raising a class, you should raise an instance.

Oops, thanks. I added a wrapper to take care of this.

> Note, that's actually rather pointless. Gpg validation, in practice, already does that. Except the gpg signatures from rust-lang.org are using a sha1 digest... (filed https://github.com/rust-lang/rust/issues/44714)

Good catch on the SHA-1 digests!

It doesn't provide much extra integrity checking, but in the past it's been useful as validation of what the rust packaging is providing, so I think it's helpful to verify the various channels they support while we're here.

> return platforms.get(name, name)

Much better, thanks.
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review186246

::: taskcluster/taskgraph/transforms/job/toolchain.py:196
(Diff revision 6)
>      bash = r'c:\mozilla-build\msys\bin\bash'
>      worker['command'] = [
>          ' '.join(hg_command),
>          # do something intelligent.
> -        r'{} -c ./build/src/taskcluster/scripts/misc/{}'.format(bash, run['script'])
> +        r'{} -c {} ./build/src/taskcluster/scripts/misc/{}'.format(
> +            bash, wrapper, run['script'])

Unfortunately, this doesn't work. `mach` is invoked but doesn't recognize its argument list as containing a valid command.

I tried a few variations but wasn't able to figure it out today. I'll see if I can make progress with a loaner. However, this doesn't need to block this bug; the repack job for windows-hosted toolchains works equally well on a linux-docker worker type.
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review186978

> as per previous review, we shouldn't need more than one windows rust package: --host win64 --target win64 --target win32.

Sorry, it looks like reviewboard at my response to this in the earlier review. Building win32 with the win64-hosted toolchain fails in the `heapsize` crate's `build.rs` script. Having them separate here continues how I built the tooltool packages we're currently using.

I looked at the issue more today but couldn't reproduce locally. I opened bug 1401647 to address this issue later.
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review187352
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review187434

::: taskcluster/taskgraph/transforms/job/toolchain.py:138
(Diff revision 8)
>          '--vcs-checkout=/builds/worker/workspace/build/src',
>          '--',
>          'bash',
>          '-c',
>          'cd /builds/worker && '
> -        './workspace/build/src/taskcluster/scripts/misc/{}'.format(
> +        '{} workspace/build/src/taskcluster/scripts/misc/{}'.format(

you could move the space to wrapper.

::: taskcluster/taskgraph/transforms/job/toolchain.py:139
(Diff revision 8)
>          '--',
>          'bash',
>          '-c',
>          'cd /builds/worker && '
> -        './workspace/build/src/taskcluster/scripts/misc/{}'.format(
> -            run['script'])
> +        '{} workspace/build/src/taskcluster/scripts/misc/{}'.format(
> +                wrapper, run['script'])

seems like too much indentation.

::: taskcluster/taskgraph/transforms/job/toolchain.py:182
(Diff revision 8)
> +    # Use `mach` to invoke python scripts so in-tree libraries are available.
> +    if run['script'].endswith('.py'):
> +        wrapper = 'build/src/mach python'
> +    else:
> +        wrapper = ''
> +
>      bash = r'c:\mozilla-build\msys\bin\bash'
>      worker['command'] = [
>          ' '.join(hg_command),
>          # do something intelligent.
> -        r'{} -c ./build/src/taskcluster/scripts/misc/{}'.format(bash, run['script'])
> +        r'{} {} build/src/taskcluster/scripts/misc/{}'.format(
> +            bash, wrapper, run['script'])

If you haven't managed to make this work and don't want to block on it working on windows, don't do this. Make it throw an error if a python script is given to a windows job.
Attachment #8907353 - Flags: review?(mh+mozilla)
Comment on attachment 8906178 [details]
Bug 1391427 - Port the repack_rust script to taskcluster.

https://reviewboard.mozilla.org/r/177932/#review187438

::: taskcluster/scripts/misc/repack_rust.py:389
(Diff revision 9)
> +    parser.add_argument('--suffix',
> +                        help='suffix to append to the tarball filename.'
> +                             ' Useful for distinguising different target combinations.')

This switch is not used anymore.
Comment on attachment 8906179 [details]
Bug 1391427 - Package upstream rust in taskcluster.

https://reviewboard.mozilla.org/r/177934/#review187440

::: taskcluster/ci/toolchain/linux.yml:231
(Diff revision 10)
> +    run:
> +        using: toolchain-script
> +        script: repack_rust.py
> +        arguments: [
> +           '--channel', '1.19.0',
> +           '--suffix', 'macos-cross',

this is not useful anymore

::: taskcluster/ci/toolchain/linux.yml:257
(Diff revision 10)
> +    run:
> +        using: toolchain-script
> +        script: repack_rust.py
> +        arguments: [
> +           '--channel', '1.19.0',
> +           '--suffix', 'android-cross',

this is not useful anymore
Attachment #8906179 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review187434

> If you haven't managed to make this work and don't want to block on it working on windows, don't do this. Make it throw an error if a python script is given to a windows job.

Ok. I'll raise a `NotImplementError` for now.
Comment on attachment 8906178 [details]
Bug 1391427 - Port the repack_rust script to taskcluster.

https://reviewboard.mozilla.org/r/177932/#review187438

> This switch is not used anymore.

I've removed it.
After responding to review comments I rebased and added a win64-rust toolchain to the windows spidermonkey tasks for sm-plain-windows and sm-compating-windows jobs. The patchset is green on try again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0061f5df2bb49179f5738235f39b20e07eb5a6ab
Comment on attachment 8907353 [details]
Bug 1391427 - Execute python toolchain scripts with mach.

https://reviewboard.mozilla.org/r/179014/#review189076
Attachment #8907353 - Flags: review?(mh+mozilla) → review+
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1db94c2f3321
Execute python toolchain scripts with mach. r=glandium
https://hg.mozilla.org/integration/autoland/rev/5e5e9ad6ed9a
Add script 'arguments' key to toolchain tasks. r=glandium
https://hg.mozilla.org/integration/autoland/rev/c97badfbe3cf
Port the repack_rust script to taskcluster. r=glandium
https://hg.mozilla.org/integration/autoland/rev/f069d6b4d486
Package upstream rust in taskcluster. r=glandium
Product: Taskcluster → Taskcluster Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: