Closed Bug 881344 Opened 11 years ago Closed 10 years ago

move SHARED_LIBRARY_LIBS to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joey, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [leave open])

Attachments

(6 files, 9 obsolete files)

5.52 KB, patch
Details | Diff | Splinter Review
7.61 KB, patch
gps
: review+
Details | Diff | Splinter Review
6.90 KB, patch
Details | Diff | Splinter Review
21.02 KB, patch
Details | Diff | Splinter Review
6.97 KB, patch
mshal
: review+
Details | Diff | Splinter Review
4.85 KB, patch
Details | Diff | Splinter Review
      No description provided.
Blocks: 881345
No longer blocks: 881345
No longer depends on: 881341
Assignee: nobody → joey
Comment on attachment 762682 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (logic).

Add SHARED_LIBRARY_LIBS as a pass-through variable.
Attachment #762682 - Flags: review?(gps)
Comment on attachment 762682 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (logic).

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

This looks good. I'm interested in how you'll handle $(call EXPAND_LIBNAME_PATH) as part of the conversion!
Attachment #762682 - Flags: review?(gps) → review+
Attachment #762682 - Attachment is obsolete: true
Assignee: joey → jarmstrong
Comment on attachment 764753 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (logic).

rebase, r=gps carried forward
Comment on attachment 764753 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (logic).

Push to inbound
committed changeset 135624:9d7ab37cf1d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7ab37cf1d3
Comment on attachment 765989 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)

Checked locally, try job pending.
Move SHARED_LIBRARY_LIBS to mozbuild
Attachment #765989 - Flags: review?(mshal)
Comment on attachment 765990 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #2).

Move SHARED_LIBRARY_LIBS to mozbuild, second batch of files.
Attachment #765990 - Flags: review?(mshal)
Comment on attachment 765989 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)

