| Summary: | Add support for new StagedFrameworks layout | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Rollin <krollin> | ||||||||||
| Component: | New Bugs | Assignee: | Keith Rollin <krollin> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ap, commit-queue, eric.carlson, ews-watchlist, jberlin, jeffm, joepeck, keith_miller, mark.lam, mitz, msaboff, saam, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Keith Rollin
2019-03-10 21:14:05 PDT
Created attachment 364451 [details]
Patch
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 (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. Created attachment 364460 [details]
Fix mangled USE_STAGING_INSTALL_PATH.
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. Created attachment 364515 [details]
Also update DYLD_VERSIONED_FRAMEWORK_PATH to include new prefix.
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? (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. (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. (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! Created attachment 364585 [details]
Added #includes of PlatformSupport.xcconfig.
Comment on attachment 364585 [details] Added #includes of PlatformSupport.xcconfig. Clearing flags on attachment: 364585 Committed r242918: <https://trac.webkit.org/changeset/242918> All reviewed patches have been landed. Closing bug. |