mach artifact currently only handles Android builds. This bug is to extend it to handle other type of builds (or at least desktop Firefox builds)

Just a status update: Bug 1213418 removed some of the blockers to this. We can now handle artifacts with the same package name (previously we needed the build ID or something similar to differentiate them), and we're set up to handle packages in general. So this ticket is converging on the issues I outline in https://bugzilla.mozilla.org/show_bug.cgi?id=1207888#c1. Let's keep it open to track adding the entries to |mach artifact install|, though.

Bug 1207890 - Part 1: Add rich ArtifactJob extension point. r?glandium

Bug 1207890 - Part 2: Stop extracting build ID from artifacts. r?glandium Sadly, it's slow to extract the build ID from Mac OS X DMG artifacts. It's better to sacrifice human-legible names in order to know the final name for an artifact quickly.

Bug 1207890 - Part 3: Post-process downloaded artifacts. r?glandium For Android, this just copies .so files from the APK assets/ and libs/ directories into the root, which ends up in dist/bin. I include .ini files as a partial replacement for having the build ID in the downloaded file name, and then don't put the .ini files in dist/bin.

Bug 1207890 - Part 4: Download and process Mac OS X artifacts. r?glandium This mounts the downloaded DMG and copies a subset of the libraries into the correct places in the processed archive. They'll be installed, with paths, into dist/bin from there.

Bug 1207890 - Part 5: Hard-code browsercomps binary-component. r?glandium Since binary targets aren't handled at all in !COMPILE_ENVIRONMENT builds, the master chrome.manifest does not include components/components.manifest and components/components.manifest does not include 'binary-component libbrowsercomps.dylib'.

Bug 1207890 - Part 6: Hacks to make --disable-compile-environment work on Mac OS X. r?glandium

glandium: With these patches, I get a functional artifact-based Mac OS X build. The first 4 parts are pretty straight-forward, although they don't implement your original "unpack completely" idea. (I tried that, and it's not workable due to conflicts between updated artifacts and local changes to the front-end code.) Part 5 is a hack, but I have no better ideas. Part 6 is a bunch of hacks that I don't really know what to do with. Rework? Land and file follow-up to make Mac OS X |mach run| better, so the build-time copying is not necessary? I've built this on top of Bug 1216817, although it probably could be separated out. gps: do you have bandwidth to comment on this, or steal the reviews?

mhoye: this is relevant to your interests.

Comment on attachment 8691745 [details] MozReview Request: Bug 1207890 - Part 5: Hard-code browsercomps binary-component. r?glandium https://reviewboard.mozilla.org/r/26179/#review23561 ::: browser/components/build/Makefile.in:11 (Diff revision 1) > +export:: > + $(call py_action,buildlist,$(FINAL_TARGET)/chrome.manifest 'manifest components/components.manifest') > + $(call py_action,buildlist,$(FINAL_TARGET)/components/components.manifest 'binary-component $(DLL_PREFIX)browsercomps.$(DLL_SUFFIX)') This is a quite horrible thing to do... and I'll address that in bug 1227892

Comment on attachment 8691741 [details] MozReview Request: Bug 1207890 - Part 1: Add rich ArtifactJob extension point. r=glandium https://reviewboard.mozilla.org/r/26171/#review24259 ::: python/mozbuild/mozbuild/artifacts.py:77 (Diff revision 1) > -# TODO: handle installing binaries from different types of artifacts (.tar.bz2, .dmg, etc). > +class ArtifactJob: class ArtifactJob(object): ::: python/mozbuild/mozbuild/artifacts.py:100 (Diff revision 1) > - # 'android-api-9': {'re': re.compile('public/build/fennec-(.*)\.android-arm\.apk')}, > + # 'android-api-9': ArtifactJob('public/build/fennec-(.*)\.android-arm\.apk'), The commented calls lack the log argument. OTOH, the lambda business is not very nice, you /could/ get rid of it if you did something like: def ArtifactJob(regexp): class _ArtifactJob(object): _regexp = regexp def __init__(self, log): self._log = log (...) JOB_DETAILS = { 'android-api-11': ArtifactJob('public/build/fennec-(.*)\.android-arm\.apk'), (...) } artifact_job = JOB_DETAILS[job](self._log)

Comment on attachment 8691741 [details] MozReview Request: Bug 1207890 - Part 1: Add rich ArtifactJob extension point. r=glandium https://reviewboard.mozilla.org/r/26171/#review24261

