1254115 - Use GENERATED_FILES to generate dependentlibs.list

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

We do this in two makefiles because we link two copies of libxul (the normal one and the gtest one):
https://hg.mozilla.org/mozilla-central/file/e7319545eb3819da67ffe1d4233022ae71e3a9a1/toolkit/library/Makefile.in#l20
https://hg.mozilla.org/mozilla-central/file/e7319545eb3819da67ffe1d4233022ae71e3a9a1/toolkit/library/gtest/Makefile.in#l24

The gtest one just runs the copy built in the libxul Makefile through sed.

We already use a Python script to generate it:
https://hg.mozilla.org/mozilla-central/file/e7319545eb3819da67ffe1d4233022ae71e3a9a1/toolkit/library/dependentlibs.py

Of the parameters it gets on the commandline, FINAL_TARGET is just dist/bin in this Makefile, which we can get from buildconfig, TOOLCHAIN_PREFIX is available in buildconfig.substs. It does get $SHARED_LIBRARY passed in, which we don't have an easy way to get currently, but we could probably just hardcode platform-defaults here--xul.dll for Windows, XUL for OS X, libxul.so for everything else.

Comment 1

[Michael Shal [:mshal]]

Attachment: Bug 1254115 - Move dependentlibs.py invocation to moz.build;

Review commit: https://reviewboard.mozilla.org/r/52495/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52495/

Comment 2

[Michael Shal [:mshal]]

I didn't move the gtest one - that depends on LINK_GTEST, which is passed in via the command-line rather than something we can access in CONFIG.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8752309 [details]
Bug 1254115 - Move dependentlibs.py invocation to moz.build;

https://reviewboard.mozilla.org/r/52495/#review55494

::: toolkit/library/Makefile.in:16
(Diff revision 1)
>  
> -ifdef COMPILE_ENVIRONMENT
> -target:: $(FINAL_TARGET)/dependentlibs.list
> -endif
> -
>  $(FINAL_TARGET)/dependentlibs.list: $(topsrcdir)/toolkit/library/dependentlibs.py $(SHARED_LIBRARY) $(wildcard $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list),$(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list))))

Per my review on that other bug, this should be even easier to replace: just have `gen_list` return the list of files it's writing to the output as a set. You should definitely fix that while you're here.

::: toolkit/library/dependentlibs.py:129
(Diff revision 1)
> -        options.libpaths = [os.path.dirname(lib)]
>  
> -    print '\n'.join(dependentlibs(lib, options.libpaths, func) + [lib])
> +    output.write('\n'.join(dependentlibs(lib, libpaths, func) + [lib]) + '\n')
> +
> +def main():
> +    lib = args[0]

This probably doesn't work, does it?

::: toolkit/library/moz.build:382
(Diff revision 1)
>  
> +if CONFIG['COMPILE_ENVIRONMENT']:
> +    if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('cocoa', 'uikit'):
> +        full_libname = SHARED_LIBRARY_NAME
> +    else:
> +        full_libname = '%s%s%s' % (

This is gross. :-/

Comment 4

[Michael Shal [:mshal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> ::: toolkit/library/Makefile.in:16
> (Diff revision 1)
> >  
> > -ifdef COMPILE_ENVIRONMENT
> > -target:: $(FINAL_TARGET)/dependentlibs.list
> > -endif
> > -
> >  $(FINAL_TARGET)/dependentlibs.list: $(topsrcdir)/toolkit/library/dependentlibs.py $(SHARED_LIBRARY) $(wildcard $(if $(wildcard $(FINAL_TARGET)/dependentlibs.list),$(addprefix $(FINAL_TARGET)/,$(shell cat $(FINAL_TARGET)/dependentlibs.list))))
> 
> Per my review on that other bug, this should be even easier to replace: just
> have `gen_list` return the list of files it's writing to the output as a
> set. You should definitely fix that while you're here.

Ahh, good call. This was much easier here since we don't have to work in CPP like in the other bugs, though we have to make sure the full library path is returned rather than just the basename that goes into dependentlibs.list.

> ::: toolkit/library/dependentlibs.py:129
> (Diff revision 1)
> > -        options.libpaths = [os.path.dirname(lib)]
> >  
> > -    print '\n'.join(dependentlibs(lib, options.libpaths, func) + [lib])
> > +    output.write('\n'.join(dependentlibs(lib, libpaths, func) + [lib]) + '\n')
> > +
> > +def main():
> > +    lib = args[0]
> 
> This probably doesn't work, does it?

Nope... I'm not sure how I managed to hose that up so bad. Should be working now, though there is no usage information for the argument. Since the script normally isn't invoked manually I'm not sure how important that is.

> ::: toolkit/library/moz.build:382
> (Diff revision 1)
> >  
> > +if CONFIG['COMPILE_ENVIRONMENT']:
> > +    if CONFIG['MOZ_WIDGET_TOOLKIT'] in ('cocoa', 'uikit'):
> > +        full_libname = SHARED_LIBRARY_NAME
> > +    else:
> > +        full_libname = '%s%s%s' % (
> 
> This is gross. :-/

Agreed. Is there a way I could get the library name from the template? I tried just returning the name from Libxul() but it seems that's not so straightforward. I think that would help simplify a few other moz.build files that do the library-name expansion as well.

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8752309 [details]
Bug 1254115 - Move dependentlibs.py invocation to moz.build;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52495/diff/1-2/

Comment 6

[Michael Shal [:mshal]]

Comment on attachment 8752309 [details]
Bug 1254115 - Move dependentlibs.py invocation to moz.build;

Meant to ask for re-review on this.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Michael Shal [:mshal] from comment #4)
> Agreed. Is there a way I could get the library name from the template? I
> tried just returning the name from Libxul() but it seems that's not so
> straightforward. I think that would help simplify a few other moz.build
> files that do the library-name expansion as well.

I don't know that there is, but I know I've groused about this before. I think the last time I looked it wasn't a widespread thing so I didn't bother, but if it's showing up in a few more places maybe we should just add it.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8752309 [details]
Bug 1254115 - Move dependentlibs.py invocation to moz.build;

https://reviewboard.mozilla.org/r/52495/#review64866

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/281bbbf61f36
Move dependentlibs.py invocation to moz.build; r=ted

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/281bbbf61f36