Closed Bug 1047267 Opened 5 years ago Closed 5 years ago

Move EXTRA_LIBS to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: glandium, Assigned: glandium)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Depends on: 1045783
Depends on: 1049281
Depends on: 1049311
The parts in config/external/* are kind of awkward, especially those related to MOZ_FOLD_LIBS, but I don't have any better idea until we have more expressiveness in moz.build and/or we move nss/nspr to moz.build.

Note there are interesting subtle differences between what was in Makefile.in and what ands up in backend.mk through what's added as a replacement to moz.build, but in practice, there's no significant difference. The most funny one is in media/mtransport/test, where you'd think libssl is being statically linked even in the MOZ_FOLD_LIBS case before this patch, but it actually wasn't (the joys of static libraries magically being dropped).
Attachment #8468542 - Flags: review?(gps)
Sadly, even after this queue we can't remove EXTRA_LIBS and OS_LIBS just yet. EXTRA_LIBS because of some hackish thing in tools/trace-malloc, and the stlport stuff in some test Makefiles (which, in fact, I think can just be removed, but I'll do that in a followup), and OS_LIBS because it's defined in autoconf.mk (we'd need to move the _MOZBUILD_EXTERNAL_VARIABLES stuff to baseconfig.mk and change some of the tests, I'll leave that to a followup too)
Attachment #8468545 - Flags: review?(gps)
Heh, turns out there's actually one test in mtransport that does use a small part of libssl.a because the corresponding symbols are not exported from the folded libnss, and that only links if libssl.a comes after the folded libnss, which is the kind of setup moz.build doesn't support. I just exported the missing symbols, instead of pretending adding libssl.a had actually more effect than it has and add workarounds for that. There were also missing ifs in the top-level moz.build.
Attachment #8468586 - Flags: review?(gps)
Attachment #8468542 - Attachment is obsolete: true
Attachment #8468542 - Flags: review?(gps)
Attachment #8468346 - Flags: review?(gps) → review+
Comment on attachment 8468354 [details] [diff] [review]
Allow to reference libraries from third-party build systems in USE_LIBS

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +284,4 @@
>                      raise SandboxValidationError(
>                          '%s contains "%s", but there is no "%s" %s in %s.'
>                          % (variable, path, name,
>                          self.LIBRARY_NAME_VAR[obj.KIND], dir), sandbox)

Adding a test that verifies a USE_LIBS pointing to a non-external directory would be nice. But this code looks solid.

@@ +363,5 @@
> +
> +        if force_static:
> +            return ExternalStaticLibrary(sandbox, name)
> +        else:
> +            return ExternalSharedLibrary(sandbox, name)

Clever.

@@ +992,5 @@
>          o.dirs = sandbox.get('DIRS', [])
>          o.test_dirs = sandbox.get('TEST_DIRS', [])
>          o.affected_tiers = sandbox.get_affected_tiers()
>  
> +        self._external_paths -= set([o.relobjdir])

It isn't obvious to me why this is needed. I'm not too concerned about its presence, but this really needs an inline comment.

You could use the set literal syntax if you wanted: -= {o.relobjdir}
Attachment #8468354 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #6)
> It isn't obvious to me why this is needed.

We have subconfigured things that are still handled by moz.build (like jemalloc3)
Comment on attachment 8468586 [details] [diff] [review]
Move remaining OS_LIBS and EXTRA_LIBS to moz.build

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

::: b2g/app/moz.build
@@ -24,5 @@
>  
> -    if not CONFIG['MOZ_NATIVE_ZLIB'] and not CONFIG['ZLIB_IN_MOZCONFIG']:
> -        USE_LIBS += [
> -            'mozz',
> -        ]

I like how library names are unified in the new world!

::: config/external/sqlite/moz.build
@@ +8,5 @@
> +
> +if CONFIG['MOZ_NATIVE_SQLITE']:
> +    OS_LIBS += CONFIG['SQLITE_LIBS']
> +else:
> +    DIRS += ['../../../db/sqlite3/src']

Can't this be '/db/sqlite3/src'?

::: config/external/zlib/moz.build
@@ +16,5 @@
> +        #     'mozglue'
> +        # ]
> +        pass
> +    DIRS += [
> +        '../../../modules/zlib',

'/modules/zlib' ?

::: content/media/webaudio/compiledtest/moz.build
@@ +17,5 @@
>  ]
>  
>  USE_LIBS += [
>      'mozalloc',
> +    'nspr',

The case for templates is quickly growing.

::: toolkit/library/Makefile.in
@@ -4,5 @@
>  
>  include $(topsrcdir)/toolkit/library/libxul.mk
> -# Please don't remove the following comment, because it does some magic:
> -# libxul.mk adds FT2_LIBS, NSS_LIBS and NSPR_LIBS, but we want to have the
> -# string in this file to trigger a dependency on freetype2, nss and nspr.

Can we remove the code that looks for this in the backend yet?
Attachment #8468586 - Flags: review?(gps) → review+
Comment on attachment 8468545 [details] [diff] [review]
Remove the trigger hacks added in bug 1043344

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

And this answers my question from the last review. Thanks for splitting the patches into self-contained work. It makes my life as reviewer much easier. I should have paid better attention to the patch descriptions.

::: python/mozbuild/mozbuild/frontend/reader.py
@@ -298,5 @@
>                  raise Exception('Directory has already been registered with '
>                      'tier: %s' % path)
>  
> -            path = PathWithTrigger(path)
> -            path.trigger = trigger

I'm pretty sure there is a "trigger" argument to this method that still needs deleted.
Attachment #8468545 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8468586 [details] [diff] [review]
> Move remaining OS_LIBS and EXTRA_LIBS to moz.build
> 
> Review of attachment 8468586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/app/moz.build
> @@ -24,5 @@
> >  
> > -    if not CONFIG['MOZ_NATIVE_ZLIB'] and not CONFIG['ZLIB_IN_MOZCONFIG']:
> > -        USE_LIBS += [
> > -            'mozz',
> > -        ]
> 
> I like how library names are unified in the new world!
> 
> ::: config/external/sqlite/moz.build
> @@ +8,5 @@
> > +
> > +if CONFIG['MOZ_NATIVE_SQLITE']:
> > +    OS_LIBS += CONFIG['SQLITE_LIBS']
> > +else:
> > +    DIRS += ['../../../db/sqlite3/src']
> 
> Can't this be '/db/sqlite3/src'?

Sadly no. There are plenty of variables that don't support those paths, and DIRS is one:

The error occurred while processing the following file:

    /home/glandium/mozilla-central/config/external/sqlite/moz.build

The error occurred when validating the result of the execution. The reported error is:

    Attempting to process file outside of allowed paths: /db/sqlite3/src/moz.build

That said, iirc the patch from bug 1035599 would make it work.

> ::: content/media/webaudio/compiledtest/moz.build
> @@ +17,5 @@
> >  ]
> >  
> >  USE_LIBS += [
> >      'mozalloc',
> > +    'nspr',
> 
> The case for templates is quickly growing.

And that's next on my list.
Blocks: 1050021
Blocks: 1050037
With bug 1050037 and bug 1050029, this allows to forbis OS_LIBS and EXTRA_LIBS.
Attachment #8468978 - Flags: review?(gps)
Comment on attachment 8468978 [details] [diff] [review]
To fold with "Move remaining OS_LIBS and EXTRA_LIBS to moz.build"

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

::: tools/trace-malloc/Makefile.in
@@ +23,5 @@
>  
> +# This is the last use of EXTRA_LIBS, and it would be sad to not have an
> +# error if it's used in other Makefiles just because of it. So hack around
> +# the check.
> +_DEPRECATED_VARIABLES := $(filter-out EXTRA_LIBS,$(_DEPRECATED_VARIABLES))

It saddens me this hack works. I hope it doesn't get cargo culted.
Attachment #8468978 - Flags: review?(gps) → review+
Blocks: 1050086
Depends on: 1050560
Blocks: 1052013
Depends on: 1053508
Depends on: 1053554
Depends on: 1052508
QA Whiteboard: [qa-]
Blocks: 1090111
Duplicate of this bug: 1058778
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.