870396 - Move defs.mk files to moz.build

Status: NEW

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

+++ This bug was initially created as a clone of Bug #870394 +++

>> There are several defs.mk files throughout the tree - these should be converted to a moz.build format.

Comment 1

[(not currently active) Ted Mielczarek]

There appear to be 3 of these left:
https://dxr.mozilla.org/mozilla-central/source/browser/defs.mk
https://dxr.mozilla.org/mozilla-central/source/webapprt/defs.mk
https://dxr.mozilla.org/mozilla-central/source/mobile/android/defs.mk

The first two set a var that's used in rules.mk to package XPIs properly per-app. Moving that to moz.build shouldn't be hard, although we lose the nice property of defs.mk applying to directories recursively, which helps ensure that it doesn't get forotten.

The third seems to be a collection of things that mobile/android wants in various Makefiles, I'd have to pick through them on a case-by-case basis to see what moving them to moz.build would look like.

Comment 2

[Nick Alexander :nalexander [he/him]]

> The third seems to be a collection of things that mobile/android wants in
> various Makefiles, I'd have to pick through them on a case-by-case basis to
> see what moving them to moz.build would look like.

Correct.  I've always regretted using defs.mk for this.

These are really branding options; it would be good to set them in the mobile/android/branding/*/configure.sh scripts.  The only consumers outside of mobile/android are in build/mobile/robocop and that should move into mobile/android shortly, which might simplify this business.  I'll use Bug 938994 to track that work.

N.b.: the sharedId fields are forever.  The _sync user account fields will disappear entirely with Bug 1220906.

Comment 3

[Nick Alexander :nalexander [he/him]]

We're down to just https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/browser/defs.mk, which contains only

XPI_ROOT_APPID=$(MOZ_APP_ID)

Looks like that is true for comm-central, too: https://dxr.mozilla.org/comm-central/search?q=XPI_ROOT.

There's a comment about XPI_ROOT_APPID that suggests to me that we should just s/XPI_ROOT_APPID/MOZ_APP_ID/ and get rid of the cruft.  (And defs.mk at the same time.)

# if DIST_SUBDIR is defined but XPI_ROOT_APPID is not there's
# no way langpacks will get packaged right, so error out.
ifneq (,$(DIST_SUBDIR))
ifndef XPI_ROOT_APPID
$(error XPI_ROOT_APPID is not defined - langpacks will break.)
endif
endif
endif

I recently helped get langpacks building on Fennec -- see https://bugzilla.mozilla.org/show_bug.cgi?id=1413240 -- but for Fennec, DIST_SUBDIR is not browser/, so this all Just Works.

Comment 4

[(not currently active) Ted Mielczarek]

XPI_ROOT_APPID seems to be only actually *used* once:
https://dxr.mozilla.org/mozilla-central/rev/dc70d241f90df43505ece5ac12261339e9694c50/config/rules.mk#1245

I agree that we should just change that to MOZ_APP_ID and get rid of all other references to XPI_ROOT_APPID. This might make it slightly harder if we ever want to build a second app in the same build as Firefox, but I think we can cross that bridge when we get to it.

Comment 5

[Nick Alexander :nalexander [he/him]]

I started to dig into this, and it's more complicated than I thought.  If I understand correctly (and restricting to --enable-application=browser only), we need to make sure everything that's non-GRE has XPI_ROOT_APPID applied.  Fine, that's all of browser/ -- but also 

	@$(MAKE) -C ../../toolkit/locales libs-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
	@$(MAKE) -C ../../devtools/client/locales AB_CD=$* XPI_NAME=locale-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'
	@$(MAKE) -C ../../devtools/startup/locales AB_CD=$* XPI_NAME=locale-$* XPI_ROOT_APPID='$(XPI_ROOT_APPID)'

per

https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/browser/locales/Makefile.in#74-84

Is this actually _everything_ that's locale-aware in jar.mn files for --enable-application=browser?  I haven't yet figured that out.  If it is everything, that would be simple to change: we just set the JAR maker flag to $(MOZ_APP_ID) and move on with our lives.  If not, we need to figure out which jar.mn files _don't_ get the XPI_ROOT_APPID treatment, and somehow annotate jar.mn files with this information.

ted: do you know what XPI_ROOT_APPID applies to for --enable-application=browser?

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 870396 - Replace XPI_ROOT_APPID with MOZ_APP_ID; remove last defs.mk. .mielczarek

The rationale is in the comment in recursivemake.py: as best I can
tell, _if_ we're using split GRE and app (which everything but
mobile/android uses), anything under MOZ_BUILD_APP needs the appid,
_and_ everything that registers locale-aware chrome.  Right now, the
former is handled by defs.mk and the latter is handled by manually
passing XPI_ROOT_APPID around in sub-Make invocations.  This is better
than both of those approaches.

With this patch I produce bit-identical langpack XPIs, and the
chrome.manifest files in the langpacks really do include the expected
appids.

Review commit: https://reviewboard.mozilla.org/r/247276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247276/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8981190 [details]
Bug 870396 - Replace XPI_ROOT_APPID with MOZ_APP_ID; remove last defs.mk. .mielczarek

https://reviewboard.mozilla.org/r/247276/#review253362


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/moz.configure:7
(Diff revision 1)
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  imply_option('MOZ_PLACES', True)

Error: Undefined name 'imply_option' [flake8: F821]

::: browser/moz.configure:8
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  imply_option('MOZ_PLACES', True)
>  imply_option('MOZ_SERVICES_HEALTHREPORT', True)

Error: Undefined name 'imply_option' [flake8: F821]

::: browser/moz.configure:9
(Diff revision 1)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  imply_option('MOZ_PLACES', True)
>  imply_option('MOZ_SERVICES_HEALTHREPORT', True)
>  imply_option('MOZ_SERVICES_SYNC', True)

Error: Undefined name 'imply_option' [flake8: F821]

::: browser/moz.configure:11
(Diff revision 1)
>  
>  imply_option('MOZ_PLACES', True)
>  imply_option('MOZ_SERVICES_HEALTHREPORT', True)
>  imply_option('MOZ_SERVICES_SYNC', True)
>  
>  include('../toolkit/moz.configure')

Error: Undefined name 'include' [flake8: F821]

::: browser/moz.configure:14
(Diff revision 1)
>  imply_option('MOZ_SERVICES_SYNC', True)
>  
>  include('../toolkit/moz.configure')
> +
> +
> +@dependable

Error: Undefined name 'dependable' [flake8: F821]

::: browser/moz.configure:19
(Diff revision 1)
> +@dependable
> +def xpi_root_appid():
> +    return '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}'
> +
> +
> +set_config('XPI_ROOT_APPID', xpi_root_appid)

Error: Undefined name 'set_config' [flake8: F821]

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Nick Alexander :nalexander from comment #5)
> Is this actually _everything_ that's locale-aware in jar.mn files for
> --enable-application=browser?  I haven't yet figured that out.  If it is
> everything, that would be simple to change: we just set the JAR maker flag
> to $(MOZ_APP_ID) and move on with our lives.  If not, we need to figure out
> which jar.mn files _don't_ get the XPI_ROOT_APPID treatment, and somehow
> annotate jar.mn files with this information.
> 
> ted: do you know what XPI_ROOT_APPID applies to for
> --enable-application=browser?

I think maybe I didn't communicate my intent well enough? I thought we would simply replace `$(XPI_ROOT_APPID)` with `$(MOZ_APP_ID)` on this line here:
https://dxr.mozilla.org/mozilla-central/rev/dc70d241f90df43505ece5ac12261339e9694c50/config/rules.mk#1245

Then remove all references to `XPI_ROOT_APPID` elsewhere in the tree. That line would continue to work as it currently does--it'd pass the app ID to make_jar.py when building langpacks.

If at some point we rewrite how langpacks are created we'll just have to make sure that we replicate this functionality. Given that the jar manifest parser already knows whether manifest entries are localized, it seems like we would simply filter manifests based on that information and always use the root app ID with JarMaker when building a langpack.

Does that make sense?

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8981190 [details]
Bug 870396 - Replace XPI_ROOT_APPID with MOZ_APP_ID; remove last defs.mk. .mielczarek

https://reviewboard.mozilla.org/r/247276/#review254118

::: python/mozbuild/mozbuild/backend/recursivemake.py:634
(Diff revision 1)
> +
> +                return False
> +
> +            if 'XPI_ROOT_APPID' in self.environment.substs:
> +                if needs_xpi_root_appid(obj.path.full_path):
> +                    backend_file.write('XPI_ROOT_APPID := %s\n' %

Isn't this redundant with the `set_config` you added in browser/moz.configure? That ought to set `XPI_ROOT_APPID` globally in autoconf.mk, right? (Which should be fine, since the rules.mk bit is conditioned on `XPI_NAME` anyway.)

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> (In reply to Nick Alexander :nalexander from comment #5)
> > Is this actually _everything_ that's locale-aware in jar.mn files for
> > --enable-application=browser?  I haven't yet figured that out.  If it is
> > everything, that would be simple to change: we just set the JAR maker flag
> > to $(MOZ_APP_ID) and move on with our lives.  If not, we need to figure out
> > which jar.mn files _don't_ get the XPI_ROOT_APPID treatment, and somehow
> > annotate jar.mn files with this information.
> > 
> > ted: do you know what XPI_ROOT_APPID applies to for
> > --enable-application=browser?
> 
> I think maybe I didn't communicate my intent well enough? I thought we would
> simply replace `$(XPI_ROOT_APPID)` with `$(MOZ_APP_ID)` on this line here:
> https://dxr.mozilla.org/mozilla-central/rev/
> dc70d241f90df43505ece5ac12261339e9694c50/config/rules.mk#1245

You did communicate this, but it's not possible directly: see below.

> Then remove all references to `XPI_ROOT_APPID` elsewhere in the tree. That
> line would continue to work as it currently does--it'd pass the app ID to
> make_jar.py when building langpacks.

It's not true that we should be passing the appid generally: it definitely doesn't happen on mobile/android.  I tried to say this in the commit message:

"""
The rationale is in the comment in recursivemake.py: as best I can
tell, _if_ we're using split GRE and app (which everything but
mobile/android uses), anything under MOZ_BUILD_APP needs the appid,
_and_ everything that registers locale-aware chrome.
"""

I think it's probably safe to set the application=... line for mobile/android everywhere, even though it doesn't split GRE and app, since there's no XULRunner-like re-use allowing the application to vary.  But that's a change from what we do now, which is harder to verify.

That's why I introduced XPI_ROOT_APPID and only set it for browser/ (and, eventually, mail/ or whatever comm-central is).

> If at some point we rewrite how langpacks are created we'll just have to
> make sure that we replicate this functionality. Given that the jar manifest
> parser already knows whether manifest entries are localized, it seems like
> we would simply filter manifests based on that information and always use
> the root app ID with JarMaker when building a langpack.
> 
> Does that make sense?

I thought of pushing this into jar.py, which should know whether it's needed, but wasn't confident that it would play nicely with all uses of that code.  This is at least isomorphic to what existed.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> Comment on attachment 8981190 [details]
> Bug 870396 - Handle XPI_ROOT_APPID in RecursiveMake backend; remove last
> defs.mk. .mielczarek
> 
> https://reviewboard.mozilla.org/r/247276/#review254118
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py:634
> (Diff revision 1)
> > +
> > +                return False
> > +
> > +            if 'XPI_ROOT_APPID' in self.environment.substs:
> > +                if needs_xpi_root_appid(obj.path.full_path):
> > +                    backend_file.write('XPI_ROOT_APPID := %s\n' %
> 
> Isn't this redundant with the `set_config` you added in
> browser/moz.configure? That ought to set `XPI_ROOT_APPID` globally in
> autoconf.mk, right? (Which should be fine, since the rules.mk bit is
> conditioned on `XPI_NAME` anyway.)

I'm... not sure.  I'll need to investigate.  I know for sure that in some iterations of this code, `set_config` was not enough, but now I'm questioning myself.  It's not clear to me that we can just set this globally for browser/; if we can, that would be nicer than writing this to backend.mk.  I'll report back.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8981190 [details]
Bug 870396 - Replace XPI_ROOT_APPID with MOZ_APP_ID; remove last defs.mk. .mielczarek

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247276/diff/1-2/

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8981190 [details]
Bug 870396 - Replace XPI_ROOT_APPID with MOZ_APP_ID; remove last defs.mk. .mielczarek

https://reviewboard.mozilla.org/r/247276/#review258912

Great detective work, and great to see another chunk of the recursive make backend go away!

::: config/rules.mk:1222
(Diff revision 2)
>  ifdef XPI_NAME
> -ifdef XPI_ROOT_APPID
> +ifneq (,$(DIST_SUBDIR))
>  # For add-on packaging we may specify that an application
>  # sub-dir should be added to the root chrome manifest with
>  # a specific application id.
> -MAKE_JARS_FLAGS += --root-manifest-entry-appid='$(XPI_ROOT_APPID)'
> +MAKE_JARS_FLAGS += --root-manifest-entry-appid='$(MOZ_APP_ID)'

Can you file a followup on moving this somewhere like the common backend? We have this info there and we'd be able to replicate this for other build backends.

Thanks for digging in and sorting this out, this is much clearer!

Comment 13

[Nick Alexander :nalexander [he/him]]

ted: can you rebase this and land it?  I'm really far out of the mozilla-central world these days.