Bug 137816 - [ninja] Don't remove response files for verbose builds
Summary: [ninja] Don't remove response files for verbose builds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Éva Balázsfalvi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-17 06:03 PDT by Csaba Osztrogonác
Modified: 2014-11-03 03:46 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.29 KB, patch)
2014-10-29 07:58 PDT, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff
Patch (1.46 KB, patch)
2014-10-29 08:05 PDT, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2014-10-29 08:43 PDT, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff
Patch for landing (1.68 KB, patch)
2014-11-03 01:07 PST, Éva Balázsfalvi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2014-10-17 06:03:03 PDT
In bug137606 I suggested to disable using response files if we use 
VERBOSE build to be able copy/paste the command and be able run manually.
Unfortunately the linking fails in this case with "Argument list too long".

The another problem is that ninja automatically removes response files. But we 
can use "-d keeprsp" not to remove rsp files if we have ninja 1.5.1 or newer.
Comment 1 Martin Robinson 2014-10-17 07:42:35 PDT
Response file are necessary because CMake + Ninja does not know how to add files to ar archives in chunks. Instead it will try to add them using one command invocation.
Comment 2 Éva Balázsfalvi 2014-10-29 07:58:40 PDT
Created attachment 240601 [details]
Patch

"-d keeprsp" is available since ninja version 1.4.0, so created the feature according to this.
Comment 3 Éva Balázsfalvi 2014-10-29 08:05:03 PDT
Created attachment 240602 [details]
Patch
Comment 4 Éva Balázsfalvi 2014-10-29 08:43:57 PDT
Created attachment 240603 [details]
Patch
Comment 5 Éva Balázsfalvi 2014-11-02 23:25:06 PST
Ping?
Comment 6 Csaba Osztrogonác 2014-11-03 00:31:05 PST
Comment on attachment 240603 [details]
Patch

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

The idea is good, but I suggest some minor style changes.

> Tools/Scripts/webkitdirs.pm:170
> +    my $ninjaVersion = `ninja --version`;
> +    $ninjaVersion =~ s/\R//g;

I think chomp is better choice here:
chomp(my $ninjaVersion = `ninja --version`);

> Tools/Scripts/webkitdirs.pm:1841
> +
> +        if (version->parse(determineNinjaVersion()) >= version->parse("1.4.0")) {
> +            push @args, "-d keeprsp";
> +        }

We can do it in one line:
push @args, "-d keeprsp" if (version->parse(determineNinjaVersion()) >= version->parse("1.4.0"));
Comment 7 Éva Balázsfalvi 2014-11-03 01:07:03 PST
Created attachment 240834 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2014-11-03 03:45:59 PST
Comment on attachment 240834 [details]
Patch for landing

Clearing flags on attachment: 240834

Committed r175469: <http://trac.webkit.org/changeset/175469>
Comment 9 WebKit Commit Bot 2014-11-03 03:46:03 PST
All reviewed patches have been landed.  Closing bug.