1305157 - Support FINAL_TARGET_PP_FILES (and similar) in the tup backend

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

This is mostly straightforward since we now have preprocessor support in the tup backend. However, there are a few cases where the preprocessed file includes other GENERATED_FILES, and we don't currently have a way to specify those files as inputs. For example, toolkit/modules/moz.build has:

EXTRA_PP_JS_MODULES += [
    'AppConstants.jsm',
]

AppConstants.jsm does this:

#include @TOPOBJDIR@/source-repo.h

And source-repo.h is generated by the top-level moz.build:

    GENERATED_FILES['source-repo.h'].script = 'build/variables.py:source_repo_header'

I tried to add a .inputs field or something to the FINAL_TARGET_PP_FILES, but I'm running into a similar problem as in bug 1259530 - specifically, the ContextDerivedHierarchicalStringLists aren't compatible with StrictOrderingOnAppendListWithFlagsFactory. In that bug, glandium recommended using contexts to add things like this. gps/glandium: is that what I should be trying to implement here? Or do you have other suggestions?

In short, I'd like to be able to do something like:

EXTRA_PP_JS_MODULES['AppConstants.jsm'].inputs += ['!/source-repo.h']

So the backend knows the correct DAG.

Comment 1

[Mike Hommey [:glandium]]

Do we really want to go down the path where we need to doubly declare dependencies, once as the #include in the preprocessed file and once in the moz.build file? That doesn't seem very desirable. Arguably, the build system then doesn't know it needs to build source-repo.h first. That's kind of why we have make export going first. That's how building C++ works, and there's not much we can do about that for C++. I'm not convinced this is much different and needs special treatment.

Comment 2

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #1)
> Do we really want to go down the path where we need to doubly declare
> dependencies, once as the #include in the preprocessed file and once in the
> moz.build file? That doesn't seem very desirable. Arguably, the build system
> then doesn't know it needs to build source-repo.h first. That's kind of why
> we have make export going first. That's how building C++ works, and there's
> not much we can do about that for C++. I'm not convinced this is much
> different and needs special treatment.

Excellent point. For some reason I thought this was handled in the make backend more by ordering directories correctly in DIRS rather than by using the export tier. I should be able to replicate the equivalent of an export tier in tup rather than explicitly identifying all these...

Comment 3

[Michael Shal [:mshal]]

I tried to make this more tier-like by having the GENERATED_FILES all process before the FINAL_TARGET_PP_FILES, so that source-repo.h and buildid.h would be available to the preprocessor without manually specifying the dependencies. Unfortunately, we also have some GENERATED_FILES that depend on FINAL_TARGET_PP_FILES (eg: build/application.ini & application.ini.h), so we can't build one whole group before the other. This somewhat magically works now in the make backend, because even though we generate application.ini in the misc tier and application.ini.h (which needs application.ini) in the export tier, these rules are fortunately in the same Makefile, so application.ini actually gets built in export as well.

Though while doing this I realized that I intended to have buildid.h generated by the make backend before tup runs in order to make use of the FORCE rule to generate a new buildid on each invocation, though I neglected to account for this when adding GENERATED_FILES support. Given that this should already be available to the build system, it seems silly to add a whole tier just to get source-repo.h available for the preprocessor. For now I think it's easiest to just generate source-repo.h before tup runs, along with buildid.h

This does also reveal two other annoyances:

1) In a single Tupfile, tup expects the rules to be listed in topological order. So a rule that creates an output is listed before another rule that uses it as an input. The emitter doesn't emit objects in any particular order, so in the case of the build/ directory, I had to use the delayed_generated_files hack to order things properly. If I run into more cases like this I'll either implement a more generic topological sort in BackendTupfile, or just fix this in tup.

2) The install manifests delete all the files that we create in dist/bin/ via FINAL_TARGET_PP_FILES, so tup rebuilds ~53 targets on a no-op build. We do the same thing in the recursive make backend as well, though it's not as obvious. Perhaps I should finally move the install manifest processing into tup anyway, which would fix this issue.

Comment 4

[Michael Shal [:mshal]]

Attachment: Bug 1305157 - Move GeneratedFile handling into its own function;

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

Comment 5

