Build XPIDL in the tup backend

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: mshal, Assigned: mshal)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
XPIDL seems like an easy place to start, since they only depend on the header.py invocation. Until I can add GENERATED_FILES to the tup backend, I just write this rule directly.

I'm not adding Tup to the default list of backends yet. The backend generation still takes a decent amount of additional time (largely due to CommonBackend.consume_object(), which we need to get a list of XPIDLs). So this is how to run it:

1) Add ac_add_options --build-backends=RecursiveMake,FasterMake,Tup to .mozconfig
2) $ ./mach configure
3) $ ./mach build tup

This runs the 'tup' target in the top-level Makefile.in, which can use the recursive make backend to do a few things before invoking tup itself. At the moment that's just the install manifests and buildid.h, though it can be expanded to help bootstrap the backend faster.

I can get the list of XPIDLs by using the common backend, though this also has the side-effect that the tup backend generates things like Unified*.cpp all-tests.json, binaries.json, etc, which is redundant with the RecursiveMake backend. Is there a good way to process this information only once? Or should that not matter in the future if we're intending to only generate one backend or the other (so if Tup is generated, RecursiveMake wouldn't be).

Unfortunately this does also highlight how bad running things in a sandboxed FUSE environment is - the overhead of building an XPIDL can be anywhere from +10% to +100%. That's something I'll need to investigate in tup.

On the plus side, the build system overhead to *start* processing a changed XPIDL is ~1.5s by default, and under 1s when using the tup monitor. Most of this time is from processing the install manifests. I don't expect this part of the overhead to change much, unless a lot of things need to be added to the Makefile bootstrap.

Any suggestions, feedback, or things to try are welcome.
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review67402

There's a lot to like here!

I'm not sure how I feel about hacking a "tup" target into the root Makefile. But I can't really suggest anything better. As long as we get the user-facing API right, I don't care too strongly how hacky things are in the backend. This means that `mach build` should "just work" with Tup if Tup is present. How we're going to make that work, I'm not sure. Perhaps it involves ifdef'ing a lot of Makefile.in rules :/

::: Makefile.in:176
(Diff revision 1)
>  	$(MAKE) -C faster FASTER_RECURSIVE_MAKE=1
>  endif
>  
> +.PHONY: tup
> +tup: install-manifests buildid.h
> +	@if [ ! -d $(topsrcdir)/.tup ]; then cd $(topsrcdir); tup init; fi

I feel like `tup init` should be handled during backend generation, before any build pre-requisites are evaluated.

Are there pros or cons to either approach?

::: python/mozbuild/mozbuild/backend/tup.py:78
(Diff revision 1)
> +    def include_rules(self):
> +        if not self.rules_included:
> +            self.write('include_rules\n')
> +            self.rules_included = True
> +
> +    def rule(self, cmd, inputs=[], outputs=[], display=None, extra_outputs=None):

Nit: Don't use lists or dicts in default args because the value used the first time the function is evaluated gets used for the lifetime of the process.

Instead, default to None then write code like `inputs = inputs or []`

::: python/mozbuild/mozbuild/backend/tup.py:261
(Diff revision 1)
> +        for var in ['SHELL', 'MOZILLABUILD', 'COMSPEC']:
> +            backend_file.write('export %s\n' % var)

Should these be in every Tupfile or exported from the main Tupfile?

::: python/mozbuild/mozbuild/backend/tup.py:264
(Diff revision 1)
> +        # These are used by mach/mixin/process.py to determine the current
> +        # shell.
> +        for var in ['SHELL', 'MOZILLABUILD', 'COMSPEC']:
> +            backend_file.write('export %s\n' % var)
> +
> +        for module, data in manager.modules.iteritems():

This needs a `sorted()` to ensure Tupfile output is deterministic.

