Bug 170105 - FreeBSD ar command doesn't support response files
Summary: FreeBSD ar command doesn't support response files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-26 05:36 PDT by Ting-Wei Lan
Modified: 2017-06-07 10:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.85 KB, patch)
2017-03-26 05:43 PDT, Ting-Wei Lan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (993.92 KB, application/zip)
2017-03-27 01:08 PDT, Build Bot
no flags Details
Patch (2.09 KB, patch)
2017-04-07 08:27 PDT, Ting-Wei Lan
no flags Details | Formatted Diff | Diff
Patch (2.24 KB, patch)
2017-04-08 09:30 PDT, Ting-Wei Lan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ting-Wei Lan 2017-03-26 05:36:14 PDT
When cmake is set to generate build files for ninja, Source/cmake/OptionsCommon.cmake automatically enables the use of response files by setting CMAKE_NINJA_FORCE_RESPONSE_FILE to 1. This causes linking failure on FreeBSD because its ar command doesn't understand the response file syntax, and all object files listed in response files are not added.

FreeBSD ar is implemented using libarchive and it is part of elftoolchain project (https://sourceforge.net/projects/elftoolchain/). We can detect it by checking the output of 'ar -V' command and disable the use of response files for it.
Comment 1 Ting-Wei Lan 2017-03-26 05:43:29 PDT
Created attachment 305419 [details]
Patch
Comment 2 Build Bot 2017-03-27 01:08:45 PDT
Comment on attachment 305419 [details]
Patch

Attachment 305419 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3416117

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html
fast/scrolling/ios/scroll-events-back-forward.html
Comment 3 Build Bot 2017-03-27 01:08:47 PDT
Created attachment 305454 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 4 Michael Catanzaro 2017-04-07 08:11:03 PDT
Comment on attachment 305419 [details]
Patch

FYI: you're going to have a (trivial) merge conflict here.
Comment 5 Ting-Wei Lan 2017-04-07 08:27:58 PDT
Created attachment 306503 [details]
Patch
Comment 6 Michael Catanzaro 2017-04-07 11:34:49 PDT
Comment on attachment 306503 [details]
Patch

OK

Then again, if the build works without response files, and response files are broken on so many platforms, maybe we should just get rid of them?
Comment 7 Ting-Wei Lan 2017-04-07 21:04:55 PDT
(In reply to Michael Catanzaro from comment #6)
> Then again, if the build works without response files, and response files
> are broken on so many platforms, maybe we should just get rid of them?

Response files were added in bug 129771 to avoid 'Argument list too long' error, but we don't know which platform really requires them. If there are only a few platforms requiring response files, maybe we can disable them by default and only enable them for these specific platforms?
Comment 8 Michael Catanzaro 2017-04-08 07:04:51 PDT
I'm suspect Martin was using Linux.

Let's just try removing it and see if something breaks! If it doesn't work, then we can go with the patch we have here.
Comment 9 Ting-Wei Lan 2017-04-08 09:30:54 PDT
Created attachment 306570 [details]
Patch
Comment 10 WebKit Commit Bot 2017-04-08 15:44:43 PDT
Comment on attachment 306570 [details]
Patch

Clearing flags on attachment: 306570

Committed r215150: <http://trac.webkit.org/changeset/215150>
Comment 11 WebKit Commit Bot 2017-04-08 15:44:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Chris Dumez 2017-04-08 16:08:11 PDT
(In reply to WebKit Commit Bot from comment #11)
> All reviewed patches have been landed.  Closing bug.

Broke GTK bot?

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Build%29/builds/640/steps/compile-webkit/logs/stdio
Comment 13 Michael Catanzaro 2017-04-08 17:21:27 PDT
Reverted r215150 for reason:

Broke

Committed r215155: <http://trac.webkit.org/changeset/215155>
Comment 14 WebKit Commit Bot 2017-04-08 17:50:36 PDT
Comment on attachment 306503 [details]
Patch

Clearing flags on attachment: 306503

Committed r215156: <http://trac.webkit.org/changeset/215156>
Comment 15 WebKit Commit Bot 2017-04-08 17:50:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryosuke Niwa 2017-04-10 14:43:13 PDT
Committed r215203: <http://trac.webkit.org/changeset/215203>
Comment 17 Loïc Yhuel 2017-06-07 07:18:51 PDT
(In reply to Ting-Wei Lan from comment #7)
> (In reply to Michael Catanzaro from comment #6)
> > Then again, if the build works without response files, and response files
> > are broken on so many platforms, maybe we should just get rid of them?
> 
> Response files were added in bug 129771 to avoid 'Argument list too long'
> error, but we don't know which platform really requires them. If there are
> only a few platforms requiring response files, maybe we can disable them by
> default and only enable them for these specific platforms?


Actually the "argument list too long" issue was probably specific to Linux, and fixed in CMake 3.2 (https://cmake.org/Bug/view.php?id=14892). CMake already used response files when needed, but the limit was wrong on Linux.
So so we don't need to force response files any more.

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

FreeBSD probably wasn't affected by the original issue, but even without CMAKE_NINJA_FORCE_RESPONSE_FILE, CMake will use a response file if the command line is larger than sysconf(_SC_ARG_MAX) on FreeBSD.
So if the linker (or ar) command line becomes too long, there would be an issue.

Btw, this commit disabled the response files with FreeBSD ar, but only for CMake >= 3.6, so the issue would remain for older versions.
Comment 18 Ting-Wei Lan 2017-06-07 08:05:15 PDT
(In reply to Loïc Yhuel from comment #17)
> CMake 3.6 added response file support for compile rules (which created
> https://bugs.webkit.org/show_bug.cgi?id=168770).
> 
> FreeBSD probably wasn't affected by the original issue, but even without
> CMAKE_NINJA_FORCE_RESPONSE_FILE, CMake will use a response file if the
> command line is larger than sysconf(_SC_ARG_MAX) on FreeBSD.
> So if the linker (or ar) command line becomes too long, there would be an
> issue.

Yes, I know the problem can still happen if the command line is really too long, but I never see it on my machines. WebKitGTK+ in FreeBSD ports was only switched to ninja recently, so it may take some time to see bug reports on FreeBSD Bugzilla if it really happens on users' machines.

> 
> Btw, this commit disabled the response files with FreeBSD ar, but only for
> CMake >= 3.6, so the issue would remain for older versions.

The 'latest' repository of FreeBSD uses CMake 3.8.0 and the 'quarterly' repository uses CMake 3.7.2, so I think it is unlikely to happen on FreeBSD.
Comment 19 Michael Catanzaro 2017-06-07 09:13:01 PDT
Loïc, what you're saying is that we can stop forcing response files now, right? Then we won't need to maintain platform-specific exceptions anymore. If so, it would be helpful to file a new bug for that.
Comment 20 Loïc Yhuel 2017-06-07 10:34:34 PDT
(In reply to Michael Catanzaro from comment #19)
> Loïc, what you're saying is that we can stop forcing response files now,
> right? Then we won't need to maintain platform-specific exceptions anymore.
> If so, it would be helpful to file a new bug for that.

We only need to force response files on Linux hosts with CMake older than 3.2.
It's still platform-specific, but it's simpler that we do currently.

I'm checking WebKitGTK build correctly, I will create a bug after.