Comment on attachment 8691742 [details] MozReview Request: Bug 1207890 - Part 2: Stop extracting build ID from artifacts. r=glandium https://reviewboard.mozilla.org/r/26173/#review24263

Comment on attachment 8691743 [details] MozReview Request: Bug 1207890 - Part 3: Post-process downloaded artifacts. r?glandium https://reviewboard.mozilla.org/r/26175/#review24265 ::: python/mozbuild/mozbuild/artifacts.py:107 (Diff revision 1) > + raise NotImplementedError("Subclasses must specialize process_artifact!") You /could/ use abc.abstractmethod. ::: python/mozbuild/mozbuild/artifacts.py:114 (Diff revision 1) > + with zipfile.ZipFile(filename) as zf: You might as well use JarReader from mozpack.mozjar, it will happily reuse the deflate streams from the original JarWriter directly. with JarWriter(...) as writer: for j in JarReader(...): if not j.filename.endswith('.so') ...: continue writer.add(j.filename, j) ::: python/mozbuild/mozbuild/artifacts.py:131 (Diff revision 1) > # 'android-api-9': ArtifactJob('public/build/fennec-(.*)\.android-arm\.apk'), You should change this one as well. ::: python/mozbuild/mozbuild/artifacts.py:408 (Diff revision 1) > + 'Installing from processed {processed_filename}') Do we need to keep the non-processed file once we have the processed file? ::: python/mozbuild/mozbuild/artifacts.py:425 (Diff revision 1) > + os.chmod(n, info.external_attr >> 16) # See http://stackoverflow.com/a/434689. using shutil.copyfileobj with a FileAvoidWrite shouldn't require this.

Comment on attachment 8691744 [details] MozReview Request: Bug 1207890 - Part 4: Download and process Mac OS X artifacts. r?glandium https://reviewboard.mozilla.org/r/26177/#review24267 ::: mobile/android/mach_commands.py:193 (Diff revision 1) > description='Use pre-built artifacts to build Fennec.', The description needs to be updated, and the class should move out of mobile/android. ::: mobile/android/mach_commands.py:239 (Diff revision 1) > + if self.defines.get('ANDROID', False): It's better to use self.substs['OS_TARGET'] ::: mobile/android/mach_commands.py:244 (Diff revision 1) > + # XXX 64 bits? substs['HAVE_64BIT_BUILD'] would test that. ::: python/mozbuild/mozbuild/artifacts.py:156 (Diff revision 1) > + # subprocess.call('hdiutil detach %s -quiet' % appDir, Looks like you found a bug in mozinstall, where it can follow a path where it doesn't set appDir, which suggests something fishy going on with `hdiutil attach -nobrowser -noautoopen your_dmg` in _install_dmg. ::: python/mozbuild/mozbuild/artifacts.py:158 (Diff revision 1) > + # How to find this locally? We don't have easy access to the mach > + # context and hence substs and defines here. It would be > + # 'MOZ_MACBUNDLE_NAME'. You actually always have access to `import buildconfig` which, while it doesn't give you a dict, still provides a list of tuples for substs. ::: python/mozbuild/mozbuild/artifacts.py:186 (Diff revision 1) > + 'Contents/Resources/gmp-clearkey/0.1/libclearkey.dylib', wtf? the clearkey gmp is in Resources? worth filing a separate bug. ::: python/mozbuild/mozbuild/artifacts.py:189 (Diff revision 1) > + 'Contents/Resources/webapprt-stub', Same with the webapprt-stub. ::: python/mozbuild/mozbuild/artifacts.py:199 (Diff revision 1) > + writer.add(os.path.basename(p).encode('utf-8'), f.read(), mode=os.stat(mozpath.join(source, p)).st_mode) it's probably worth changing JarWriter.add such that passing f instead of f.read() works (so, adding a hasattr(data, 'seek') before using data.seek. That would allow to possibly improve memory usage if necessary at the JarWriter level without having to care about it here. ::: python/mozbuild/mozbuild/artifacts.py:204 (Diff revision 1) > + pass Should probably warn here.

https://reviewboard.mozilla.org/r/26171/#review24259 > The commented calls lack the log argument. OTOH, the lambda business is not very nice, you /could/ get rid of it if you did something like: > > def ArtifactJob(regexp): > class _ArtifactJob(object): > _regexp = regexp > > def __init__(self, log): > self._log = log > > (...) > > JOB_DETAILS = { > 'android-api-11': ArtifactJob('public/build/fennec-(.*)\.android-arm\.apk'), > (...) > } > > artifact_job = JOB_DETAILS[job](self._log) mmmm the subclassing in subsequent patches makes that less practical...

