Bug 169067 - [Mac] WebCore.framework is missing a symlink from Frameworks to Versions/Current/Frameworks when the latter exists
Summary: [Mac] WebCore.framework is missing a symlink from Frameworks to Versions/Curr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-01 20:38 PST by mitz
Modified: 2017-03-03 15:02 PST (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.79 KB, patch)
2017-03-02 10:55 PST, Joseph Pecoraro
mitz: review+
Details | Formatted Diff | Diff
[PATCH] Proposed Patch (2.97 KB, patch)
2017-03-03 13:34 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Patch (2.98 KB, patch)
2017-03-03 13:38 PST, Joseph Pecoraro
mitz: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (961.30 KB, application/zip)
2017-03-03 14:41 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2017-03-01 20:38:36 PST
When libwebrtc installs into WebCore’s Frameworks directory, there should be a symlink from Frameworks at the top level of the framework to Versions/Current/Frameworks. It makes sense for the WebCore target to create the symlink when needed (which, if I remember correctly, is iff WK_USE_OVERRIDE_FRAMEWORKS_DIR is NO).
Comment 1 Joseph Pecoraro 2017-03-01 21:09:26 PST
I see WebKit has a Make Frameworks Symbolic Link build phase that this can mimic.

Should we be making libwebrtc's install path go to WebCore.frameworks/Versions/Current/Frameworks instead of to WebCore.frameworks/Frameworks which will now be a symlink?
Comment 2 mitz 2017-03-01 21:14:02 PST
(In reply to comment #1)
> I see WebKit has a Make Frameworks Symbolic Link build phase that this can
> mimic.
> 
> Should we be making libwebrtc's install path go to
> WebCore.frameworks/Versions/Current/Frameworks instead of to
> WebCore.frameworks/Frameworks which will now be a symlink?

libwebrtc’s own install path is, and should remain, …/WebCore.framework/Versions/A/Frameworks .
Comment 3 Joseph Pecoraro 2017-03-01 21:21:27 PST
Ahh yes, sorry. iOS has shallow bundles, and we would only want to be adding the symlink for non-shallow bundle cases like macOS.
Comment 4 mitz 2017-03-01 21:22:59 PST
(In reply to comment #3)
> Ahh yes, sorry. iOS has shallow bundles, and we would only want to be adding
> the symlink for non-shallow bundle cases like macOS.

That’s why the bug title says [Mac] :|
Comment 5 Joseph Pecoraro 2017-03-02 10:55:19 PST
Created attachment 303219 [details]
[PATCH] Proposed Fix
Comment 6 Joseph Pecoraro 2017-03-02 10:56:01 PST
Comment on attachment 303219 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:29344
> +			shellScript = "if [[ ${PLATFORM_NAME} != \"macosx\" ]]; then\n    exit 0\nfi\n\nif [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == \"YES\" ]]; then\n    exit 0\nfi\n\nln -shf Versions/Current/Frameworks \"$TARGET_BUILD_DIR/WebCore.framework/Frameworks\"\n";

The script is:

    if [[ ${PLATFORM_NAME} != "macosx" ]]; then
        exit 0
    fi

    if [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == "YES" ]]; then
        exit 0
    fi

    ln -shf Versions/Current/Frameworks "$TARGET_BUILD_DIR/WebCore.framework/Frameworks"

This still creates a symlink in engineering Release/Debug builds, but I think thats harmless.
Comment 7 mitz 2017-03-02 12:22:45 PST
Comment on attachment 303219 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:29344
>> +			shellScript = "if [[ ${PLATFORM_NAME} != \"macosx\" ]]; then\n    exit 0\nfi\n\nif [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == \"YES\" ]]; then\n    exit 0\nfi\n\nln -shf Versions/Current/Frameworks \"$TARGET_BUILD_DIR/WebCore.framework/Frameworks\"\n";
> 
> The script is:
> 
>     if [[ ${PLATFORM_NAME} != "macosx" ]]; then
>         exit 0
>     fi
> 
>     if [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == "YES" ]]; then
>         exit 0
>     fi
> 
>     ln -shf Versions/Current/Frameworks "$TARGET_BUILD_DIR/WebCore.framework/Frameworks"
> 
> This still creates a symlink in engineering Release/Debug builds, but I think thats harmless.

