Bug 167555

Summary: Check USE(APPLE_INTERNAL_SDK) instead of specific headers when importing from WebKitAdditions
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, jfbastien, mitz, saam, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Wenson Hsieh 2017-01-28 11:52:10 PST
Check USE(APPLE_INTERNAL_SDK) instead of specific headers when importing WebKitAdditions
Comment 1 Wenson Hsieh 2017-01-28 11:56:24 PST
Created attachment 300046 [details]
Patch
Comment 2 Wenson Hsieh 2017-01-28 14:48:44 PST
Comment on attachment 300046 [details]
Patch

Thanks Dan!
Comment 3 WebKit Commit Bot 2017-01-28 15:14:39 PST
Comment on attachment 300046 [details]
Patch

Clearing flags on attachment: 300046

Committed r211342: <http://trac.webkit.org/changeset/211342>
Comment 4 WebKit Commit Bot 2017-01-28 15:14:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 JF Bastien 2017-01-30 13:48:34 PST
This seems to break the cmake build:

FAILED: Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.12.xctoolchain/usr/bin/c++   -DBUILDING_WITH_CMAKE=1 -DBUILDING_WTF -DHAVE_CONFIG_H=1 -I../Source/bmalloc -I../Source/WTF -I../Source/WTF/wtf -I../Source/WTF/wtf/dtoa -I../Source/WTF/wtf/persistence -I../Source/WTF/wtf/text -I../Source/WTF/wtf/text/icu -I../Source/WTF/wtf/threads -I../Source/WTF/wtf/unicode -I../Source/ThirdParty -I. -IDerivedSources -I../Source/WTF/wtf/spi/darwin -I../Source/WTF/icu -std=c++1y -fcolor-diagnostics -Qunused-arguments -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti   -Wall -Wextra -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wundef -Wwrite-strings -Wno-parentheses-equality -fPIC -MD -MT Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -MF Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o.d -o Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -c ../Source/WTF/wtf/Atomics.cpp
In file included from ../Source/WTF/wtf/Atomics.cpp:26:
In file included from ../Source/WTF/config.h:26:
../Source/WTF/wtf/Platform.h:673:10: fatal error: 'WebKitAdditions/AdditionalFeatureDefines.h' file not found
#include <WebKitAdditions/AdditionalFeatureDefines.h>
         ^


Repro with:

    xcrun cmake -Wno-dev .. -G Ninja \
          -DCMAKE_BUILD_TYPE="Release" \
          -DPORT=Mac
    ninja jsc
