Bug 220951 – REGRESSION(r269309): [Cocoa] RemoteInspectorCocoa files are being compiled twice

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

Introduced in https://bugs.webkit.org/show_bug.cgi?id=218451

[Blaze Burg]

Created attachment 418373 [details]
Patch v1.0

[Don Olmstead]

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.

[Blaze Burg]

(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?

[Don Olmstead]

(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.

[Don Olmstead]

Created attachment 418434 [details]
Speculative patch

[Don Olmstead]

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.

[Radar WebKit Bug Importer]

<rdar://problem/73848263>

[Blaze Burg]

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)

[EWS]

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

[Blaze Burg]

Comment on attachment 418373 [details]
Patch v1.0

Clearing flags from my patch, which isn't needed.

[Blaze Burg]

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

[Don Olmstead]

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

[Blaze Burg]

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