Bug 139463 – Switch from using PLATFORM_NAME to SDK selectors in WebCore, WebInspectorUI, WebKit, WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[David Kilzer (:ddkilzer)]

Switch from using PLATFORM_NAME to SDK selectors in the WebCore, WebInspectorUI, WebKit and WebKit2 projects.

See also:
Bug 138813: FeatureDefines.xcconfig: Switch from using PLATFORM_NAME to SDK selectors
Bug 139212: Switch from using PLATFORM_NAME to SDK selectors in ANGLE, bmalloc, gtest, JavaScriptCore, WTF

[David Kilzer (:ddkilzer)]

Created attachment 243081 [details]
Patch v1 (Testing build; DO NOT REVIEW YET)

[David Kilzer (:ddkilzer)]

Comment on attachment 243081 [details]
Patch v1 (Testing build; DO NOT REVIEW YET)

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

> Source/WebCore/Configurations/WebCore.xcconfig:58
> +INSTALL_PATH_ios = $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks;
> +INSTALL_PATH[sdk=iphoneos*] = $(INSTALL_PATH_ios);
> +INSTALL_PATH_ACTUAL[sdk=iphonesimulator*] = $(INSTALL_PATH_ios);
> +INSTALL_PATH[sdk=macosx*] = $(WEBCORE_FRAMEWORKS_DIR);

FYI, the INSTALL_PATH settings below in WebCoreTestSupport.xcconfig do NOT override the above settings (see next comment for the reason):

INSTALL_PATH = $(INSTALL_PATH_$(CONFIGURATION));
INSTALL_PATH_Production[sdk=iphonesimulator*] = $(INDIGO_INSTALL_PATH_PREFIX)/usr/local/lib;
INSTALL_PATH_Production = /usr/local/lib;

> Source/WebCore/Configurations/WebCore.xcconfig:68
> +OTHER_LDFLAGS_BASE_ios = $(OTHER_LDFLAGS_BASE) -framework CFNetwork -framework CoreGraphics -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework OpenGLES -lMobileGestalt;
> +OTHER_LDFLAGS[sdk=iphoneos*] = $(inherited) $(OTHER_LDFLAGS_BASE_ios) -framework IOSurface;
> +OTHER_LDFLAGS[sdk=iphonesimulator*] = $(inherited) $(OTHER_LDFLAGS_BASE_ios);
> +OTHER_LDFLAGS[sdk=macosx*] = $(inherited) $(OTHER_LDFLAGS_BASE) -sub_library libobjc -umbrella WebKit -allowable_client WebCoreTestSupport -allowable_client WebKit2 -allowable_client WebKitLegacy -framework ApplicationServices -framework AudioUnit -framework Carbon -framework Cocoa -framework IOSurface -framework OpenGL;

This change causes this error when building DumpRenderTree:

ld: cannot link directly with /Volumes/Data/Build/Release/libWebCoreTestSupport.dylib.  Link against the umbrella framework 'WebKit.framework' instead. for architecture x86_64

Here's the reason:  WebCoreTestSupport.xcconfig has OTHER_LDFLAGS defined thusly:

OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS);

However, because the [sdk=] selectors are *more specific* than the variable without an [sdk=] selector, they take precedence.

Pain.

[David Kilzer (:ddkilzer)]

Created attachment 243230 [details]
Patch v2 (Testing build; DO NOT REVIEW unless is passes)

[David Kilzer (:ddkilzer)]

Created attachment 243242 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 243242 [details]
Patch v3

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

> Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:38
> +INSTALL_PATH_ACTUAL[sdk=iphonesimulator*] = $(INSTALL_PATH_ACTUAL_$(CONFIGURATION));
> +INSTALL_PATH_ACTUAL_Production[sdk=iphonesimulator*] = $(INSTALL_PATH_$(CONFIGURATION));

This could be simplified to:

INSTALL_PATH_ACTUAL[sdk=iphonesimulator*] = $(INSTALL_PATH_$(CONFIGURATION));

[Darin Adler]

Comment on attachment 243242 [details]
Patch v3

I think Mark Rowe should review. Looks great to me.

[David Kilzer (:ddkilzer)]

Created attachment 243295 [details]
Patch v4

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Created attachment 243295 [details]
> Patch v4

This patch makes the following changes over "Patch v3":

