Currently, rules.mk uses $(wildcard) to look for jar.mn's: > JAR_MANIFEST := $(srcdir)/jar.mn > > ifneq (,$(wildcard $(JAR_MANIFEST))) > ifndef NO_DIST_INSTALL > libs realchrome:: $(CHROME_DEPS) $(FINAL_TARGET)/chrome > $(PYTHON) $(MOZILLA_DIR)/config/JarMaker.py \ > $(QUIET) -j $(FINAL_TARGET)/chrome \ > $(MAKE_JARS_FLAGS) $(XULPPFLAGS) $(DEFINES) $(ACDEFINES) \ > $(JAR_MANIFEST) > endif > endif This is suboptimal because it has to go to the filesystem to resolve things. It also goes against our principle that actions in rules.mk should be the result of variables defined in the Makefile. There are a few ways to approach this. One is to establish a variable whose presence triggers JarMaker processing. We could also establish a central manifest enumerating all jar.mn files. Or, we could leave things the way they are. I'll defer to others.

I would prefer a make variable that triggers jar.mn processing.

Might as well move JAR_MANIFEST in Makefile.in, and replace the ifneq (,$(wildcard)) with an ifdef.

although, the value is not used, so in practice, this would be a trigger for jar.mn processing

PROCESS_JAR_MN := 1 ?

See patch commit message. I only converted part of the tree (accessible/ and browser/) because I'm lazy. I'm thinking we can get the rest in a follow-up bug. I added a $(warning) on missing variable to grease the wheels of progress.

Comment on attachment 657150 [details] [diff] [review] Add PROCESS_JAR_MN, v1 Review of attachment 657150 [details] [diff] [review]: ----------------------------------------------------------------- I'm thinking it would make sense to do that after bug 784841.

(In reply to Mike Hommey [:glandium] from comment #6) > I'm thinking it would make sense to do that after bug 784841. I agree. This is certainly a very low-hanging fruit.

Note of interest, being able to specify a different filename than jar.mn might be helpful. Say, non-desktop apps only wanting to pick parts of toolkit. One way is to put a ton of ifdefs into jar.mn, the other way would be to maintain a parallel "pick on purpose" jar.android.mn or so. I'm not really convinced on what to do, gotta do a post to .platform today.

I think it's time we start to look into parsing jar.mn in emitter.py so we can start hooking things up to install manifests, etc. First step is this bug. Old comments seem to indicate we want an explicit variable rather than relying on file discovery. Do we want a boolean or list of filenames (with jar.mn being the only entry for most people)? How do we want to handle USE_EXTENSION_MANIFEST, which will need to be ported at the same time? How about a new function: jar_manifest(filename, extension_manifest=False). The side-effect is a jar manifest with the specified filename is registered for processing. glandium: I believe you had thoughts on this in IRC a short while ago...

The biggest challenge with this is that we have a lot of in-tree "hacks" that makes us treat the same jar.mns several times with different values of FINAL_TARGET, and/or DEFINES. (Also, i don't like using functions in moz.build but that's a different matter)

JAR_MANIFESTS can now be defined in moz.build files. However, due to limitations in rules.mk, only 1 file may be defined at a time. In the future, this restriction will be lifted. But first, better support for JAR manifests in the build config must be built. rules.mk will be updated in the subsequent conversion patch so this patch applied alone doesn't break the build.

Every directory with a jar.mn now has JAR_MANIFESTS defined in its moz.build file.

Every directory with a jar.mn now has JAR_MANIFESTS defined in its moz.build file. In addition, we removed the manual may_skip since JAR_MANIFESTS is now properly accounted for in moz.build.

Now with no bit rot.

Removed bit rot.

Comment on attachment 8345714 [details] [diff] [review] Part 1: Support for defining JAR manifests in moz.build Review of attachment 8345714 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +361,5 @@ > > + jar_manifests = sandbox.get('JAR_MANIFESTS', []) > + if len(jar_manifests) > 1: > + raise SandboxValidationError('While JAR_MANIFESTS is a list, ' > + 'we currently limit it to one value.') "it is currently limited to ..." @@ +364,5 @@ > + raise SandboxValidationError('While JAR_MANIFESTS is a list, ' > + 'we currently limit it to one value.') > + > + for path in jar_manifests: > + yield JARManifest(sandbox, mozpath.join(sandbox['SRCDIR'], path)) Why not keep it relative? ::: python/mozbuild/mozbuild/test/backend/common.py @@ -45,5 @@ > - 'external_make_dirs': { > - 'defines': [], > - 'non_global_defines': [], > - 'substs': [], > - }, Oh, a conflict with my patch in bug 778236 :) ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py @@ -528,5 @@ > """Test that FINAL_TARGET is written to backend.mk correctly.""" > env = self._consume('final_target', RecursiveMakeBackend) > > final_target_rule = "FINAL_TARGET = $(if $(XPI_NAME),$(DIST)/xpi-stage/$(XPI_NAME),$(DIST)/bin)$(DIST_SUBDIR:%=/%)" > - print([x for x in os.walk(env.topobjdir)]) yay for less noise when running tests.