Creating the symlink in engineering builds seems fine. But you should not create it when ENABLE_WEB_RTC is empty.
Comment 8 Radar WebKit Bug Importer 2017-03-03 00:58:24 PST
<rdar://problem/30828068>
Comment 9 Joseph Pecoraro 2017-03-03 13:34:45 PST
Created attachment 303335 [details]
[PATCH] Proposed Patch

Since this was a non-obvious review comment to address I'm putting up a new PFR.
Comment 10 Joseph Pecoraro 2017-03-03 13:38:05 PST
Created attachment 303336 [details]
[PATCH] Proposed Patch

Rebaselined.
Comment 11 Joseph Pecoraro 2017-03-03 13:38:40 PST
Comment on attachment 303336 [details]
[PATCH] Proposed Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:29367
> +			shellScript = "if [[ ${PLATFORM_NAME} != \"macosx\" ]]; then\n    exit 0\nfi\n\nif [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == \"YES\" ]]; then\n    exit 0\nfi\n\n# ENABLE_WEB_RTC cannot be relied on because of its conditionals.\nif [[ ${CONFIGURATION} == \"Production\" && ${TARGET_MAC_OS_X_VERSION_MAJOR} != \"101300\" ]]; then\n    exit 0\nfi\n\nln -shf Versions/Current/Frameworks \"$TARGET_BUILD_DIR/WebCore.framework/Frameworks\"\n";

This is now:

    if [[ ${PLATFORM_NAME} != "macosx" ]]; then
        exit 0
    fi

    if [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == "YES" ]]; then
        exit 0
    fi

    # ENABLE_WEB_RTC cannot be relied on because of its conditionals.
    if [[ ${CONFIGURATION} == "Production" && ${TARGET_MAC_OS_X_VERSION_MAJOR} != "101300" ]]; then
        exit 0
    fi

    ln -shf Versions/Current/Frameworks "$TARGET_BUILD_DIR/WebCore.framework/Frameworks"
Comment 12 Build Bot 2017-03-03 14:41:41 PST
Comment on attachment 303336 [details]
[PATCH] Proposed Patch

Attachment 303336 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3231559

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html
Comment 13 Build Bot 2017-03-03 14:41:43 PST
Created attachment 303344 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 mitz 2017-03-03 14:53:25 PST
Comment on attachment 303336 [details]
[PATCH] Proposed Patch

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

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:29367
>> +			shellScript = "if [[ ${PLATFORM_NAME} != \"macosx\" ]]; then\n    exit 0\nfi\n\nif [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == \"YES\" ]]; then\n    exit 0\nfi\n\n# ENABLE_WEB_RTC cannot be relied on because of its conditionals.\nif [[ ${CONFIGURATION} == \"Production\" && ${TARGET_MAC_OS_X_VERSION_MAJOR} != \"101300\" ]]; then\n    exit 0\nfi\n\nln -shf Versions/Current/Frameworks \"$TARGET_BUILD_DIR/WebCore.framework/Frameworks\"\n";
> 
> This is now:
> 
>     if [[ ${PLATFORM_NAME} != "macosx" ]]; then
>         exit 0
>     fi
> 
>     if [[ ${WK_USE_OVERRIDE_FRAMEWORKS_DIR} == "YES" ]]; then
>         exit 0
>     fi
> 
>     # ENABLE_WEB_RTC cannot be relied on because of its conditionals.
>     if [[ ${CONFIGURATION} == "Production" && ${TARGET_MAC_OS_X_VERSION_MAJOR} != "101300" ]]; then
>         exit 0
>     fi
> 
>     ln -shf Versions/Current/Frameworks "$TARGET_BUILD_DIR/WebCore.framework/Frameworks"

Someone might forget to update this when WebCore starts targeting newer macOS versions. Instead, I suggest that you use an inequality like
    ${TARGET_MAC_OS_X_VERSION_MAJOR} < 101300
.
Comment 15 Joseph Pecoraro 2017-03-03 15:02:43 PST
<https://trac.webkit.org/changeset/213392>