Bug 220951

Created attachment 418373 [details] Patch v1.0

Is there something special in the XCode projects that would make this file get picked up? If you look at the CMake code this file is only included for a JSCOnly build that enabled remote inspector on Apple platforms. https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/PlatformJSCOnly.cmake#L5 My preference would be to remove the files from the SourcesCocoa.txt and use the inspector/remote/SourcesCocoa.txt instead to support use of the Remote Inspector on JSCOnly.

(In reply to Don Olmstead from comment #2) > Is there something special in the XCode projects that would make this file > get picked up? If you look at the CMake code this file is only included for > a JSCOnly build that enabled remote inspector on Apple platforms. > > https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/ > PlatformJSCOnly.cmake#L5 > > My preference would be to remove the files from the SourcesCocoa.txt and use > the inspector/remote/SourcesCocoa.txt instead to support use of the Remote > Inspector on JSCOnly. I wasn't aware that we maintained JSCOnly builds on Apple platforms. Does it actually work? There are no other places where we have >1 SourcesCocoa.txt per project. It seems likely to cause problems to do it differently in one place. Why can't the other Sources.txt in this directory become top-level?

(In reply to BJ Burg from comment #3) > (In reply to Don Olmstead from comment #2) > > Is there something special in the XCode projects that would make this file > > get picked up? If you look at the CMake code this file is only included for > > a JSCOnly build that enabled remote inspector on Apple platforms. > > > > https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/ > > PlatformJSCOnly.cmake#L5 > > > > My preference would be to remove the files from the SourcesCocoa.txt and use > > the inspector/remote/SourcesCocoa.txt instead to support use of the Remote > > Inspector on JSCOnly. > > I wasn't aware that we maintained JSCOnly builds on Apple platforms. Does it > actually work? I'm told by Yusuke that people working on WebKit do build an Apple JSCOnly build. > There are no other places where we have >1 SourcesCocoa.txt per project. It > seems likely to cause problems to do it differently in one place. > > Why can't the other Sources.txt in this directory become top-level? For ports using CMake there's a precedent to include ${PLATFORM}.cmake files which include a Sources${PLATFORM}.txt. So for example the PlayStation and Windows ports include a Curl.cmake. Look in https://github.com/WebKit/WebKit/tree/main/Source/WebCore/platform for more examples of this. There's a similar scenario at the JSC level around the remote inspector. A Socket implementation is used by WinCairo and PlayStation, while a GLib is used by WPE and GTK. The JSCOnly could use any of those but also the Cocoa one for Apple platforms which is why I split that file as well. So is my assumption correct that the xcode just searches for a SourcesCocoa.txt in the directory? If that's the case and had I known I would've removed the files from the root SourcesCocoa.txt in the previous patch. Also on Slack if you want a less async conversation. Saw you pinged me yesterday about this.

Created attachment 418434 [details] Speculative patch

Comment on attachment 418434 [details] Speculative patch View in context: https://bugs.webkit.org/attachment.cgi?id=418434&action=review Talked with Keith Miller to see if there was anything that would cause a SourcesCocoa.txt to get picked up into the build. He pointed me at a few things with the XCode build so here's my attempt to use the inspector/remote/SourcesCocoa.txt within the XCode build. > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:11151 > "$(SRCROOT)/Scripts/generate-unified-sources.sh", > "$(SRCROOT)/Sources.txt", > "$(SRCROOT)/SourcesCocoa.txt", > + "$(SRCROOT)/inspector/remote/SourcesCocoa.txt", Added the dependency here. I don't have an XCode install so I couldn't add inspector/remote/SourcesCocoa.txt to the project where you could edit it. > Source/JavaScriptCore/Scripts/generate-unified-sources.sh:24 > if [ $# -eq 0 ]; then > - echo "Using unified source list files: Sources.txt, SourcesCocoa.txt" > + echo "Using unified source list files: Sources.txt, SourcesCocoa.txt, inspector/remote/SourcesCocoa.txt" > fi > > -/usr/bin/env ruby "${BUILD_SCRIPTS_DIR}/generate-unified-source-bundles.rb" --derived-sources-path "${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore" --source-tree-path "${SRCROOT}" --max-cpp-bundle-count ${UnifiedSourceCppFileCount} --max-obj-c-bundle-count ${UnifiedSourceMmFileCount} Sources.txt SourcesCocoa.txt "${ARGS[@]}" > /dev/null > +/usr/bin/env ruby "${BUILD_SCRIPTS_DIR}/generate-unified-source-bundles.rb" --derived-sources-path "${BUILT_PRODUCTS_DIR}/DerivedSources/JavaScriptCore" --source-tree-path "${SRCROOT}" --max-cpp-bundle-count ${UnifiedSourceCppFileCount} --max-obj-c-bundle-count ${UnifiedSourceMmFileCount} Sources.txt SourcesCocoa.txt inspector/remote/SourcesCocoa.txt "${ARGS[@]}" > /dev/null Added the inspector/remote/SourcesCocoa.txt file here. It was never referenced so I'm really not sure how the original patch would've resulted in the behavior you're seeing.

<rdar://problem/73848263>

Comment on attachment 418434 [details] Speculative patch Don's patch seems to pass Mac EWS, so let's proceed. (Win EWS redness is unrelated layout test failures)

Unable to find any modified ChangeLog in Attachment 418434 [details]

Comment on attachment 418373 [details] Patch v1.0 Clearing flags from my patch, which isn't needed.

Created attachment 419021 [details] Patch v1.1 (w/ Xcode project changes)

Comment on attachment 419021 [details] Patch v1.1 (w/ Xcode project changes) View in context: https://bugs.webkit.org/attachment.cgi?id=419021&action=review r=me with nit > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:4338 > + 99A3273725C9CBEE00DA8CAF /* Cocoa.cmake */ = {isa = PBXFileReference; lastKnownFileType = text; path = Cocoa.cmake; sourceTree = "<group>"; }; You shouldn't need to add the .cmake to the xcode > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:8776 > + 99A3273725C9CBEE00DA8CAF /* Cocoa.cmake */, Ditto

Committed r272222: <https://trac.webkit.org/changeset/272222>