Support FINAL_TARGET_FILES in the tup backend

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: mshal, Assigned: mshal)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

3 years ago
With this support we can stop using make to process install manifests and just do it in tup directly. Unfortunately I discovered that tup didn't handle filenames with a '^' in them, so the latest master version is required to build it. There are a few TODOs in this that can be addressed in the future as more things are added.

No-op build times go from ~4s down to ~1.5s.
Comment hidden (mozreview-request)

Comment 3

3 years ago
mozreview-review
Comment on attachment 8808393 [details]
Bug 1315810 - Use tup's internal symlinking instead of build manifests;

https://reviewboard.mozilla.org/r/91208/#review91728

I have a question, but I'm not entirely up to speed on the tup stuff. Let me know if there's a prior discussion of this I should be aware of.

::: python/mozbuild/mozbuild/backend/tup.py:88
(Diff revision 1)
> +        # The !tup_ln macro does a symlink or file copy (depending on the
> +        # platform) without shelling out to a subprocess.
> +        self.rule(

I can imagine a version of this that keeps using install manifests, is there something about them that's inherently incompatible with tup? This gives me pause in the next patch as well...
Attachment #8808393 - Flags: review?(cmanchester)

Comment 4

3 years ago
mozreview-review
Comment on attachment 8808394 [details]
Bug 1315810 - Support some FinalTargetFiles in the tup backend;

https://reviewboard.mozilla.org/r/91210/#review91738

::: python/mozbuild/mozbuild/backend/tup.py:300
(Diff revision 1)
> +                            # TODO: This is needed for tests
> +                            pass

Is this something that is yet to be implemented in tup? Unlike things needed for Windows this seems like it might be important for people working with these builds locally, even on one platform.
Attachment #8808394 - Flags: review?(cmanchester)
Assignee

Comment 5

3 years ago
> I can imagine a version of this that keeps using install manifests, is there something about them
> that's inherently incompatible with tup? This gives me pause in the next patch as well...

The two motivations that made me want to support this in tup are:

 1) We'll need similar logic to install libraries & binaries (eg: "symlink/copy this file to this other directory" is really the same for both)

 2) Install manifests remove a bunch of files that they didn't create, causing tup to regenerate them every build. Though glandium also suggested using the --track flag in process_install_manifest() as an alternative solution here.

> ::: python/mozbuild/mozbuild/backend/tup.py:300
> (Diff revision 1)
> > +                            # TODO: This is needed for tests
> > +                            pass
> 
> Is this something that is yet to be implemented in tup? Unlike things needed
> for Windows this seems like it might be important for people working with
> these builds locally, even on one platform.

I'm primarily just focused on getting a working build at this point, so I was ignoring anything to do with tests :). Looking at this a bit further, this particular branch happens in a few moz.build files (testing/mozbase/, testing/runtimes/, and testing/web-platform/), where we have a '**' wildcard like:

TEST_HARNESS_FILES.testing.mochitest.runtimes += [
    '**/mochitest-*.runtimes.json',
]

I tried to follow through bug 1242051, but I'm still a little unclear on the distinction between TEST_HARNESS_FILES and support-files. Are there plans to defer TEST_HARNESS_FILES until test runtime instead of at build time, or is there some reason they have to be done at build time? Or is there a way to pull them directly from the srcdir rather than recursively copying things into the objdir? If so it might make sense to just ignore them for now. Otherwise, I can either try to implement it in tup, or just fallback to using install manifests for these few cases. I could do that now or as a followup - let me know which you'd prefer.
Flags: needinfo?(cmanchester)
(In reply to Michael Shal [:mshal] from comment #5)
> > I can imagine a version of this that keeps using install manifests, is there something about them
> > that's inherently incompatible with tup? This gives me pause in the next patch as well...
> 
> The two motivations that made me want to support this in tup are:
> 
>  1) We'll need similar logic to install libraries & binaries (eg:
> "symlink/copy this file to this other directory" is really the same for both)
> 
>  2) Install manifests remove a bunch of files that they didn't create,
> causing tup to regenerate them every build. Though glandium also suggested
> using the --track flag in process_install_manifest() as an alternative
> solution here.

You may also have found in comment 0 that this is faster. I guess we can go with this. I'm not too excited about seeing all the logic around handling various types of files in the install manifest code duplicated, but maybe it's not as bad as I'm anticipating.

> 
> > ::: python/mozbuild/mozbuild/backend/tup.py:300
> > (Diff revision 1)
> > > +                            # TODO: This is needed for tests
> > > +                            pass
> > 
> > Is this something that is yet to be implemented in tup? Unlike things needed
> > for Windows this seems like it might be important for people working with
> > these builds locally, even on one platform.
> 
> I'm primarily just focused on getting a working build at this point, so I
> was ignoring anything to do with tests :). Looking at this a bit further,
> this particular branch happens in a few moz.build files (testing/mozbase/,
> testing/runtimes/, and testing/web-platform/), where we have a '**' wildcard
> like:
> 
> TEST_HARNESS_FILES.testing.mochitest.runtimes += [
>     '**/mochitest-*.runtimes.json',
> ]
> 
> I tried to follow through bug 1242051, but I'm still a little unclear on the
> distinction between TEST_HARNESS_FILES and support-files. Are there plans to
> defer TEST_HARNESS_FILES until test runtime instead of at build time, or is
> there some reason they have to be done at build time? Or is there a way to
> pull them directly from the srcdir rather than recursively copying things
> into the objdir? If so it might make sense to just ignore them for now.
> Otherwise, I can either try to implement it in tup, or just fallback to
> using install manifests for these few cases. I could do that now or as a
> followup - let me know which you'd prefer.

TEST_HARNESS_FILES are used by many tests, so we always install them. support-files required between directories need to be annotated individually.

I think we could install TEST_HARNESS_FILES at test runtime rather than build time, but I'd imagine we'd still use the same install manifest to do so, that might not help. Pulling these files right from the srcdir would probably work for most of these cases, I'd wager they do it this way now to make sure things get packaged (looking at bug 976733, that seems to be where we got this idea), but we have another way to do this now.

Anyway, if you'd rather we not tackle this right now of course that's fine.
Flags: needinfo?(cmanchester)

Comment 7

3 years ago
mozreview-review
Comment on attachment 8808393 [details]
Bug 1315810 - Use tup's internal symlinking instead of build manifests;

https://reviewboard.mozilla.org/r/91208/#review92874
Attachment #8808393 - Flags: review+

Comment 8

3 years ago
mozreview-review
Comment on attachment 8808394 [details]
Bug 1315810 - Support some FinalTargetFiles in the tup backend;

https://reviewboard.mozilla.org/r/91210/#review92876
Attachment #8808394 - Flags: review+

Comment 9

3 years ago
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a1621d1bea1
Use tup's internal symlinking instead of build manifests; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/cad7e38527b9
Support some FinalTargetFiles in the tup backend; r=chmanchester

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a1621d1bea1
https://hg.mozilla.org/mozilla-central/rev/cad7e38527b9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

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