Closed Bug 1045783 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: froydnj, Assigned: glandium)

References

Details

Attachments

(5 files, 9 obsolete files)

18.73 KB, patch
mshal
: review+
Details | Diff | Splinter Review
14.68 KB, patch
gps
: review+
Details | Diff | Splinter Review
81.39 KB, patch
mshal
: review+
Details | Diff | Splinter Review
4.05 KB, patch
mshal
: review+
Details | Diff | Splinter Review
555 bytes, patch
Details | Diff | Splinter Review
      No description provided.
This patch just makes MockConfig act a little more like a real config object.
Attachment #8464206 - Flags: review?(gps)
Just replicating EXPAND_LIBNAME from config.mk...
Attachment #8464207 - Flags: review?(gps)
The almost-mechanical part of the patch series.

Getting rid of the chromium-config.mozbuild aberration is a nice bonus.
Attachment #8464209 - Flags: review?(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 :)
Attachment #8464209 - Flags: review?(gps) → review?(mh+mozilla)
Attachment #8464207 - Flags: review?(gps) → review?(mh+mozilla)
Attachment #8464206 - Flags: review?(gps) → review?(mh+mozilla)
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.
Attachment #8464207 - Flags: review?(mh+mozilla) → review-
(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?
Flags: needinfo?(mh+mozilla)
> > 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).
Flags: needinfo?(mh+mozilla)
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.
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Assignee: mh+mozilla → nfroyd
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).
Attachment #8466055 - Attachment is obsolete: true
Blocks: 1047267
See bug 1042878 for a similar discussion, esp. wrt the removal of some of these.
Attachment #8467426 - Flags: review?(mshal)
Assignee: nfroyd → mh+mozilla
Attachment #8464206 - Attachment is obsolete: true
Attachment #8464206 - Flags: review?(mh+mozilla)
Attachment #8464207 - Attachment is obsolete: true
Attachment #8464209 - Attachment is obsolete: true
Attachment #8464209 - Flags: review?(mh+mozilla)
Attachment #8466065 - Attachment is obsolete: true
Added one more note in build/docs.
Attachment #8467435 - Flags: review?(gps)
Attachment #8467433 - Attachment is obsolete: true
Attachment #8467433 - Flags: review?(gps)
Attachment #8467426 - Attachment is obsolete: true
Attachment #8467426 - Flags: review?(mshal)
Depends on: 1048654
Depends on: 1048702
Attachment #8467451 - Attachment is obsolete: true
Attachment #8467451 - Flags: review?(mshal)
Attachment #8467435 - Attachment is obsolete: true
Attachment #8467435 - Flags: review?(gps)
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.
Attachment #8467747 - Flags: review?(mshal) → review+
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?
Attachment #8467750 - Flags: review?(mshal) → review+
Attachment #8467751 - Flags: review?(mshal) → review+
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.
Attachment #8467748 - Flags: review?(gps) → review+
(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 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'
Blocks: 1049935
Blocks: 1052526
QA Whiteboard: [qa-]
Depends on: 1051209
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.