1044162 - make install locations for EXTRA_{PP_,}JS_MODULES better

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

...by getting rid of the need for JS_MODULES_PATH

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 0a - make additions to EXTRA_JS_MODULES use += instead of =

Straight assignments to HierarchicalStringList, which we'll change
EXTRA_JS_MODULES to in a later patch, don't work.  This change, in
addition to making things work as expected later on, also is more
consistent with existing practice.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 0b - add __{get,set}item__ to HierarchicalStringList

I am slightly surprised that we haven't needed this before, but the
motivating idea behind this is wanting to do:

VARIABLE.with.some["non-pythonic-identifier"] += ...

It seems unfortunate that the behavior is completely overridden by
subclasses, but I don't see a good way around that.

This is semi-controversial, but I couldn't make the alternatives with getattr
or __getattr__ work in the context of moz.build.  Ted suggested something like
this, so all blame to him and all credit to me. ;)

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 0c - fixup tiny typo in _process_hierarchy docstring

Spelling fix while in the process of fixing this, self-reviewing.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make EXTRA_{PP_,}JS_MODULES communicate their installation path

This patch makes EXTRA_{PP_,}JS_MODULES similar in functionality to
TESTING_JS_MODULES: we indicate the path relative to
$(FINAL_TARGET)/modules with an appropriate hierarchy of paths.

Fortunately, everybody declared a JS_MODULES_PATH starting with modules/, so
there's no special need for cleverness here.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove JS_MODULES_PATH from the build system

No moz.build files set this, so let's get rid of it.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8462742 [details] [diff] [review]
part 1 - make EXTRA_{PP_,}JS_MODULES communicate their installation path

Review of attachment 8462742 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +946,5 @@
> +
> +        if obj.flavor == 'extra':
> +            manifest = self._install_manifests['dist_bin']
> +            def on_extra_module(source, dest, flags):
> +                manifest.add_symlink(source, dest)

Apparently the complaint about items already being in the manifest that I was getting during incremental builds is actually supposed to be fatal, as it was in a clobber.  Eagerly adding things here isn't appropriate, because DIST_SUBDIR (among other things?) can change where FINAL_TARGET winds up.

I guess the more-correct thing to do here is to use INSTALL_TARGETS for everything?  Or should we just force people to set .subdir on the toplevel if they need resources to wind up in a different place?

Comment 7

[(not currently active) Ted Mielczarek]

I think forcing things to be more explicit is a good idea. If we've got a nice data structure to use then hacks like DIST_SUBDIR can die.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 0c - add a way to recurse over the 'directory' elements of a hierarchy

We're going to build on top of INSTALL_TARGETS for the next patch, and
it's easiest to do so if we can look at the 'directories' in the
hierarchy, rather than the individual strings.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - make EXTRA_{PP_,}JS_MODULES communicate their installation path

This patch makes EXTRA_{PP_,}JS_MODULES similar in functionality to
TESTING_JS_MODULES: we indicate the path relative to
$(FINAL_TARGET)/modules with an appropriate hierarchy of paths.

Re-using INSTALL_TARGETS seemed a lot easier than trying to get DIST_SUBDIR
support correct.  My apologies in advance if this is not the best way to do
things.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> I think forcing things to be more explicit is a good idea. If we've got a
> nice data structure to use then hacks like DIST_SUBDIR can die.

I think a nice replacement for DIST_SUBDIR would be a derivative of the templates from bug 1041941.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8462743 [details] [diff] [review]
part 2 - remove JS_MODULES_PATH from the build system

Woot! Looks good to me.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: fixup patch for part 1 (intended to be rolled up with part 1)

This patch addresses several problems that builds and tests noticed:

- We shouldn't pass through EXTRA_{PP_,}JS_MODULES anymore, since they have some
  unusual moz.build-ish definition now.

- ...and we shouldn't verify that we pass them through in tests.

- The way PP_TARGETS was generated was wrong and has been fixed.

- Since we don't define EXTRA_JS_MODULES in backend.mk anymore, we can't
  assume that we have it to use in crashreporter/test/Makefile.in.  I
  think the best solution here is to simply list the relevant file as a
  dependency.

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/bf3d95bc43cd696fc186ab079075859ac6f9d034
Bug 1044162 - part 1 - make EXTRA_{PP_,}JS_MODULES communicate their installation path; r=mshal

This patch makes EXTRA_{PP_,}JS_MODULES similar in functionality to
TESTING_JS_MODULES: we indicate the path relative to
$(FINAL_TARGET)/modules with an appropriate hierarchy of paths.

Comment 14

[Nathan Froyd [:froydnj]]

This landed:

http://hg.mozilla.org/mozilla-central/rev/50991b8899ee
http://hg.mozilla.org/mozilla-central/rev/66ed4e26d9b4
http://hg.mozilla.org/mozilla-central/rev/275a064b9345
http://hg.mozilla.org/mozilla-central/rev/b961ba8f0892
http://hg.mozilla.org/mozilla-central/rev/85b40c4b8af7

I guess mcMerge missed this.