Bug 137816

Summary: [ninja] Don't remove response files for verbose builds
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Éva Balázsfalvi <evab.u-szeged>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, dbates, evab.u-szeged, gyuyoung.kim, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

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.