>+lib_fields = (CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
>+depth_fields = ('$(DEPTH)', CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
>+SHARED_LIBRARY_LIBS += [
>+    "%s/uriloader/base/%suriloaderbase_s.%s" % (depth_fields),
>+    "%s/uriloader/exthandler/%sexthandler_s.%s" % (depth_fields),
>+    "%s/uriloader/prefetch/%sprefetch_s.%s" % (depth_fields),
>+    "../base/%sbasedocshell_s.%s" % (lib_fields),
>+    "../shistory/src/%sshistory_s.%s" % (lib_fields),
>+]

Do we have a moz.build mechanism to get $(DEPTH) rather than relying on make to expand it? I see a get_depth function in configenvironment.py, but I'm not sure if we can use it from moz.build.
Attachment #765989 - Flags: review?(mshal) → review+
Attachment #765572 - Attachment is obsolete: true
Comment on attachment 766070 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #3)

move SHARED_LIBRARY_FILES to mozbuild (large file batch)
Attachment #766070 - Flags: review?(mshal)
Comment on attachment 766071 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #4).

move SHARED_LIBRARY_LIBS to mozbuild.
minor eyeball hemorrhaging with this conversion so moved to a separate patch.
Attachment #766071 - Flags: review?(mshal)
(In reply to Michael Shal [:mshal] from comment #13)
> Comment on attachment 765989 [details] [diff] [review]
> move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)
> 
> >+lib_fields = (CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
> >+depth_fields = ('$(DEPTH)', CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
> >+SHARED_LIBRARY_LIBS += [
> >+    "%s/uriloader/base/%suriloaderbase_s.%s" % (depth_fields),
> >+    "%s/uriloader/exthandler/%sexthandler_s.%s" % (depth_fields),
> >+    "%s/uriloader/prefetch/%sprefetch_s.%s" % (depth_fields),
> >+    "../base/%sbasedocshell_s.%s" % (lib_fields),
> >+    "../shistory/src/%sshistory_s.%s" % (lib_fields),
> >+]
> 
> Do we have a moz.build mechanism to get $(DEPTH) rather than relying on make
> to expand it? I see a get_depth function in configenvironment.py, but I'm
> not sure if we can use it from moz.build.

I did not find a usable var/value earlier when I went looking.  Yes an access method will be needed for $DEPTH {and a handful of other values} to avoid the direct variable reference.  To be added as phase2/cleanup bug(s) for this conversion.
I think we should just define these values as we did with DIRS, where an absolute path is actually relative to the root of the objdir, so you could just say
SHARED_LIBRARY_LIBS += [
  '/uriloader/base/whatever'
]

Also, it feels like we should really figure out something better than all these substs for LIB_PREFIX/LIB_SUFFIX. That actually looks *uglier* than the original Makefile.
(In reply to Ted Mielczarek [:ted.mielczarek] (away June 25th-30th) from comment #19)
> I think we should just define these values as we did with DIRS, where an
> absolute path is actually relative to the root of the objdir, so you could
> just say
> SHARED_LIBRARY_LIBS += [
>   '/uriloader/base/whatever'
> ]

Depth can be hardcoded easily enough I'll add that in this patch.
 
> Also, it feels like we should really figure out something better than all
> these substs for LIB_PREFIX/LIB_SUFFIX. That actually looks *uglier* than
> the original Makefile.

These are only passthrough conversions to quicken removal of Makefile.in and some may not be elegant.  A followup bug can be opened to address synatx when the passthrough variable is deprecated by mozbuild support for the variable.
(In reply to Ted Mielczarek [:ted.mielczarek] (away June 25th-30th) from comment #19)
> Also, it feels like we should really figure out something better than all
> these substs for LIB_PREFIX/LIB_SUFFIX. That actually looks *uglier* than
> the original Makefile.

We can almost do this by just removing the prefix/suffix from the definition and expanding it in moz.build.

Some of the SHARED_LIBRARY_LIBS are defined in configure though, so that would need to be updated as well.

There are also some odd-ball cases, like NSS_STATIC_LIBS, or js/src with libffi:

ifeq ($(OS_ARCH),OS2)
SHARED_LIBRARY_LIBS += \
    ctypes/libffi/.libs/ffi.a \
    $(NULL)
else
SHARED_LIBRARY_LIBS += \
    ctypes/libffi/.libs/libffi.$(LIB_SUFFIX) \
    $(NULL)
endif
endif

But I agree we should clean it up in a followup.
Comment on attachment 765990 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #2).

># HG changeset patch
># User Joey Armstrong <joey@mozilla.com>
># Date 1371837668 14400
># Node ID de19a1c322793a9b7bc98abb69f9dabd7f7378ba
># Parent  e479de9f15c836254c309c183d5669872c5d1c61
>bug 881344: move SHARED_LIBRARY_LIBS to mozbuild (file batch #2).
>
>diff --git a/embedding/components/build/moz.build b/embedding/components/build/moz.build
>--- a/embedding/components/build/moz.build
>+++ b/embedding/components/build/moz.build
>@@ -9,19 +9,19 @@ MODULE = 'embedcomponents'
> CPP_SOURCES += [
>     'nsEmbeddingModule.cpp',
> ]
> 
> LIBRARY_NAME = 'embedcomponents'
> 
> lib_fields = (CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
> SHARED_LIBRARY_LIBS += [
>-    "../windowwatcher/src/%swindowwatcher_s.%s" % (lib_fields),
>     "../appstartup/src/%sappstartupnotifier_s.%s" % (lib_fields),
>+    "../commandhandler/src/%scommandhandler_s.%s" % (lib_fields),
>     "../find/src/%sfind_s.%s" % (lib_fields),
>     "../webbrowserpersist/src/%swebbrowserpersist_s.%s" % (lib_fields),
>-    "../commandhandler/src/%scommandhandler_s.%s" % (lib_fields),
>+    "../windowwatcher/src/%swindowwatcher_s.%s" % (lib_fields),
> ]

Are we sure that SHARED_LIBRARY_LIBS should be a StrictOrderingOnAppendList rather than just a list? Library link order actually does matter (as opposed to say, listing EXPORTS). I don't know if m-c relies on the library link order for anything, but it is definitely something to watch out for. I guess if try is happy we can continue sorting this?
Attachment #765990 - Flags: review?(mshal) → review+
Comment on attachment 766070 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #3)

>diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in
>--- a/toolkit/xre/Makefile.in
>+++ b/toolkit/xre/Makefile.in
>@@ -51,82 +51,82 @@ CMMSRCS += MacApplicationDelegate.mm
> CMMSRCS += MacAutoreleasePool.mm
> ENABLE_CXX_EXCEPTIONS = 1
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),android)
> DEFINES += -DANDROID_PACKAGE_NAME='"$(ANDROID_PACKAGE_NAME)"'
> endif
> 
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   ../profile/$(LIB_PREFIX)profile_s.$(LIB_SUFFIX) \
>   $(NULL)
> 
> ifdef MOZ_UPDATER
> ifneq (android,$(MOZ_WIDGET_TOOLKIT))
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   ../mozapps/update/common/$(LIB_PREFIX)updatecommon.$(LIB_SUFFIX) \
>   $(NULL)
> endif
> endif
> 
> ifdef MOZ_ENABLE_XREMOTE
>-SHARED_LIBRARY_LIBS += $(DEPTH)/widget/xremoteclient/$(LIB_PREFIX)xremote_client_s.$(LIB_SUFFIX)
>+DISABLED_SHARED_LIBRARY_LIBS += $(DEPTH)/widget/xremoteclient/$(LIB_PREFIX)xremote_client_s.$(LIB_SUFFIX)
> LOCAL_INCLUDES += -I$(topsrcdir)/widget/xremoteclient
> endif
> 
> ifneq (,$(MOZ_CRASHREPORTER)$(MOZ_ENABLE_PROFILER_SPS))
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/$(LIB_PREFIX)breakpad_common_s.$(LIB_SUFFIX) \
>   $(NULL)
> 
> ifeq ($(OS_ARCH),Darwin)
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/mac/$(LIB_PREFIX)breakpad_mac_common_s.$(LIB_SUFFIX)
>   $(NULL)
> endif
> ifeq ($(OS_ARCH),Linux)
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/linux/$(LIB_PREFIX)breakpad_linux_common_s.$(LIB_SUFFIX) \
>   $(NULL)
> endif
> endif
> 
> ifdef MOZ_ENABLE_PROFILER_SPS
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/processor/$(LIB_PREFIX)breakpad_sps_common_s.$(LIB_SUFFIX) \
>   $(NULL)
> endif
> 
> ifdef MOZ_CRASHREPORTER
>-SHARED_LIBRARY_LIBS += $(DEPTH)/toolkit/crashreporter/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX)
>+DISABLED_SHARED_LIBRARY_LIBS += $(DEPTH)/toolkit/crashreporter/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX)
> ifeq ($(OS_ARCH),WINNT)
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/breakpad-windows-libxul/$(LIB_PREFIX)google_breakpad_libxul_s.$(LIB_SUFFIX)
> endif
> 
> ifeq ($(OS_ARCH),Darwin)
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/$(LIB_PREFIX)minidump_file_writer_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/mac/crash_generation/$(LIB_PREFIX)crash_generation_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/mac/handler/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX) \
>   $(NULL)
> endif
> 
> ifeq ($(OS_ARCH),Linux)
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/linux/crash_generation/$(LIB_PREFIX)crash_generation_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/linux/handler/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/linux/minidump_writer/$(LIB_PREFIX)minidump_writer_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/$(LIB_PREFIX)minidump_file_writer_s.$(LIB_SUFFIX) \
>   $(NULL)
> endif
> 
> ifeq ($(OS_ARCH),SunOS)
>-SHARED_LIBRARY_LIBS += \
>+DISABLED_SHARED_LIBRARY_LIBS += \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/solaris/handler/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/$(LIB_PREFIX)minidump_file_writer_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/$(LIB_PREFIX)breakpad_common_s.$(LIB_SUFFIX) \
>   $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/solaris/$(LIB_PREFIX)breakpad_solaris_common_s.$(LIB_SUFFIX) \
>   $(NULL)
> endif
> endif

The corresponding toolkit/xre/moz.build seem to be missing? All other changes in this patch look good.
Attachment #766070 - Flags: review?(mshal) → review-
Patch refresh - toolkit/xre/{Makefile.in,moz.build} was moved into patch #4 for easier review but only one file made the trip over.
Attachment #766070 - Attachment is obsolete: true
Attachment #767240 - Flags: review?(mshal)
Attachment #767240 - Attachment description: moved SHARED_LIBRARY_LIBS to mozbuild → moved SHARED_LIBRARY_LIBS to mozbuild (file batch #3)
Patch refresh - toolkit/xre/{Makefile.in,moz.build} was moved into patch #4 for easier review but only one file made the trip over.
Attachment #766071 - Attachment is obsolete: true
Attachment #766071 - Flags: review?(mshal)
Attachment #767241 - Flags: review?(mshal)
Comment on attachment 765990 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #2).

Question from comment #22 - remove sorting to preserve order for symbol resolution or will mozbuild have enough hints available to figure out the order.
Attachment #765990 - Flags: feedback?(gps)
Comment on attachment 767240 [details] [diff] [review]
moved SHARED_LIBRARY_LIBS to mozbuild (file batch #3)

This one looks good now!
Attachment #767240 - Flags: review?(mshal) → review+
Comment on attachment 767241 [details] [diff] [review]
moved SHARED_LIBRARY_LIBS to mozbuild (file batch #4)

>diff --git a/toolkit/xre/moz.build b/toolkit/xre/moz.build
>--- a/toolkit/xre/moz.build
>+++ b/toolkit/xre/moz.build
>@@ -82,8 +82,77 @@ if CONFIG['MOZ_INSTRUMENT_EVENT_LOOP']:
>         'EventTracer.cpp',
>     ]
> 
> if CONFIG['MOZ_UPDATER']:
>     if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':
>         CPP_SOURCES += [
>             'nsUpdateDriver.cpp',
>         ]
>+
>+lib_fields = (CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
>+depth_fields = ('$(DEPTH)', CONFIG['LIB_PREFIX'], CONFIG['LIB_SUFFIX'])
>+SHARED_LIBRARY_LIBS += [
>+    '../profile/%sprofile_s.%s' % (lib_fields),
>+]
>+
>+if CONFIG['MOZ_CRASHREPORTER']:
>+    SHARED_LIBRARY_LIBS += [
>+        '%s/toolkit/crashreporter/%sexception_handler_s.%s' % (depth_fields),
>+    ]

Can we move this MOZ_CRASHREPORTER block in with the same one later (line 127)? The Makefile had them in the same if-block - not sure why they were separated.

>+if not CONFIG['MOZ_CRASHREPORTER'] and not ['MOZ_ENABLE_PROFILER_SPS']: 

This does not seem to match the Makefile.in logic:

ifneq (,$(MOZ_CRASHREPORTER)$(MOZ_ENABLE_PROFILER_SPS))

I believe it should be:

if CONFIG['MOZ_CRASHREPORTER'] or ['MOZ_ENABLE_PROFILER_SPS']:

Once this is fixed I can r+.

Also a nit: there is extra whitespace at the end of the above if-statement.
Attachment #767241 - Flags: review?(mshal) → review-
Comment on attachment 765989 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)

https://tbpl.mozilla.org/?tree=Try&rev=774aa0b278b9

build/preferences.js:140: ReferenceError: PROFILE_FOLDER is not defined
  Profile directory not found on fedora, fedora64 & osx 10.6

b2g emulator logged a timeout in the xpcshell test that did not fail on the last job.
(In reply to Michael Shal [:mshal] from comment #28)
> Comment on attachment 767241 [details] [diff] [review]
> moved SHARED_LIBRARY_LIBS to mozbuild (file batch #4)
> 
> >diff --git a/toolkit/xre/moz.build b/toolkit/xre/moz.build
> >--- a/toolkit/xre/moz.build
> >+++ b/toolkit/xre/moz.build
> >@@ -82,8 +82,77 @@ if CONFIG['MOZ_INSTRUMENT_EVENT_LOOP']:
> 
> Can we move this MOZ_CRASHREPORTER block in with the same one later (line
> 127)? The Makefile had them in the same if-block - not sure why they were
> separated.

This will need to be manually combined into the conditional.  The converson does not attempt to merge assignments into existing conditional blocks it is a simple append.  Most are fixed after the fact but some slip through.
Attachment #767241 - Attachment is obsolete: true
Assignee: jarmstrong → joey
Comment on attachment 770392 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #4).

Removed trailing space
expand $(DEPTH)=='../..' from patch #3 comemnts
combine CRASHREPORTER blocks and negated the other conditional
Attachment #770392 - Flags: review?(mshal)
Comment on attachment 765989 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)

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

Putting my foot down on more DISABLED_ prefixing.
Attachment #765989 - Flags: review+ → review-
(In reply to Gregory Szorc [:gps] from comment #33)
> Comment on attachment 765989 [details] [diff] [review]
> move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)
> 
> Review of attachment 765989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Putting my foot down on more DISABLED_ prefixing.

Do you have time to talk about this today ?  Rejecting patches will just bottleneck the convesrions.
Joey, the request to stop using _DISABLED was made several times, both by ted and others while gps was on vacation and by gps when he returned. Writing the patches correctly the first time would be a better way to un-bottleneck this.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> Joey, the request to stop using _DISABLED was made several times, both by
> ted and others while gps was on vacation and by gps when he returned.
> Writing the patches correctly the first time would be a better way to
> un-bottleneck this.

If variables are removed outright {allowing Makefile.in to be removed early when 'empty'}.  Followed by conversion patch later being backed out.  We may be left in a state where targets cannot build because rules no longer exist in either mozbuild or Makefile.in.

I see these edits as a system for checks&balances.  Makefile.in should not be removed until after we are sure a conversion patche has landed and will not be backed out.  Removing them prematurely would just give a false sense that we are finished.

If the token were renamed from 'DISABLED_' to 'MOVED_TO_MOZBUILD_' would that help with any confusion induced or is token prefix use just flatly being frowned on.
Attachment #765989 - Attachment is obsolete: true
Comment on attachment 770850 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)

Same patch as before w/rebase.  Just run cleanup script early to nuke DISABLED_ tokens.
Attachment #770850 - Flags: review?(gps)
r=mshal
rebase
run cleanup script to purge DISABLED_ tokens early.

Try results: https://tbpl.mozilla.org/?tree=Try&rev=774aa0b278b9
  Fedora(64)?, osx 10.6 - B2g failure
  B2G Arm (vm) - xpcshell failure
Attachment #765990 - Attachment is obsolete: true
Attachment #765990 - Flags: feedback?(gps)
refresh patch.
purge DISABLED_ tokens
r=mshal carried forward.
Attachment #767240 - Attachment is obsolete: true
Same patch as before.
1) nit fixes from the last code review.
2) cleanup script run to strip DISABLED_ tokens from Makefile.in
Attachment #770392 - Attachment is obsolete: true
Attachment #770392 - Flags: review?(mshal)
Attachment #770862 - Flags: review?(mshal)
Comment on attachment 770862 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #4)

This looks good for now. Do we have a follow-up bug on file yet for cleaning up lib_prefix/suffix and allowing root-relative paths? I think we'll want to tackle that soon after all SHARED_LIBRARY_LIBS on are moz.build
Attachment #770862 - Flags: review?(mshal) → review+
(In reply to Joey Armstrong [:joey] from comment #36)
> If variables are removed outright {allowing Makefile.in to be removed early
> when 'empty'}.  Followed by conversion patch later being backed out.  We may
> be left in a state where targets cannot build because rules no longer exist
> in either mozbuild or Makefile.in.

Huh? If the patch is backed out, a moz.build will change, forcing a build config rescan and thus recreating Makefile in the objdir from the restored Makefile.in. I don't see how what you describe can occur (assuming the moz.build generation dependencies are all sane).

> If the token were renamed from 'DISABLED_' to 'MOVED_TO_MOZBUILD_' would
> that help with any confusion induced or is token prefix use just flatly
> being frowned on.

Making a copy of code and essentially creating dead code in Makefile.in is what's mostly frowned upon (for various reasons).
One thing to note is that file removals are marked in hg patches as simple removals, not depending on the file content. Which means, if you have a bitrotted patch that removes a Makefile.in and that Makefile.in changed in the meanwhile, applying the patch will happily remove the new Makefile.in. Which I guess is what Joey wants to avoid. But you don't need to use DISABLED_ or whatever for that. Just don't delete emptyish Makefile.ins just yet.
Try jobs:

http://tbpl.mozilla.org/?tree=Try&rev=774aa0b278b9
  Patches 1 & 2 failing on fedora[Bg], osx10.6[Bg] & B2G Arm(VM) xpcshell.

Patch 3(full): bloodbath
  https://tbpl.mozilla.org/?tree=Try&rev=047de40dd251

Patch 3(subset of files) failing on osx 10.7 build, win7 debug (JP), B2G Arm (blood bath)
    http://tbpl.mozilla.org/?tree=Try&rev=5d43fe08b4c6

Patch #4: bloodbath
https://tbpl.mozilla.org/?tree=Try&rev=156d253573e8
Comment on attachment 770850 [details] [diff] [review]
move SHARED_LIBRARY_LIBS to mozbuild (file batch #1)

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

I wish we didn't have that $(DEPTH) in there, but we can address that in a follow-up.
Attachment #770850 - Flags: review?(gps) → review+
Lingering var references

% find . -name Makefile.in | xargs grep SHARED_LIBRARY_LIBS

./gfx/thebes/Makefile.in:SHARED_LIBRARY_LIBS += \
./js/src/Makefile.in:SHARED_LIBRARY_LIBS += \
./js/src/Makefile.in:SHARED_LIBRARY_LIBS += \
./js/src/Makefile.in:SHARED_LIBRARY_LIBS += $(MOZ_ICU_LIBS)
./js/xpconnect/src/Makefile.in:SHARED_LIBRARY_LIBS = \
./widget/android/Makefile.in:SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
./widget/gonk/Makefile.in:SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
./widget/gtk2/Makefile.in:SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
./widget/xpwidgets/Makefile.in:SHARED_LIBRARY_LIBS = ../shared/$(LIB_PREFIX)widget_shared.$(LIB_SUFFIX)
./widget/xpwidgets/Makefile.in:SHARED_LIBRARY_LIBS += ../shared/x11/$(LIB_PREFIX)widget_shared_x11.$(LIB_SUFFIX)
./widget/windows/Makefile.in:SHARED_LIBRARY_LIBS = \
./widget/os2/Makefile.in:SHARED_LIBRARY_LIBS = \
./widget/qt/Makefile.in:SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
./widget/cocoa/Makefile.in:SHARED_LIBRARY_LIBS = ../xpwidgets/libxpwidgets_s.a
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS = \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS += $(MOZ_CAIRO_LIBS)
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS += $(MOZ_PIXMAN_LIBS)
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS += $(MOZ_SKIA_LIBS)
./layout/media/Makefile.in:SHARED_LIBRARY_LIBS += $(WEBRTC_LIBS)
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += $(DEPTH)/dom/bluetooth/$(LIB_PREFIX)dombluetooth_s.$(LIB_SUFFIX)
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS	+= $(DEPTH)/dom/camera/$(LIB_PREFIX)domcamera_s.$(LIB_SUFFIX)
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS     += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS 	+= \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += ../inspector/src/$(LIB_PREFIX)inspector_s.$(LIB_SUFFIX)
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./layout/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./storage/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./mozglue/build/Makefile.in:SHARED_LIBRARY_LIBS = $(call EXPAND_LIBNAME_PATH,memory,$(DEPTH)/memory/build)
./mozglue/build/Makefile.in:SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,android,$(DEPTH)/other-licenses/android)
./mozglue/build/Makefile.in:SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,android,../android)
./mozglue/build/Makefile.in:SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,linker,../linker)
./mozglue/build/Makefile.in:SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,mfbt,$(DEPTH)/mfbt)
./extensions/spellcheck/src/Makefile.in:SHARED_LIBRARY_LIBS += ../hunspell/src/$(LIB_PREFIX)hunspell_s.$(LIB_SUFFIX)
./extensions/universalchardet/src/xpcom/Makefile.in:SHARED_LIBRARY_LIBS = \
./xpcom/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./xpcom/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./memory/build/Makefile.in:SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,jemalloc,$(DEPTH)/memory/jemalloc)
./memory/build/Makefile.in:SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,jemalloc,$(DEPTH)/memory/mozjemalloc)
./memory/replace/jemalloc/Makefile.in:SHARED_LIBRARY_LIBS = $(call EXPAND_LIBNAME_PATH,jemalloc,$(DEPTH)/memory/jemalloc)
./toolkit/library/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/library/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/library/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/library/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/library/Makefile.in:SHARED_LIBRARY_LIBS += $(DEPTH)/accessible/src/xul/$(LIB_PREFIX)accessibility_xul_s.$(LIB_SUFFIX)
./toolkit/library/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/crashreporter/injector/Makefile.in:SHARED_LIBRARY_LIBS += ../breakpad-windows-standalone/$(LIB_PREFIX)google_breakpad_standalone_s.$(LIB_SUFFIX)
./toolkit/components/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./toolkit/components/build/Makefile.in:SHARED_LIBRARY_LIBS += ../parentalcontrols/$(LIB_PREFIX)parentalcontrols_s.$(LIB_SUFFIX)
./toolkit/components/build/Makefile.in:SHARED_LIBRARY_LIBS += ../url-classifier/$(LIB_PREFIX)urlclassifier_s.$(LIB_SUFFIX)
./toolkit/components/build/Makefile.in:SHARED_LIBRARY_LIBS += ../feeds/$(LIB_PREFIX)feed_s.$(LIB_SUFFIX)
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += $(DEPTH)/widget/xremoteclient/$(LIB_PREFIX)xremote_client_s.$(LIB_SUFFIX)
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += $(DEPTH)/toolkit/crashreporter/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX)
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./toolkit/xre/Makefile.in:SHARED_LIBRARY_LIBS += \
./rdf/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./image/decoders/icon/Makefile.in:SHARED_LIBRARY_LIBS = $(PLATFORM)/$(LIB_PREFIX)imgicon$(PLATFORM)_s.$(LIB_SUFFIX)
./image/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./intl/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./intl/uconv/src/Makefile.in:SHARED_LIBRARY_LIBS += \
./embedding/components/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./embedding/components/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./embedding/browser/build/Makefile.in:SHARED_LIBRARY_LIBS= \
./parser/htmlparser/src/Makefile.in:SHARED_LIBRARY_LIBS = \
./security/build/Makefile.in:SHARED_LIBRARY_LIBS = $(addprefix ../,$(NSS_STATIC_LIBS))
./security/build/Makefile.in:$(SHARED_LIBRARY): $(SHARED_LIBRARY_LIBS)
./xpfe/components/build/Makefile.in:SHARED_LIBRARY_LIBS += ../directory/$(LIB_PREFIX)directory_s.$(LIB_SUFFIX)
./docshell/build/Makefile.in:SHARED_LIBRARY_LIBS= \
./browser/components/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./browser/components/build/Makefile.in:SHARED_LIBRARY_LIBS += ../shell/src/$(LIB_PREFIX)shellservice_s.$(LIB_SUFFIX)
./browser/components/build/Makefile.in:SHARED_LIBRARY_LIBS += ../migration/src/$(LIB_PREFIX)migration_s.$(LIB_SUFFIX)
./netwerk/build/Makefile.in:SHARED_LIBRARY_LIBS = \
./netwerk/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:    SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:    SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:    SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:    SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:    SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:SHARED_LIBRARY_LIBS += \
./netwerk/build/Makefile.in:SHARED_LIBRARY_LIBS += \
(In reply to Joey Armstrong [:joey] from comment #48)
> Created attachment 806259 [details] [diff] [review]
> move SHARED_LIBRARY_LIBS to mozbuild

 https://tbpl.mozilla.org/?tree=Try&rev=5efcc23ff5c0
Blocks: 922231
Assignee: joey → nobody
Fixed by one the linking patches. Don't know which one.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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: