Bug 195543 – Add support for new StagedFrameworks layout

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Rollin]

When creating the WebKit layout for out-of-band Safari/WebKit updates, use an optional path prefix when called for.

[Keith Rollin]

rdar://problem/47716452

[Keith Rollin]

Created attachment 364451 [details]
Patch

[Jessie Berlin]

Comment on attachment 364451 [details]
Patch

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:168
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH should probably be USE_STAGING_INSTALL_PATH here and throughout the patch.

> Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:110
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH

> Source/WTF/Configurations/Base.xcconfig:-115
> -

Why was this removed?

> Source/WebCore/Configurations/WebCore.xcconfig:174
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH

> Source/WebCore/PAL/Configurations/PAL.xcconfig:65
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH

> Source/WebInspectorUI/Configurations/Base.xcconfig:99
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH

> Source/WebInspectorUI/Configurations/WebKitTargetConditionals.xcconfig:1
> +// Copyright (C) 2018 Apple Inc. All rights reserved.

I think you mean 2019.

> Source/WebKit/Configurations/BaseTarget.xcconfig:105
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH

> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:136
> +WK_INSTALL_PATH_PREFIX = $(WK_INSTALL_PATH_PREFIX_DEPLOYMENT_$(DEPLOYMENT_LOCATION)$(WK_MACOS_1015)_USE_STAGING_INSTALL_PATH_$(USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH));

USE_USE_STAGING_INSTALL_PATH_INSTALL_PATH

[Keith Rollin]