Comment 6 mitz 2017-01-30 15:48:26 PST
(In reply to comment #5)
> This seems to break the cmake build:
> 
> FAILED: Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o 
> /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.12.xctoolchain/
> usr/bin/c++   -DBUILDING_WITH_CMAKE=1 -DBUILDING_WTF -DHAVE_CONFIG_H=1
> -I../Source/bmalloc -I../Source/WTF -I../Source/WTF/wtf
> -I../Source/WTF/wtf/dtoa -I../Source/WTF/wtf/persistence
> -I../Source/WTF/wtf/text -I../Source/WTF/wtf/text/icu
> -I../Source/WTF/wtf/threads -I../Source/WTF/wtf/unicode
> -I../Source/ThirdParty -I. -IDerivedSources -I../Source/WTF/wtf/spi/darwin
> -I../Source/WTF/icu -std=c++1y -fcolor-diagnostics -Qunused-arguments -O3
> -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti   -Wall -Wextra
> -Wcast-align -Wformat-security -Wmissing-format-attribute -Wpointer-arith
> -Wundef -Wwrite-strings -Wno-parentheses-equality -fPIC -MD -MT
> Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -MF
> Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o.d -o
> Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -c
> ../Source/WTF/wtf/Atomics.cpp
> In file included from ../Source/WTF/wtf/Atomics.cpp:26:
> In file included from ../Source/WTF/config.h:26:
> ../Source/WTF/wtf/Platform.h:673:10: fatal error:
> 'WebKitAdditions/AdditionalFeatureDefines.h' file not found
> #include <WebKitAdditions/AdditionalFeatureDefines.h>
>          ^
> 
> 
> Repro with:
> 
>     xcrun cmake -Wno-dev .. -G Ninja \
>           -DCMAKE_BUILD_TYPE="Release" \
>           -DPORT=Mac
>     ninja jsc

Are you building with the Apple Internal SDK? If so, and if your Apple Internal SDK or build directory doesn’t contain AdditionalFeatureDefines.h, then please use Apple-internal channels to resolve it.

If you are not building with the Apple Internal SDK, then it’s a bug in the cmake build system that USE(APPLE_INTERNAL_SDK) is true when you build, and a cmake-build-system expert would need to fix that.
Comment 7 JF Bastien 2017-01-30 16:57:00 PST
> Are you building with the Apple Internal SDK? If so, and if your Apple
> Internal SDK or build directory doesn’t contain AdditionalFeatureDefines.h,
> then please use Apple-internal channels to resolve it.
> 
> If you are not building with the Apple Internal SDK, then it’s a bug in the
> cmake build system that USE(APPLE_INTERNAL_SDK) is true when you build, and
> a cmake-build-system expert would need to fix that.

Sorry, I had an in-person chat with Wenson and should have sent an update here.

The problem is:

#if PLATFORM(COCOA)
#if defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)
#define USE_APPLE_INTERNAL_SDK 1
#endif
#endif

Some internal headers are found in cmake's /DerivedSources/ForwardingHeaders/WebCore/, including CFPriv.h, but new ones aren't found. I was building jsc only, so some part of the build cmake process is missing a dependency.

Maybe that bug only affects Apple employees using the cmake build. Wenson said he'd look into it.
Comment 8 mitz 2017-01-30 17:53:29 PST
(In reply to comment #7)
> > Are you building with the Apple Internal SDK? If so, and if your Apple
> > Internal SDK or build directory doesn’t contain AdditionalFeatureDefines.h,
> > then please use Apple-internal channels to resolve it.
> > 
> > If you are not building with the Apple Internal SDK, then it’s a bug in the
> > cmake build system that USE(APPLE_INTERNAL_SDK) is true when you build, and
> > a cmake-build-system expert would need to fix that.
> 
> Sorry, I had an in-person chat with Wenson and should have sent an update
> here.
> 
> The problem is:
> 
> #if PLATFORM(COCOA)
> #if defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)
> #define USE_APPLE_INTERNAL_SDK 1
> #endif
> #endif

Yeah, that code isn’t great (I reviewed it!). USE_APPLE_INTERNAL_SDK should be passed by the build system so that it matches up with the USE_INTERNAL_SDK build setting (normally defined in Base.xcconfig).
Comment 9 Daniel Bates 2017-01-31 03:57:55 PST
(In reply to comment #8)
> > [...]
> > Sorry, I had an in-person chat with Wenson and should have sent an update
> > here.
> > 
> > The problem is:
> > 
> > #if PLATFORM(COCOA)
> > #if defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)
> > #define USE_APPLE_INTERNAL_SDK 1
> > #endif
> > #endif
> 
> Yeah, that code isn’t great (I reviewed it!). USE_APPLE_INTERNAL_SDK should
> be passed by the build system so that it matches up with the
> USE_INTERNAL_SDK build setting (normally defined in Base.xcconfig).

The disagreement of the macro define and Xcode build variable is tracked in bug #141947.
Comment 10 Saam Barati 2017-01-31 11:24:06 PST
This also builds "./Tools/Scripts/build-jsc --release" for Apple employees. It would be nice to fix this.
Comment 11 Saam Barati 2017-01-31 11:25:09 PST
(In reply to comment #10)
> This also builds "./Tools/Scripts/build-jsc --release" for Apple employees.
> It would be nice to fix this.

Typo:
"This also builds" => "This also breaks"
Comment 12 Alexey Proskuryakov 2017-02-01 09:44:45 PST
This also caused rdar://problem/30308635