875934 - Move LIBRARY_NAME to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Support Library_Name in moz.build files

Adds LIBRARY_NAME as a passthrough variable

Comment 2

[Joey Armstrong [:joey]]

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 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1: Support Library_Name in moz.build files, for check-in (r=ted)

Addresses Ted's review nits.

Thanks Ted for reviewing!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44875d60a7fb

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44875d60a7fb

Comment 7

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Move LIBRARY_NAME to moz.build, batch 1

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

Comment 8

[Brian O'Keefe [:bokeefe]]

Attachment: Part 32: Move LIBRARY_NAME to moz.build, batch 2

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 9

[Michael Shal [:mshal]]

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 10

[Michael Shal [:mshal]]

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?

Comment 11

[Michael Shal [:mshal]]

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

Comment 12

[(not currently active) Ted Mielczarek]

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.

Comment 13

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2: Move LIBRARY_NAME to moz.build, batch 1

(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.

Comment 14

[Brian O'Keefe [:bokeefe]]

Attachment: Part 3: Move LIBRARY_NAME to moz.build, batch 2

(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.

Comment 15

[Gregory Szorc [:gps]]

What ted said in comment #12.

Comment 16

[Michael Shal [:mshal]]

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 17

[Michael Shal [:mshal]]

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!

Comment 18

[Brian O'Keefe [:bokeefe]]

(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)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Bitrotted :(

Also, can the [leave open] go?

Comment 20

[Gregory Szorc [:gps]]

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

Comment 21

[Gregory Szorc [:gps]]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 24

[Brian O'Keefe [:bokeefe]]

(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).

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Great, thanks!

Comment 26

[Brian O'Keefe [:bokeefe]]

Attachment: Part 4: Move LIBRARY_NAME to moz.build, batch 3

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.

Comment 27

[Brian O'Keefe [:bokeefe]]

Attachment: Part 5: Move LIBRARY_NAME to moz.build, batch 4

Comment 28

[Brian O'Keefe [:bokeefe]]

Attachment: Part 5: Forbid LIBRARY_NAME in Makefile.ins

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

Comment 29

[Michael Shal [:mshal]]

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 30

[Michael Shal [:mshal]]

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 31

[Michael Shal [:mshal]]

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

Looks good!

Comment 32

[Brian O'Keefe [:bokeefe]]

(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)

Comment 33

[Gregory Szorc [:gps]]

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

Comment 34

[Michael Shal [:mshal]]

(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 35

[Michael Shal [:mshal]]

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.

Comment 36

[Brian O'Keefe [:bokeefe]]

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

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Ed Morley [:emorley]]

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

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7ad90353e3ff
Move LIBRARY_NAME to moz.build (batch #1); r=mshal