(In reply to Jessie Berlin from comment #3)

> > Source/WTF/Configurations/Base.xcconfig:-115
> > -
> 
> Why was this removed?

You mean the removal of JAVASCRIPTCORE_FRAMEWORKS_DIR? In my search for path variables that needed to be updated, I found that this was unused.

[Keith Rollin]

Created attachment 364460 [details]
Fix mangled USE_STAGING_INSTALL_PATH.

[Jessie Berlin]

Comment on attachment 364460 [details]
Fix mangled USE_STAGING_INSTALL_PATH.

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

I think you are also going to need to change all the lines that look like this:

OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH = $(OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH_$(USE_STAGING_INSTALL_PATH));
OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH_YES = -Wl,-dyld_env,DYLD_VERSIONED_FRAMEWORK_PATH=/System/Library/StagedFrameworks/Safari;

> Source/WTF/Configurations/Base.xcconfig:-114
> -JAVASCRIPTCORE_FRAMEWORKS_DIR = $(SYSTEM_LIBRARY_DIR)/Frameworks;

It would be nice to call this out as opportunistic refactoring in the ChangeLog for future confused people like me.

[Keith Rollin]

Created attachment 364515 [details]
Also update DYLD_VERSIONED_FRAMEWORK_PATH to include new prefix.

[Jessie Berlin]

Comment on attachment 364515 [details]
Also update DYLD_VERSIONED_FRAMEWORK_PATH to include new prefix.

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:169
> +WK_INSTALL_PATH_PREFIX_DEPLOYMENT_YES_MACOS_SINCE_1015_USE_STAGING_INSTALL_PATH_YES = $(PLATFORM_OOB_SYSTEM_CONTENT_DIR);

After sleeping on this overnight, I realized you probably don't need the WK_MACOS_1015 check at all since PLATFORM_OOB_SYSTEM_CONTENT_DIR won't be defined in older SDKs. However, it is fine to keep it here if you think it makes it more understandable.

> Source/WebInspectorUI/Configurations/Base.xcconfig:-81
> -OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH = -Wl,-dyld_env -Wl,DYLD_VERSIONED_FRAMEWORK_PATH=/System/Library/StagedFrameworks/Safari;

I am not sure that removing this is right. It seems like we might have an error here that this isn't used by an OTHER_LDFLAGS somewhere in the WebInspectorUI framework configs. Otherwise, how will dyld know to look in staged frameworks for a newer WebInspectorUI.framework instead of just using the system version?

[Keith Rollin]

(In reply to Jessie Berlin from comment #8)
> Comment on attachment 364515 [details]
> 
> > Source/WebInspectorUI/Configurations/Base.xcconfig:-81
> > -OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH = -Wl,-dyld_env -Wl,DYLD_VERSIONED_FRAMEWORK_PATH=/System/Library/StagedFrameworks/Safari;
> 
> I am not sure that removing this is right. It seems like we might have an
> error here that this isn't used by an OTHER_LDFLAGS somewhere in the
> WebInspectorUI framework configs. Otherwise, how will dyld know to look in
> staged frameworks for a newer WebInspectorUI.framework instead of just using
> the system version?

OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH is not referenced anywhere in the project. And checking the output of `otool -l WebInspectorUI` shows that there is no command to set up the dyld environment with DYLD_VERSIONED_FRAMEWORK_PATH (whereas I did find such a command in the WebKit XPC process binaries). So I'm pretty sure it's not used. That said, I have the same question you do. I don't know how WebInspectorUI is used. Perhaps the dyld environment environment comes from a host such as Safari.

[Keith Rollin]

(In reply to Keith Rollin from comment #9)
> (In reply to Jessie Berlin from comment #8)
> > Comment on attachment 364515 [details]
> > 
> > > Source/WebInspectorUI/Configurations/Base.xcconfig:-81
> > > -OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH = -Wl,-dyld_env -Wl,DYLD_VERSIONED_FRAMEWORK_PATH=/System/Library/StagedFrameworks/Safari;
> > 
> > I am not sure that removing this is right. It seems like we might have an
> > error here that this isn't used by an OTHER_LDFLAGS somewhere in the
> > WebInspectorUI framework configs. Otherwise, how will dyld know to look in
> > staged frameworks for a newer WebInspectorUI.framework instead of just using
> > the system version?
> 
> OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH is not referenced anywhere in the
> project. And checking the output of `otool -l WebInspectorUI` shows that
> there is no command to set up the dyld environment with
> DYLD_VERSIONED_FRAMEWORK_PATH (whereas I did find such a command in the
> WebKit XPC process binaries). So I'm pretty sure it's not used. That said, I
> have the same question you do. I don't know how WebInspectorUI is used.
> Perhaps the dyld environment environment comes from a host such as Safari.

I forgot to add, checking the `ld` statement doesn't show the inclusion of -Wl,-dyld_env -Wl,DYLD_...

(In reply to Jessie Berlin from comment #8)
> Comment on attachment 364515 [details]
> Also update DYLD_VERSIONED_FRAMEWORK_PATH to include new prefix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364515&action=review
> 
> > Source/JavaScriptCore/Configurations/Base.xcconfig:169
> > +WK_INSTALL_PATH_PREFIX_DEPLOYMENT_YES_MACOS_SINCE_1015_USE_STAGING_INSTALL_PATH_YES = $(PLATFORM_OOB_SYSTEM_CONTENT_DIR);
> 
> After sleeping on this overnight, I realized you probably don't need the
> WK_MACOS_1015 check at all since PLATFORM_OOB_SYSTEM_CONTENT_DIR won't be
> defined in older SDKs. However, it is fine to keep it here if you think it
> makes it more understandable.
> 
> > Source/WebInspectorUI/Configurations/Base.xcconfig:-81
> > -OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH = -Wl,-dyld_env -Wl,DYLD_VERSIONED_FRAMEWORK_PATH=/System/Library/StagedFrameworks/Safari;
> 
> I am not sure that removing this is right. It seems like we might have an
> error here that this isn't used by an OTHER_LDFLAGS somewhere in the
> WebInspectorUI framework configs. Otherwise, how will dyld know to look in
> staged frameworks for a newer WebInspectorUI.framework instead of just using
> the system version?

LC_DYLD_ENVIRONMENT only makes sense for executables, so if a target doesn’t produce an executable, there’s no reason for it to have -dyld_env in its linker flags.

[Jessie Berlin]

(In reply to mitz from comment #11)
> (In reply to Jessie Berlin from comment #8)
> > Comment on attachment 364515 [details]
> > Also update DYLD_VERSIONED_FRAMEWORK_PATH to include new prefix.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=364515&action=review
> > 
> > > Source/JavaScriptCore/Configurations/Base.xcconfig:169
> > > +WK_INSTALL_PATH_PREFIX_DEPLOYMENT_YES_MACOS_SINCE_1015_USE_STAGING_INSTALL_PATH_YES = $(PLATFORM_OOB_SYSTEM_CONTENT_DIR);
> > 
> > After sleeping on this overnight, I realized you probably don't need the
> > WK_MACOS_1015 check at all since PLATFORM_OOB_SYSTEM_CONTENT_DIR won't be
> > defined in older SDKs. However, it is fine to keep it here if you think it
> > makes it more understandable.
> > 
> > > Source/WebInspectorUI/Configurations/Base.xcconfig:-81
> > > -OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH = -Wl,-dyld_env -Wl,DYLD_VERSIONED_FRAMEWORK_PATH=/System/Library/StagedFrameworks/Safari;
> > 
> > I am not sure that removing this is right. It seems like we might have an
> > error here that this isn't used by an OTHER_LDFLAGS somewhere in the
> > WebInspectorUI framework configs. Otherwise, how will dyld know to look in
> > staged frameworks for a newer WebInspectorUI.framework instead of just using
> > the system version?
> 
> LC_DYLD_ENVIRONMENT only makes sense for executables, so if a target doesn’t
> produce an executable, there’s no reason for it to have -dyld_env in its
> linker flags.

Ah, that makes sense. Thanks Dan!

[Keith Rollin]

Created attachment 364585 [details]
Added #includes of PlatformSupport.xcconfig.

[WebKit Commit Bot]

Comment on attachment 364585 [details]
Added #includes of PlatformSupport.xcconfig.

Clearing flags on attachment: 364585

Committed r242918: <https://trac.webkit.org/changeset/242918>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.