https://reviewboard.mozilla.org/r/26171/#review24259 > mmmm the subclassing in subsequent patches makes that less practical... You could go with something like: JOB_DETAILS = { 'android-api-11': (ArtifactJob, 'public/build/fennec-(.*)\.android-arm\.apk'), (...) } cls, re = JOB_DETAILS[job] artifact_job = cls(self._log, re)

Comment on attachment 8691746 [details] MozReview Request: Bug 1207890 - Part 6: Hacks to make --disable-compile-environment work on Mac OS X. r?glandium https://reviewboard.mozilla.org/r/26181/#review24273 ::: browser/app/Makefile.in:93 (Diff revision 1) > -tools repackage:: $(PROGRAM) > +tools repackage:: $(DIST)/bin/$(MOZ_APP_NAME) $(FINAL_TARGET) would be less worse than $(DIST)/bin. ::: browser/app/Makefile.in:103 (Diff revision 1) > - rsync -aL $(PROGRAM) $(dist_dest)/Contents/MacOS > + rsync -aL $(DIST)/bin/$(MOZ_APP_NAME) $(dist_dest)/Contents/MacOS likewise ::: browser/confvars.sh:26 (Diff revision 1) > -MOZ_ENABLE_SIGNMAR=1 > +MOZ_ENABLE_SIGNMAR=$COMPILE_ENVIRONMENT This seems error-prone-ish. I'd rather leave 1 as a value here and reset it before the MOZ_ARG_ENABLE_BOOL in configure.in if !COMPILE_ENVIRONMENT. ::: ipc/app/Makefile.in:34 (Diff revision 1) > $(NSINSTALL) -D $(DIST)/bin/$(PROGRAM).app > - rsync -a -C --exclude '*.in' $(srcdir)/macbuild/Contents $(DIST)/bin/$(PROGRAM).app > - sed -e 's/%PROGRAM%/$(PROGRAM)/' $(srcdir)/macbuild/Contents/Info.plist.in > $(DIST)/bin/$(PROGRAM).app/Contents/Info.plist > + rsync -a -C --exclude '*.in' $(srcdir)/macbuild/Contents $(DIST)/bin/$(MOZ_CHILD_PROCESS_NAME).app > + sed -e 's/%PROGRAM%/$(MOZ_CHILD_PROCESS_NAME)/' $(srcdir)/macbuild/Contents/Info.plist.in > $(DIST)/bin/$(MOZ_CHILD_PROCESS_NAME).app/Contents/Info.plist > sed -e 's/%APP_NAME%/$(MOZ_APP_DISPLAYNAME)/' $(srcdir)/macbuild/Contents/Resources/English.lproj/InfoPlist.strings.in | \ > - iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/$(PROGRAM).app/Contents/Resources/English.lproj/InfoPlist.strings > - $(NSINSTALL) -D $(DIST)/bin/$(PROGRAM).app/Contents/MacOS > - $(NSINSTALL) $(PROGRAM) $(DIST)/bin/$(PROGRAM).app/Contents/MacOS > + iconv -f UTF-8 -t UTF-16 > $(DIST)/bin/$(MOZ_CHILD_PROCESS_NAME).app/Contents/Resources/English.lproj/InfoPlist.strings > + $(NSINSTALL) -D $(DIST)/bin/$(MOZ_CHILD_PROCESS_NAME).app/Contents/MacOS > + $(NSINSTALL) $(DIST)/bin/$(MOZ_CHILD_PROCESS_NAME) $(DIST)/bin/$(MOZ_CHILD_PROCESS_NAME).app/Contents/MacOS while here, please replace $(DIST)/bin with $(FINAL_TARGET)

I think we hashed things out at Mozlando. Re-request needinfo if you still want my opinion.

https://reviewboard.mozilla.org/r/26171/#review24259 > You could go with something like: > > JOB_DETAILS = { > 'android-api-11': (ArtifactJob, 'public/build/fennec-(.*)\.android-arm\.apk'), > (...) > } > > cls, re = JOB_DETAILS[job] > artifact_job = cls(self._log, re) I've addressed this by adding a `get_job_details(job, log=None)` helper and using this dictionary-of-pairs approach to make the factory function explicit. We can re-visit if we find this pattern too limiting.

