Bug 109052 - [Qt] Use GNU ar's thin archive format for intermediate static libs
Summary: [Qt] Use GNU ar's thin archive format for intermediate static libs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andras Becsi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-06 07:29 PST by Andras Becsi
Modified: 2013-02-07 03:04 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.44 KB, patch)
2013-02-06 07:30 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
Patch (7.10 KB, patch)
2013-02-06 09:23 PST, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2013-02-06 07:29:00 PST
[Qt] Use GNU ar's thin archive format for intermediate static libs
Comment 1 Andras Becsi 2013-02-06 07:30:11 PST
Created attachment 186851 [details]
Patch
Comment 2 Jocelyn Turcotte 2013-02-06 07:42:16 PST
Comment on attachment 186851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186851&action=review

> Tools/ChangeLog:16
> +        Currently qmake does not support GNU ar's thin archive format so for

What kind of support would we need from qmake?

> Tools/qmake/mkspecs/features/default_post.prf:275
> +    gnu_thin_archives {

Why do we need the extra flag, can't we just scope it to *-g++* directly here and use it even in release?
I'm wondering if it's possible to try supporting less different configurations in the build system.

> Tools/qmake/mkspecs/features/functions.prf:28
>  defineReplace(activeBuildConfig) {
> -    CONFIG(debug, debug|release): return(debug)
> +    # The paths to the elements of the thin archive are stored

It feels like this doesn't fit with the name of the function, could you do it where activeBuildConfig is called?
Comment 3 Csaba Osztrogonác 2013-02-06 07:45:32 PST
Great patch, many thanks for it! ;) This patch decreased the size
of the WebKitBuild directory from 15Gb to 9.3G. Not mentioning
sparing IO operations after touching a simple file. (read objets
write the archive, read the archive for linking the shared object)

HDDs of the debug buildbots would be very happy. :)
Comment 4 Andras Becsi 2013-02-06 07:53:23 PST
(In reply to comment #2)
> (From update of attachment 186851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186851&action=review
> 
> > Tools/ChangeLog:16
> > +        Currently qmake does not support GNU ar's thin archive format so for
> 
> What kind of support would we need from qmake?
An extra configuration option (see: https://bugreports.qt-project.org/browse/QTBUG-27218).
Especially because of the issues related to DESTDIR, where the library is created and then moved, which break the build with thin archives.


> 
> > Tools/qmake/mkspecs/features/default_post.prf:275
> > +    gnu_thin_archives {
> 
> Why do we need the extra flag, can't we just scope it to *-g++* directly here and use it even in release?
> I'm wondering if it's possible to try supporting less different configurations in the build system.
That would be a possibility, but because of the relative paths to the objects in the archive this is just a "glorified workaround".
I'm also hesitating to make it default for debug builds.

> 
> > Tools/qmake/mkspecs/features/functions.prf:28
> >  defineReplace(activeBuildConfig) {
> > -    CONFIG(debug, debug|release): return(debug)
> > +    # The paths to the elements of the thin archive are stored
> 
> It feels like this doesn't fit with the name of the function, could you do it where activeBuildConfig is called?

The problem is that activeBuildDir is called in a lot of places, and this would make the patch much bigger than needed, so I would rather leave this hack in there.
Comment 5 Andras Becsi 2013-02-06 09:23:23 PST
Created attachment 186870 [details]
Patch
Comment 6 Andras Becsi 2013-02-06 09:26:18 PST
(In reply to comment #5)
> Created an attachment (id=186870) [details]
> Patch

As discussed on IRC, this patch creates thin archives by default unless it is a debug_and_release build.
Comment 7 Jocelyn Turcotte 2013-02-06 09:43:45 PST
Comment on attachment 186870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186870&action=review

> Tools/qmake/mkspecs/features/default_post.prf:280
> +    debug_and_release: DESTDIR = $$targetSubDir())

The old isEmpty line would still work here since you added some logic inside targetSubDir that would return an empty string.

Looks good to me otherwise, r=me if you revert that line (and if what I'm saying makes any sense).
Comment 8 Andras Becsi 2013-02-07 03:03:50 PST
Committed r142088: <http://trac.webkit.org/changeset/142088>
Comment 9 Andras Becsi 2013-02-07 03:04:40 PST
Comment on attachment 186870 [details]
Patch

Clearing flags from attachment.