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)
Firefox Build System
General
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.
Reporter | ||
Comment 1•10 years ago
|
||
This patch just makes MockConfig act a little more like a real config object.
Attachment #8464206 -
Flags: review?(gps)
Reporter | ||
Comment 2•10 years ago
|
||
Just replicating EXPAND_LIBNAME from config.mk...
Attachment #8464207 -
Flags: review?(gps)
Reporter | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
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 :)
Reporter | ||
Updated•10 years ago
|
Attachment #8464209 -
Flags: review?(gps) → review?(mh+mozilla)
Reporter | ||
Updated•10 years ago
|
Attachment #8464207 -
Flags: review?(gps) → review?(mh+mozilla)
Reporter | ||
Updated•10 years ago
|
Attachment #8464206 -
Flags: review?(gps) → review?(mh+mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
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-
Reporter | ||
Comment 6•10 years ago
|
||
(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)
Assignee | ||
Comment 7•10 years ago
|
||
> > 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)
Assignee | ||
Comment 8•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Assignee: mh+mozilla → nfroyd
Assignee | ||
Comment 9•10 years ago
|
||
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).
Assignee | ||
Updated•10 years ago
|
Attachment #8466055 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
See bug 1042878 for a similar discussion, esp. wrt the removal of some of these.
Attachment #8467426 -
Flags: review?(mshal)
Assignee | ||
Updated•10 years ago
|
Assignee: nfroyd → mh+mozilla
Assignee | ||
Updated•10 years ago
|
Attachment #8464206 -
Attachment is obsolete: true
Attachment #8464206 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8464207 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8464209 -
Attachment is obsolete: true
Attachment #8464209 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8467433 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8466065 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Added one more note in build/docs.
Attachment #8467435 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8467433 -
Attachment is obsolete: true
Attachment #8467433 -
Flags: review?(gps)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8467451 -
Flags: review?(mshal)
Assignee | ||
Updated•10 years ago
|
Attachment #8467426 -
Attachment is obsolete: true
Attachment #8467426 -
Flags: review?(mshal)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8467747 -
Flags: review?(mshal)
Assignee | ||
Updated•10 years ago
|
Attachment #8467451 -
Attachment is obsolete: true
Attachment #8467451 -
Flags: review?(mshal)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8467748 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8467435 -
Attachment is obsolete: true
Attachment #8467435 -
Flags: review?(gps)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8467750 -
Flags: review?(mshal)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8467751 -
Flags: review?(mshal)
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8467751 -
Flags: review?(mshal) → review+
Comment 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
(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.
Assignee | ||
Comment 22•10 years ago
|
||
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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 24•10 years ago
|
||
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•10 years ago
|
||
IRC r=ted
Updated•10 years ago
|
QA Whiteboard: [qa-]
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•