https://reviewboard.mozilla.org/r/26175/#review24265 > You /could/ use abc.abstractmethod. Meh. > Do we need to keep the non-processed file once we have the processed file? As written, yes: we follow the chain from HG commit -> task -> URL -> downloaded file -> postprocessed file; if there's a missing link, we won't short circuit. If we remove the downloaded file, we'll re-download it. The caching logic here isn't very robust; but making this better isn't necessary for this ticket. I anticipate dminor will want to improve this in Q1. > using shutil.copyfileobj with a FileAvoidWrite shouldn't require this. I've added a comment noting that libs and exes may need to be executable, depending on platform.

https://reviewboard.mozilla.org/r/26177/#review24267 > The description needs to be updated, and the class should move out of mobile/android. I'll extract in a post patch. > It's better to use self.substs['OS_TARGET'] I decided to go for testing `MOZ_BUILD_APP == 'mobile/android'`, since this is Fennec-only for now. > substs['HAVE_64BIT_BUILD'] would test that. Done. > Looks like you found a bug in mozinstall, where it can follow a path where it doesn't set appDir, which suggests something fishy going on with `hdiutil attach -nobrowser -noautoopen your_dmg` in _install_dmg. Yeah, but I'm not going to try to address it just now. I think it's actually a weirdness in `hdiutil`, where it tries to be clever about mounting at sequential locations. > You actually always have access to `import buildconfig` which, while it doesn't give you a dict, still provides a list of tuples for substs. I'm just going to make this a TODO. It's not clear we'll ever see anything but 'Nightly.app' in the wild. > wtf? the clearkey gmp is in Resources? worth filing a separate bug. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1234959. > Same with the webapprt-stub. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1234960. > it's probably worth changing JarWriter.add such that passing f instead of f.read() works (so, adding a hasattr(data, 'seek') before using data.seek. That would allow to possibly improve memory usage if necessary at the JarWriter level without having to care about it here. I've done this as a Pre: patch.

https://reviewboard.mozilla.org/r/26181/#review24273 > $(FINAL_TARGET) would be less worse than $(DIST)/bin. FINAL_TARGET appears to be '../../dist/bin/browser' here: ``` nalexander@chocho ~/M/gecko> make -C objdir-dce/browser/app echo-variable-FINAL_TARGET ../../dist/bin/browser ``` I'm going to leave these; if you want, file follow-up and lets do this stuff right.

