Don't enumerate components/*.manifest and chrome/*.manifest

RESOLVED FIXED in mozilla2.0b4

Status

()

Core
XPCOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
mozilla2.0b4
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0a1+)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
We currently enumerate components/*.manifest and chrome/*.manifest. Enumerating directories is a known source of bad I/O during startup, and also means that manifests may be loaded in any order. Instead, we should just read the root chrome.manifest and load sub-manifests as necessary.

This patch ends up with five manifests total in a packaged build:

chrome.manifest
-> components/interfaces.manifest
-> components/components.manifest
-> chrome/nonlocalized.manifest
-> chrome/localized.manifest

Unpackaged builds keep lots of manifests around.

Oh, and I'm going to need some removed-files for trunk builds... I forgot that, but I'll catch it later.
(Assignee)

Comment 1

7 years ago
Created attachment 457678 [details] [diff] [review]
Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1

There are a few tests I may need to clean up as well. We'll see.
Attachment #457678 - Flags: review?(dtownsend)
(Assignee)

Comment 2

7 years ago
Created attachment 457679 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 1
Attachment #457679 - Flags: review?(me)
(Assignee)

Comment 3

7 years ago
Comment on attachment 457679 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 1

Tests need work, they drop stuff in the appdir.
Attachment #457679 - Flags: review?(me)
(Assignee)

Comment 4

7 years ago
Created attachment 457919 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 2

Changes:

* package mochitest chrome in distribution/bundles instead of dropping a manifest into chrome/
* ship tp-cmdline in release builds
Attachment #457679 - Attachment is obsolete: true
Attachment #457919 - Flags: review?(me)
(Assignee)

Comment 5

7 years ago
Created attachment 457921 [details] [diff] [review]
Part C - Allow xpcshell to register additional manifests, rev. 1
Attachment #457921 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #457921 - Flags: review? → review?(jwalden+bmo)
Comment on attachment 457919 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 2

Looks good except that I don't see any reason to move AB_CD.manifest to localized.manifest.  It looks like we can keep it as AB_CD.manifest the whole way through.  If there's no compelling reason not to keep it that way, lets do that.
Attachment #457919 - Flags: review?(me) → review+
Attachment #457921 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 457678 [details] [diff] [review]
Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1

>diff --git a/xpcom/components/nsIManifestLoader.idl b/xpcom/components/nsIManifestLoader.idl
>deleted file mode 100644

>diff --git a/modules/libjar/nsManifestZIPLoader.h b/modules/libjar/nsManifestZIPLoader.h
> #include "nsIManifestLoader.h"

nsIManifestLoader.idl was removed, and this file has no references left to anything that was in this .h, but it will fail in a clobber, please remove.

>diff --git a/modules/libjar/objs.mk b/modules/libjar/objs.mk
> MODULES_LIBJAR_LEXPORTS = \
> 		zipstruct.h \
> 		nsZipArchive.h \
>-		nsManifestZIPLoader.h \
> 		$(NULL)

>diff --git a/xpcom/components/Makefile.in b/xpcom/components/Makefile.in
>+	-I$(topsrcdir)/modules/libjar \

>diff --git a/xpcom/components/nsComponentManager.h b/xpcom/components/nsComponentManager.h
>--- a/xpcom/components/nsComponentManager.h
>+++ b/xpcom/components/nsComponentManager.h
>-#include "nsIManifestLoader.h"
>+#include "nsManifestZIPLoader.h"

Even by adding libjar to the include path, don't we want to keep LEXPORTS including this if its used outside of libjar? (You did however remove the other use of it in the tree).

From my skim of everything else this is a great cleanup!
Attachment #457678 - Flags: feedback+
Comment on attachment 457919 [details] [diff] [review]
Part B - packaging and repackaging changes, rev. 2

>diff --git a/config/tests/unit-JarMaker.py b/config/tests/unit-JarMaker.py
>-    debug = False # set to True to debug failing tests on disk
>+    debug = True # set to True to debug failing tests on disk

Please remove this hunk for checkin.

>diff --git a/config/rules.mk b/config/rules.mk
>+EXTRA_MANIFESTS = $(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))
>+ifneq (,$(EXTRA_MANIFESTS))
>+libs::
>+	$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/chrome.manifest $(patsubst %,"manifest components/%",$(notdir $(EXTRA_MANIFESTS)))
> endif

Why chrome.manifest here, doesn't it make more sense to use components.manifest instead? [and make sure components.manifest is linked in chrome.manifest as well]

>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
The hunk here looks unrelated to this bug, I always like changes to have their own bug so reasoning/followup Q's etc. can be found easily; That said, I see no reason a separate changeset/bug is needed for that [since it is already here] :-)
Attachment #457919 - Flags: feedback+
(In reply to comment #4)
> Created attachment 457919 [details] [diff] [review]
> Part B - packaging and repackaging changes, rev. 2
> 
...
> * ship tp-cmdline in release builds

Why is this patch planning on shipping what appears to be test-only code in release builds? Surely we should be correcting the test harness to work properly with the new code...
Duplicate of this bug: 580044
Blocks: 578626, 578653
I tested these patches on Fennec, and they do fix the component registration ordering issues.

I do agree with comments about using "chrome.manifest" as the name, since chrome/chrome.manifest exists too. Using "components.manifest" sound good to me.
(Assignee)

Comment 12

7 years ago
> Even by adding libjar to the include path, don't we want to keep LEXPORTS
> including this if its used outside of libjar? (You did however remove the other
> use of it in the tree).

No, I don't.

> >diff --git a/config/rules.mk b/config/rules.mk
> >+EXTRA_MANIFESTS = $(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))
> >+ifneq (,$(EXTRA_MANIFESTS))
> >+libs::
> >+	$(PYTHON) $(MOZILLA_DIR)/config/buildlist.py $(FINAL_TARGET)/chrome.manifest $(patsubst %,"manifest components/%",$(notdir $(EXTRA_MANIFESTS)))
> > endif
> 
> Why chrome.manifest here, doesn't it make more sense to use components.manifest
> instead? [and make sure components.manifest is linked in chrome.manifest as
> well]

No, why would I create an extra file if I don't need to? There will just be /chrome.manifest and whatever it loads.

> >diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
> The hunk here looks unrelated to this bug, I always like changes to have their
> own bug so reasoning/followup Q's etc. can be found easily; That said, I see no
> reason a separate changeset/bug is needed for that [since it is already here]
> :-)

It isn't unrelated at all: because you can no longer drop in manifests or components into the Firefox install directory, Talos will not work unless we either ship the tp-commandline file by default or somehow have special code to allow it to be dropped in. I opted to ship the -tp commandline code by default.
OS: Windows 7 → All
Hardware: x86 → All
tracking-fennec: --- → 2.0a1+
Comment on attachment 457678 [details] [diff] [review]
Part A - XPCOM changes to load submodules and stop enumerating directories, rev. 1

>diff --git a/modules/libjar/nsManifestZIPLoader.cpp b/modules/libjar/nsManifestZIPLoader.cpp

>-nsManifestZIPLoader::nsManifestZIPLoader() {
>+nsManifestZIPLoader::nsManifestZIPLoader() 
>+    : mZipReader(new nsJAR())
>+{
>+    nsresult rv = reader->Open(mozilla::OmnijarPath());

What is "reader"? Do you mean mZipReader? Is this path tested?

I also wonder if there is any gain in using the nsIZipReaderCache for stuff talking to the omnijar?

>diff --git a/xpcom/components/ManifestParser.cpp b/xpcom/components/ManifestParser.cpp
>--- a/xpcom/components/ManifestParser.cpp
>+++ b/xpcom/components/ManifestParser.cpp
>@@ -83,16 +83,18 @@ struct ManifestDirective
>   void (nsChromeRegistry::*regfunc)
>     (nsChromeRegistry::ManifestProcessingContext& cx,
>      int lineno, char *const *argv,
>      bool platform, bool contentaccessible);
> 
>   bool isContract;
> };
> static const ManifestDirective kParsingTable[] = {
>+  { "manifest", 1, false, true, false,

Can you line this up like the others.

>     if (!ok ||
>         stApp == eBad ||
>         stAppVersion == eBad ||
>         stOs == eBad ||
>         stOsVersion == eBad ||
>         stABI == eBad)
>       continue;
> 
>-    if (directive->ischrome) {
>+    if (directive->regfunc) {
> #ifdef MOZ_IPC
>       if (GeckoProcessType_Default != XRE_GetProcessType())
>         continue;
> #endif
> 

I wonder if it would be better to just put something higher up in the loop, before all the flag checks to the effect of:

if (!directive->ischrome && aChromeOnly)
  continue;

Then just need an if/else here.
Attachment #457678 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 459065 [details] [diff] [review]
When checking for sorted-matching, keep version A as the unified version, not the sorted version
Attachment #459065 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

7 years ago
Depends on: 580698
Blocks: 579098
The mochitest-chrome stuff sounds like it overlaps some work Joel was doing.
Thanks ted, it doesn't look like much overlap.  Actually when I do get around the landing my mochikit.jar stuff, it will be installed in the profile instead of the application directory (a side effect of android)
Comment on attachment 459065 [details] [diff] [review]
When checking for sorted-matching, keep version A as the unified version, not the sorted version

>diff --git a/build/Makefile.in b/build/Makefile.in
>--- a/build/Makefile.in
>+++ b/build/Makefile.in
>@@ -181,17 +181,17 @@ check::
>         else \
>           echo "TEST-PASS | build/ | unify unified a Java class file!"; \
>         fi
> # try unifying some files that differ only in line ordering
> 	rm -rf unify-sort-test
> 	mkdir unify-sort-test unify-sort-test/a unify-sort-test/b
> 	printf "lmn\nabc\nxyz\n" > unify-sort-test/a/file.foo
> 	printf "xyz\nlmn\nabc" > unify-sort-test/b/file.foo
>-	printf "abc\nlmn\nxyz\n" > unify-sort-test/expected-result
>+	printf "lmn\nabc\nxyz\n" > unify-sort-test/expected-result
> 	@if ! $(srcdir)/macosx/universal/unify --unify-with-sort "\.foo$$" \
>           ./unify-sort-test/a ./unify-sort-test/b \
>           ./unify-sort-test/c; then \
>           echo "TEST-UNEXPECTED-FAIL | build/ | unify failed to unify files with differing line ordering!"; \
>           false; \
>         fi
> 	@if ! diff -q ./unify-sort-test/expected-result ./unify-sort-test/c/file.foo; then \

You could instead just pass unify-sort-test/a instead of expected-result here. Doesn't matter much to me. All this shell scriptery is gross, I should have written it in Python.
Attachment #459065 - Flags: review?(ted.mielczarek) → review+
Blocks: 576746
(Assignee)

Updated

7 years ago
Whiteboard: Blocked on Talos changes
Blocks: 582695

Updated

7 years ago
Blocks: 518250
Blocks: 573612

Comment 18

7 years ago
Will this break multilocale Maemo builds? (jars+manifests in chrome/)
(Assignee)

Comment 19

7 years ago
I don't know, how are they built?

Comment 20

7 years ago
Putting jars and manifests in chrome/, plus some pref changes and maybe some other magic i don't know of.

Pretty sure this'll break jmaher's fennecmark stuff, but that's already currently broken.
(Assignee)

Comment 21

7 years ago
I was looking for a pointer to the code: if it is just dropping stuff in, yes, that will no longer work. You probably want to generate a chrome/localized.manifest which contains the directives for all the locales combined.
(Assignee)

Comment 22

7 years ago
Aki sent a link to http://hg.mozilla.org/mobile-browser/file/55ecfa303d51/locales/Makefile.in#l117

Where and how is this called?

Comment 23

7 years ago
This is called after building we run |make chrome-AB_CD| for every locale we want added to the multi-locale deb, then make package/make deb.
(Assignee)

Comment 24

7 years ago
In that case I suspect this will "just work".

We'll find out when it gets checked in... waiting for tryserver results, and I'm on vacation tomorrow, so ugh.
for fennecmark (talos test for fennec), what needs to be done?  Should this be installed in the profile instead of the appdir?
(Assignee)

Comment 26

7 years ago
I don't know. Why can't it do what desktop fennec does and install into distribution/bundles?
Depends on: 585628
(Assignee)

Comment 27

7 years ago
http://hg.mozilla.org/mozilla-central/rev/66e79b756b39
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: Blocked on Talos changes

Updated

7 years ago
Depends on: 585693

Updated

7 years ago
Depends on: 585721
Depends on: 586350

Updated

7 years ago
Blocks: 586190

Comment 28

7 years ago
dev-doc-needed: for the new "manifest" command to chrome.manifest (to be documented at https://developer.mozilla.org/en/Chrome_Registration ). Anything else documentation-worthy?
Keywords: dev-doc-needed
(Assignee)

Comment 29

7 years ago
If there are XULRunner docs which say that manifests may be dropped into chrome/ those should be updated as well.

Comment 30

7 years ago
Thanks, I found https://developer.mozilla.org/en/XUL_Tutorial/Manifest_Files#Manifest_Files and possibly https://developer.mozilla.org/en/Creating_XULRunner_Apps_with_the_Mozilla_Build_System#Okay.2c_okay.2c_can_I_build_it_now.3f
Target Milestone: --- → mozilla2.0b4
Version: unspecified → Trunk

Updated

7 years ago
Depends on: 592574
The manifest command has been documented for a while. The other articles mentioned in comment 30 still need updating.
Documentation has been updated here:

https://developer.mozilla.org/en/XUL_Tutorial/Manifest_Files#Manifest_files
https://developer.mozilla.org/en/Chrome_Registration

Linked to from:

https://developer.mozilla.org/en/Firefox_4_for_developers#Other_changes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.