Bug 220951 - REGRESSION(r269309): [Cocoa] RemoteInspectorCocoa files are being compiled twice
Summary: REGRESSION(r269309): [Cocoa] RemoteInspectorCocoa files are being compiled twice
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-25 15:00 PST by BJ Burg
Modified: 2022-03-01 02:25 PST (History)
17 users (show)

See Also:


Attachments
Patch v1.0 (3.46 KB, patch)
2021-01-25 22:21 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Speculative patch (2.57 KB, patch)
2021-01-26 10:02 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch v1.1 (w/ Xcode project changes) (5.87 KB, patch)
2021-02-02 10:08 PST, BJ Burg
don.olmstead: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-01-25 15:00:52 PST
Introduced in https://bugs.webkit.org/show_bug.cgi?id=218451
Comment 1 BJ Burg 2021-01-25 22:21:56 PST
Created attachment 418373 [details]
Patch v1.0
Comment 2 Don Olmstead 2021-01-26 08:20:06 PST
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.
Comment 3 BJ Burg 2021-01-26 08:29:46 PST
(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?
Comment 4 Don Olmstead 2021-01-26 08:57:37 PST
(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.
Comment 5 Don Olmstead 2021-01-26 10:02:03 PST
Created attachment 418434 [details]
Speculative patch
Comment 6 Don Olmstead 2021-01-26 10:05:35 PST
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.
Comment 7 Radar WebKit Bug Importer 2021-02-01 15:01:15 PST
<rdar://problem/73848263>
Comment 8 BJ Burg 2021-02-02 09:50:02 PST
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)
Comment 9 EWS 2021-02-02 09:50:53 PST
Unable to find any modified ChangeLog in Attachment 418434 [details]
Comment 10 BJ Burg 2021-02-02 09:55:33 PST
Comment on attachment 418373 [details]
Patch v1.0

Clearing flags from my patch, which isn't needed.
Comment 11 BJ Burg 2021-02-02 10:08:40 PST
Created attachment 419021 [details]
Patch v1.1 (w/ Xcode project changes)
Comment 12 Don Olmstead 2021-02-02 10:12:23 PST
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
Comment 13 BJ Burg 2021-02-02 10:43:33 PST
Committed r272222: <https://trac.webkit.org/changeset/272222>