(In reply to Nick Alexander :nalexander [Vacation PTO from Dec 23--Jan 4] from comment #22) > I'm just going to make this a TODO. It's not clear we'll ever see anything > but 'Nightly.app' in the wild. Build with --enable-debug, and you get NightlyDebug.app. And depending on the branding you build, you might get Firefox.app or FirefoxDeveloperEdition.app.

(In reply to Mike Hommey [:glandium] from comment #24) > (In reply to Nick Alexander :nalexander [Vacation PTO from Dec 23--Jan 4] > from comment #22) > > I'm just going to make this a TODO. It's not clear we'll ever see anything > > but 'Nightly.app' in the wild. > > Build with --enable-debug, and you get NightlyDebug.app. And depending on > the branding you build, you might get Firefox.app or > FirefoxDeveloperEdition.app. Aye, but we're pulling from upstream binaries, so it's not really helpful to know the local configuration. As we handle more situations (debug binaries, especially), we can generalize.

Comment on attachment 8691741 [details] MozReview Request: Bug 1207890 - Part 1: Add rich ArtifactJob extension point. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26171/diff/1-2/

Comment on attachment 8691742 [details] MozReview Request: Bug 1207890 - Part 2: Stop extracting build ID from artifacts. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26173/diff/1-2/

Comment on attachment 8691743 [details] MozReview Request: Bug 1207890 - Part 3: Post-process downloaded artifacts. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26175/diff/1-2/

Review commit: https://reviewboard.mozilla.org/r/29015/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29015/

Comment on attachment 8691744 [details] MozReview Request: Bug 1207890 - Part 4: Download and process Mac OS X artifacts. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26177/diff/1-2/

Comment on attachment 8691745 [details] MozReview Request: Bug 1207890 - Part 5: Hard-code browsercomps binary-component. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26179/diff/1-2/

Comment on attachment 8691746 [details] MozReview Request: Bug 1207890 - Part 6: Hacks to make --disable-compile-environment work on Mac OS X. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26181/diff/1-2/

Review commit: https://reviewboard.mozilla.org/r/29017/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29017/

Comment on attachment 8691743 [details] MozReview Request: Bug 1207890 - Part 3: Post-process downloaded artifacts. r?glandium https://reviewboard.mozilla.org/r/26175/#review25931 ::: python/mozbuild/mozbuild/artifacts.py:119 (Diff revisions 1 - 2) > - if not info.filename.endswith('.so') and \ > - not info.filename == 'platform.ini' and \ > + not f.filename == 'platform.ini' and \ > + not f.filename == 'application.ini': not foo == bar is foo != bar :) But since you have two, you can actually do foo not in ('a', 'b').

Comment on attachment 8701709 [details] MozReview Request: Bug 1207890 - Pre: Make JarWriter handle inputs with read() but not seek(). r?glandium https://reviewboard.mozilla.org/r/29015/#review25933

Comment on attachment 8701710 [details] MozReview Request: Bug 1207890 - Post: Move |mach artifact| command out of mobile/android. r?glandium https://reviewboard.mozilla.org/r/29017/#review25937 ::: mobile/android/mach_commands.py (Diff revision 1) > - conditions.is_android, # mobile/android only for now. removing this line would be better to do in the patch that adds support for osx. ::: python/mozbuild/mozbuild/mach_commands.py:1398 (Diff revision 1) > + def __init__(self, *args, **kwargs): > + SubCommand.__init__(self, *args, **kwargs) You can remove this method, since it doesn't do more than calling the parent class's method with the same arguments.

Comment on attachment 8691745 [details] MozReview Request: Bug 1207890 - Part 5: Hard-code browsercomps binary-component. r?glandium Not necessary anymore.

Comment on attachment 8691744 [details] MozReview Request: Bug 1207890 - Part 4: Download and process Mac OS X artifacts. r?glandium https://reviewboard.mozilla.org/r/26177/#review25947

https://hg.mozilla.org/integration/fx-team/rev/b1a1e53f1112e2e25f14d3f2f0f602b015fe7f48 Bug 1207890 - Pre: Make JarWriter handle inputs with read() but not seek(). r=glandium https://hg.mozilla.org/integration/fx-team/rev/2d648483bb0b55faad2f952e2131d479ae907e36 Bug 1207890 - Part 1: Add rich ArtifactJob extension point. r=glandium https://hg.mozilla.org/integration/fx-team/rev/9a4c86290fd0b738c5bf71792faea6bc9e78299a Bug 1207890 - Part 2: Stop extracting build ID from artifacts. r=glandium https://hg.mozilla.org/integration/fx-team/rev/877082fa7e881725ba836db8f206cc9a76954277 Bug 1207890 - Part 3: Post-process downloaded artifacts. r=glandium https://hg.mozilla.org/integration/fx-team/rev/75b6de069dd76d09001e013808c2e6c3e8cc47b7 Bug 1207890 - Part 4: Download and process Mac OS X artifacts. r=glandium https://hg.mozilla.org/integration/fx-team/rev/8207354399fce757d858dfc41235fe637b0db364 Bug 1207890 - Post: Hacks to make --disable-compile-environment work on Mac OS X. r=glandium https://hg.mozilla.org/integration/fx-team/rev/6855fb8ac20e5c90f68ab1fb036657cfe0ead0b5 Bug 1207890 - Post: Move |mach artifact| command out of mobile/android. r=glandium https://hg.mozilla.org/integration/fx-team/rev/a48a8eb8415f38e25c8672d0f5d58be865cca71f Bug 1207890 - Post: Hack to make |mach run| for Mac OS X artifact builds. r=me

No idea if this patchset is related or not, but I can't build from fx-team using artifacts anymore. See the attached error.

Comment on attachment 8703159 [details] mach-artifact-error.txt mfinkle: I can confirm that this is an issue, introduced by this bug (1207890), Part 3. I think that we're setting permissions from the contents of the APK, which has no permissions set. I'll provide a work-around shortly but I have some errands to run first.

https://hg.mozilla.org/integration/fx-team/rev/e1273a6122e56df24bdaac52efe73b0d5556bc1c Bug 1207890 - Follow-up: Fix |mach artifact install| for mobile/android. r=me

https://hg.mozilla.org/mozilla-central/rev/b1a1e53f1112 https://hg.mozilla.org/mozilla-central/rev/2d648483bb0b https://hg.mozilla.org/mozilla-central/rev/9a4c86290fd0 https://hg.mozilla.org/mozilla-central/rev/877082fa7e88 https://hg.mozilla.org/mozilla-central/rev/75b6de069dd7 https://hg.mozilla.org/mozilla-central/rev/8207354399fc https://hg.mozilla.org/mozilla-central/rev/6855fb8ac20e https://hg.mozilla.org/mozilla-central/rev/a48a8eb8415f https://hg.mozilla.org/mozilla-central/rev/e1273a6122e5