Bug 151399 – Fix Ninja build on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Fix Ninja build on Mac

[Alex Christensen]

Created attachment 265766 [details]
Patch

[Alex Christensen]

Created attachment 265770 [details]
Patch

[Alex Christensen]

Created attachment 265771 [details]
Patch

[Chris Dumez]

Broke GTK and EFL?

[Alex Christensen]

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.

[Michael Catanzaro]

(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!

[Alex Christensen]

Created attachment 265794 [details]
Patch

[Michael Catanzaro]

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....

[Michael Catanzaro]

Created attachment 265928 [details]
Patch

[Michael Catanzaro]

I don't know why EFL won't build :(

[Gyuyoung Kim]

(In reply to comment #10)
> I don't know why EFL won't build :(

Now I'm struggling with this break on EFL.

[Gyuyoung Kim]

Created attachment 266079 [details]
Patch for ews

[Gyuyoung Kim]

Alex, I wonder if my fix is fine for mac with cmake-ninja. Could you check it ?

[Michael Catanzaro]

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).

[Gyuyoung Kim]

Created attachment 266111 [details]
Patch for efl-ews

[Gyuyoung Kim]

(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. :(

[Michael Catanzaro]

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.

[Michael Catanzaro]

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).

[Gyuyoung Kim]

Alex, I wonder if you want to land this patch. I still meet a linking error when building Mac port using cmake.

[Alex Christensen]

Yep, I still want to land this patch eventually.  It's a low priority, though.

[Alex Christensen]

Created attachment 274990 [details]
Patch

[Alex Christensen]

Created attachment 275024 [details]
Patch

[Alex Christensen]

Created attachment 275028 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 275068 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/changeset/198766

[Alex Christensen]

(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.

[Alex Christensen]

http://trac.webkit.org/changeset/198772
http://trac.webkit.org/changeset/198773

[Gyuyoung Kim]

(In reply to comment #30)
> http://trac.webkit.org/changeset/198773

r198773 caused EFL port build is broken.

[Gyuyoung Kim]

(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.

[Csaba Osztrogonác]

(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

[Michael Catanzaro]

(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.

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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.

[Michael Catanzaro]

Anyway, this is all fixed properly except for EFL, which has multiple hacks in PlatformEfl.cmake now....