::: python/mozbuild/mozbuild/backend/tup.py:266
(Diff revision 1)
> +            cmd = ('$(PYTHON_PATH) $(PLY_INCLUDE) -I$(IDL_PARSER_DIR) -I$(IDL_PARSER_CACHE_DIR) '
> +                   '$(topsrcdir)/python/mozbuild/mozbuild/action/xpidl-process.py '
> +                   '--cache-dir $(MOZ_OBJ_ROOT)/xpcom/idl-parser '
> +                   '$(DIST)/idl $(DIST)/include $(MOZ_OBJ_ROOT)/%(dest)s/components '
> +                   '%(module)s '
> +                   '%(idls)s') % {

I want to say we should consolidate this code somehow. Do you think it is worth it to create a `get_xpidl_rules()` or something and have it consumed by multiple backends?

::: python/mozbuild/mozbuild/backend/tup.py:274
(Diff revision 1)
> +                   '$(DIST)/idl $(DIST)/include $(MOZ_OBJ_ROOT)/%(dest)s/components '
> +                   '%(module)s '
> +                   '%(idls)s') % {
> +                       'dest': dest,
> +                       'module': module,
> +                       'idls': ' '.join(idls),

This also likely needs a `sorted()`.

::: python/mozbuild/mozbuild/backend/tup.py:277
(Diff revision 1)
> +                       'dest': dest,
> +                       'module': module,
> +                       'idls': ' '.join(idls),
> +                   }
> +            outputs = ['$(MOZ_OBJ_ROOT)/%s/components/%s.xpt' % (dest, module)]
> +            outputs.extend(['$(MOZ_OBJ_ROOT)/dist/include/%s.h' % f for f in idls])

`sorted()`
Attachment #8779084 - Flags: review?(gps)
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review67402

I agree just a 'mach build' should do the right thing, though I'm not sure of the best way to implement that at the moment. Perhaps a --default-build-backend configure option would be needed? Or the first build backend in --build-backends could be the default. Either mach would have to know what to run instead of 'make -f client.mk', or perhaps the top-level Makefile could have a different default target-per-backend instead of always doing 'default'. That way if you have the 'faster' backend listed first, it would run the 'faster' make target, etc. I'll file a followup?

> I feel like `tup init` should be handled during backend generation, before any build pre-requisites are evaluated.
> 
> Are there pros or cons to either approach?

I don't think there is much of a difference either way, so I moved it into the backend. We could instead just create a "Tupfile.ini" in the top of the source tree, in which case tup runs the init there if it can't find an existing .tup directory. It looks like we already have a number of stub build files for things like gradle and B2G, so maybe that wouldn't be unreasonable. But this way we don't have to clutter the source tree.

> Nit: Don't use lists or dicts in default args because the value used the first time the function is evaluated gets used for the lifetime of the process.
> 
> Instead, default to None then write code like `inputs = inputs or []`

I'm surprised I haven't run into this before! Thanks for the heads up.

> Should these be in every Tupfile or exported from the main Tupfile?

At this point I don't know how many things we do that end up including mach's process.py, which is what ultimately needs these environment variables. I can move them into the shared Tuprules.tup file if it needs to be global, but I think it's better to limit the scope as much as possible in the meantime. Tup tracks these as dependencies, so if one of these environment variables is changed, it will re-run the xpidl-process.py commands. If it's a global export, changing the environment variable would rebuild everything.

> I want to say we should consolidate this code somehow. Do you think it is worth it to create a `get_xpidl_rules()` or something and have it consumed by multiple backends?

