Closed Bug 1319223 Opened 3 years ago Closed 2 years ago

Process chrome manifest entries in the tup backend

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mshal, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

This will handle the ChromeManifestEntry moz.build objects.
Assignee: nobody → cmanchester
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 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?
Attachment #8874648 - Flags: review?(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
(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.
(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.
(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.
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.
Attachment #8874648 - Flags: review?(mshal)
Attachment #8875517 - Flags: review?(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?
Attachment #8874648 - Flags: review+
Comment on attachment 8875517 [details]
Bug 1319223 - Generate interfaces.manifest files in the tup backend.

https://reviewboard.mozilla.org/r/146944/#review151450
Attachment #8875517 - Flags: review?(mshal) → review+
(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.
Sounds good!
Pushed by cmanchester@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f0fc4ee3e655
https://hg.mozilla.org/mozilla-central/rev/0900434f2415
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.