JAR manifests should be defined in moz.build

RESOLVED FIXED in mozilla29

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks 1 bug)

Trunk
mozilla29
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 6 obsolete attachments)

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 ?
Posted patch Add PROCESS_JAR_MN, v1 (obsolete) — Splinter Review
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.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #657150 - Flags: review?(mh+mozilla)
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.
Attachment #657150 - Flags: review?(mh+mozilla)
(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.
Depends on: 784841
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.
Summary: JAR manifests should be defined in Makefile.in → JAR manifests should be defined in moz.build
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...
Flags: needinfo?(mh+mozilla)
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)
Flags: needinfo?(mh+mozilla)
Attachment #657150 - Attachment is obsolete: true
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.
Attachment #8345148 - Flags: review?(mh+mozilla)
Every directory with a jar.mn now has JAR_MANIFESTS defined in its
moz.build file.
Attachment #8345160 - Flags: review?(mh+mozilla)
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.
Attachment #8345709 - Flags: review?(mh+mozilla)
Attachment #8345160 - Attachment is obsolete: true
Attachment #8345160 - Flags: review?(mh+mozilla)
Now with no bit rot.
Attachment #8345714 - Flags: review?(mh+mozilla)
Attachment #8345148 - Attachment is obsolete: true
Attachment #8345148 - Flags: review?(mh+mozilla)
Removed bit rot.
Attachment #8345717 - Flags: review?(mh+mozilla)
Attachment #8345709 - Attachment is obsolete: true
Attachment #8345709 - Flags: review?(mh+mozilla)
Blocks: 929147
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.
Attachment #8345714 - Flags: review?(mh+mozilla) → review+
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).
Attachment #8345717 - Flags: review?(mh+mozilla) → feedback+
(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.
Attachment #8346318 - Flags: review?(mh+mozilla)
Attachment #8345717 - Attachment is obsolete: true
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
Attachment #8346442 - Flags: review?(mh+mozilla)
Attachment #8346318 - Attachment is obsolete: true
Attachment #8346318 - Flags: review?(mh+mozilla)
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
Attachment #8346442 - Flags: review?(mh+mozilla) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 952120
Whiteboard: [qa-]
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/f3e87352a592
Part 2: Define JAR_MANIFESTS in moz.build files; r=glandium
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.