Ultimately yes, though I'd prefer not to block on it. Mind if a file a followup as part of some build backend consolidation work? There are probably going to be a few of these for the one-off things we still have in Makefiles.
Comment hidden (mozreview-request)
(In reply to Michael Shal [:mshal] from comment #0)
> I can get the list of XPIDLs by using the common backend, though this also
> has the side-effect that the tup backend generates things like Unified*.cpp
> all-tests.json, binaries.json, etc, which is redundant with the
> RecursiveMake backend. Is there a good way to process this information only
> once? Or should that not matter in the future if we're intending to only
> generate one backend or the other (so if Tup is generated, RecursiveMake
> wouldn't be).

I'd suggest using an hybrid build backend. See how the FasterMake+RecursiveMake backend is defined in mozbuild.backend.

(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8779084 [details]
> Bug 1293448 - Build XPIDL files in the tup backend;
> 
> https://reviewboard.mozilla.org/r/70128/#review67402
> 
> I agree just a 'mach build' should do the right thing, though I'm not sure
> of the best way to implement that at the moment. Perhaps a
> --default-build-backend configure option would be needed? Or the first build
> backend in --build-backends could be the default. Either mach would have to
> know what to run instead of 'make -f client.mk', or perhaps the top-level
> Makefile could have a different default target-per-backend instead of always
> doing 'default'. That way if you have the 'faster' backend listed first, it
> would run the 'faster' make target, etc. I'll file a followup?

It seems to me this is knowledge that should be in the backend class. Mach would need to poke at the backend code, then, to know what it needs to run for the default backend (which means it also needs to know what the default backend is by pocking at config.status) (and I agree that the first backend in --build-backends should be the default)
 
> > Nit: Don't use lists or dicts in default args because the value used the first time the function is evaluated gets used for the lifetime of the process.
> > 
> > Instead, default to None then write code like `inputs = inputs or []`
> 
> I'm surprised I haven't run into this before! Thanks for the heads up.

In practice, it's not a problem if you don't do things like inputs.append in the function.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review69280

::: Makefile.in:175
(Diff revision 2)
>  faster: install-dist/idl
>  	$(MAKE) -C faster FASTER_RECURSIVE_MAKE=1
>  endif
>  
> +.PHONY: tup
> +tup: install-manifests buildid.h

Somehow, this tells me install manifests and buildid.h should move to tup first :)

::: python/mozbuild/mozbuild/backend/tup.py:68
(Diff revision 2)
> +    def write_once(self, buf):
> +        if isinstance(buf, unicode):
> +            buf = buf.encode('utf-8')
> +        if b'\n' + buf not in self.fh.getvalue():
> +            self.write(buf)
> +

You should probably not put this function here so that you don't end up having to rely on it. It's an unfortunate requirement in the recursive make backend, but it's all due to how the backend works, and should die eventually.

::: python/mozbuild/mozbuild/backend/tup.py:86
(Diff revision 2)
> +        outputs = outputs or []
> +        self.include_rules()
> +        self.write(': %(inputs)s |> %(display)s%(cmd)s |> %(outputs)s%(extra_outputs)s\n' % {
> +            'inputs': ' '.join(inputs),
> +            'display': '^ %s^ ' % display if display else '',
> +            'cmd': cmd,

how does tup handle commands? especially wrt quoting and such. In any case, I think the cmd argument here should be a list, and quoting/normalization/whatever should be done here instead of in the caller.

::: python/mozbuild/mozbuild/backend/tup.py:113
(Diff revision 2)
> +    def _get_backend_file_for_obj(self, obj):
> +        if obj.objdir not in self._backend_files:
> +            self._backend_files[obj.objdir] = \
> +                    BackendTupfile(obj.srcdir, obj.objdir, obj.config,
> +                                   obj.topsrcdir, self.environment.topobjdir)
> +        return self._backend_files[obj.objdir]
> +
> +    def _get_backend_file(self, relativedir):
> +        objdir = mozpath.join(self.environment.topobjdir, relativedir)
> +        srcdir = mozpath.join(self.environment.topsrcdir, relativedir)
> +        if objdir not in self._backend_files:
> +            self._backend_files[objdir] = \
> +                    BackendTupfile(srcdir, objdir, self.environment,
> +                                   self.environment.topsrcdir, self.environment.topobjdir)
> +        return self._backend_files[objdir]

Do we actually want one tup file per directory? Why not have one tup file for everything? Tup needs to read them all anyways, right? It's necessary to have plenty of files for make because it would be awfully slow if it weren't the case, but that doesn't seem to be a good thing to do for other backends.

::: python/mozbuild/mozbuild/backend/tup.py:238
(Diff revision 2)
> +            fh.write('MOZ_OBJ_ROOT = $(TUP_CWD)\n')
> +            fh.write('DIST = $(MOZ_OBJ_ROOT)/dist\n')

Here and everywhere else, what happens when one of those paths contain a space?

::: python/mozbuild/mozbuild/backend/tup.py:240
(Diff revision 2)
> +                pass
> +
> +        with self._write_file(mozpath.join(self.environment.topobjdir, 'Tuprules.tup')) as fh:
> +            fh.write('MOZ_OBJ_ROOT = $(TUP_CWD)\n')
> +            fh.write('DIST = $(MOZ_OBJ_ROOT)/dist\n')
> +            fh.write('ACDEFINES = %s\n' % self.environment.substs['ACDEFINES'])

substs['ACDEFINES'] is quoted for make purposes. You should probably derive your own from defines instead, so that whatever quoting tup needs is handled properly.
Attachment #8779084 - Flags: review?(mh+mozilla)
(Assignee)

Comment 7

2 years ago
(In reply to Mike Hommey [:glandium] from comment #5)
> I'd suggest using an hybrid build backend. See how the
> FasterMake+RecursiveMake backend is defined in mozbuild.backend.

Ahh, I see. That seems to work pretty well.

> > I agree just a 'mach build' should do the right thing, though I'm not sure
> > of the best way to implement that at the moment. Perhaps a
> > --default-build-backend configure option would be needed? Or the first build
> > backend in --build-backends could be the default. Either mach would have to
> > know what to run instead of 'make -f client.mk', or perhaps the top-level
> > Makefile could have a different default target-per-backend instead of always
> > doing 'default'. That way if you have the 'faster' backend listed first, it
> > would run the 'faster' make target, etc. I'll file a followup?
> 
> It seems to me this is knowledge that should be in the backend class. Mach
> would need to poke at the backend code, then, to know what it needs to run
> for the default backend (which means it also needs to know what the default
> backend is by pocking at config.status) (and I agree that the first backend
> in --build-backends should be the default)

I took a stab at this, though I feel like I might be overthinking it. The first patch doesn't do anything by itself, though it allows the TupBackend to create its own build() method. For now it just invokes make as if 'mach build tup' were executed, though in the future it could just call tup directly. The downside is we have to make sure configure is called before trying to read the backends, otherwise mach would default to a 'make -f client.mk' if there is no objdir, even if the first backend has a build() function.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review67402

> I don't think there is much of a difference either way, so I moved it into the backend. We could instead just create a "Tupfile.ini" in the top of the source tree, in which case tup runs the init there if it can't find an existing .tup directory. It looks like we already have a number of stub build files for things like gradle and B2G, so maybe that wouldn't be unreasonable. But this way we don't have to clutter the source tree.

I'm not a huge fan of all the stub files in the root of the directory. They add clutter and often give the wrong impression as to how things work. e.g. a lot of people see Makefile.in or configure.in and they think they can run autoconf to build things. Our "public API" to the build system is `mach`. Plus we do other things there, such as record system resource usage. So let's try to avoid a /Tupfile.ini to mislead people about the ability to use Tup directly.

> Ultimately yes, though I'd prefer not to block on it. Mind if a file a followup as part of some build backend consolidation work? There are probably going to be a few of these for the one-off things we still have in Makefiles.

I don't mind a follow-up.

I have some other patches related to XPIDL generation that I've been mulling over. So it might be me that gets hit with the cleanup effort :)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review70688

What are your thoughts on putting variable references in Tupfiles versus expanding them in the backend?

::: python/mozbuild/mozbuild/backend/tup.py:248
(Diff revisions 1 - 3)
> +        if not os.path.exists(mozpath.join(self.environment.topsrcdir, ".tup")):
> +            self._cmd.run_process(cwd=self.environment.topsrcdir, args=['tup', 'init'])

Does the .tup directory strictly have to be in the source directory?

Can we move it to the objdir? I ask because we should strive for the source directory to be read only.
Attachment #8779084 - Flags: review?(gps)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review69280

> Somehow, this tells me install manifests and buildid.h should move to tup first :)

Perhaps - I was trying to leverage some of the existing infrastructure that works well enough in make. Install manifests already purge old files, so porting them over would just remove the ~1s incremental build time. Certainly worthwhile, but I was hoping to get something working more quickly :). The buildid.h is more tricky since tup doesn't have a FORCE target, so even if it's not re-using make here, it will either need something in the build() function to update this on every build, or I'll have to patch tup to support FORCE-like things.

> You should probably not put this function here so that you don't end up having to rely on it. It's an unfortunate requirement in the recursive make backend, but it's all due to how the backend works, and should die eventually.

Gah, good catch. That was leftover from copy/pasting the recursive make backend.

> how does tup handle commands? especially wrt quoting and such. In any case, I think the cmd argument here should be a list, and quoting/normalization/whatever should be done here instead of in the caller.

They are passed in as strings to '/bin/sh -e -c' on OSX/Linux. (Windows uses CreateProcess directly in most cases). I've updated cmd to use a list - that seems to simplify things. I can add quoting there as necessary, though using shellutil.quote doesn't quite do the right thing here since it tries to quote strings that will be expanded by tup, like $(PYTHON_PATH).

> Do we actually want one tup file per directory? Why not have one tup file for everything? Tup needs to read them all anyways, right? It's necessary to have plenty of files for make because it would be awfully slow if it weren't the case, but that doesn't seem to be a good thing to do for other backends.

Tup doesn't parse all the Tupfiles during every invocation. Instead, it re-parses only those Tupfiles that have changed since the last invocation. The new rules from that directory are compared against the previously known rules, so it can remove obsolete output files and re-run commands if the command-line changes. It is better to diff a small subset of commands rather than all the commands in the tree, so tup actually works better with many smaller Tupfiles rather than a single large one.

> Here and everywhere else, what happens when one of those paths contain a space?

The TUP_CWD variable is similar to the DEPTH variable we use in the generated Makefiles, so it only contains '../'. And it looks like we don't support a topsrcdir with spaces? Trying it gives me: client.mk:53: *** The mozilla directory cannot be located in a path with spaces..  Stop.
(Assignee)

Comment 13

2 years ago
(In reply to Gregory Szorc [:gps] from comment #10)
> I'm not a huge fan of all the stub files in the root of the directory. They
> add clutter and often give the wrong impression as to how things work. e.g.
> a lot of people see Makefile.in or configure.in and they think they can run
> autoconf to build things. Our "public API" to the build system is `mach`.
> Plus we do other things there, such as record system resource usage. So
> let's try to avoid a /Tupfile.ini to mislead people about the ability to use
> Tup directly.

Yeah, that's a good point. I'll leave the backend to run 'tup init' automatically, since that has largely the same effect without the confusion.

> > Ultimately yes, though I'd prefer not to block on it. Mind if a file a followup as part of some build backend consolidation work? There are probably going to be a few of these for the one-off things we still have in Makefiles.
> 
> I don't mind a follow-up.
> 
> I have some other patches related to XPIDL generation that I've been mulling
> over. So it might be me that gets hit with the cleanup effort :)

Filed bug 1296708 - feel free to collect your thoughts there if you have ideas already!
(Assignee)

Comment 14

2 years ago
(In reply to Gregory Szorc [:gps] from comment #11)
> What are your thoughts on putting variable references in Tupfiles versus
> expanding them in the backend?

You mean writing out ../../dist/idl in the Tupfile instead of $(DIST)/idl and such? I don't think I see a significant difference either way. Is there something you're looking to avoid by expanding them in the backend? I can give it a shot if you think it would be preferable.

> ::: python/mozbuild/mozbuild/backend/tup.py:248
> (Diff revisions 1 - 3)
> > +        if not os.path.exists(mozpath.join(self.environment.topsrcdir, ".tup")):
> > +            self._cmd.run_process(cwd=self.environment.topsrcdir, args=['tup', 'init'])
> 
> Does the .tup directory strictly have to be in the source directory?
> 
> Can we move it to the objdir? I ask because we should strive for the source
> directory to be read only.

Unfortunately, yes. The .tup directory signifies where tup starts scanning the tree, and by default it ignores things that are "outside" the tree (like /usr/bin, though there is an option to enable those). It's more of a cache for commands & dependencies rather than an output of the build.
(Assignee)

Comment 15

2 years ago
Comment on attachment 8782692 [details]
Bug 1293448 - Allow build backends to specify custom build commands;

Hmm, this fails on OSX universal builds, since config.status is in the arch-specific subdirs of topobjdir. I think in that case we need to call client.mk regardless.
Attachment #8782692 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review71198

One issue I found when testing this: we don't verify in configure that `tup` is available. I got all the way to `mach build` before it failed due to the missing binary. We need a configure check.

::: python/mozbuild/mozbuild/backend/tup.py:110
(Diff revision 4)
> +    def summary(self):
> +        summary = super(TupBackend, self).summary()
> +        return summary

Do you need to define this method?

::: python/mozbuild/mozbuild/backend/tup.py:299
(Diff revision 4)
> +    def _handle_linked_rust_crates(self, obj, extern_crate_file):
> +        pass
> +
> +    def _handle_ipdl_sources(self, ipdl_dir, sorted_ipdl_sources,
> +                             unified_ipdl_cppsrcs_mapping):
> +        pass
> +
> +    def _handle_webidl_build(self, bindings_dir, unified_source_mapping,
> +                             webidls, expected_build_output_files,
> +                             global_define_files):
> +        pass

Does some abstract base class require these to be defined? (Hint: whatever the reason, this is probably a sign there needs to be an inline comment.)
Attachment #8779084 - Flags: review?(gps)
(Assignee)

Comment 19

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review71198

Good point. I've added this to moz.configure now (with help from glandium), and it only runs the check when the Tup backend is being built.

> Do you need to define this method?

No, and in fact it's not even called anymore now that I'm using the Hybrid backend. I also noticed that get_backend_file_for_obj isn't used anymore either.

> Does some abstract base class require these to be defined? (Hint: whatever the reason, this is probably a sign there needs to be an inline comment.)

The common backend calls the ipdl and webidl ones, so we need to implement those. I've added TODO comments since I'll need to implement them anyway once I add support for those files. Is that what your hint was getting at? Is this a python good practice I should be aware of? :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review71198

> The common backend calls the ipdl and webidl ones, so we need to implement those. I've added TODO comments since I'll need to implement them anyway once I add support for those files. Is that what your hint was getting at? Is this a python good practice I should be aware of? :)

Yes, a comment explaining why they exist is all that is needed.

The only Python practice to help with this would be @abc.abstractmethod, which when combined with some meta class magic allows you to define abstract base classes that check for abstract method implementation at type instantiation time. That can be quite nice if you absolutely must have interfaces or base classes that implement required methods. But at the point you are using abstract base classes, you've probably wandered pretty far from the "zen of Python" which says (among other things) your code shouldn't look like Java :)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review72534

This is essentially an r+. This looks awesome.

::: Makefile.in:176
(Diff revision 5)
>  	$(MAKE) -C faster FASTER_RECURSIVE_MAKE=1
>  endif
>  
> +.PHONY: tup
> +tup: install-manifests buildid.h
> +	@tup

Shouldn't this be using the TUP variable discovered from configure?

::: python/mozbuild/mozbuild/backend/tup.py:257
(Diff revision 5)
> +
> +        backend_file = self._get_backend_file('xpcom/xpidl')
> +
> +        # These are used by mach/mixin/process.py to determine the current
> +        # shell.
> +        for var in ['SHELL', 'MOZILLABUILD', 'COMSPEC']:

Nit: use a tuple instead of a list (since the data structure is immutable).
Attachment #8779084 - Flags: review?(gps)
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review72534

> Shouldn't this be using the TUP variable discovered from configure?

Yep, good catch. There's also the 'tup' in TupBackend.consume_finished() for the 'tup init' call.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review73344

\o/
Attachment #8779084 - Flags: review?(gps) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review73428

Most comments from the previous review still apply.

::: python/mozbuild/mozbuild/backend/__init__.py:13
(Diff revision 6)
> +    'Tup': 'mozbuild.backend.tup',
> +    'Tup+RecursiveMake': None,

Considering the Tup backend is not useful on its own, I'd suggest making the Tup backend the hybrid backend. You don't need the a+b magic to create hybrid backends, you can define something like:

class TupOnly(PartialBackend):
    (...)

class Tup(HybridBackend(TupOnly, RecursiveMakeBackend)):
    (...)

or Tup = HybridBackend(TupOnly, RecursiveMakeBackend

in tup.py.

::: python/mozbuild/mozbuild/backend/tup.py:78
(Diff revision 6)
> +            'inputs': ' '.join(inputs),
> +            'display': '^ %s^ ' % display if display else '',
> +            'cmd': ' '.join(cmd),
> +            'outputs': ' '.join(outputs),
> +            'extra_outputs': ' | ' + ' '.join(extra_outputs) if extra_outputs else '',

Don't you need shell quoting in here (mozbuild.shellutil.quote)

::: python/mozbuild/mozbuild/backend/tup.py:132
(Diff revision 6)
> +        if isinstance(obj, (Sources, GeneratedSources)):
> +            pass

You can remove all these until you actually implement them.

::: python/mozbuild/mozbuild/backend/tup.py:258
(Diff revision 6)
> +        for var in ('SHELL', 'MOZILLABUILD', 'COMSPEC'):
> +            backend_file.write('export %s\n' % var)

surprising you need this. Does tup remove everything from the environment unless you explicitly export them?
Attachment #8779084 - Flags: review?(mh+mozilla)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8782692 [details]
Bug 1293448 - Allow build backends to specify custom build commands;

https://reviewboard.mozilla.org/r/72758/#review73434

::: python/mozbuild/mozbuild/backend/base.py:294
(Diff revision 4)
> +        def build(self, config, output, jobs, verbose):
> +            for backend in self._backends:
> +                status = backend.build(config, output, jobs, verbose)
> +                if status is not None:
> +                    return status
> +            return None

This should probably be limited to the first one. Or we can leave this out entirely, and you can implement the build method on the Tup class that inherits from HybridBackend(). I'd go for the latter for now.
Attachment #8782692 - Flags: review?(mh+mozilla)
(Assignee)

Comment 30

2 years ago
mozreview-review-reply
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review73428

I think I provided explanations for why some suggestions weren't implemented in #c12. Are there particular ones that I missed or that you'd still like to see implemented?

> Don't you need shell quoting in here (mozbuild.shellutil.quote)

I don't believe so. At the moment adding shellutil.quote conflicts with using variables like $(PYTHON_PATH), since that will be replaced with '$(PYTHON_PATH)', so the shell sees it as a single argument rather than multiple arguments ('.../python', '-B', '.../pythonpath.py'). We don't do any kind of global shell quoting in the RecursiveMake backend as far as I can tell - it looks like it is selectively applied where necessary (Defines, LocalIncludes, ChromeManifests, and some VariablePassthrus).

I can revisit this if we run into quoting issues after more things are added to the backend, or if you feel strongly about it now I can try to use gps' suggestion of expanding variables like $(PYTHON_PATH) and others in the backend rather than in the Tupfile.

> You can remove all these until you actually implement them.

Removed for now.

> surprising you need this. Does tup remove everything from the environment unless you explicitly export them?

Yes, the environment is mostly cleared, except for a few OS-specific variables to get normal compiler toolchains working (so things like gcc or cl work by default). By explicitly adding environment variables, tup can track when they are changed and re-execute the commands that use them. AFAIK there is no way to tell when a program actually calls getenv on a specific environment variable, so instead it is done at the Tupfile level - exporting a variable like MOZILLABUILD adds that variable to the environment and adds it as a dependency to the remaining commands in the Tupfile.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

2 years ago
mozreview-review
Comment on attachment 8782692 [details]
Bug 1293448 - Allow build backends to specify custom build commands;

https://reviewboard.mozilla.org/r/72758/#review74910
Attachment #8782692 - Flags: review?(mh+mozilla) → review+

Comment 34

2 years ago
mozreview-review
Comment on attachment 8779084 [details]
Bug 1293448 - Build XPIDL files in the tup backend;

https://reviewboard.mozilla.org/r/70128/#review74912

Mmmm I completely had missed comment 12. How did this happen?
Attachment #8779084 - Flags: review?(mh+mozilla) → review+

Comment 35

2 years ago
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34767d6a0a23
Allow build backends to specify custom build commands; r=glandium
https://hg.mozilla.org/integration/autoland/rev/5c231de97dec
Build XPIDL files in the tup backend; r=glandium,gps

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34767d6a0a23
https://hg.mozilla.org/mozilla-central/rev/5c231de97dec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
So, an unintended consequence of "Allow build backends to specify custom build commands" is that postflight, which used to run before configure, now runs after configure. One of the things we were doing in preflight with the explicit intent for it to run at the beginning of the build was to cleanup sccache servers that could be leftover from a previous build (and it did happen on buildbot). The funny thing is that having it run after configure now saves us from some additional pains (see https://github.com/rust-lang/rust/issues/42867#issuecomment-314996145)

Updated

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