1216817 - Run |mach artifact| automatically when --disable-compile-environment is set

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

We want to make artifact based builds transparent when we can.  This ticket tracks extracting enough build environment information from the build configuration to run |mach artifact| at an appropriate time.

Right now, mobile/android needs |mach artifact| before |mach package| only.  (|mach build| does not interact with the binary artifacts.)  glandium's |mach build faster| work wants artifacts to be in place before the build begins, since there is code that copies artifacts into place at build time.  Desktop developers likely won't use |mach package| at all, but they will use |mach run|.

Comment 1

[Nick Alexander :nalexander [he/him]]

It's important that artifacts be updated as part of incremental builds, because developers navigate between local commits in their source trees which may not trigger a configure (or even a build-backend).

glandium: can you suggest a good place to hook this updating?  Maybe into the same mechanism that checks if build-backend (or configure) is needed?  Maybe into the .xpt refreshing mechanism?

Comment 2

[Mike Hommey [:glandium]]

> Run |mach artifact| automatically when --disable-compile-environment is set

Not a good idea. There are different use cases for --disable-compile-environment that _don't_ require artifacts.

(In reply to Nick Alexander :nalexander from comment #1)
> glandium: can you suggest a good place to hook this updating?  Maybe into
> the same mechanism that checks if build-backend (or configure) is needed? 
> Maybe into the .xpt refreshing mechanism?

That's a difficult question. I'd rather avoid anything that requires different build backends to have their own version of the hooking code. I think the right place would be in the mach build command handler, but aiui, mach artifact needs to run after configure, and mach build doesn't handle configure separately itself. So I guess at this point the right place would be client.mk, with the expected outcome of being moved along other things from client.mk into mach build itself (or configure/build-backend, depending on how this all ends up being hooked up together)

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #2)
> > Run |mach artifact| automatically when --disable-compile-environment is set
> 
> Not a good idea. There are different use cases for
> --disable-compile-environment that _don't_ require artifacts.

I agree.  I'll add a --enable-artifact-downloading flag to opt-in to this.  I wish we had a mach-specific rc file for this kind of configuration switch, but it's not so bad since that flag will imply --d-c-e, which isn't something you can turn on and off without needing to clobber.

> (In reply to Nick Alexander :nalexander from comment #1)
> > glandium: can you suggest a good place to hook this updating?  Maybe into
> > the same mechanism that checks if build-backend (or configure) is needed? 
> > Maybe into the .xpt refreshing mechanism?
> 
> That's a difficult question. I'd rather avoid anything that requires
> different build backends to have their own version of the hooking code. I
> think the right place would be in the mach build command handler, but aiui,
> mach artifact needs to run after configure, and mach build doesn't handle
> configure separately itself. So I guess at this point the right place would
> be client.mk, with the expected outcome of being moved along other things
> from client.mk into mach build itself (or configure/build-backend, depending
> on how this all ends up being hooked up together)

You're correct that |mach artifact| wants a configured tree, for both build flags and the Python virtualenv.

I've dug into this and it is a difficult problem.  I found I could wedge a target into Makefile.in that would execute 1-3 times for incremental builds, but I didn't find such a place in client.mk.

The more I think about it, the less I think we should put any artifact stuff in Make at all.  |mach artifact| just doesn't have nice dependencies -- it depends on the source revision, and the state of the upstream builders.

I see that bare client.mk is executed from |mach build| at https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#379.  How hard would it be to make the configure step explicit?  |make -f client.mk configure| forces a reconfigure, so that's bad; could we check for the presence of $OBJDIR/Makefile (like |mach build what| does) and explicitly run configure then, and then insert the artifact step, and then run |make -f client.mk|?  That should work save in unusual circumstances.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #3)
> I see that bare client.mk is executed from |mach build| at
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> mach_commands.py#379.  How hard would it be to make the configure step
> explicit?  |make -f client.mk configure| forces a reconfigure, so that's
> bad; could we check for the presence of $OBJDIR/Makefile (like |mach build
> what| does) and explicitly run configure then, and then insert the artifact
> step, and then run |make -f client.mk|?  That should work save in unusual
> circumstances.

I'm sure that will lead to many corner cases where configure runs twice or something. That's why I suggested to put this into client.mk somehow, until the day we actually get rid of client.mk. It seems to me to be the least worse option.

Comment 5

[Mike Hommey [:glandium]]

> It seems to me to be the least worse option.

At the moment, that is.

Comment 6

[Nick Alexander :nalexander [he/him]]

glandium: I have a WIP patch that runs |mach artifact install| early in the build.  That extracts .so libraries into dist/bin.  In #build, we talked about the dist/bin install manifest deleting those .so libraries immediately after they are installed, and you suggested modifying the emitter to capture the .so files that would be produced.

Sadly, !COMPILE_ENVIRONMENT skips nss/ and other parts of the Makefile.in-based build system and not all the relevant SharedLibraries are produced.

The set of .so files is encoded in the package-manifest.in, but reading that at the beginning of the build seems totally wrong.

The simplest thing to do is, for these artifact based builds, not purge *.so (and some other stuff) in dist/bin.  Or we could just not purge for dist/bin altogether.

To purge just *.so is non-trivial, since right now, optional_exists doesn't accept a pattern.  Patterns in the manifest code evaluate patterns at manifest creation time, which doesn't make sense in this case (we need the pattern to be evaluated at manifest install time, not at manifest creation time).  In a different ticket I once added a filter to the manifest deleter to allow keeping file names that match a predicate, which would work here.

Is there some terrible repercussion to not purging dist/bin for artifact based builds?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #6)
> Is there some terrible repercussion to not purging dist/bin for artifact
> based builds?

About as bad as FasterMake builds not doing so. And in both cases, the right thing to do is actually to have install manifest processing keep track of the files it installed, and remove those from the list that it doesn't have to install anymore. That's something on my todo list, but I don't know when I'll get to it. I think the simplest thing to do is to just copy the install manifest after processing, and pass the (old) copy to subsequent calls to the processor.

Comment 8

[Mike Hommey [:glandium]]

(with the full copy of the install manifest, we can also correctly skip preprocessing if dependencies haven't changed *and* defines haven't changed ; although if we kept track of the defines actually used, that would allow even more fine-grained skipping, but I don't think it's worth the extra effort)

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1216817 - Part 1: Add install_callback to artifacts. r=gps

Bug 1216817 - Part 1: Add install_callback to artifacts. r?glandium

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r=gps

Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r?glandium

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r?glandium

This allows to run |mach artifact install| immediately after |mach
configure| and not have a subsequent |mach build| delete the files
that |mach artifact install| copied into dist/bin.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1216817 - Part 4: Add --enable-artifact-builds and MOZ_ARTIFACT_BUILDS. r=glandium

Bug 1216817 - Part 4: Add MOZ_ARTIFACT_BUILDS; run |mach artifact install| automatically. r?glandium

It turns out to be much easier to hook |mach artifact install| into
config.status and |mach build| than to hook into client.mk.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681468 [details]
MozReview Request: Bug 1216817 - Part 1: Add install_callback to artifacts. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23833/diff/1-2/

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681469 [details]
MozReview Request: Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23835/diff/1-2/

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681470 [details]
MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23837/diff/1-2/

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681471 [details]
MozReview Request: Bug 1216817 - Part 4: Add --enable-artifact-builds and MOZ_ARTIFACT_BUILDS. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23839/diff/1-2/

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

It turns out to be much easier to hook |mach artifact install| into
config.status and |mach build| than to hook into client.mk.

Comment 18

[Nick Alexander :nalexander [he/him]]

glandium: here's a rebased version that puts the somewhat controversial part at the end.  I would like you to either respond to this or delegate it very quickly; it's been more than three weeks.

gps: do you have bandwidth to review this?

Since it's behind a flag, it would only impact folks who opt-in, and that will be only Android consumers for some time.

Comment 19

[Nick Alexander :nalexander [he/him]]

mhoye: this is relevant to your interests.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8681468 [details]
MozReview Request: Bug 1216817 - Part 1: Add install_callback to artifacts. r=gps

https://reviewboard.mozilla.org/r/23833/#review23625

This patch by itself is fine.

An alternate would be to refactor this whole thing to be a generator of some sort. But that feels like a lot of work. Callbacks feel easier given the code you have.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8681469 [details]
MozReview Request: Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r=gps

https://reviewboard.mozilla.org/r/23835/#review23627

Nothing controversial here. Just factoring out the os.path.join(X, 'bin', Y).

Comment 22

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/23837/#review23629

This patch is a bit contentious because it means that files from the artifact will linger in the objdir. I could imagine getting into trouble where the artifact files are installed, the source directory is updated to a version that removed a file, but the file still lingers in the objdir.

I'm going to give glandium a chance to weigh in here.

::: mobile/android/mach_commands.py:251
(Diff revision 2)
> +        help='Do not add files from artifacts to _build_manifests/install/dist_bin manifest.')

That's too low level. How about "force the build system to delete and replace files from the artifact."

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8681471 [details]
MozReview Request: Bug 1216817 - Part 4: Add --enable-artifact-builds and MOZ_ARTIFACT_BUILDS. r=glandium

https://reviewboard.mozilla.org/r/23839/#review24277

::: configure.in:153
(Diff revision 2)
> +MOZ_ARG_ENABLE_BOOL(artifact-builds,

Why plural?

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8691739 [details]
MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

https://reviewboard.mozilla.org/r/26167/#review23559

::: python/mozbuild/mozbuild/config_status.py:213
(Diff revision 1)
> +        # Execute |mach artifact install| from the top source directory.
> +        os.chdir(topsrcdir) 
> +        os.execlp('sh', 'sh', '-c', ' '.join([os.path.join(topsrcdir, 'mach'), 'artifact', 'install']))

It would feel better if you could somehow invoke _run_mach_artifact_install here instead of doing manual work with os.execlp.

::: python/mozbuild/mozbuild/mach_commands.py:412
(Diff revision 1)
> +                    self.log(logging.INFO, 'artifact',

Seems like something unnecessary to print out.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8681470 [details]
MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

https://reviewboard.mozilla.org/r/23837/#review24279

I'm still not sure about this part. I have something baking that should make this unnecessary, which aiui would make part 1 and 2 unnecessary as well. Let's see how far I can take the baking tomorrow.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #25)
> Comment on attachment 8681470 [details]
> MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to
> $OBJDIR/_build_manifests/install/dist_bin. r?glandium
> 
> https://reviewboard.mozilla.org/r/23837/#review24279
> 
> I'm still not sure about this part. I have something baking that should make
> this unnecessary, which aiui would make part 1 and 2 unnecessary as well.
> Let's see how far I can take the baking tomorrow.

Filed as bug 1230060 with patches, which don't handle the RecursiveMake backend, but that could be done.

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #25)
> Comment on attachment 8681470 [details]
> MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to
> $OBJDIR/_build_manifests/install/dist_bin. r?glandium
> 
> https://reviewboard.mozilla.org/r/23837/#review24279
> 
> I'm still not sure about this part. I have something baking that should make
> this unnecessary, which aiui would make part 1 and 2 unnecessary as well.
> Let's see how far I can take the baking tomorrow.

glandium: you and I didn't get a chance to talk about this.  Can you explain what has happened that changes what these patches use, or what you want to see happen, in order to get |mach artifact install| running automatically as part of |mach configure| and each |mach build| invocation?

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8681470 [details]
MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

https://reviewboard.mozilla.org/r/23837/#review25705

Turns out bug 1230060 can't be extended to the recursive make backend without leaving dangling files on incremental builds when things are removed from jar.mn. This has been waiting for long enough already, let's go for it.

Comment 29

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/23837/#review25707

::: mobile/android/mach_commands.py:250
(Diff revision 2)
> -    def artifact_install(self, source=None, tree=None, job=None, verbose=False):
> +    @CommandArgument('--no-update-manifest', dest='update_manifest', action='store_false',

That said, I'm not sure why provide an override here...

Comment 30

[Mike Hommey [:glandium]]

And sorry again for the various delays on this bug :(

Comment 31

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #29)
> https://reviewboard.mozilla.org/r/23837/#review25707
> 
> ::: mobile/android/mach_commands.py:250
> (Diff revision 2)
> > -    def artifact_install(self, source=None, tree=None, job=None, verbose=False):
> > +    @CommandArgument('--no-update-manifest', dest='update_manifest', action='store_false',
> 
> That said, I'm not sure why provide an override here...

I've removed this option.

Comment 32

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #23)
> Comment on attachment 8681471 [details]
> MozReview Request: Bug 1216817 - Part 4: Add --enable-artifact-builds and
> MOZ_ARTIFACT_BUILDS. r?glandium
> 
> https://reviewboard.mozilla.org/r/23839/#review24277
> 
> ::: configure.in:153
> (Diff revision 2)
> > +MOZ_ARG_ENABLE_BOOL(artifact-builds,
> 
> Why plural?

No significant reason.  To me, |mach artifact| isn't really a "build flag" but a "mode switch" that doesn't apply to just a single build but instead to a a whole sequence of builds (and source directory updates).

Comment 33

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #24)
> Comment on attachment 8691739 [details]
> MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install|
> automatically when asked. r?glandium
> 
> https://reviewboard.mozilla.org/r/26167/#review23559
> 
> ::: python/mozbuild/mozbuild/config_status.py:213
> (Diff revision 1)
> > +        # Execute |mach artifact install| from the top source directory.
> > +        os.chdir(topsrcdir) 
> > +        os.execlp('sh', 'sh', '-c', ' '.join([os.path.join(topsrcdir, 'mach'), 'artifact', 'install']))
> 
> It would feel better if you could somehow invoke _run_mach_artifact_install
> here instead of doing manual work with os.execlp.

I agree but I don't know how to arrange it, since the mach environment is very different from the config.status environment.  I cribbed this from similar existing code: https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/python/mozbuild/mozbuild/config_status.py#168.

> ::: python/mozbuild/mozbuild/mach_commands.py:412
> (Diff revision 1)
> > +                    self.log(logging.INFO, 'artifact',
> 
> Seems like something unnecessary to print out.

I've dropped this down to DEBUG.  I'm looking ahead to when we try to debug issues with this in the wild; it might help spot issues where unconfigured or freshly clobbered trees don't get artifacts correctly.

Comment 34

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681468 [details]
MozReview Request: Bug 1216817 - Part 1: Add install_callback to artifacts. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23833/diff/2-3/

Comment 35

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681469 [details]
MozReview Request: Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23835/diff/2-3/

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681470 [details]
MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23837/diff/2-3/

Comment 37

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681471 [details]
MozReview Request: Bug 1216817 - Part 4: Add --enable-artifact-builds and MOZ_ARTIFACT_BUILDS. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23839/diff/2-3/

Comment 38

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8691739 [details]
MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26167/diff/1-2/

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8691739 [details]
MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

https://reviewboard.mozilla.org/r/26167/#review25929

::: python/mozbuild/mozbuild/config_status.py:216
(Diff revision 2)
> +        os.execlp('sh', 'sh', '-c', ' '.join([os.path.join(topsrcdir, 'mach'), 'artifact', 'install']))

I'm still not comfortable with this, but if we're going to go with an execlp, we might as well avoid using sh, and use python directly, through sys.executable.

Comment 40

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #39)
> Comment on attachment 8691739 [details]
> MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install|
> automatically when asked. r?glandium
> 
> https://reviewboard.mozilla.org/r/26167/#review25929
> 
> ::: python/mozbuild/mozbuild/config_status.py:216
> (Diff revision 2)
> > +        os.execlp('sh', 'sh', '-c', ' '.join([os.path.join(topsrcdir, 'mach'), 'artifact', 'install']))
> 
> I'm still not comfortable with this, but if we're going to go with an
> execlp, we might as well avoid using sh, and use python directly, through
> sys.executable.

I don't really care about execlp -- would you prefer just subprocess.check_call (followed by return)?  I was just following suit.  So:
```
subprocess.check_call([sys.executable, os.path.join(topsrcdir, 'mach'), 'artifact', 'install'])
return 0
```

I'd really like to get some form of this in the tree before you PTO in a few days, so tell me whatever you think is appropriate and we'll make it happen.

Comment 41

[Mike Hommey [:glandium]]

os.execlp or subprocess.check_call, it doesn't matter wrt comment 24. If you use the latter, then you should make it return the return value of the check_call.

Comment 42

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #41)
> os.execlp or subprocess.check_call, it doesn't matter wrt comment 24. If you
> use the latter, then you should make it return the return value of the
> check_call.

I just tried to use sys.executable and check_call; both fail with a Python import error (below).  I wonder if the existing execlp is a work-around for this.  In any case, this shell invocation doesn't really seem worth fighting for.  If you feel strongly, can you work out the updated invocation?

0:45.89 Traceback (most recent call last):
 0:45.89   File "/Users/nalexander/Mozilla/gecko/mach", line 148, in <module>
 0:45.89     main(sys.argv[1:])
 0:45.89   File "/Users/nalexander/Mozilla/gecko/mach", line 76, in main
 0:45.90     mach = get_mach()
 0:45.90   File "/Users/nalexander/Mozilla/gecko/mach", line 67, in get_mach
 0:45.90     mach = check_and_get_mach(dir_path)
 0:45.90   File "/Users/nalexander/Mozilla/gecko/mach", line 42, in check_and_get_mach
 0:45.90     return load_mach(dir_path, mach_path)
 0:45.90   File "/Users/nalexander/Mozilla/gecko/mach", line 30, in load_mach
 0:45.90     return mach_bootstrap.bootstrap(dir_path)
 0:45.90   File "/Users/nalexander/Mozilla/gecko/build/mach_bootstrap.py", line 312, in bootstrap
 0:45.90     mach.load_commands_from_file(os.path.join(mozilla_dir, path))
 0:45.90   File "/Users/nalexander/Mozilla/gecko/python/mach/mach/main.py", line 258, in load_commands_from_file
 0:45.90     imp.load_source(module_name, path)
 0:45.90   File "/Users/nalexander/Mozilla/gecko/testing/xpcshell/mach_commands.py", line 28, in <module>
 0:45.90     from xpcshellcommandline import parser_desktop, parser_remote, parser_b2g
 0:45.90   File "/Users/nalexander/Mozilla/gecko/build/mach_bootstrap.py", line 338, in __call__
 0:45.90     module = self._original_import(name, globals, locals, fromlist, level)
 0:45.90   File "/Users/nalexander/Mozilla/gecko/build/mach_bootstrap.py", line 338, in __call__
 0:45.90     module = self._original_import(name, globals, locals, fromlist, level)
 0:45.90 ImportError: No module named xpcshellcommandline

Comment 43

[Mike Hommey [:glandium]]

That fails because mach apparently fails to run with the virtualenv python, which I think is an unintended bug. In fact, considering the bootstrap code, I'm pretty sure mach was intended to also run with the virtualenv python when it was written, but nothing enforces that it's actually true. So there's discrepancy between the list of SEARCH_PATHS in mach_bootstrap.py, and things available in the virtualenv as per virtualenv_packages.txt. Adding "xpcshell.pth:testing/xpcshell" to that file fixes it.

Comment 44

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681468 [details]
MozReview Request: Bug 1216817 - Part 1: Add install_callback to artifacts. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23833/diff/3-4/

Comment 45

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681469 [details]
MozReview Request: Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23835/diff/3-4/

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681470 [details]
MozReview Request: Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23837/diff/3-4/

Comment 47

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681471 [details]
MozReview Request: Bug 1216817 - Part 4: Add --enable-artifact-builds and MOZ_ARTIFACT_BUILDS. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23839/diff/3-4/

Comment 48

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8691739 [details]
MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26167/diff/2-3/

Comment 49

[Mike Hommey [:glandium]]

Comment on attachment 8691739 [details]
MozReview Request: Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r?glandium

https://reviewboard.mozilla.org/r/26167/#review25991

::: python/mozbuild/mozbuild/config_status.py:216
(Diff revision 3)
> +        os.chdir(topsrcdir) 

trailing whitespace.

Comment 50

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/2f95996c84cfce8687880a05f80943b2ce8a9a21
Bug 1216817 - Part 1: Add install_callback to artifacts. r=gps

https://hg.mozilla.org/integration/fx-team/rev/c37ff20166fe7bdcb108bc193db32d6b88e8ca3a
Bug 1216817 - Part 2: Narrow distdir to bindir in artifacts. r=gps

https://hg.mozilla.org/integration/fx-team/rev/b602d16fa13bde9932e71d14c72132dd1b39489d
Bug 1216817 - Part 3: Add files from artifacts to $OBJDIR/_build_manifests/install/dist_bin. r=glandium

https://hg.mozilla.org/integration/fx-team/rev/90b7e77b7f67f1ae0c9394d9e6db7c869a39011b
Bug 1216817 - Part 4: Add --enable-artifact-builds and MOZ_ARTIFACT_BUILDS. r=glandium

https://hg.mozilla.org/integration/fx-team/rev/e9e84784daf92455a07a563864284ed6047eb50b
Bug 1216817 - Part 5: Run |mach artifact install| automatically when asked. r=glandium

Comment 51

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/71b1a1b14cdac71bb236dfb6599dd85d16cbed0a
Bug 1216817 - Follow-up: Fix "KeyError: u'MOZ_ARTIFACT_BUILDS'". r=bustage

Comment 52

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/ebaa931f095af094dbccca6c760cf923e2211257
Bug 1216817 - Follow-up: Fix "KeyError: uMOZ_ARTIFACT_BUILDS" in config.status. r=bustage

Comment 53

[Nigel Babu [:nigelb]]

This has busted B2GDroid builds like this:

ValueError: Don't know how to compute android:versionCode for CPU arch armeabi-v7a and min SDK 11
ValueError: Don't know how to compute android:versionCode for CPU arch armeabi-v7a and min SDK 11
AndroidManifest.xml:2: error: Error: String types not allowed (at 'versionCode' with value '').
gmake[5]: *** [.aapt.deps] Error 1
gmake[5]: *** Deleting file `.aapt.deps'
gmake[5]: *** Waiting for unfinished jobs....
gmake[4]: *** [mobile/android/base/libs] Error 2
gmake[3]: *** [libs] Error 2
gmake[2]: *** [default] Error 2
gmake[1]: *** [realbuild] Error 2
gmake: *** [build] Error 2
Return code: 2
'mach build' did not run successfully. Please check log for errors.
Running post_fatal callback... 

https://treeherder.mozilla.org/logviewer.html#?job_id=6379062&repo=fx-team

Investigating with glandium's help.

Comment 54

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/fx-team/rev/961695453ecedd06b0ad1a6c3aa6cb7844845e78
Bug 1216817 - Followup Clobber as an attempt clear up B2GDroid bustage

Comment 55

[Nigel Babu [:nigelb]]

Looks like rnewman was at fault and not this bug. Thanks to glandium for help in figuring it out :)

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f95996c84cf
https://hg.mozilla.org/mozilla-central/rev/c37ff20166fe
https://hg.mozilla.org/mozilla-central/rev/b602d16fa13b
https://hg.mozilla.org/mozilla-central/rev/90b7e77b7f67
https://hg.mozilla.org/mozilla-central/rev/e9e84784daf9
https://hg.mozilla.org/mozilla-central/rev/71b1a1b14cda
https://hg.mozilla.org/mozilla-central/rev/ebaa931f095a
https://hg.mozilla.org/mozilla-central/rev/961695453ece

Comment 57

[Gregory Szorc [:gps]]

I don't think my needinfo is still needed.