- Simplifies INSTALL_PATH_ACTUAL in WebCoreTestSupport.xcconfig per Comment #5.
- Sorts INSTALL_PATH_ACTUAL and INSTALL_PATH_ios in WebKitLegacy.xcconfig and WebKit2.xcconfig.
- Fixes definition of OTHER_LDFLAGS in PluginProcessShim.xcconfig to use OTHER_LDFLAGS_PLATFORM pattern.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (In reply to comment #7)
> > Created attachment 243295 [details]
> > Patch v4
> 
> This patch makes the following changes over "Patch v3":
> 
> - Simplifies INSTALL_PATH_ACTUAL in WebCoreTestSupport.xcconfig per Comment
> #5.
> - Sorts INSTALL_PATH_ACTUAL and INSTALL_PATH_ios in WebKitLegacy.xcconfig
> and WebKit2.xcconfig.
> - Fixes definition of OTHER_LDFLAGS in PluginProcessShim.xcconfig to use
> OTHER_LDFLAGS_PLATFORM pattern.

I just installed the patchutils package and fixed the path to interdiff:

https://bugs.webkit.org/attachment.cgi?oldid=243242&action=interdiff&newid=243295&headers=1

[Mark Rowe (bdash)]

Comment on attachment 243295 [details]
Patch v4

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

> Source/WebCore/Configurations/Base.xcconfig:109
>  WEBCORE_SQLITE3_HEADER_SEARCH_PATHS = $(NEXT_ROOT)/usr/local/include/WebCoreSQLite3;
> -SQLITE3_HEADER_SEARCH_PATHS = $(SQLITE3_HEADER_SEARCH_PATHS_$(PLATFORM_NAME));
> -SQLITE3_HEADER_SEARCH_PATHS_macosx = $(SQLITE3_HEADER_SEARCH_PATHS_macosx_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> +SQLITE3_HEADER_SEARCH_PATHS = $(SQLITE3_HEADER_SEARCH_PATHS_$(PLATFORM_NAME)_$(TARGET_MAC_OS_X_VERSION_MAJOR));

I don't think either of these settings is used for anything at this point.

> Source/WebCore/Configurations/Base.xcconfig:118
> +TOOLCHAINS = $(TOOLCHAINS_$(PLATFORM_NAME)_$(MAC_OS_X_VERSION_MAJOR));

Is there a reason to do the OS X case in this fashion rather than using TOOLCHAINS[sdk=macosx*] = $(TOOLCHAINS_macosx_$(MAC_OS_X_VERSION_MAJOR))?

> Source/WebCore/Configurations/WebCore.xcconfig:36
> +EXPORTED_SYMBOLS_FILE_i386[sdk=iphonesimulator*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.LP64.i386.exp;
> +EXPORTED_SYMBOLS_FILE_i386[sdk=macosx*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.exp;
> +EXPORTED_SYMBOLS_FILE_x86_64[sdk=iphonesimulator*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.LP64.x86_64.exp;
> +EXPORTED_SYMBOLS_FILE_x86_64[sdk=macosx*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.LP64.exp;

I think we'll be able to update this to use [arch=foo] in the future rather than the current nested expansions.

> Source/WebCore/Configurations/WebCore.xcconfig:43
> +FRAMEWORK_SEARCH_PATHS[sdk=iphone*] = $(FRAMEWORK_SEARCH_PATHS_ios_$(CONFIGURATION));
> +FRAMEWORK_SEARCH_PATHS_ios_Debug = $(BUILT_PRODUCTS_DIR) $(PRODUCTION_FRAMEWORKS_DIR);
> +FRAMEWORK_SEARCH_PATHS_ios_Release = $(FRAMEWORK_SEARCH_PATHS_ios_Debug);
> +FRAMEWORK_SEARCH_PATHS_ios_Production = $(PRODUCTION_FRAMEWORKS_DIR);
> +FRAMEWORK_SEARCH_PATHS[sdk=macosx*] = $(STAGED_FRAMEWORKS_SEARCH_PATH) $(FRAMEWORK_SEARCH_PATHS);

And this to use [config=Foo].

> Source/WebCore/Configurations/WebCore.xcconfig:62
> +DYLIB_INSTALL_NAME_BASE = $(DYLIB_INSTALL_NAME_BASE_PLATFORM);
> +DYLIB_INSTALL_NAME_BASE_PLATFORM[sdk=iphoneos*] = $(DYLIB_INSTALL_NAME_BASE_ios);
> +DYLIB_INSTALL_NAME_BASE_PLATFORM[sdk=iphonesimulator*] =  $(SDKROOT)$(DYLIB_INSTALL_NAME_BASE_ios);
> +DYLIB_INSTALL_NAME_BASE_PLATFORM[sdk=macosx*] = $(NORMAL_WEBCORE_FRAMEWORKS_DIR);

Why is the indirection through DYLIB_INSTALL_NAME_BASE_PLATFORM necessary? Is it because DYLIB_INSTALL_NAME_BASE is set at the same level by WebCoreTestSupport.xcconfig after it includes this file?

> Source/WebInspectorUI/Configurations/DebugRelease.xcconfig:7
> +MACOSX_DEPLOYMENT_TARGET = $(MACOSX_DEPLOYMENT_TARGET_$(PLATFORM_NAME)_$(TARGET_MAC_OS_X_VERSION_MAJOR));

Why is this not using [sdk=macosx*]?

> Source/WebInspectorUI/Configurations/DebugRelease.xcconfig:23
> +SDKROOT = $(SDKROOT_$(PLATFORM_NAME)_$(USE_INTERNAL_SDK));

Ditto.

> Source/WebInspectorUI/Configurations/Version.xcconfig:10
> +SYSTEM_VERSION_PREFIX = $(SYSTEM_VERSION_PREFIX_$(PLATFORM_NAME)_$(TARGET_MAC_OS_X_VERSION_MAJOR));

Ditto.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> Comment on attachment 243295 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243295&action=review
> 
> > Source/WebCore/Configurations/Base.xcconfig:109
> >  WEBCORE_SQLITE3_HEADER_SEARCH_PATHS = $(NEXT_ROOT)/usr/local/include/WebCoreSQLite3;
> > -SQLITE3_HEADER_SEARCH_PATHS = $(SQLITE3_HEADER_SEARCH_PATHS_$(PLATFORM_NAME));
> > -SQLITE3_HEADER_SEARCH_PATHS_macosx = $(SQLITE3_HEADER_SEARCH_PATHS_macosx_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> > +SQLITE3_HEADER_SEARCH_PATHS = $(SQLITE3_HEADER_SEARCH_PATHS_$(PLATFORM_NAME)_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> 
> I don't think either of these settings is used for anything at this point.

Will be removed in Patch v5.

> > Source/WebCore/Configurations/Base.xcconfig:118
> > +TOOLCHAINS = $(TOOLCHAINS_$(PLATFORM_NAME)_$(MAC_OS_X_VERSION_MAJOR));
> 
> Is there a reason to do the OS X case in this fashion rather than using
> TOOLCHAINS[sdk=macosx*] = $(TOOLCHAINS_macosx_$(MAC_OS_X_VERSION_MAJOR))?

The line above will trigger the Xcode 5.1.1 bug on Mac OS X 10.8.

> > Source/WebCore/Configurations/WebCore.xcconfig:36
> > +EXPORTED_SYMBOLS_FILE_i386[sdk=iphonesimulator*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.LP64.i386.exp;
> > +EXPORTED_SYMBOLS_FILE_i386[sdk=macosx*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.exp;
> > +EXPORTED_SYMBOLS_FILE_x86_64[sdk=iphonesimulator*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.LP64.x86_64.exp;
> > +EXPORTED_SYMBOLS_FILE_x86_64[sdk=macosx*] = $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore/WebCore.LP64.exp;
> 
> I think we'll be able to update this to use [arch=foo] in the future rather
> than the current nested expansions.

Agreed, but I don't want to change too much more with this patch.  It's already pretty big, and I'd rather isolate that change to a separate commit.

> > Source/WebCore/Configurations/WebCore.xcconfig:43
> > +FRAMEWORK_SEARCH_PATHS[sdk=iphone*] = $(FRAMEWORK_SEARCH_PATHS_ios_$(CONFIGURATION));
> > +FRAMEWORK_SEARCH_PATHS_ios_Debug = $(BUILT_PRODUCTS_DIR) $(PRODUCTION_FRAMEWORKS_DIR);
> > +FRAMEWORK_SEARCH_PATHS_ios_Release = $(FRAMEWORK_SEARCH_PATHS_ios_Debug);
> > +FRAMEWORK_SEARCH_PATHS_ios_Production = $(PRODUCTION_FRAMEWORKS_DIR);
> > +FRAMEWORK_SEARCH_PATHS[sdk=macosx*] = $(STAGED_FRAMEWORKS_SEARCH_PATH) $(FRAMEWORK_SEARCH_PATHS);
> 
> And this to use [config=Foo].

Patch v5 will use [config=Production] here.

> > Source/WebCore/Configurations/WebCore.xcconfig:62
> > +DYLIB_INSTALL_NAME_BASE = $(DYLIB_INSTALL_NAME_BASE_PLATFORM);
> > +DYLIB_INSTALL_NAME_BASE_PLATFORM[sdk=iphoneos*] = $(DYLIB_INSTALL_NAME_BASE_ios);
> > +DYLIB_INSTALL_NAME_BASE_PLATFORM[sdk=iphonesimulator*] =  $(SDKROOT)$(DYLIB_INSTALL_NAME_BASE_ios);
> > +DYLIB_INSTALL_NAME_BASE_PLATFORM[sdk=macosx*] = $(NORMAL_WEBCORE_FRAMEWORKS_DIR);
> 
> Why is the indirection through DYLIB_INSTALL_NAME_BASE_PLATFORM necessary?
> Is it because DYLIB_INSTALL_NAME_BASE is set at the same level by
> WebCoreTestSupport.xcconfig after it includes this file?

This is so DYLIB_INSTALL_NAME_BASE in WebCoreTestSupport.xcconfig will properly override these settings.  Using the DYLIB_INSTALL_NAME_BASE[sdk=] would be more specific than what WebCoreTestSupport.xcconfig has, so I'd then have to change DYLIB_INSTALL_NAME_BASE in WebCoreTestSupport.xcconfig to work around this change.

Also, I'm not sure whether [sdk=] is more or less specific than [config=], as I could change DYLIB_INSTALL_NAME_BASE in WebCoreTestSupport.xcconfig to use [config=] and WebCore.xcconfig to use [sdk=], but I honestly don't know which one would take precedence.

> > Source/WebInspectorUI/Configurations/DebugRelease.xcconfig:7
> > +MACOSX_DEPLOYMENT_TARGET = $(MACOSX_DEPLOYMENT_TARGET_$(PLATFORM_NAME)_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> 
> Why is this not using [sdk=macosx*]?

Workaround for Xcode 5.1.1 bug.

> > Source/WebInspectorUI/Configurations/DebugRelease.xcconfig:23
> > +SDKROOT = $(SDKROOT_$(PLATFORM_NAME)_$(USE_INTERNAL_SDK));
> 
> Ditto.

Workaround for Xcode 5.1.1 bug.

> > Source/WebInspectorUI/Configurations/Version.xcconfig:10
> > +SYSTEM_VERSION_PREFIX = $(SYSTEM_VERSION_PREFIX_$(PLATFORM_NAME)_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> 
> Ditto.

Workaround for Xcode 5.1.1 bug.

[David Kilzer (:ddkilzer)]

Created attachment 243459 [details]
Patch v5

[David Kilzer (:ddkilzer)]

Created attachment 243519 [details]
Patch v6 (applies to trunk for EWS testing)

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Created attachment 243519 [details]
> Patch v6 (applies to trunk for EWS testing)

And...UMBRELLA_FRAMEWORKS_DIR is not being set properly.  Here's the new *.xcconfig file:

UMBRELLA_FRAMEWORKS_DIR[config=Debug] = $(UMBRELLA_FRAMEWORKS_DIR_engineering);
UMBRELLA_FRAMEWORKS_DIR[config=Release] = $(UMBRELLA_FRAMEWORKS_DIR_engineering);
UMBRELLA_FRAMEWORKS_DIR[config=Production][sdk=iphone*] = $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks;
UMBRELLA_FRAMEWORKS_DIR[config=Production][sdk=macosx*] = $(SDKROOT)$(UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_$(USE_STAGING_INSTALL_PATH));
UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_ = $(UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_NO);
UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_NO = $(NEXT_ROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/WebKit.framework/Versions/A/Frameworks;
UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_YES = $(NEXT_ROOT)$(SYSTEM_LIBRARY_DIR)/StagedFrameworks/Safari;
UMBRELLA_FRAMEWORKS_DIR_engineering = $(BUILT_PRODUCTS_DIR);

However, only these variables are set in a Release build:

    setenv UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_ /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks
    setenv UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_NO /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks
    setenv UMBRELLA_FRAMEWORKS_DIR_Production_macosx_USE_STAGING_INSTALL_PATH_YES /System/Library/StagedFrameworks/Safari
    setenv UMBRELLA_FRAMEWORKS_DIR_engineering /Volumes/Data/EWS/WebKit/WebKitBuild/Release

I was expecting Debug/Release builds to use [config=Debug] and [config=Release] since they were the most specific and still matched.  But maybe the [config=Production][sdk=macosx*] variable is most specific, doesn't match, and so the variable isn't set?  :|

Let's try switching it back in Patch v7.

[David Kilzer (:ddkilzer)]

Created attachment 243524 [details]
Patch v7 (try to fix UMBRELLA_FRAMEWORKS_DIR in BaseTarget.xcconfig)

[David Kilzer (:ddkilzer)]

Committed r177574: <http://trac.webkit.org/changeset/177574>

[David Kilzer (:ddkilzer)]

Comment on attachment 243524 [details]
Patch v7 (try to fix UMBRELLA_FRAMEWORKS_DIR in BaseTarget.xcconfig)

Clearing review flag.  I landed "Patch v7" since it built on both Mac and iOS.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 139821

[David Kilzer (:ddkilzer)]

Created attachment 243574 [details]
Patch v8 (remove all uses of [config=])

[David Kilzer (:ddkilzer)]

Committed r177621: <http://trac.webkit.org/changeset/177621>

[David Kilzer (:ddkilzer)]

Comment on attachment 243574 [details]
Patch v8 (remove all uses of [config=])

Patch v8 landed as r177621.  Clearing review flag.