Bug 109052

Summary: [Qt] Use GNU ar's thin archive format for intermediate static libs
Product: WebKit Reporter: Andras Becsi <abecsi>
Component: New BugsAssignee: Andras Becsi <abecsi>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, jturcotte, ossy, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

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.