Comment on attachment 758736 [details] [diff] [review] move EXTRA_COMPONENTS to moz.build (logic). Add EXTRA_COMPONENTS as a passthrough variable.

Comment on attachment 758736 [details] [diff] [review] move EXTRA_COMPONENTS to moz.build (logic). Inbound push https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab5517620a5

Comment on attachment 759309 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #1) patch is light, should be a lot more than two files.

Comment on attachment 759309 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #1) Only 1 this time? :)

Comment on attachment 759332 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #1) Looks good to me!

Comment on attachment 759897 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #2) >+# XXX - Until Suite builds off XULRunner we can't guarantee our implementation >+# of nsIDownloadManagerUI overrides toolkit's. >+if not CONFIG['MOZ_SUITE']: >+ EXTRA_COMPONENTS += [ >+ 'nsDownloadManagerUI.js', >+ 'nsDownloadManagerUI.manifest', >+ ] FYI it looks like MOZ_SUITE was removed in bug 445143 - maybe we can remove it in a followup? (I think the if-statement just goes away and keep the EXTRA_COMPONENTS line, since it will always be not-defined). MOZ_SUITE shows up in a couple other Makefiles as well. But that shouldn't stop this from landing since it's what the Makefile does - looks good!

(In reply to Michael Shal [:mshal] from comment #12) > FYI it looks like MOZ_SUITE was removed in bug 445143 - maybe we can remove > it in a followup? (I think the if-statement just goes away and keep the > EXTRA_COMPONENTS line, since it will always be not-defined). MOZ_SUITE shows > up in a couple other Makefiles as well. Nevermind about this - according to jcranmer, MOZ_SUITE is actually defined in c-c, so these Makefiles/moz.build files will pick up a definition from there when building c-c. Guess we have to be careful if we limit CONFIG to only valid configuration variables, since some are not defined by m-c.

Comment on attachment 759897 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #2) Try job: http://tbpl.mozilla.org/?tree=Try&rev=9ddcc73a5427 Push to inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound committed changeset: 134515:1857f54eb730

Comment on attachment 759332 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #1) Try job: https://tbpl.mozilla.org/?tree=Try&rev=2660bd9279d1 Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c15d9da2ae6 committed changeset 134516:1c15d9da2ae6

https://hg.mozilla.org/mozilla-central/rev/1857f54eb730 https://hg.mozilla.org/mozilla-central/rev/1c15d9da2ae6

Comment on attachment 768470 [details] [diff] [review] EXTRA_COMPONENTS cleanup for conversion patches #1 & #2. File conversion patches #1 & #2 landed on the 10th and stuck. DISABLED_ tokens can now be purged.

Comment on attachment 768470 [details] [diff] [review] EXTRA_COMPONENTS cleanup for conversion patches #1 & #2. >diff --git a/dom/network/src/Makefile.in b/dom/network/src/Makefile.in >--- a/dom/network/src/Makefile.in >+++ b/dom/network/src/Makefile.in >@@ -9,27 +9,18 @@ VPATH = $(srcdir) > > include $(DEPTH)/config/autoconf.mk > > LIBRARY_NAME = dom_network_s > LIBXUL_LIBRARY = 1 > FORCE_STATIC_LIB = 1 > FAIL_ON_WARNINGS := 1 > >-DISABLED_EXTRA_COMPONENTS = \ >- TCPSocket.js \ >- TCPSocketParentIntermediary.js \ >- TCPSocket.manifest \ >- $(NULL) > > ifdef MOZ_B2G_RIL >-DISABLED_EXTRA_COMPONENTS += \ >- NetworkStatsManager.manifest \ >- NetworkStatsManager.js \ >- $(NULL) > endif The now-empty ifdef MOZ_B2G_RIL can go away. >diff --git a/toolkit/components/places/Makefile.in b/toolkit/components/places/Makefile.in >--- a/toolkit/components/places/Makefile.in >+++ b/toolkit/components/places/Makefile.in >@@ -14,27 +14,18 @@ LIBRARY_NAME = places > MSVC_ENABLE_PGO := 1 > LIBXUL_LIBRARY = 1 > EXPORT_LIBRARY = 1 > MODULE_NAME = nsPlacesModule > IS_COMPONENT = 1 > > LOCAL_INCLUDES += -I$(srcdir)/../build > >-DISABLED_EXTRA_COMPONENTS = \ >- toolkitplaces.manifest \ >- nsLivemarkService.js \ >- nsTaggingService.js \ >- nsPlacesExpiration.js \ >- PlacesCategoriesStarter.js \ >- ColorAnalyzer.js \ >- $(NULL) > > ifdef MOZ_XUL >-DISABLED_EXTRA_COMPONENTS += nsPlacesAutoComplete.js nsPlacesAutoComplete.manifest > endif This ifdef can go away too. >diff --git a/toolkit/mozapps/update/Makefile.in b/toolkit/mozapps/update/Makefile.in >--- a/toolkit/mozapps/update/Makefile.in >+++ b/toolkit/mozapps/update/Makefile.in >@@ -4,22 +4,15 @@ > > DEPTH = @DEPTH@ > topsrcdir = @top_srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk > >-DISABLED_EXTRA_COMPONENTS = \ >- nsUpdateTimerManager.js \ >- nsUpdateTimerManager.manifest \ >- $(NULL) > > ifdef MOZ_UPDATER > >-DISABLED_EXTRA_COMPONENTS += \ >- nsUpdateService.manifest \ >- $(NULL) > > endif And one more ifdef :)

Remove patch files that have been deleted. Rebase

Comment on attachment 761106 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #3). https://tbpl.mozilla.org/?tree=Try&rev=b56869240047 Failures: win7-jetpack osx 10.8 - jsreftest linux64/ubuntu-bc

Comment on attachment 770756 [details] [diff] [review] cleanup for EXTRA_COMPONENTS patches 1 & 2 Inbound push https://hg.mozilla.org/integration/mozilla-inbound/rev/064524edbea2 committed changeset 137345:064524edbea2

Comment on attachment 770914 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #3, rebased) >diff --git a/config/rules.mk b/config/rules.mk >--- a/config/rules.mk >+++ b/config/rules.mk >@@ -1456,17 +1456,17 @@ > endif > endif #} XPIDLSRCS > > ################################################################################ > # Copy each element of EXTRA_COMPONENTS to $(FINAL_TARGET)/components > ifneq (,$(filter %.js,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))) > ifeq (,$(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))) > ifndef NO_JS_MANIFEST >-$(error .js component without matching .manifest. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0) >+$(error .js component without matching .manifest. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0 $(CURDIR)) This change seems out of scope for this bug. >diff --git a/content/base/src/Makefile.in b/content/base/src/Makefile.in >--- a/content/base/src/Makefile.in >+++ b/content/base/src/Makefile.in >@@ -21,17 +21,17 @@ > endif > > GQI_SRCS = contentbase.gqi > > # we don't want the shared lib, but we want to force the creation of a > # static lib. > FORCE_STATIC_LIB = 1 > >-EXTRA_COMPONENTS = \ >+DISABLED_EXTRA_COMPONENTS = \ > contentSecurityPolicy.manifest \ > contentAreaDropListener.js \ > contentAreaDropListener.manifest \ > messageWakeupService.js \ > messageWakeupService.manifest \ > $(NULL) gps: Am I just supposed to r- these now? >diff --git a/layout/tools/reftest/moz.build b/layout/tools/reftest/moz.build >--- a/layout/tools/reftest/moz.build >+++ b/layout/tools/reftest/moz.build >@@ -1,8 +1,16 @@ > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > # 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/. > > MODULE = 'reftest' > >+EXTRA_COMPONENTS += [ >+ 'reftest-cmdline.js', >+] >+ >+if not CONFIG['XPI_NAME']: >+ EXTRA_COMPONENTS += [ >+ 'reftest-cmdline.manifest', >+ ] I don't think XPI_NAME is a CONFIG variable. Normally it is set by the Makefile.in as a regular varible, which would not be in CONFIG. In this particular case it's even more bizarre - it appears that the Makefile is setup to re-invoke itself and pass XPI_NAME as a parameter to the make command-line. Are we sure this if statement is doing what is intended (whatever that may be)? Or am I just missing some context in how XPI_NAME is used? >diff --git a/netwerk/test/httpserver/moz.build b/netwerk/test/httpserver/moz.build >--- a/netwerk/test/httpserver/moz.build >+++ b/netwerk/test/httpserver/moz.build >@@ -6,8 +6,17 @@ > > XPIDL_SOURCES += [ > 'nsIHttpServer.idl', > ] > > MODULE = 'test_necko' > > XPCSHELL_TESTS_MANIFESTS += ['test/xpcshell.ini'] >+ >+EXTRA_COMPONENTS += [ >+ 'httpd.js', >+] >+ >+if not CONFIG['XPI_NAME']: >+ EXTRA_COMPONENTS += [ >+ 'httpd.manifest', >+ ] Similar question about XPI_NAME here.

Comment on attachment 770914 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #3, rebased) Review of attachment 770914 [details] [diff] [review]: ----------------------------------------------------------------- DISABLED_* make hulk angry.

Comment on attachment 770914 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #3, rebased) Marking r- until we get a patch without DISABLED_*

Comment on attachment 780274 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #3, rebased, no DISABLED) >diff --git a/config/rules.mk b/config/rules.mk >--- a/config/rules.mk >+++ b/config/rules.mk >@@ -1458,17 +1458,17 @@ > endif > endif #} XPIDLSRCS > > ################################################################################ > # Copy each element of EXTRA_COMPONENTS to $(FINAL_TARGET)/components > ifneq (,$(filter %.js,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))) > ifeq (,$(filter %.manifest,$(EXTRA_COMPONENTS) $(EXTRA_PP_COMPONENTS))) > ifndef NO_JS_MANIFEST >-$(error .js component without matching .manifest. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0) >+$(error .js component without matching .manifest. See https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0 $(CURDIR)) I'm still not sure what this change does - can you explain why it's part of moving EXTRA_COMPONENTS? >diff --git a/dom/system/Makefile.in b/dom/system/Makefile.in >--- a/dom/system/Makefile.in >+++ b/dom/system/Makefile.in >@@ -15,36 +15,16 @@ > > DEFINES += -DDLL_PREFIX=\"$(DLL_PREFIX)\" -DDLL_SUFFIX=\"$(DLL_SUFFIX)\" > > # We fire the nsDOMDeviceAcceleration > LOCAL_INCLUDES += \ > -I$(topsrcdir)/content/events/src \ > -I$(topsrcdir)/dom/base \ > -I$(topsrcdir)/dom/bindings \ >- $(NULL) >- >-# On Systems that have build in geolocation providers, >-# we really do not need these. >-ifneq (Android,$(OS_TARGET)) >-EXTRA_COMPONENTS = \ >- NetworkGeolocationProvider.js \ >- NetworkGeolocationProvider.manifest \ >- $(NULL) >-endif >- >-ifeq (gonk,$(MOZ_WIDGET_TOOLKIT)) >-EXTRA_COMPONENTS = \ >- NetworkGeolocationProvider.js \ >- NetworkGeolocationProvider.manifest \ >- $(NULL) >-endif >- >-LOCAL_INCLUDES += \ >- -I$(topsrcdir)/content/events/src \ > -I$(topsrcdir)/js/xpconnect/loader \ > $(NULL) nit: I'd prefer if the LOCAL_INCLUDES change was in a separate patch for easier reviewing & more accurate 'blame', but not worth un-doing now I think. >diff --git a/layout/tools/reftest/moz.build b/layout/tools/reftest/moz.build >--- a/layout/tools/reftest/moz.build >+++ b/layout/tools/reftest/moz.build >@@ -1,8 +1,16 @@ > # -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- > # 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/. > > MODULE = 'reftest' > >+EXTRA_COMPONENTS += [ >+ 'reftest-cmdline.js', >+] >+ >+if not CONFIG['XPI_NAME']: >+ EXTRA_COMPONENTS += [ >+ 'reftest-cmdline.manifest', >+ ] I think my earlier question was missed - this file and netwerk/test/httpserver/moz.build both use CONFIG['XPI_NAME'], but XPI_NAME is not a config variable, so I don't think this logic matches the original Makefile. It looks like XPI_NAME is normally passed in as a command-line argument to make (eg: toolkit/locales/Makefile.in). I tried a quick test and was unable to get CONFIG['XPI_NAME'] to have any value when doing 'make XPI_NAME=foo'. If I'm misunderstanding how this is supposed to work please let me know - withholding r+ until this is resolved. gps: Does moz.build have a way of testing command-line variables like this?

Comment on attachment 780275 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #4) This part looks ready to go!

For the sake of getting this batch landed, let's drop the XPI_NAME changes (and the change to rules.mk) and deal with those elsewhere.

Comment on attachment 780849 [details] [diff] [review] move EXTRA_COMPONENTS to mozbuild (file batch #3, rebased, no DISABLED, no XPI_NAME) Looks good!

https://hg.mozilla.org/mozilla-central/rev/f2528126addb https://hg.mozilla.org/mozilla-central/rev/225a4dbdeb1f

fyi> There are two other references to the var that can be submitted with the block patch: ./netwerk/test/httpserver/Makefile.in:EXTRA_COMPONENTS += \ ./layout/tools/reftest/Makefile.in:EXTRA_COMPONENTS += reftest-cmdline.manifest

Those are dependent on XPI_NAME. XPI_NAME is a variable set like $(MAKE) libs XPI_NAME=... and is thus not available in moz.build files.

Is there any work left to do on this conversion bug ? There are two var references remaining in Makefile.in % find mozilla-central/ -name Makefile.in |xargs grep EXTRA_COMPO mozilla-central/layout/tools/reftest/Makefile.in:EXTRA_COMPONENTS += reftest-cmdline.manifest mozilla-central/netwerk/test/httpserver/Makefile.in:EXTRA_COMPONENTS += \

(In reply to Joey Armstrong [:joey] from comment #39) > Is there any work left to do on this conversion bug ? > There are two var references remaining in Makefile.in > > % find mozilla-central/ -name Makefile.in |xargs grep EXTRA_COMPO > mozilla-central/layout/tools/reftest/Makefile.in:EXTRA_COMPONENTS += > reftest-cmdline.manifest > mozilla-central/netwerk/test/httpserver/Makefile.in:EXTRA_COMPONENTS += \ Filed bug 914247 for those.