1198179 - TaskCluster build-linux.sh artifact support broken

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[(not currently active) Ted Mielczarek]

My cross-mac build in TC was almost successful, but failed copying build artifacts out:
```
for file in $DIST_TARGET_UPLOADS
do
    mv *.$file $HOME/artifacts/target.$file
done
++ for file in '$DIST_TARGET_UPLOADS'
++ mv firefox-43.0a1.en-US.mac64.dmg /home/worker/artifacts/target.mac64.dmg
++ for file in '$DIST_TARGET_UPLOADS'
++ mv firefox-43.0a1.en-US.mac64.json /home/worker/artifacts/target.mac64.json
++ for file in '$DIST_TARGET_UPLOADS'
++ mv firefox-43.0a1.en-US.mac64.common.tests.zip firefox-43.0a1.en-US.mac64.cppunittest.tests.zip firefox-43.0a1.en-US.mac64.mochitest.tests.zip firefox-43.0a1.en-US.mac64.reftest.tests.zip firefox-43.0a1.en-US.mac64.web-platform.tests.zip firefox-43.0a1.en-US.mac64.xpcshell.tests.zip /home/worker/artifacts/target.tests.zip
mv: target '/home/worker/artifacts/target.tests.zip' is not a directory
```

It's failing because bug 917999 split parts of tests.zip out into separate zip files, so having "tests.zip" in DIST_TARGET_UPLOADS is causing us to try to `mv *tests.zip $HOME/artifacts/target.tests.zip`, which is failing because *tests.zip expands to multiple files.

Comment 1

[(not currently active) Ted Mielczarek]

Morgan: you mentioned on IRC that you were looking into doing something better with uploads. Does that cover this?

Comment 2

[Morgan Phillips [:mrrrgn]]

Attachment: build_artifacts_mover.py

I found this in my "utilities folder," I believe it's what I had written to solve this problem.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: build_artifacts_mover.py

s/mach/mozharness/g

Comment 4

[(not currently active) Ted Mielczarek]

Unfortunately mshal literally just changed this last week in bug 1118778 so that the uploadFiles property is set from upload.py as part of `make upload`:
https://dxr.mozilla.org/mozilla-central/source/build/upload.py#200

...so this won't work because we're not calling `make upload` on TC. :-/

That being said, I think this is the right approach, we just need to wire up "have the build system emit info about what it wants to upload" in some other fashion. We could tweak `make upload` to have an option to simply upload files to a local directory, and set that for taskcluster builds so that the build system would do the work for us here. The only fiddly bit is the renaming we're currently doing, we'll still have to sort something out there.

Comment 5

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1198179-Kill-gen_mach_buildprops.py.patch

I was already fiddling with upload.py. This is somewhat of a precursor patch that just kills gen_mach_buildprops.py and does all the properties stuff in upload.py

Comment 6

[Michael Shal [:mshal]]

Attachment: 0002-Bug-1198179-make-upload.py-write-properties-even-if-.patch

This reworks upload.py to always spit out a properties file, even if there are no UPLOAD_HOST/UPLOAD_USER environment settings. In this case, you'd just get the packageFilename and uploadFiles properties. So TC builds should be able to remove the UPLOAD_* environment settings and still get the properties out. Does that seem feasible?

Comment 7

[(not currently active) Ted Mielczarek]

That sounds pretty good. It doesn't *completely* solve our problems with TC, in that TC wants to rename the build files to $target.whatever.ext so that the routes are easy to bake into the task descriptions (without version numbers). I don't think it'd be hard for the build system to stick that extra info into the properties file, though. Maybe an additional annotation on files specifying what route they should have?

Comment 8

[Michael Shal [:mshal]]

Yeah, it seems like upload-files.mk has most of the information we need here. It already has variables for PACKAGE, INSTALLER_PACKAGE, COMPLETE_MAR, etc. I think we'd probably need a better way to export that info either to upload.py or directly into some other properties file. Right now only $(PACKAGE) is passed into upload.py to get the packageFilename property.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8652911 [details] [diff] [review]
0001-Bug-1198179-Kill-gen_mach_buildprops.py.patch

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

Sorry, nitpicked the Python code a bit before realizing you were copying it from gen_mach_buildprops.py.

::: build/upload.py
@@ +127,5 @@
> +
> +    try:
> +        # open in binary mode to make sure we get consistent results
> +        # across all platforms
> +        f = open(filename, "rb")

We generally use `with open(filename, "rb") as f:`

@@ +133,5 @@
> +        sha512Hash = shaObj.hexdigest()
> +
> +        size = os.path.getsize(filename)
> +    except:
> +        pass

It'd be good to at least log the exception here, unless you expect us to hit this during every execution for some reason. Actually, you're already checking for file existence in GetMarProperties so maybe this should be fatal?

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8652916 [details] [diff] [review]
0002-Bug-1198179-make-upload.py-write-properties-even-if-.patch

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

This is good, but I'd like to take it one step further and give upload.py the ability to copy files to a local directory, so Taskcluster builds could simply set a variable and have the build artifacts wind up where it wants them. The build system knows all the files it wants to upload, so it makes sense to do things there.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36cd6ef44a0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b41a88070b1b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36cd6ef44a0d
https://hg.mozilla.org/mozilla-central/rev/b41a88070b1b

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

