Closed Bug 1454771 Opened 3 years ago Closed 2 years ago

Move tup invocation out of Makefile.in and into build mach command

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(2 files)

This will fix a few papercuts we've encountered so far
The other thing Makefile.in is giving us is the build-backend rebuild detection, which is still quite useful.
Comment on attachment 8968704 [details]
Bug 1454771 - Move tup invocation from Makefile.in to mach.

https://reviewboard.mozilla.org/r/237402/#review245392

Looks great overall! Just a few small issues to address before landing, but I don't think they will require re-review.

::: python/mozbuild/mozbuild/backend/tup.py:228
(Diff revision 4)
>  
>          self._built_in_addons = set()
>          self._built_in_addons_file = 'dist/bin/browser/chrome/browser/content/browser/built_in_addons.json'
>  
> +    def build(self, config, output, jobs, verbose, what=None):
> +        if what is None:

It seems 'what' is never None, but an empty list (maybe because it is an nargs='*' argument?), so we don't actually build the objdir by default.

::: python/mozbuild/mozbuild/backend/tup.py:235
(Diff revision 4)
> +        args = [self.environment.substs['TUP'], 'upd'] + what
> +        if self.environment.substs.get('MOZ_AUTOMATION'):
> +            args += ['--quiet']
> +        if verbose:
> +            args += ['--verbose']
> +        return config.run_process(args=args,

We should also pass the jobs argument in (using '-j', just like make) with some logic like this: https://dxr.mozilla.org/mozilla-central/rev/26e53729a10976f52e75efa44e17b5e054969fec/python/mozbuild/mozbuild/base.py#616

::: python/mozbuild/mozbuild/controller/building.py:1021
(Diff revision 4)
> +
> +            if config is None:
> -            if what:
> +                if what:
> -                top_make = os.path.join(self.topobjdir, 'Makefile')
> -                if not os.path.exists(top_make):
>                      print('Your tree has not been configured yet. Please run '

I think we can get rid of this error, since 'what' isn't used until after configure runs anyway. So if you have no objdir, './mach build foo' should work fine if the check is removed.

::: python/mozbuild/mozbuild/controller/building.py:1035
(Diff revision 4)
> +                    config = self.config_environment
> +
> +            status = maybe_invoke_backend(config.substs.get('BUILD_BACKENDS',
> +                                                            [None])[0])
>  
> +            if what and status is None:

How hard would it be to move this whole if-block into the make backend's build() method? It can be done in a followup - just curious what the potential hazards are.
Attachment #8968704 - Flags: review?(mshal) → review+
Comment on attachment 8969670 [details]
Bug 1454771 - Add mozconfig variables to the environment when running tup.

https://reviewboard.mozilla.org/r/238472/#review245424
Attachment #8969670 - Flags: review?(mshal) → review+
Comment on attachment 8968704 [details]
Bug 1454771 - Move tup invocation from Makefile.in to mach.

https://reviewboard.mozilla.org/r/237402/#review245392

> It seems 'what' is never None, but an empty list (maybe because it is an nargs='*' argument?), so we don't actually build the objdir by default.

I'll change this to `if not what:`

> How hard would it be to move this whole if-block into the make backend's build() method? It can be done in a followup - just curious what the potential hazards are.

It would probably be relatively simple, but we'd have to figure out how to propogate errors correctly. This whole function is pretty big and messy and should probably be re-written at some point.
Comment on attachment 8968704 [details]
Bug 1454771 - Move tup invocation from Makefile.in to mach.

https://reviewboard.mozilla.org/r/237402/#review245392

> I think we can get rid of this error, since 'what' isn't used until after configure runs anyway. So if you have no objdir, './mach build foo' should work fine if the check is removed.

It's a slight behavior change, but yeah, I'll just get rid of it.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dee93aadc721
Move tup invocation from Makefile.in to mach. r=mshal
https://hg.mozilla.org/integration/autoland/rev/fc793911d371
Add mozconfig variables to the environment when running tup. r=mshal
https://hg.mozilla.org/mozilla-central/rev/dee93aadc721
https://hg.mozilla.org/mozilla-central/rev/fc793911d371
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.