Closed Bug 870370 Opened 11 years ago Closed 11 years ago

Move EXTRA_COMPONENTS to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: joey, Assigned: Ms2ger)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 6 obsolete files)

5.44 KB, patch
ted
: review+
Details | Diff | Splinter Review
11.09 KB, patch
mshal
: review+
Details | Diff | Splinter Review
34.90 KB, patch
mshal
: review+
Details | Diff | Splinter Review
17.72 KB, patch
Details | Diff | Splinter Review
8.96 KB, patch
mshal
: review+
Details | Diff | Splinter Review
1.26 KB, patch
Details | Diff | Splinter Review
29.20 KB, patch
mshal
: review+
Details | Diff | Splinter Review
      No description provided.
No longer depends on: 870366
Blocks: 870406
No longer blocks: 870406
Assignee: nobody → joey
Comment on attachment 758736 [details] [diff] [review]
move EXTRA_COMPONENTS to moz.build (logic).

Add EXTRA_COMPONENTS as a passthrough variable.
Attachment #758736 - Flags: review?(ted)
Attachment #758736 - Flags: review?(ted) → review+
Whiteboard: [leave open]
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
Blocks: 880245
Blocks: 880246
No longer blocks: 880246
Blocks: 880260
Attachment #759309 - Flags: review?(mshal)
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.
Attachment #759309 - Flags: review?(mshal)
Comment on attachment 759309 [details] [diff] [review]
move EXTRA_COMPONENTS to mozbuild (file batch #1)

Only 1 this time? :)
Attachment #759309 - Flags: review+
Attachment #759309 - Attachment is obsolete: true
Attachment #759332 - Flags: review?(mshal)
Comment on attachment 759332 [details] [diff] [review]
move EXTRA_COMPONENTS to mozbuild (file batch #1)

Looks good to me!
Attachment #759332 - Flags: review?(mshal) → review+
Attachment #759893 - Attachment is obsolete: true
Attachment #759897 - Flags: review?(mshal)
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!
Attachment #759897 - Flags: review?(mshal) → review+
(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.
No longer blocks: 880245
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
Blocks: 882963
Assignee: joey → jarmstrong
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.
Attachment #768470 - Flags: review?(mshal)
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 :)
Attachment #768470 - Flags: review?(mshal) → review+
Remove patch files that have been deleted.
Rebase
Attachment #768470 - Attachment is obsolete: true
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.
Attachment #770914 - Flags: feedback?(gps)
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.
Attachment #770914 - Flags: feedback?(gps) → feedback-
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_*
Attachment #770914 - Flags: review?(mshal) → review-
Attachment #770914 - Attachment is obsolete: true
Attachment #780274 - Flags: review?(mshal)
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?
Attachment #780274 - Flags: feedback?(gps)
Comment on attachment 780275 [details] [diff] [review]
move EXTRA_COMPONENTS to mozbuild (file batch #4)

This part looks ready to go!
Attachment #780275 - Flags: review?(mshal) → review+
Attachment #780336 - Flags: review?(gps)
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.
Attachment #780274 - Attachment is obsolete: true
Attachment #780274 - Flags: review?(mshal)
Attachment #780274 - Flags: feedback?(gps)
Attachment #780849 - Flags: review?(mshal)
Comment on attachment 780849 [details] [diff] [review]
move EXTRA_COMPONENTS to mozbuild (file batch #3, rebased, no DISABLED, no XPI_NAME)

Looks good!
Attachment #780849 - Flags: review?(mshal) → review+
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.
Assignee: jarmstrong → Ms2ger
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 += \
Blocks: 914247
(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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla25
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: