Move LIBRARY_NAME to moz.build

RESOLVED FIXED in mozilla26

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: bokeefe, Assigned: bokeefe)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla26
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

Assignee

Description

6 years ago
No description provided.
Assignee

Comment 1

6 years ago
Adds LIBRARY_NAME as a passthrough variable
Attachment #754151 - Flags: review?(gps)
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
Attachment #754151 - Flags: review?(gps) → review?(ted)
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.
Attachment #754151 - Flags: review?(ted) → review+
Assignee

Comment 4

6 years ago
Addresses Ted's review nits.

Thanks Ted for reviewing!
Attachment #754151 - Attachment is obsolete: true
Attachment #761376 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [leave open]
Assignee

Comment 7

6 years ago
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
Attachment #764095 - Flags: review?(mshal)
Assignee

Comment 8

6 years ago
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
Attachment #764099 - Flags: review?(mshal)
Assignee

Updated

6 years ago
Depends on: 883503
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!
Attachment #764099 - Flags: review?(mshal) → feedback+
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?
Attachment #764095 - Flags: review?(mshal) → review-
gps: Please see VERSION_NUMBER question in https://bugzilla.mozilla.org/show_bug.cgi?id=875934#c10
Flags: needinfo?(gps)
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.
Assignee

Comment 13

6 years ago
(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.
Attachment #764095 - Attachment is obsolete: true
Attachment #764265 - Flags: review?(mshal)
Assignee

Comment 14

6 years ago
(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.
Attachment #764099 - Attachment is obsolete: true
Attachment #764270 - Flags: review?(mshal)
What ted said in comment #12.
Flags: needinfo?(gps)
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!
Attachment #764265 - Flags: review?(mshal) → review+
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!
Attachment #764270 - Flags: review?(mshal) → review+
Assignee

Comment 18

6 years ago
(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)
Keywords: checkin-needed
Bitrotted :(

Also, can the [leave open] go?
Keywords: checkin-needed
I now see there are many LIBRARY_NAME still lingering in the tree. Leaving open...
Whiteboard: [leave open]
Assignee

Updated

6 years ago
Depends on: 896177
Brian, do you have time to work on this? Otherwise I'll see if I can get rid of some.
Flags: needinfo?(bokeefe)
Assignee

Comment 24

6 years ago
(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).
Flags: needinfo?(bokeefe)
Great, thanks!
Assignee

Comment 26

6 years ago
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.
Attachment #796124 - Flags: review?(mshal)
Assignee

Comment 28

6 years ago
As usual, this part will break comm-central until bug 884825 lands
Attachment #796127 - Flags: review?(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.
Attachment #796124 - Flags: review?(mshal) → review+
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!
Attachment #796127 - Flags: review?(mshal) → review+
Assignee

Comment 32

6 years ago
(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.
Attachment #796126 - Flags: review?(mshal) → review+
Assignee

Updated

6 years ago
Attachment #761376 - Flags: checkin+
Assignee

Updated

6 years ago
Attachment #764265 - Flags: checkin+
Assignee

Updated

6 years ago
Attachment #764270 - Flags: checkin+
Assignee

Updated

6 years ago
Attachment #796124 - Flags: checkin?
Assignee

Updated

6 years ago
Attachment #796126 - Flags: checkin?
Assignee

Updated

6 years ago
Attachment #796127 - Flags: checkin?
Assignee

Comment 36

6 years ago
Try came back green, finally (other than some intermittent failures), so this should be good to land now.
Keywords: checkin-needed
Whiteboard: [leave open]
Attachment #796124 - Flags: checkin? → checkin+
Attachment #796126 - Flags: checkin? → checkin+
Attachment #796127 - Flags: checkin? → checkin+

Comment 40

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

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.