1045783 - move OS_LIBS += $(call EXPAND_LIBNAME,...) calls to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add a __getitem__ method to MockConfig

This patch just makes MockConfig act a little more like a real config object.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add an expand_libname function to the sandbox

Just replicating EXPAND_LIBNAME from config.mk...

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - move OS_LIBS += $(call EXPAND_LIBNAME,...) calls to moz.build

The almost-mechanical part of the patch series.

Getting rid of the chromium-config.mozbuild aberration is a nice bonus.

Comment 4

[Gregory Szorc [:gps]]

I'll be on PTO for a few days starting in a few hours. You may want to preemptively shift review to glandium. Actually, I'm pretty sure he's a better reviewer for this stuff anyway, considering he just rewrote all this code :)

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8464207 [details] [diff] [review]
part 2 - add an expand_libname function to the sandbox

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

We don't have a function for USE_LIBS, I don't see a reason to have one for OS_LIBS. Just do OS_LIBS += ['foo', 'bar'] and expand appropriately in emitter.py. And since we have things coming from CONFIG[] added to OS_LIBS, you also need to not alter strings starting with '-'. Or, move those to OS_LDFLAGS.

You could also leave the expansion to the backend, which could then expand using $(call EXPAND_LIBNAME).

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +344,5 @@
> +
> +        if self.config['GNU_CC']:
> +            return wrap_value(libname, prefix='-l')
> +        else:
> +            suffix = self.config['LIB_SUFFIX']

You want IMPORT_LIB_SUFFIX/PREFIX.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #5)
> We don't have a function for USE_LIBS, I don't see a reason to have one for
> OS_LIBS. Just do OS_LIBS += ['foo', 'bar'] and expand appropriately in
> emitter.py. And since we have things coming from CONFIG[] added to OS_LIBS,
> you also need to not alter strings starting with '-'. Or, move those to
> OS_LDFLAGS.

OK, this seems not much more complicated than what I already have, I'll try this.

> You could also leave the expansion to the backend, which could then expand
> using $(call EXPAND_LIBNAME).

I think doing it in Python is preferable, since that means all backends handle it automagically.

> ::: python/mozbuild/mozbuild/frontend/reader.py
> @@ +344,5 @@
> > +
> > +        if self.config['GNU_CC']:
> > +            return wrap_value(libname, prefix='-l')
> > +        else:
> > +            suffix = self.config['LIB_SUFFIX']
> 
> You want IMPORT_LIB_SUFFIX/PREFIX.

Why is it LIB_SUFFIX in config.mk, then?  Just imprecise usage of variables?

Comment 7

[Mike Hommey [:glandium]]

> > You want IMPORT_LIB_SUFFIX/PREFIX.
> 
> Why is it LIB_SUFFIX in config.mk, then?  Just imprecise usage of variables?

Yes.

Note that since you'll probably move this to emitter.py, there's sandbox.config.import_suffix, there (which does include the dot).

Comment 8

[Mike Hommey [:glandium]]

Attachment: Move OS_LIBS from a passthrough to a more fully supported variable

I needed this to go with bug 1047267. This goes as far as I needed it to be to go further. It doesn't have the necessary documentation update, for instance.

Nathan, if you want to bring this to the finish line, be my guest. I won't do it before next week. Please just avoid moving OS_LIBS that match things we otherwise have in the tree (like system nspr, nss, zlib, etc.), as those are going to be handled together with EXTRA_LIBS.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Move OS_LIBS from a passthrough to a more fully supported variable

With some previously missing propagation. Note that eventually, the propagation needs to be done in emitter.py, but that's out of scope for this bug (and involves more than OS_LIBS).

Comment 10

[Mike Hommey [:glandium]]

Attachment: Make most *_LIBS variables from configure lists in moz.build

See bug 1042878 for a similar discussion, esp. wrt the removal of some of these.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Move OS_LIBS from a passthrough to a more fully supported variable

Comment 12

[Mike Hommey [:glandium]]

Attachment: Move OS_LIBS from a passthrough to a more fully supported variable

Added one more note in build/docs.

Comment 13

[Mike Hommey [:glandium]]

Attachment: Make most *_LIBS variables from configure lists in moz.build

Comment 14

[Mike Hommey [:glandium]]

Attachment: Make most *_LIBS variables from configure lists in moz.build

Comment 15

[Mike Hommey [:glandium]]

Attachment: Move OS_LIBS from a passthrough to a more fully supported variable

Comment 16

[Mike Hommey [:glandium]]

Attachment: Move most OS_LIBS to moz.build and do some related cleanup

Comment 17

[Mike Hommey [:glandium]]

Attachment: Move HOST_EXTRA_LIBS to moz.build as HOST_OS_LIBS

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 8467747 [details] [diff] [review]
Make most *_LIBS variables from configure lists in moz.build

Note MOZ_ICU_LIBS ends up in config.status as:

(''' MOZ_ICU_LIBS ''', list(r''' $(call EXPAND_LIBNAME_PATH,$(addsuffix $(MOZ_ICU_DBG_SUFFIX),$(ICU_LIB_NAMES)),$(DEPTH)/intl/icu/target/lib) '''.split())),

Which probably isn't what we want if we ever want to use that in moz.build. But since we don't yet, I guess it doesn't matter for now.

Comment 19

[Michael Shal [:mshal]]

Comment on attachment 8467750 [details] [diff] [review]
Move most OS_LIBS to moz.build and do some related cleanup