Comment on attachment 8345717 [details] [diff] [review] Part 2: Define JAR_MANIFESTS in moz.build files Review of attachment 8345717 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +1291,5 @@ > $(LOOP_OVER_TOOL_DIRS) > > $(FINAL_TARGET)/chrome: $(call mkdir_deps,$(FINAL_TARGET)/chrome) > > +ifneq (,$(JAR_MANIFEST)) For a few weeks, i'd rather add a test that JAR_MANIFEST has a value if there's a jar.mn in the directory (it makes things more obvious for people that add a jar.mn locally, or worse, if one makes it to, say, fx-team, before it's merged to m-i after this patch is landed).

(In reply to Mike Hommey [:glandium] from comment #16) > @@ +364,5 @@ > > + raise SandboxValidationError('While JAR_MANIFESTS is a list, ' > > + 'we currently limit it to one value.') > > + > > + for path in jar_manifests: > > + yield JARManifest(sandbox, mozpath.join(sandbox['SRCDIR'], path)) > > Why not keep it relative? Many of our paths in the emitted objects are absolute. IMO it's easier for backends to consume absolutes because they can just operate on the raw paths. It does make it not possible to "reassociate" an object directory with a difference topsrcdir. But why would you want to do that? > ::: python/mozbuild/mozbuild/test/backend/common.py > @@ -45,5 @@ > > - 'external_make_dirs': { > > - 'defines': [], > > - 'non_global_defines': [], > > - 'substs': [], > > - }, > > Oh, a conflict with my patch in bug 778236 :) Yeah. This boilerplate has bothered me for too long. It had to go :)

Added a check to emitter.py to look for a jar.mn in srcdir that isn't in JAR_MANIFESTS.

One of these patches broke a jar manifest test under /config. https://tbpl.mozilla.org/?tree=Try&rev=246d5463223c

I added a check in emitter.py *and* in rules.mk to look for jar.mn files without the corresponding moz.build entry. It's in both so emitter.py can fail fast and rules.mk can catch directories without corresponding moz.build files. I wrote a script that looked for directories with jar.mn but no moz.build file. It found a test under /config. I updated that test to be an "externally managed make file" and to define JAR_MANIFEST manually. Hacky, but gets the job done. https://tbpl.mozilla.org/?tree=Try&rev=915945de962f

There was a BS Python unit test failure due to refactoring in part 1. Fixed locally. New try at https://tbpl.mozilla.org/?tree=Try&rev=0252215cfccc

Let's try that again: https://tbpl.mozilla.org/?tree=Try&rev=3fc3edc5df85

https://hg.mozilla.org/integration/mozilla-inbound/rev/940b2974f35b https://hg.mozilla.org/integration/mozilla-inbound/rev/9872d86dfa0c The failures on last try were due to a line endings normalization issue. I applied a very trivial test-only change to part 1 to strip line endings from the comparison.

https://hg.mozilla.org/mozilla-central/rev/940b2974f35b https://hg.mozilla.org/mozilla-central/rev/9872d86dfa0c

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/f3e87352a592 Part 2: Define JAR_MANIFESTS in moz.build files; r=glandium