Closed Bug 1044162 Opened 5 years ago Closed 5 years ago

make install locations for EXTRA_{PP_,}JS_MODULES better

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: froydnj, Assigned: froydnj)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(6 files, 2 obsolete files)

7.61 KB, patch
mshal
: review+
Details | Diff | Splinter Review
2.56 KB, patch
mshal
: review+
Details | Diff | Splinter Review
4.76 KB, patch
mshal
: review+
Details | Diff | Splinter Review
3.68 KB, patch
mshal
: review+
Details | Diff | Splinter Review
40.45 KB, patch
mshal
: review+
Details | Diff | Splinter Review
5.89 KB, patch
mshal
: review+
Details | Diff | Splinter Review
...by getting rid of the need for JS_MODULES_PATH
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.
Attachment #8462738 - Flags: review?(mshal)
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. ;)
Attachment #8462739 - Flags: review?(mshal)
Spelling fix while in the process of fixing this, self-reviewing.
Attachment #8462741 - Flags: review+
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.
Attachment #8462742 - Flags: review?(mshal)
No moz.build files set this, so let's get rid of it.
Attachment #8462743 - Flags: review?(mshal)
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?
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.
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.
Attachment #8462741 - Attachment is obsolete: true
Attachment #8462818 - Flags: review?(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.

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.
Attachment #8462742 - Attachment is obsolete: true
Attachment #8462742 - Flags: review?(mshal)
Attachment #8462819 - Flags: review?(mshal)
(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.
Attachment #8462738 - Flags: review?(mshal) → review+
Attachment #8462739 - Flags: review?(mshal) → review+
Attachment #8462818 - Flags: review?(mshal) → review+
Attachment #8462819 - Flags: review?(mshal) → review+
Comment on attachment 8462743 [details] [diff] [review]
part 2 - remove JS_MODULES_PATH from the build system

Woot! Looks good to me.
Attachment #8462743 - Flags: review?(mshal) → review+
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.
Attachment #8463613 - Flags: review?(mshal)
Attachment #8463613 - Flags: review?(mshal) → review+
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.
Blocks: 1046638
Assignee: nobody → nfroyd
Duplicate of this bug: 948851
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.