Bug 151399 - Fix Ninja build on Mac
Summary: Fix Ninja build on Mac
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: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-18 11:27 PST by Alex Christensen
Modified: 2016-03-30 08:30 PDT (History)
9 users (show)

See Also:


Attachments
Patch (24.05 KB, patch)
2015-11-18 11:41 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.37 KB, patch)
2015-11-18 12:04 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (25.57 KB, patch)
2015-11-18 12:07 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.24 KB, patch)
2015-11-18 15:51 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (22.74 KB, patch)
2015-11-19 19:12 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for ews (23.01 KB, patch)
2015-11-22 22:47 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for efl-ews (22.99 KB, patch)
2015-11-23 20:24 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (23.66 KB, patch)
2016-03-26 18:12 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (23.65 KB, patch)
2016-03-28 09:36 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.30 KB, patch)
2016-03-28 10:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (25.44 KB, patch)
2016-03-28 17:02 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-11-18 11:27:19 PST
Fix Ninja build on Mac
Comment 1 Alex Christensen 2015-11-18 11:41:07 PST
Created attachment 265766 [details]
Patch
Comment 2 Alex Christensen 2015-11-18 12:04:58 PST
Created attachment 265770 [details]
Patch
Comment 3 Alex Christensen 2015-11-18 12:07:36 PST
Created attachment 265771 [details]
Patch
Comment 4 Chris Dumez 2015-11-18 12:44:27 PST
Broke GTK and EFL?
Comment 5 Alex Christensen 2015-11-18 13:00:17 PST
Yes, I'm not sure what went wrong there.  This works on Mac and Windows.  Could someone try this on Linux and let me know what's wrong?  I was under the impression that the Linux ports make everything into a static library and link everything into one big dynamic library.
Comment 6 Michael Catanzaro 2015-11-18 15:47:17 PST
(In reply to comment #5)
> Yes, I'm not sure what went wrong there.  This works on Mac and Windows. 
> Could someone try this on Linux and let me know what's wrong? 

The problem is that the new target is not compiled with -fPIC, so it's not built with position-independent code and can't be linked into a shared library. I think the fix is to add a call to WEBKIT_SET_EXTRA_COMPILER_FLAGS(WebCoreDerivedSources ${ADDITIONAL_COMPILER_FLAGS}) in Source/CMakeLists.txt.

I wonder why those calls are located in Source/CMakeLists.txt. Seems to me it'd be better to put them right after targets are first defined, else it's hard to know this is required when adding a new target.

> I was under
> the impression that the Linux ports make everything into a static library
> and link everything into one big dynamic library.

Yup, that's right!
Comment 7 Alex Christensen 2015-11-18 15:51:49 PST
Created attachment 265794 [details]
Patch
Comment 8 Michael Catanzaro 2015-11-18 17:31:21 PST
Also, most of the libraries previously linked to WebCore now need to be linked to  WebCoreDerivedSources as well. I'll try to get it working locally....
Comment 9 Michael Catanzaro 2015-11-19 19:12:48 PST
Created attachment 265928 [details]
Patch
Comment 10 Michael Catanzaro 2015-11-20 06:36:16 PST
I don't know why EFL won't build :(
Comment 11 Gyuyoung Kim 2015-11-22 17:34:53 PST
(In reply to comment #10)
> I don't know why EFL won't build :(

Now I'm struggling with this break on EFL.
Comment 12 Gyuyoung Kim 2015-11-22 22:47:22 PST
Created attachment 266079 [details]
Patch for ews
Comment 13 Gyuyoung Kim 2015-11-22 23:23:45 PST
Alex, I wonder if my fix is fine for mac with cmake-ninja. Could you check it ?
Comment 14 Michael Catanzaro 2015-11-23 07:32:40 PST
Comment on attachment 266079 [details]
Patch for ews

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

> Source/WebCore/CMakeLists.txt:3728
> +add_library(WebCore ${WebCore_LIBRARY_TYPE} ${WebCore_SOURCES} ${WebCore_DERIVEDSOURCES})

This isn't right; if you compile the derived sources into the WebCore library, that defeats the purpose of compiling them into the separate WebCoreDerivedSources library (which was to reduce the command line length).
Comment 15 Gyuyoung Kim 2015-11-23 20:24:43 PST
Created attachment 266111 [details]
Patch for efl-ews
Comment 16 Gyuyoung Kim 2015-11-23 20:59:26 PST
(In reply to comment #14)
> Comment on attachment 266079 [details]
> Patch for ews
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266079&action=review
> 
> > Source/WebCore/CMakeLists.txt:3728
> > +add_library(WebCore ${WebCore_LIBRARY_TYPE} ${WebCore_SOURCES} ${WebCore_DERIVEDSOURCES})
> 
> This isn't right; if you compile the derived sources into the WebCore
> library, that defeats the purpose of compiling them into the separate
> WebCoreDerivedSources library (which was to reduce the command line length).

Yes, we just need to restart efl-ews from scratch. I find it by accident because I fail to find what is wrong in current fix. Previous build error log made me confuse. :(
Comment 17 Michael Catanzaro 2015-11-24 08:33:41 PST
Thanks Gyuyoung.

Alex, I don't know if this works on Windows or not; the EWS is broken. Basically my change to your patch was to link WebCore to WebCoreDerivedSources in WebCore/CMakeLists.txt, rather than in a bunch of different WebKit/Platform*.cmake and WebKit2/Platform*.cmake files.
Comment 18 Michael Catanzaro 2015-11-24 15:24:19 PST
Alex, I will leave this for you to land, so you can satisfy yourself that I haven't broken Windows or Ninja on OS X (which would be rather silly).
Comment 19 Gyuyoung Kim 2015-12-18 23:49:36 PST
Alex, I wonder if you want to land this patch. I still meet a linking error when building Mac port using cmake.
Comment 20 Alex Christensen 2016-01-04 11:15:58 PST
Yep, I still want to land this patch eventually.  It's a low priority, though.
Comment 21 Alex Christensen 2016-03-26 18:12:28 PDT
Created attachment 274990 [details]
Patch
Comment 22 Alex Christensen 2016-03-28 09:36:03 PDT
Created attachment 275024 [details]
Patch
Comment 23 Alex Christensen 2016-03-28 10:32:46 PDT
Created attachment 275028 [details]
Patch
Comment 24 Michael Catanzaro 2016-03-28 10:38:10 PDT
Comment on attachment 275028 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:3469
> +list(APPEND WebCore_DERIVEDSOURCES ${DERIVED_SOURCES_WEBCORE_DIR}/InspectorOverlayPage.h)

Konstantin noticed this is another bug, it should be WebCore_DERIVED_SOURCES, not WebCore_DERIVED_SOURCES, for consistency with WebKit2_DERIVED_SOURCES, which already exists.

> Tools/Scripts/webkitdirs.pm:943
> +    return;

Mistake? Or is it for testing on EWS?

> Tools/Scripts/webkitdirs.pm:2474
> +    my $productDir = "/Users/alexchristensen/webkit/wkb/lib";#productDir();

Ditto.
Comment 25 Michael Catanzaro 2016-03-28 10:38:59 PDT
(In reply to comment #24)
> Konstantin noticed this is another bug, it should be
> WebCore_DERIVED_SOURCES, not WebCore_DERIVED_SOURCES, for consistency with
> WebKit2_DERIVED_SOURCES, which already exists.

Oops. Try again:

Konstantin noticed this is another bug, it should be WebCore_DERIVED_SOURCES, not WebCore_DERIVEDSOURCES, for consistency with WebKit2_DERIVED_SOURCES, which already exists.
Comment 26 Alex Christensen 2016-03-28 16:25:07 PDT
Comment on attachment 275028 [details]
Patch

Yep, and the ews bots were in a bad state this morning.  I'll try again soon.
Those script changes are an accidental upload.
Comment 27 Alex Christensen 2016-03-28 17:02:44 PDT
Created attachment 275068 [details]
Patch
Comment 28 Alex Christensen 2016-03-28 17:19:16 PDT
http://trac.webkit.org/changeset/198766
Comment 29 Alex Christensen 2016-03-28 18:35:58 PDT
(In reply to comment #25)
> (In reply to comment #24)
> > Konstantin noticed this is another bug, it should be
> > WebCore_DERIVED_SOURCES, not WebCore_DERIVED_SOURCES, for consistency with
> > WebKit2_DERIVED_SOURCES, which already exists.
> 
> Oops. Try again:
> 
> Konstantin noticed this is another bug, it should be
> WebCore_DERIVED_SOURCES, not WebCore_DERIVEDSOURCES, for consistency with
> WebKit2_DERIVED_SOURCES, which already exists.
Nope, this worked without this change but not with this change.  The macro puts everything in the add_library for WebCore, which defeats the whole purpose of this change.  I'm going to make WebKit2 different.
Comment 31 Gyuyoung Kim 2016-03-28 21:51:22 PDT
(In reply to comment #30)
> http://trac.webkit.org/changeset/198773

r198773 caused EFL port build is broken.
Comment 32 Gyuyoung Kim 2016-03-29 02:15:09 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > http://trac.webkit.org/changeset/198773
> 
> r198773 caused EFL port build is broken.

Alex, I land a build fix for EFL port - http://trac.webkit.org/changeset/198777.
If there is concern on it, please let me know.
Comment 33 Csaba Osztrogonác 2016-03-29 07:03:08 PDT
(In reply to comment #30)
> http://trac.webkit.org/changeset/198772
> http://trac.webkit.org/changeset/198773

It broke the WinCairo build too:
ninja: warning: multiple rules generate Source/WebCore/WebCore_DERIVED_SOURCES/WebCoreDerivedSourcesPrefix.pch. builds involving this target will not be correct; continuing anyway [-w dupbuild=warn]
ninja: error: dependency cycle: Source/WebCore/WebCore_DERIVED_SOURCES/WebCoreDerivedSourcesPrefix.pch -> Source/WebCore/CMakeFiles/WebCore.dir/WebCoreDerivedSourcesPrefix.cpp.obj -> cmake_order_depends_target_WebCore -> lib64/WebCoreDerivedSources.lib -> Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/ColorData.cpp.obj -> Source/WebCore/WebCore_DERIVED_SOURCES/WebCoreDerivedSourcesPrefix.pch
Comment 34 Michael Catanzaro 2016-03-29 07:12:51 PDT
(In reply to comment #32)
> Alex, I land a build fix for EFL port -
> http://trac.webkit.org/changeset/198777.
> If there is concern on it, please let me know.

It's the same issue as in comment #14: putting the source files into both the original WebCore library and the new one defeats the point of adding the new one, so we need to roll that out.

What's the linker error for EFL?

This patch might require a clean build.
Comment 35 Csaba Osztrogonác 2016-03-29 07:21:04 PDT
(In reply to comment #34)
> (In reply to comment #32)
> > Alex, I land a build fix for EFL port -
> > http://trac.webkit.org/changeset/198777.
> > If there is concern on it, please let me know.
> 
> It's the same issue as in comment #14: putting the source files into both
> the original WebCore library and the new one defeats the point of adding the
> new one, so we need to roll that out.
> 
> What's the linker error for EFL?
> 
> This patch might require a clean build.

Clean build didn't help:
https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/8177
Comment 36 Csaba Osztrogonác 2016-03-30 03:13:15 PDT
Just to document ...

The EFL buildfix landed in http://trac.webkit.org/changeset/198777 
broke the Windows build. 

Then Windows fixes landed in 
- http://trac.webkit.org/changeset/198800
- http://trac.webkit.org/changeset/198801
- http://trac.webkit.org/changeset/198804
- http://trac.webkit.org/changeset/198811
which broke the EFL build

And then EFL workaround landed in http://trac.webkit.org/changeset/198830
which isn't correct at all, but at least it builds.

It would be great if you guys will stop landing random unreviewed 
changes and break each others build system again and again. Please
try to cooperate and help each other fixing issues before landing.
Comment 37 Michael Catanzaro 2016-03-30 08:30:13 PDT
Anyway, this is all fixed properly except for EFL, which has multiple hacks in PlatformEfl.cmake now....