Give mozharness a list of artifacts to upload

RESOLVED FIXED in Firefox -esr31, Firefox OS v2.0

Status

Firefox Build System
General
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: mshal, Assigned: mshal)

Tracking

Trunk
mozilla37
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr31 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed, b2g-v2.1S fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
For bug 1100624, we will be pushing build artifacts to s3 from mozharness, so we need a way to communicate to mozharness what files should be uploaded. Right now I'm leaning towards writing the contents of UPLOAD_FILES into mach_build_properties.json, though we could also move all build artifacts into a specific directory.
The latter definitely sounds simpler. Do we still have things that need to preserve relative paths? (I can't remember.)

It might also be nice at some point to move the definition of UPLOAD_FILES out of a Makefile maybe into moz.build, but I don't know how much work that is.
(Assignee)

Comment 2

4 years ago
My only real concern with a specific upload directory is needing to have extra I/O copying things around (though I guess we can get away with symlinks on some platforms), or re-writing build rules to output in a specific directory. Probably not a big deal though - I'll give it a try.

The mach_build_properties.json route has the downside of needing to know UPLOAD_FILES, which is currently defined in packager.mk. Including that in moz-automation.mk results in conflicting build targets :/. I think it's possible to move all the variables out into package-name.mk and just keep the rules in packager.mk, though.

The UPLOAD_FILES logic is most of packager.mk - I'd prefer not to block this bug on a moz.build conversion if we can avoid it, since there's already a lot of moving pieces here.

Comment 3

4 years ago
Write out files containing a list of absolute paths to a well-defined location? Solving this with |echo filename >> file| seems like a pretty easy path forward.
(Assignee)

Comment 4

4 years ago
(In reply to Gregory Szorc [:gps] from comment #3)
> Write out files containing a list of absolute paths to a well-defined
> location? Solving this with |echo filename >> file| seems like a pretty easy
> path forward.

This is pretty much what we'd achieve by putting it in mach_build_properties.json - that's how we communicate other bits of data from the build system to mozharness, so it would mostly be getting access to UPLOAD_FILES and adding a new argument to gen_mach_buildprops.py.
(Assignee)

Comment 5

4 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Do we still have things that need to
> preserve relative paths? (I can't remember.)

I forgot to ask - what things are you concerned about here?
(In reply to Michael Shal [:mshal] from comment #5)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> > Do we still have things that need to
> > preserve relative paths? (I can't remember.)
> 
> I forgot to ask - what things are you concerned about here?

I'm thinking specifically of PKG_PATH I believe
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#100
which is used in a few paths:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#875

and we preserve relative paths in upload.py:
http://dxr.mozilla.org/mozilla-central/source/build/upload.py#101
(Assignee)

Updated

4 years ago
Assignee: nobody → mshal
(Assignee)

Comment 7

4 years ago
Created attachment 8533968 [details] [diff] [review]
0001-Bug-1109136-Move-variable-definitions-from-packager..patch

For the put-files-in-mach-build-properties route, we need access to UPLOAD_FILES (and later SOURCE_UPLOAD_FILES). This pulls out the data into upload-files.mk. We can't re-use package-name.mk for this since some other Makefiles include it, and it messes with DIST_FILES.
Attachment #8533968 - Flags: review?(mh+mozilla)
(Assignee)

Comment 8

4 years ago
Created attachment 8533969 [details] [diff] [review]
0002-Bug-1109136-add-upload-artifact-list-to-mach_build_p.patch

Add uploadFiles to the json file. Then mozharness can pull out the file list and upload them to s3.
Attachment #8533969 - Flags: review?(mh+mozilla)
(Assignee)

Comment 9

4 years ago
I should mention, I've no idea of the impact on #c6 on these patches yet. As far as I can tell, it looks like all files get uploaded into a flat directory hierarchy. I'll need to dig a bit more, but if anyone has more knowledge about that please let me know!
Comment on attachment 8533968 [details] [diff] [review]
0001-Bug-1109136-Move-variable-definitions-from-packager..patch

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

Could you reemit this patch with upload-files.mk starting as a copy of packager.mk? That would make the review so much easier.
Attachment #8533968 - Flags: review?(mh+mozilla)
Attachment #8533969 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8534614 [details] [diff] [review]
0001-Move-variable-definitions-from-packager.mk-to-upload.patch

This time with hg copy ...

The few extra differences in upload-files.mk are just some trailing spaces that I removed.
Attachment #8533968 - Attachment is obsolete: true
Attachment #8534614 - Flags: review?(mh+mozilla)
Attachment #8534614 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

4 years ago
Blocks: 1112252
(Assignee)

Comment 12

4 years ago
Created attachment 8537453 [details] [diff] [review]
0002-Bug-1109136-add-upload-artifact-list-to-mach_build_p.patch

Turns out we need to unset DIST_FILES in moz-automation.mk, otherwise android builds break. r+ carried forward
Attachment #8533969 - Attachment is obsolete: true
Attachment #8537453 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9cc41324a5e1
https://hg.mozilla.org/mozilla-central/rev/c207cbc339fd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Updated

4 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.