Bug 173066 - [CMake] Only force response files for Ninja with CMake < 3.2 on Linux
Summary: [CMake] Only force response files for Ninja with CMake < 3.2 on Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-07 10:35 PDT by Loïc Yhuel
Modified: 2017-06-07 11:41 PDT (History)
5 users (show)

See Also:

Attachments
Patch (2.07 KB, patch)
2017-06-07 10:36 PDT, Loïc Yhuel 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Loïc Yhuel 2017-06-07 10:35:10 PDT 
[CMake] Only force response files for Ninja with CMake < 3.2 on Linux
Comment 1 Loïc Yhuel 2017-06-07 10:36:41 PDT 
Created attachment 312195 [details]
Patch
Comment 2 Loïc Yhuel 2017-06-07 10:48:52 PDT 
CMake uses response files when needed, but the limit was wrong on Linux (https://cmake.org/Bug/view.php?id=14892, fixed in CMake 3.2).
It created the original https://bugs.webkit.org/show_bug.cgi?id=129771, with "argument list too long" errors. 

CMake 3.6 added response file support for compile rules (which created https://bugs.webkit.org/show_bug.cgi?id=168770 since we forced the response files).

On Linux, CMake >= 3.2 uses response files as needed (ie only for libWebCore.a).
Hopefully platforms which don't support response files (FreeBSD, Apple) have a big enough command line length limit (sysconf(_SC_ARG_MAX)) to avoid them.
Comment 3 Michael Catanzaro 2017-06-07 10:51:26 PDT 
Comment on attachment 312195 [details]
Patch

Thanks, let's try it! I only wonder if we still need the conditionals to force disable response files when using icecc. Carlos Lopez, Tom, what do you think?
Comment 4 Loïc Yhuel 2017-06-07 11:09:55 PDT 
(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 312195 [details]
> Patch
> 
> Thanks, let's try it! I only wonder if we still need the conditionals to
> force disable response files when using icecc. Carlos Lopez, Tom, what do
> you think?

The icecc test was only disabling CMAKE_NINJA_FORCE_RESPONSE_FILE for CMake > 3.5, older versions only support response files to link or create archives.
The archives are done locally with ar, and for the link icecc probably forwards all arguments to the local CXX.
Comment 5 Michael Catanzaro 2017-06-07 11:13:13 PDT 
Comment on attachment 312195 [details]
Patch

I'm not sure I understand, but let's try it!
Comment 6 Michael Catanzaro 2017-06-07 11:14:08 PDT 
Ah well yeah, I must have misread the conditionals... clearly this shouldn't affect icecc at all (unless using old CMake).
Comment 7 WebKit Commit Bot 2017-06-07 11:41:12 PDT 
Comment on attachment 312195 [details]
Patch

Clearing flags on attachment: 312195

Committed r217894: <http://trac.webkit.org/changeset/217894>
Comment 8 WebKit Commit Bot 2017-06-07 11:41:14 PDT 
All reviewed patches have been landed.  Closing bug.