1319223 - Process chrome manifest entries in the tup backend

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

This will handle the ChromeManifestEntry moz.build objects.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

The way we do these doesn't really seem to agree with Tup's execution model, so I have a patch that just writes out the manifests from the build backend. I noticed the FasterMake backend uses install manifests to achieve this and the RecursiveMake backend uses buildlist from Makefiles, but this seems about as effective.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1319223 - Handle ChromeManifestEntry objects in the tup backend.

The approach here is similar to the FasterMake backend, but rather than writing
out manifest entries to a backend file to be written out during the build
we write the manifests to their final location within the build backend.

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

Comment 3

[Michael Shal [:mshal]]

Comment on attachment 8874648 [details]
Bug 1319223 - Handle ChromeManifestEntry objects in the tup backend.

https://reviewboard.mozilla.org/r/145996/#review150868

Thanks for taking this on! I think this is a reasonable way to do it. However when comparing against the manifests generated by the RecursiveMake backend, I see a few differences. One seems to be that interfaces.manifest lines aren't generated (they are added in by config/makefiles/xpidl/Makefile to a few manifests). Another is that in some cases we get an extra manifest whereas the RecursiveMake backend generates a 'content worker' line. Eg:

--- dist/xpi-stage/worker/chrome.manifest ---
1c1
< content worker chrome/worker/content/
---
> manifest chrome/worker.manifest
3d2
< manifest components/interfaces.manifest

--- dist/xpi-stage/worker/chrome/worker.manifest ---
diff: obj-x86_64-pc-linux-gnu/dist/xpi-stage/worker/chrome/worker.manifest: No such file or directory

Are these workable with this approach?

Comment 4

[Michael Shal [:mshal]]

Also, this seems to modify a file in the srcdir for some reason:

diff --git a/tools/quitter/chrome.manifest b/tools/quitter/chrome.manifest
index b01f046..b3fc63b 100644
--- a/tools/quitter/chrome.manifest
+++ b/tools/quitter/chrome.manifest
@@ -1,4 +1 @@
-category profile-after-change @mozilla.org/quitter-observer;1 @mozilla.org/quitter-observer;1
-component {c235a986-5ac1-4f28-ad73-825dae9bad90} components/QuitterObserver.js
-content quitter chrome/quitter/content/
-contract @mozilla.org/quitter-observer;1 {c235a986-5ac1-4f28-ad73-825dae9bad90}
+manifest chrome/quitter.manifest

Comment 5

[Chris Manchester (limited bugmail, email directly)]

