Adds LIBRARY_NAME as a passthrough variable

Comment on attachment 754151 [details] [diff] [review] Part 1: Support Library_Name in moz.build files Greg on vacation for two weeks so redirecting to b::c

Comment on attachment 754151 [details] [diff] [review] Part 1: Support Library_Name in moz.build files Review of attachment 754151 [details] [diff] [review]: ----------------------------------------------------------------- Just some documentation nits. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +95,5 @@ > + 'LIBRARY_NAME': (unicode, unicode, "", > + """The name of the library generated for a directory. > + > + Example: > + In dist/bin/components/moz.build, This is a little weird for an example, since you're giving what looks like an objdir path. I'd just make a contrived path, like: example/components/moz.build @@ +96,5 @@ > + """The name of the library generated for a directory. > + > + Example: > + In dist/bin/components/moz.build, > + LIBRARY_NAME = 'libxpcomsample' This is not correct, you specify LIBRARY_NAME without the 'lib', since we don't use that on Windows.

Addresses Ted's review nits. Thanks Ted for reviewing!

This is the set of Makefile.ins that could be converted by the tool, but don't go manually including config.mk. Try run at https://tbpl.mozilla.org/?tree=Try&rev=8899e434dcb6, but that ended up on top of a cset with lots of orange, so I re-pushed later: https://tbpl.mozilla.org/?tree=Try&rev=8036e27a31e9

This is the set of Makefile.ins that needed manual conversion, due to conditionals, but don't go manually including config.mk. Try run at https://tbpl.mozilla.org/?tree=Try&rev=733941aaa156, but that ended up on top of a cset with lots of orange, so I re-pushed later: https://tbpl.mozilla.org/?tree=Try&rev=8036e27a31e9 (along with a fix for failing to quote 'XUL' in toolkit/library/moz.build

Comment on attachment 764099 [details] [diff] [review] Part 32: Move LIBRARY_NAME to moz.build, batch 2 Part 32? :) >diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build >--- a/toolkit/library/moz.build >+++ b/toolkit/library/moz.build >@@ -4,16 +4,21 @@ > # 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/. > > if CONFIG['MOZ_METRO'] and CONFIG['OS_ARCH'] == 'WINNT': > DIRS += ['winvccorlib'] > > MODULE = 'libxul' > >+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': >+ # This is going to be a framework named "XUL", not an ordinary library named >+ # "libxul.dylib" >+ LIBRARY_NAME = 'XUL' >+ In the Makefile.in, the equivalent MOZ_WIDGET_TOOLKIT==cocoa condition happens after the default LIBRARY_NAME=xul statement. In the moz.build file, you've placed the MOZ_WIDGET_TOOLKIT==cocoa condition *before* LIBRARY_NAME = 'xul', so even if we have cocoa, the LIBRARY_NAME will be overwritten to 'xul' later. I suggest moving the default LIBRARY_NAME = 'xul' into an else to make the logic more explicit (rather than relying on overriding variables in the correct order as the Makefile did) - if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': # This is going to be a framework named "XUL", not an ordinary library named # "libxul.dylib" LIBRARY_NAME = 'XUL' else: LIBRARY_NAME = 'xul' Everything else in this patch looks good!

Comment on attachment 764095 [details] [diff] [review] Part 2: Move LIBRARY_NAME to moz.build, batch 1 There appear to be a number (142 by my count) of moz.build files changed that do not have corresponding Makefile.in changes. Here are the first few: 1 diff --git a/accessible/src/windows/ia2/ 1 diff --git a/accessible/src/windows/msaa/ 1 diff --git a/accessible/src/windows/sdn/ 1 diff --git a/accessible/src/windows/uia/ 1 diff --git a/chrome/src/ Eg: chrome/src/Makefile.in still has LIBRARY_NAME defined, even though you've moved it to moz.build. I generated the list with: git show HEAD^ | grep ^diff | sed 's/Makefile.in.*//' | sed 's/moz.build.*//' | sort | uniq -c | grep -v ' 2 ' But feel free to just ping me and I can send it to you. >diff --git a/ipc/ipdl/test/cxx/moz.build b/ipc/ipdl/test/cxx/moz.build >--- a/ipc/ipdl/test/cxx/moz.build >+++ b/ipc/ipdl/test/cxx/moz.build >@@ -17,8 +17,10 @@ EXPORTS.mozilla._ipdltest += [ > > CPP_SOURCES += [ > '$(IPDLTESTSRCS)', > 'IPDLUnitTestProcessChild.cpp', > 'IPDLUnitTestSubprocess.cpp', > 'IPDLUnitTests.cpp', > ] > >+LIBRARY_NAME = '$(MODULE)_s' >+ Can we just do: LIBRARY_NAME = 'ipdlunittest_s' No other Makefile seems to make LIBRARY_NAME dependent on $(MODULE) (even when it just appends a '_s' like this). At the very least, use python rather than make to do the expansion: LIBRARY_NAME = MODULE + '_s' >diff --git a/modules/libjar/moz.build b/modules/libjar/moz.build >--- a/modules/libjar/moz.build >+++ b/modules/libjar/moz.build >@@ -20,8 +20,10 @@ EXPORTS += [ > 'nsZipArchive.h', > 'zipstruct.h', > ] > > CPP_SOURCES += [ > '$(MODULES_LIBJAR_LCPPSRCS)', > ] > >+LIBRARY_NAME = 'jar$(VERSION_NUMBER)' >+ gps: VERSION_NUMBER is defined in config.mk (not autoconf), so we can't use CONFIG['VERSION_NUMBER'] here. Any suggestions on how to do this without relying on delayed make expansion?