>--- a/b2g/app/moz.build
>+++ b/b2g/app/moz.build
>@@ -53,13 +53,36 @@ if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk
>         'display',
>         'mozpng',
>     ]
>     if not CONFIG['MOZ_NATIVE_ZLIB']:
>         USE_LIBS += [
>             'mozz',
>         ]
> 
>+    OS_LIBS += [
>+        'ui',
>+        'EGL',
>+        'hardware_legacy',
>+        'hardware',
>+        'cutils',
>+    ]
>+    OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>+    if CONFIG['ANDROID_VERSION'] in ('17', '18', '19'):
>+        OS_LIBS += [
>+            'gui',
>+            'suspend',
>+        ]
>+    OS_LIBS += [
>+        'binder',
>+        'utils',
>+    ]
>+
> USE_LIBS += [
>     'xpcomglue',
> ]
> 
> DISABLE_STL_WRAPPING = True
>+
>+if CONFIG['OS_ARCH'] == 'WINNT':
>+    OS_LIBS += [
>+        'version',
>+    ]

In the Makefile, all of the OS_LIBS were also inside 'ifndef LIBXUL_SDK' - is there a reason we don't need to carry that over to moz.build?

>+if CONFIG['OS_ARCH'] == 'FreeBSD':
>+    OS_LIBS += [
>+        '-pthread',
>+    ]

Is there a distinction between using OS_LIBS vs LDFLAGS for things like this now? I realize you have to support both 'foo' and '-foo' for configure, but it is a little odd to use both forms in moz.build. IOW - if I want to add a new -blah linker flag in my directory, why would I choose OS_LIBS vs. LDFLAGS?

>diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build
>--- a/toolkit/library/moz.build
>+++ b/toolkit/library/moz.build
>+OS_LIBS += CONFIG['MOZ_CAIRO_OSLIBS']
>+OS_LIBS += CONFIG['MOZ_WEBRTC_X11_LIBS']
>+OS_LIBS += CONFIG['MOZ_APP_EXTRA_LIBS']
>+OS_LIBS += CONFIG['SQLITE_LIBS']
>+
...
>+
>+OS_LIBS += CONFIG['ICONV_LIBS']

Why not move ICONV_LIBS up with the rest of the global OS_LIBS?

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8467748 [details] [diff] [review]
Move OS_LIBS from a passthrough to a more fully supported variable

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

I really wish there were tests around all this linking behavior. But progress is progress.

Comment 21

[Mike Hommey [:glandium]]

(In reply to Michael Shal [:mshal] from comment #18)
> Comment on attachment 8467747 [details] [diff] [review]
> Make most *_LIBS variables from configure lists in moz.build
> 
> Note MOZ_ICU_LIBS ends up in config.status as:
> 
> (''' MOZ_ICU_LIBS ''', list(r''' $(call EXPAND_LIBNAME_PATH,$(addsuffix
> $(MOZ_ICU_DBG_SUFFIX),$(ICU_LIB_NAMES)),$(DEPTH)/intl/icu/target/lib)
> '''.split())),
> 
> Which probably isn't what we want if we ever want to use that in moz.build.
> But since we don't yet, I guess it doesn't matter for now.

Yep, doesn't matter. The value will be changed in bug 1047267 when moving it to moz.build.

(In reply to Michael Shal [:mshal] from comment #19)
> In the Makefile, all of the OS_LIBS were also inside 'ifndef LIBXUL_SDK' -
> is there a reason we don't need to carry that over to moz.build?

Because a) LIBXUL_SDK is not supported anymore and more importantly b) it was most probably wrong to have this ifndef in the first place.
 
> >+if CONFIG['OS_ARCH'] == 'FreeBSD':
> >+    OS_LIBS += [
> >+        '-pthread',
> >+    ]
> 
> Is there a distinction between using OS_LIBS vs LDFLAGS for things like this
> now? I realize you have to support both 'foo' and '-foo' for configure, but
> it is a little odd to use both forms in moz.build. IOW - if I want to add a
> new -blah linker flag in my directory, why would I choose OS_LIBS vs.
> LDFLAGS?

So, I think we'll need a white list, but my rationale here is that every flag that's actually linking a library should go to OS_LIBS.
- -lfoo is obviously one
- -framework foo is one too
- -pthread is actually one too. it means -lpthread on linux, and means to use a different libc for threading on FreeBSD.

> >diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build
> >--- a/toolkit/library/moz.build
> >+++ b/toolkit/library/moz.build
> >+OS_LIBS += CONFIG['MOZ_CAIRO_OSLIBS']
> >+OS_LIBS += CONFIG['MOZ_WEBRTC_X11_LIBS']
> >+OS_LIBS += CONFIG['MOZ_APP_EXTRA_LIBS']
> >+OS_LIBS += CONFIG['SQLITE_LIBS']
> >+
> ...
> >+
> >+OS_LIBS += CONFIG['ICONV_LIBS']
> 
> Why not move ICONV_LIBS up with the rest of the global OS_LIBS?

Because I didn't change any order in the conversion.

Comment 22

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8a104509b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/95cd38a0e6a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/98614c9969ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa5813d2808

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab8a104509b4
https://hg.mozilla.org/mozilla-central/rev/95cd38a0e6a5
https://hg.mozilla.org/mozilla-central/rev/98614c9969ce
https://hg.mozilla.org/mozilla-central/rev/2aa5813d2808

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8467750 [details] [diff] [review]
Move most OS_LIBS to moz.build and do some related cleanup

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

Breaks --enable-jprof

::: tools/jprof/moz.build
@@ +22,5 @@
>  ]
>  
> +OS_LIBS += [
> +    'dl',
> +    'gfd',

Typo: 'bfd', not 'gfd'

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix typo that broke --enable-jprof NPOTB DONTBUILD

IRC r=ted

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79bd1928b7d3

Comment 27

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/79bd1928b7d3