[Michael Shal [:mshal]]

Attachment: Bug 1305157 - Fix buildid.h and create source-repo.h in make before tup runs;

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

Comment 6

[Michael Shal [:mshal]]

Attachment: Bug 1305157 - Handle FinalTargetPreprocessedFiles in the tup backend;

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

Comment 7

[Michael Shal [:mshal]]

Attachment: Bug 1305157 - Handle the build/ directory's GENERATED_FILES;

This is a little tricky since tup expects rules in a single Tupfile to
be topologically sorted, and the emitter emits the GeneratedFile object
before it emits the FinalTargetPreprocessedFile object. We work around
this by delaying when we process the application.ini.h GeneratedFile
object, but it's a hack.

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

Comment 8

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #3)
> 2) The install manifests delete all the files that we create in dist/bin/
> via FINAL_TARGET_PP_FILES, so tup rebuilds ~53 targets on a no-op build. We
> do the same thing in the recursive make backend as well, though it's not as
> obvious. Perhaps I should finally move the install manifest processing into
> tup anyway, which would fix this issue.

Look at the faster make backend. It doesn't remove all files from dist/bin/ when processing install manifests.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8795893 [details]
Bug 1305157 - Move GeneratedFile handling into its own function;

https://reviewboard.mozilla.org/r/81854/#review80476

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8795894 [details]
Bug 1305157 - Fix buildid.h and create source-repo.h in make before tup runs;

https://reviewboard.mozilla.org/r/81856/#review80480

I'm not crazy about listing one-offs like this. But as glandium said, the alternative is moving more DAG foo into moz.build files. We may end up there eventually. But I'm inclined to see how far we can get without doing that. This approach is definitely the lesser of two evils.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8795895 [details]
Bug 1305157 - Handle FinalTargetPreprocessedFiles in the tup backend;

https://reviewboard.mozilla.org/r/81858/#review80482

::: python/mozbuild/mozbuild/backend/tup.py:179
(Diff revision 1)
>                  if not name in self.environment.non_global_defines]
>              acdefines_flags = ' '.join(['-D%s=%s' % (name,
>                  shell_quote(self.environment.defines[name]))
>                  for name in sorted(acdefines)])
> +            # TODO: AB_CD only exists in Makefiles at the moment.
> +            acdefines_flags += ' -DAB_CD=en-US'

I guess this means this isn't compatible with l10n builds yet. Meh.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8795896 [details]
Bug 1305157 - Handle the build/ directory's GENERATED_FILES;

https://reviewboard.mozilla.org/r/81860/#review80484

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f65ae87fe51a
Move GeneratedFile handling into its own function; r=gps
https://hg.mozilla.org/integration/autoland/rev/1e844b504825
Fix buildid.h and create source-repo.h in make before tup runs; r=gps
https://hg.mozilla.org/integration/autoland/rev/c4e99e641104
Handle FinalTargetPreprocessedFiles in the tup backend; r=gps
https://hg.mozilla.org/integration/autoland/rev/694718beec80
Handle the build/ directory's GENERATED_FILES; r=gps

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f65ae87fe51a
https://hg.mozilla.org/mozilla-central/rev/1e844b504825
https://hg.mozilla.org/mozilla-central/rev/c4e99e641104
https://hg.mozilla.org/mozilla-central/rev/694718beec80

Comment 15

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #8)
> Look at the faster make backend. It doesn't remove all files from dist/bin/
> when processing install manifests.

Oh, I wasn't aware of the --track flag. Is there a reason we don't use that in the top-level Makefile.in? Does that conflict with 'mach build binaries' or something?

In any case, I took a stab at just doing this in tup, and it seems pretty straightforward. I'll probably finalize those patches next week.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > Look at the faster make backend. It doesn't remove all files from dist/bin/
> > when processing install manifests.
> 
> Oh, I wasn't aware of the --track flag. Is there a reason we don't use that
> in the top-level Makefile.in? Does that conflict with 'mach build binaries'
> or something?

The "traditional" build system kind of relies on stuff being removed in dist/ at the beginning of the build. Or at least that used to be true. I'm not sure it still is, but I'm not thrilled about verifying whether it's still true or not. I'd rather work on improving the faster make backend.