gps: Please see VERSION_NUMBER question in https://bugzilla.mozilla.org/show_bug.cgi?id=875934#c10

Just drop the VERSION_NUMBER nonsense, it's not useful. You'll have to fix up the link line in toolkit/library as well: http://mxr.mozilla.org/mozilla-central/source/toolkit/library/Makefile.in#147 "LIBRARY_NAME = 'jar'" is fine.

(In reply to Michael Shal [:mshal] from comment #10) > There appear to be a number (142 by my count) of moz.build files changed > that do not have corresponding Makefile.in changes. Here are the first few: These happen to go with the Makefile.ins that were automatically converted, but include config.mk manually. I separated those out into batch #3, and forgot to revert the moz.build changes that went with them. > >+LIBRARY_NAME = '$(MODULE)_s' > > Can we just do: > > LIBRARY_NAME = 'ipdlunittest_s' > > No other Makefile seems to make LIBRARY_NAME dependent on $(MODULE) (even > when it just appends a '_s' like this). At the very least, use python rather > than make to do the expansion: > > LIBRARY_NAME = MODULE + '_s' Except for libjar below, nothing else uses a variable in LIBRARY_NAME, so I'll switch it to 'ipdlunittest_s'. This happens to be in a file that belongs in the "auto with config.mk" batch, so I'll update it for that part. > >diff --git a/modules/libjar/moz.build b/modules/libjar/moz.build > > > >+LIBRARY_NAME = 'jar$(VERSION_NUMBER)' > >+ > > gps: VERSION_NUMBER is defined in config.mk (not autoconf), so we can't use > CONFIG['VERSION_NUMBER'] here. Any suggestions on how to do this without > relying on delayed make expansion? This happens to be in a file that belongs in the "auto with config.mk" batch, so I took it out of this patch. I'll fix it up once we get to that part.

(In reply to Michael Shal [:mshal] from comment #9) > Comment on attachment 764099 [details] [diff] [review] > Part 32: Move LIBRARY_NAME to moz.build, batch 2 > > Part 32? :) Yeah, Part 3. I must need a new keyboard ;) > >diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build > >--- a/toolkit/library/moz.build > >+++ b/toolkit/library/moz.build > >@@ -4,16 +4,21 @@ > > # 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/. > > > > if CONFIG['MOZ_METRO'] and CONFIG['OS_ARCH'] == 'WINNT': > > DIRS += ['winvccorlib'] > > > > MODULE = 'libxul' > > > >+if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': > >+ # This is going to be a framework named "XUL", not an ordinary library named > >+ # "libxul.dylib" > >+ LIBRARY_NAME = 'XUL' > >+ > > In the Makefile.in, the equivalent MOZ_WIDGET_TOOLKIT==cocoa condition > happens after the default LIBRARY_NAME=xul statement. In the moz.build file, > you've placed the MOZ_WIDGET_TOOLKIT==cocoa condition *before* LIBRARY_NAME > = 'xul', so even if we have cocoa, the LIBRARY_NAME will be overwritten to > 'xul' later. I suggest moving the default LIBRARY_NAME = 'xul' into an else > to make the logic more explicit (rather than relying on overriding variables > in the correct order as the Makefile did) - > > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': > # This is going to be a framework named "XUL", not an ordinary library > named > # "libxul.dylib" > LIBRARY_NAME = 'XUL' > else: > LIBRARY_NAME = 'xul' > I missed that LIBRARY_NAME = 'xul' line. It's now an if/else instead.

What ted said in comment #12.

Comment on attachment 764265 [details] [diff] [review] Part 2: Move LIBRARY_NAME to moz.build, batch 1 Review of attachment 764265 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!

Comment on attachment 764270 [details] [diff] [review] Part 3: Move LIBRARY_NAME to moz.build, batch 2 Review of attachment 764270 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

(In reply to Michael Shal [:mshal] from comment #17) > Looks good! Thanks! Just a note, checkin-needed is for parts 2 and 3 (part 1 already landed in comment #5)

Bitrotted :( Also, can the [leave open] go?

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f61f0a66fd https://hg.mozilla.org/integration/mozilla-inbound/rev/d57032f0b277

I now see there are many LIBRARY_NAME still lingering in the tree. Leaving open...

https://hg.mozilla.org/mozilla-central/rev/d4f61f0a66fd https://hg.mozilla.org/mozilla-central/rev/d57032f0b277

Brian, do you have time to work on this? Otherwise I'll see if I can get rid of some.

(In reply to :Ms2ger from comment #23) > Brian, do you have time to work on this? Otherwise I'll see if I can get rid > of some. I've been away from a solid internet connection for the past few days, but I do have patches in my queue for the rest of these. If they haven't bitrotted, I should be able to get them up Monday afternoon (EST).

Great, thanks!

This patch, plus the parts 5 and 6, have a mostly green try push at https://tbpl.mozilla.org/?tree=Try&rev=783a69de4106. B2G bustage was due to a LIBRARY_NAME that was added to security/sandbox/Makefile.in, which is fixed in this patch. I'll re-push to try later today to confirm that.

As usual, this part will break comm-central until bug 884825 lands

Comment on attachment 796124 [details] [diff] [review] Part 4: Move LIBRARY_NAME to moz.build, batch 3 This part looks straightforward. Please make sure we can get a green try before landing, though.

Comment on attachment 796126 [details] [diff] [review] Part 5: Move LIBRARY_NAME to moz.build, batch 4 > # JavaScript must be built shared, even for static builds, as it is used by > # other modules which are always built shared. Failure to do so results in > # the js code getting copied into xpinstall and jsd as well as mozilla-bin, > # and then the static data cells used for locking no longer work. > # >diff --git a/js/src/moz.build b/js/src/moz.build >--- a/js/src/moz.build >+++ b/js/src/moz.build >@@ -19,16 +19,21 @@ if not CONFIG['JS_DISABLE_SHELL']: > # FIXME: bug 530688 covers getting these working on Android > if CONFIG['OS_ARCH'] != 'ANDROID': > TEST_DIRS += ['jsapi-tests'] > > TEST_DIRS += ['tests', 'gdb'] > > MODULE = 'js' > >+if CONFIG['JS_STANDALONE']: >+ LIBRARY_NAME = 'mozjs-%s.%s%s' % (CONFIG['MOZJS_MAJOR_VERSION'], CONFIG['MOZJS_MINOR_VERSION'], CONFIG['MOZJS_ALPHA']) Are you sure the JS_STANDALONE, MOZJS_MAJOR_VERSION, MOZJS_MINOR_VERSION, and MOZJS_ALPHA variables are accessible via CONFIG? I believe those are all set in the js configure, which I don't think is available in CONFIG. Hopefully I'm wrong :) Everything else in the patch looks good to me. I can r+ once you confirm the above, or if you want to pull js/src/ out of this patch and get the rest of it landed, that would work too.

Comment on attachment 796127 [details] [diff] [review] Part 5: Forbid LIBRARY_NAME in Makefile.ins Looks good!

(In reply to Michael Shal [:mshal] from comment #30) > Are you sure the JS_STANDALONE, MOZJS_MAJOR_VERSION, MOZJS_MINOR_VERSION, > and MOZJS_ALPHA variables are accessible via CONFIG? I believe those are all > set in the js configure, which I don't think is available in CONFIG. > Hopefully I'm wrong :) I wasn't sure, so I checked. I ran configure (locally) with JS_STANDALONE=1, and ended up with "LIBRARY_NAME := mozjs-26.0a", so it looks like that worked just fine. I also pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=31e0f6fd1734 (running now)

The CONFIG for js/src is different from the CONFIG for the main repo.

(In reply to Gregory Szorc [:gps] from comment #33) > The CONFIG for js/src is different from the CONFIG for the main repo. Oh, hooray!

Comment on attachment 796126 [details] [diff] [review] Part 5: Move LIBRARY_NAME to moz.build, batch 4 I believe these are all ready to go! Hopefully try looks good.

Try came back green, finally (other than some intermittent failures), so this should be good to land now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/52e322d09476 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb719728a09 https://hg.mozilla.org/integration/mozilla-inbound/rev/c1360fef1380

https://hg.mozilla.org/mozilla-central/rev/52e322d09476 https://hg.mozilla.org/mozilla-central/rev/4bb719728a09 https://hg.mozilla.org/mozilla-central/rev/c1360fef1380

Followup post backout of bug 870676: https://hg.mozilla.org/mozilla-central/rev/73ce7d8cf054

Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/7ad90353e3ff Move LIBRARY_NAME to moz.build (batch #1); r=mshal