After much discussion in #releng, we've boiled this down into two phases, with the first better-defined than the second.  There are three modes we need to support:

 1. BB-based build jobs, BB-baseed test jobs (current configuration)
 2. TC-based build jobs, BB-based test jobs (via BBB)
 3. TC-based build jobs, TC-based test jobs

Greg has pointed out that the typical way to link build and test jobs in TC is for the build job to have a static description of its outputs in the task definition itself.  The task-graph creation code then uses this information to create appropriate artifact URLs which it substitutes into the test task definitions.  We'd like to use the same approach for consistency.

So here's the plan:

In TC jobs, re-enable MOZ_AUTOMATION_UPLOAD, and add some env-based config that "stubs out" the upload to just copy files to UPLOAD_PATH(=~/artifacts) locally, and doesn't generate properties at all.  Create simple package names (bug 1203573), and specify those names in the build task definition.

Next, we'll write task definitions for tests that stuff the artifact URLs into Buildbot properties (via BBB).

When it's time to start running test jobs in TC (mode 3), we still have the URLs we need available to the new task definitions.

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

Since Ehsan asked -- I'm hoping to get this patched today, and probably landed on Monday.  But I'll be at a work-week next week, so if things get "complicated" it'll likely take until the week after.

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

https://tools.taskcluster.net/task-inspector/#cwbX0WlTREyzyNQON0YAUA/0

Comment 16

[Dustin J. Mitchell [:dustin] (he/him)]

https://tools.taskcluster.net/task-inspector/#d0faHfaIRsa6b5Y5gmHqHQ/0

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

Here's where I'm at: I've modified upload.py to special case UPLOAD_HOST=localhost to just copy files to UPLOAD_PATH.  In this case, there's no post_upload_command and no properties generated.  I removed MOZ_AUTOMATION_UPLOAD=0 so `make upload` will run, and added MOZ_SIMPLE_PACKAGE_NAME=target (although I haven't checked whether your patch is an ancestor of this).

https://tools.taskcluster.net/task-inspector/#JJxRA2iPRpqVA60VsxHpQw/0
https://hg.mozilla.org/users/dmitchell_mozilla.com/mozilla-central/rev/7af2cb9be8c3

I think all the ducks are lined up here, and it's just squashing stupid typos after long try runs.

I'll be at a work-week for the next week, so reassigning to Ted in hopes he can put this over the finish line (or r+ and land if that try job goes green).

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

Indentation error caused a failure, but the uploads ran.  Fixed:
  https://tools.taskcluster.net/task-inspector/#JJxRA2iPRpqVA60VsxHpQw/0
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c442b8efe9d

.. that worked, but failed doing the *old* DIST_* stuff I forgot to remove

Comment 19

[Dustin J. Mitchell [:dustin] (he/him)]

I figure this next run will succeed, so I'm marking it for review.  If it does succeed and passes review, please go ahead and land it.
  https://tools.taskcluster.net/task-inspector/#QBNCUEc1TiCjyfBfyKnR2A/0

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: MozReview Request: Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal

Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal

Comment 21

[Dustin J. Mitchell [:dustin] (he/him)]

GREEN BUILD! :)

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 8660370 [details]
MozReview Request: Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal

https://reviewboard.mozilla.org/r/19121/#review17009

This looks good, we just need to change the Mac task definition to match.

::: build/upload.py:20
(Diff revision 1)
> +# properties are written out.

We decided that BBB was going to be responsible for generating the properties file from the Task data, correct?

::: build/upload.py:256
(Diff revision 1)
> +        remote_path = GetRemotePath(path, file, base_path)

Feels weird to be using `GetRemotePath` to construct a local path, even if it's doing the right thing. Can you at least change the comment to not say "remotely"?

::: testing/taskcluster/scripts/builder/build-linux.sh
(Diff revision 1)
> -done

All the removals from this script are great!

::: testing/taskcluster/tasks/builds/linux64_clobber.yml:50
(Diff revision 1)
> -      build: 'public/build/target.linux-x86_64.tar.bz2'
> +      build: 'public/build/target.tar.bz2'

We need to make this same change in the Mac task definitions.

We're sure nothing is depending on the existing naming, right?

::: testing/taskcluster/tasks/builds/linux64_clobber.yml:51
(Diff revision 1)
>        tests: 'public/build/target.tests.zip'

We could probably pull locations:tests up into the base build.yml, unless our yaml inheritance doesn't let you append to lists.

::: testing/taskcluster/tasks/builds/opt_macosx64.yml:59
(Diff revision 1)
>        build: 'public/build/target.mac64.dmg'

This needs to be changed, as per above.

Comment 23

[Michael Shal [:mshal]]

Comment on attachment 8660370 [details]
MozReview Request: Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal

ted got to it first :)

Comment 24

[Dustin J. Mitchell [:dustin] (he/him)]

I've updated the "remote path" bits (just renaming).  I didn't update the mac builds, but that should be straightforward.

Since I don't trust myself:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0526ce5cc6f
If that's green, ship it.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7b4b4b7474

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd7b4b4b7474