(In reply to Michael Shal [:mshal] from comment #4)
> Also, this seems to modify a file in the srcdir for some reason:
> 
> diff --git a/tools/quitter/chrome.manifest b/tools/quitter/chrome.manifest
> index b01f046..b3fc63b 100644
> --- a/tools/quitter/chrome.manifest
> +++ b/tools/quitter/chrome.manifest
> @@ -1,4 +1 @@
> -category profile-after-change @mozilla.org/quitter-observer;1
> @mozilla.org/quitter-observer;1
> -component {c235a986-5ac1-4f28-ad73-825dae9bad90}
> components/QuitterObserver.js
> -content quitter chrome/quitter/content/
> -contract @mozilla.org/quitter-observer;1
> {c235a986-5ac1-4f28-ad73-825dae9bad90}
> +manifest chrome/quitter.manifest

That's odd, I'll look into it. Weirdly this only seems to happen when I apply the patch and do an incremental build.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

(In reply to Chris Manchester (:chmanchester) from comment #5)
> (In reply to Michael Shal [:mshal] from comment #4)
> > Also, this seems to modify a file in the srcdir for some reason:
> > 
> > diff --git a/tools/quitter/chrome.manifest b/tools/quitter/chrome.manifest
> > index b01f046..b3fc63b 100644
> > --- a/tools/quitter/chrome.manifest
> > +++ b/tools/quitter/chrome.manifest
> > @@ -1,4 +1 @@
> > -category profile-after-change @mozilla.org/quitter-observer;1
> > @mozilla.org/quitter-observer;1
> > -component {c235a986-5ac1-4f28-ad73-825dae9bad90}
> > components/QuitterObserver.js
> > -content quitter chrome/quitter/content/
> > -contract @mozilla.org/quitter-observer;1
> > {c235a986-5ac1-4f28-ad73-825dae9bad90}
> > +manifest chrome/quitter.manifest
> 
> That's odd, I'll look into it. Weirdly this only seems to happen when I
> apply the patch and do an incremental build.

Yeah, this just isn't doing the right thing in this case (this chrome.manifest is linked from the srcdir). This is probably related to why the faster make backend doesn't attempt to handle any of these outside of dist/bin.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8874648 [details]
> Bug 1319223 - Handle ChromeManifestEntry objects in the tup backend.
> 
> https://reviewboard.mozilla.org/r/145996/#review150868
> 
> Thanks for taking this on! I think this is a reasonable way to do it.
> However when comparing against the manifests generated by the RecursiveMake
> backend, I see a few differences. One seems to be that interfaces.manifest
> lines aren't generated (they are added in by config/makefiles/xpidl/Makefile
> to a few manifests). Another is that in some cases we get an extra manifest
> whereas the RecursiveMake backend generates a 'content worker' line. Eg:
> 
> --- dist/xpi-stage/worker/chrome.manifest ---
> 1c1
> < content worker chrome/worker/content/
> ---
> > manifest chrome/worker.manifest
> 3d2
> < manifest components/interfaces.manifest
> 
> --- dist/xpi-stage/worker/chrome/worker.manifest ---
> diff: obj-x86_64-pc-linux-gnu/dist/xpi-stage/worker/chrome/worker.manifest:
> No such file or directory
> 
> Are these workable with this approach?


The interfaces.manifest lines seem to be added without moz.build knowing about it at all, I guess we'll have to deal with that first.

The "content worker" lines correspond to the "useChromeManifest" case in jar.py, but since the tup backend is borrowing fastermake's approach handling jar manifests this isn't happening the same way.

So I think we'll have to assume as with the faster make backend we're not handling anything outside of dist/bin, but in the mean time we will have to figure out how to make mozbuild aware of some of what's happening in config/makefiles/xpidl/Makefile.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Actually, it looks like we have what we need in the idl manager. It's a little hard to tell we're doing exactly what config/makefiles/xpidl/Makefile is, but I have a patch that at least produces interfaces.manifest files that agree.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1319223 - Generate interfaces.manifest files in the tup backend.

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

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8874648 [details]
Bug 1319223 - Handle ChromeManifestEntry objects in the tup backend.

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

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8874648 [details]
Bug 1319223 - Handle ChromeManifestEntry objects in the tup backend.

https://reviewboard.mozilla.org/r/145996/#review151448

These look good! The only difference I see now seems minor:

--- dist/xpi-stage/worker/chrome.manifest ---
1,2d0
< content worker chrome/worker/content/
< manifest components/WorkerTest.manifest

Any idea why that one is missed in the tup backend?

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8875517 [details]
Bug 1319223 - Generate interfaces.manifest files in the tup backend.

https://reviewboard.mozilla.org/r/146944/#review151450

Comment 13

[Chris Manchester (limited bugmail, email directly)]

(In reply to Michael Shal [:mshal] from comment #11)
> Comment on attachment 8874648 [details]
> Bug 1319223 - Handle ChromeManifestEntry objects in the tup backend.
> 
> https://reviewboard.mozilla.org/r/145996/#review151448
> 
> These look good! The only difference I see now seems minor:
> 
> --- dist/xpi-stage/worker/chrome.manifest ---
> 1,2d0
> < content worker chrome/worker/content/
> < manifest components/WorkerTest.manifest
> 
> Any idea why that one is missed in the tup backend?

We're skipping anything that isn't under dist/bin for now (comment 7). This is test related so I'm assuming we shouldn't block on it for now.

Comment 14

[Michael Shal [:mshal]]

Sounds good!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0fc4ee3e655
Handle ChromeManifestEntry objects in the tup backend. r=mshal
https://hg.mozilla.org/integration/autoland/rev/0900434f2415
Generate interfaces.manifest files in the tup backend. r=mshal

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0fc4ee3e655
https://hg.mozilla.org/mozilla-central/rev/0900434f2415