[libwebrtc] Install libwebrtc.dylib inside of WebCore.frameworks iOS: /System/Library/PrivateFrameworks/WebCore.framework/Frameworks Mac: /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks WK_OVERRIDE_FRAMEWORKS_DIR: .../WebCore.framework/Versions/A/Frameworks
Created attachment 302714 [details] [PATCH] Proposed Fix
Comment on attachment 302714 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=302714&action=review > Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:62 > +NORMAL_WEBCORE_FRAMEWORKS_DIR[sdk=iphone*] = $(PRODUCTION_FRAMEWORKS_DIR); I’d drop the [sdk=iphone*] here and just make this the default, with the macOS override below. > Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:70 > +NORMAL_PRODUCTION_FRAMEWORKS_DIR[sdk=macosx*] = $(NEXT_ROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks; No need to use NEXT_ROOT here. > Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:72 > +PRODUCTION_FRAMEWORKS_DIR[sdk=iphone*] = $(NORMAL_PRODUCTION_FRAMEWORKS_DIR); The value of PRODUCTION_FRAMEWORKS_DIR is only used in the definition of NORMAL_WEBCORE_FRAMEWORKS_DIR above, and only in the sdk=iphone* case. I’d just substitute $(NORMAL_PRODUCTION_FRAMEWORKS_DIR) in that one definition and remove all definitions of PRODUCTION_FRAMEWORKS_DIR. > Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:3 > +INSTALL_PATH = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Frameworks How does this work when the value of WEBCORE_FRAMEWORKS_DIR computes to $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks? It seems strange that the install path depends on the SDK path. > Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:5 > + I am not sure these install paths make sense as install names in all possible configurations. > Source/WebCore/Configurations/WebCore.xcconfig:44 > +LIBRARY_SEARCH_PATHS = $(inherited) "$(LIBWEBRTC_LIBRARY_DIR)"; Why the quotes? > Source/WebCore/Configurations/WebCore.xcconfig:76 > +LIBWEBRTC_LIBRARY_DIR = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Frameworks > +LIBWEBRTC_LIBRARY_DIR[sdk=macosx*] = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Versions/A/Frameworks I think you can collapse these into a single definition: LIBWEBRTC_LIBRARY_DIR = $(WEBCORE_FRAMEWORKS_DIR)/$(FRAMEWORKS_FOLDER_PATH); > Source/WebKit2/Configurations/BaseTarget.xcconfig:47 > +NORMAL_WEBCORE_FRAMEWORKS_DIR[sdk=iphone*] = $(PRODUCTION_FRAMEWORKS_DIR); I’d drop the [sdk=iphone*] here and just make this the default, with the macOS override below. > Source/WebKit2/Configurations/WebKit.xcconfig:37 > +LIBRARY_SEARCH_PATHS = $(inherited) "$(LIBWEBRTC_LIBRARY_DIR)"; Why the quotes? > Source/WebKit2/Configurations/WebKit.xcconfig:54 > +LIBWEBRTC_LIBRARY_DIR = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Frameworks > +LIBWEBRTC_LIBRARY_DIR[sdk=macosx*] = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Versions/A/Frameworks If you only use WEBCORE_FRAMEWORKS_DIR here, why is it being defined in BaseTarget.xcconfig?
Comment on attachment 302714 [details] [PATCH] Proposed Fix Attachment 302714 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3188663 New failing tests: media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag.html media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag-is-prevented-over-button.html
Created attachment 302730 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 302714 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=302714&action=review >> Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:70 >> +NORMAL_PRODUCTION_FRAMEWORKS_DIR[sdk=macosx*] = $(NEXT_ROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks; > > No need to use NEXT_ROOT here. All of the other WebKit projects have this, so I kept it consistent. Is this something we can remove from all projects then? Perhaps you can describe what NEXT_ROOT is, I can't seem to find any documentation on it. Is it a relic that is no longer needed? >> Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:72 >> +PRODUCTION_FRAMEWORKS_DIR[sdk=iphone*] = $(NORMAL_PRODUCTION_FRAMEWORKS_DIR); > > The value of PRODUCTION_FRAMEWORKS_DIR is only used in the definition of NORMAL_WEBCORE_FRAMEWORKS_DIR above, and only in the sdk=iphone* case. I’d just substitute $(NORMAL_PRODUCTION_FRAMEWORKS_DIR) in that one definition and remove all definitions of PRODUCTION_FRAMEWORKS_DIR. Great catch! >> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:3 >> +INSTALL_PATH = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Frameworks > > How does this work when the value of WEBCORE_FRAMEWORKS_DIR computes to $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks? It seems strange that the install path depends on the SDK path. All of the other WebKit projects have this, so I kept it consistent. I agree it is peculiar, and the SDKROOT prefix of the NORMAL_PRODUCTION_FRAMEWORKS_DIR paths has been around since early upstreaming of iOS WebKit >5 years ago (r110089). It may be a relic, but better to be consistent. >> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:5 >> + > > I am not sure these install paths make sense as install names in all possible configurations. You are correct. And we haven't tested all configurations just yet, only the ones where we intend to enable WebRTC on next. I expect there will be a follow-up to address the more advanced configurations. I'm also discovering that WK_OVERRIDE_FRAMEWORKS_DIR worked different from my expectations. When defined, other frameworks will make their INSTALL_PATH just be WK_USE_OVERRIDE_FRAMEWORKS_DIR. Maybe we should follow suit and treat libwebrtc.dylib like frameworks and use WK_OVERRIDE_FRAMEWORKS_DIR instead of embedding it inside of WebCore.framework. With WK_OVERRIDE_FRAMEWORKS_DIR I expect this currently produces: <override>/WebCore.framework/WebCore <override>/WebCore.framework/Versions/A/Frameworks/libwebrtc.dylib Should this instead be this?: <override>/WebCore.framework/WebCore <override>/libwebrtc.dylib Perhaps you can direct me to someone I can ask questions about the intentions and expectations of WK_OVERRIDE_FRAMEWORKS_DIR. >> Source/WebCore/Configurations/WebCore.xcconfig:44 >> +LIBRARY_SEARCH_PATHS = $(inherited) "$(LIBWEBRTC_LIBRARY_DIR)"; > > Why the quotes? There are many instances of values for *_SEARCH_PATHS with quoted values. Some even explicitly using a WK_QUOTED_* names. Better safe than sorry! >> Source/WebKit2/Configurations/WebKit.xcconfig:54 >> +LIBWEBRTC_LIBRARY_DIR[sdk=macosx*] = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Versions/A/Frameworks > > If you only use WEBCORE_FRAMEWORKS_DIR here, why is it being defined in BaseTarget.xcconfig? A few reasons: - Existing NORMAL_WEBKIT* and WEBKIT_FRAMEWORKS* are defined there so I felt it appropriate to put similarly named *WEBCORE* variables nearby - WEBCORE path information seems generic enough that another target might want to use it in the future. So putting that in a shared location seemed appropriate as well.
Created attachment 302788 [details] [PATCH] Proposed Fix
(In reply to comment #5) > Comment on attachment 302714 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302714&action=review > > >> Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:70 > >> +NORMAL_PRODUCTION_FRAMEWORKS_DIR[sdk=macosx*] = $(NEXT_ROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks; > > > > No need to use NEXT_ROOT here. > > All of the other WebKit projects have this, so I kept it consistent. Consistency doesn’t seem to improve anything here. > Is this something we can remove from all projects then? Yes. Perhaps you can describe > what NEXT_ROOT is, I can't seem to find any documentation on it. Is it a > relic that is no longer needed? NEXT_ROOT is set to the SDK directory for legacy targets or tools of which there are none in libwebrtc. > >> Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:72 > >> +PRODUCTION_FRAMEWORKS_DIR[sdk=iphone*] = $(NORMAL_PRODUCTION_FRAMEWORKS_DIR); > > > > The value of PRODUCTION_FRAMEWORKS_DIR is only used in the definition of NORMAL_WEBCORE_FRAMEWORKS_DIR above, and only in the sdk=iphone* case. I’d just substitute $(NORMAL_PRODUCTION_FRAMEWORKS_DIR) in that one definition and remove all definitions of PRODUCTION_FRAMEWORKS_DIR. > > Great catch! > > >> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:3 > >> +INSTALL_PATH = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Frameworks > > > > How does this work when the value of WEBCORE_FRAMEWORKS_DIR computes to $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks? It seems strange that the install path depends on the SDK path. > > All of the other WebKit projects have this, so I kept it consistent. I agree > it is peculiar, and the SDKROOT prefix of the > NORMAL_PRODUCTION_FRAMEWORKS_DIR paths has been around since early > upstreaming of iOS WebKit >5 years ago (r110089). It may be a relic, but > better to be consistent. You didn’t answer my question about how it works. What are the install path and the install name in that case? Do they depend on where Xcode happens to be installed on the computer used to build the project? > > >> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:5 > >> + > > > > I am not sure these install paths make sense as install names in all possible configurations. > > You are correct. And we haven't tested all configurations just yet, only the > ones where we intend to enable WebRTC on next. I expect there will be a > follow-up to address the more advanced configurations. > > I'm also discovering that WK_OVERRIDE_FRAMEWORKS_DIR worked different from > my expectations. When defined, other frameworks will make their INSTALL_PATH > just be WK_USE_OVERRIDE_FRAMEWORKS_DIR. I think you meant WK_OVERRIDE_FRAMEWORKS_DIR. > Maybe we should follow suit and > treat libwebrtc.dylib like frameworks and use WK_OVERRIDE_FRAMEWORKS_DIR > instead of embedding it inside of WebCore.framework. If you refer to bug 167293 comment 45 you will see that that’s what I proposed. > > With WK_OVERRIDE_FRAMEWORKS_DIR I expect this currently produces: > > <override>/WebCore.framework/WebCore > <override>/WebCore.framework/Versions/A/Frameworks/libwebrtc.dylib > > Should this instead be this?: > > <override>/WebCore.framework/WebCore > <override>/libwebrtc.dylib > > Perhaps you can direct me to someone I can ask questions about the > intentions and expectations of WK_OVERRIDE_FRAMEWORKS_DIR. I introduced WK_OVERRIDE_FRAMEWORKS_DIR so I may be able to answer your questions. > >> Source/WebCore/Configurations/WebCore.xcconfig:44 > >> +LIBRARY_SEARCH_PATHS = $(inherited) "$(LIBWEBRTC_LIBRARY_DIR)"; > > > > Why the quotes? > > There are many instances of values for *_SEARCH_PATHS with quoted values. > Some even explicitly using a WK_QUOTED_* names. Better safe than sorry! What breaks if the quotes aren’t there? > >> Source/WebKit2/Configurations/WebKit.xcconfig:54 > >> +LIBWEBRTC_LIBRARY_DIR[sdk=macosx*] = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Versions/A/Frameworks > > > > If you only use WEBCORE_FRAMEWORKS_DIR here, why is it being defined in BaseTarget.xcconfig? > > A few reasons: > - Existing NORMAL_WEBKIT* and WEBKIT_FRAMEWORKS* are defined there so I felt > it appropriate to put similarly named *WEBCORE* variables nearby > - WEBCORE path information seems generic enough that another target might > want to use it in the future. So putting that in a shared location seemed > appropriate as well.
(In reply to comment #7) > (In reply to comment #5) > > >> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:3 > > >> +INSTALL_PATH = $(WEBCORE_FRAMEWORKS_DIR)/WebCore.framework/Frameworks > > > > > > How does this work when the value of WEBCORE_FRAMEWORKS_DIR computes to $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks? It seems strange that the install path depends on the SDK path. > > > > All of the other WebKit projects have this, so I kept it consistent. I agree > > it is peculiar, and the SDKROOT prefix of the > > NORMAL_PRODUCTION_FRAMEWORKS_DIR paths has been around since early > > upstreaming of iOS WebKit >5 years ago (r110089). It may be a relic, but > > better to be consistent. > > You didn’t answer my question about how it works. What are the install path > and the install name in that case? Do they depend on where Xcode happens to > be installed on the computer used to build the project? In particular, you say that “all of the other WebKit projects have this”, but in reviewing the other projects I see: bmalloc, WTF and ANGLE’s INSTALL_PATH is /usr/local/lib JavaScriptCore’s INSTALL_PATH is $(JAVASCRIPTCORE_FRAMEWORKS_DIR) which is never dependent on SDKROOT WebCore’s INSTALL_PATH is either $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks or, when building for macOS, $(WEBCORE_FRAMEWORKS_DIR). The latter, when building for macOS, is never dependent on SDKROOT. WebKitLegacy’s INSTALL_PATH is either $(SYSTEM_LIBRARY_DIR)/PrivateFrameworks or, when building for macOS, $(WEBKIT_LEGACY_FRAMEWORKS_DIR). The latter, when building for macOS, is never dependent on SDKROOT. WebKit’s INSTALL_PATH is $(WEBKIT_FRAMEWORKS_DIR), which is never dependent on SDKROOT. So it seems like your assertion is incorrect, and your change is the first to introduce a dependency of an install path on the SDK root. I don’t think that makes sense.
Comment on attachment 302788 [details] [PATCH] Proposed Fix Attachment 302788 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3197968 New failing tests: media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag.html media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag-is-prevented-over-button.html
Created attachment 302790 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 302913 [details] [PATCH] Proposed Fix
Comment on attachment 302913 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=302913&action=review Looks much better than earlier iterations. > Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:10 > +DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_YES = @loader_path/../../../; I believe that the use of @loader_path-relative paths may pose a problem in the USE_STAGING_INSTALL_PATH=YES case, where all entitled binaries that link against WebKit do not opt into Library Validation. Also, should this (or its shallow-bundle equivalent) be the install name whenever the WebKit frameworks are relocatable, so in particular in DebugRelease builds? See for example how WebKit2/Configurations/DebugRelease.xcconfig defines WK_RELOCATABLE_FRAMEWORKS unconditionally.
> Also, should this (or its shallow-bundle equivalent) be the install name > whenever the WebKit frameworks are relocatable, so in particular in > DebugRelease builds? See for example how > WebKit2/Configurations/DebugRelease.xcconfig defines > WK_RELOCATABLE_FRAMEWORKS unconditionally. That sounds good to me. Previously the install name didn't make any sense: (I'm actually not sure how this works for me on macOS. My best guess right now is the DYLD_*_PATH triggered by run-safari & friends has some affect) [Build/Release]$ otool -D libwebrtc.dylib libwebrtc.dylib: /libwebrtc.dylib Updating the install name it still works, and seems more suitable if the library is moved: [Build/Release]$ otool -D libwebrtc.dylib libwebrtc.dylib: @loader_path/../../../libwebrtc.dylib I haven't been able to test iOS yet.
Comment on attachment 302913 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=302913&action=review r- because I just noticed a mistake in the definition of DYLIB_INSTALL_NAME_BASE > Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:9 > +DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_NO = $(inherited); This just sets it to the empty string, because $(inherited) has the same meaning as $(DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_NO), and DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_NO is not defined at a lower level.
(In reply to comment #14) > Comment on attachment 302913 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302913&action=review > > r- because I just noticed a mistake in the definition of > DYLIB_INSTALL_NAME_BASE > > > Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:9 > > +DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_NO = $(inherited); > > This just sets it to the empty string, because $(inherited) has the same > meaning as $(DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_NO), and > DYLIB_INSTALL_NAME_BASE_USE_OVERRIDE_FRAMEWORKS_DIR_NO is not defined at a > lower level. This explains the unexpected install name path I was seeing for local builds (/libwebrtc.dylib) and would have been a regression for non-OVERRIDE Production builds which I did not go back and check.
Created attachment 303021 [details] [PATCH] Proposed Fix
> I believe that the use of @loader_path-relative paths may pose a problem in > the USE_STAGING_INSTALL_PATH=YES case, where all entitled binaries that link > against WebKit do not opt into Library Validation. Is this something we can be addressed as a follow-up, or is it critical to address right now? My understanding is that it can be a follow-up, but you have a better understanding of when staging installs are used.
(In reply to comment #17) > > I believe that the use of @loader_path-relative paths may pose a problem in > > the USE_STAGING_INSTALL_PATH=YES case, where all entitled binaries that link > > against WebKit do not opt into Library Validation. > > Is this something we can be addressed as a follow-up, or is it critical to > address right now? My understanding is that it can be a follow-up, but you > have a better understanding of when staging installs are used. I r+ed because we think that the patch doesn’t make things any worse for the USE_STAGING_INSTALL_PATH=YES case.
Comment on attachment 303021 [details] [PATCH] Proposed Fix Clearing flags on attachment: 303021 Committed r213210: <http://trac.webkit.org/changeset/213210>
All reviewed patches have been landed. Closing bug.