Bug 235385 – [XCBuild] Build via the workspace with USE_WORKSPACE=YES

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Williams]

[XCBuild] Build via the workspace with USE_WORKSPACE=YES

[Elliott Williams]

Created attachment 449529 [details]
Patch

[Elliott Williams]

rdar://87489695

[Elliott Williams]

Comment on attachment 449529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449529&action=review

FWIW, there are some known edge cases in our Makefile logic that I'd like to address outside ths patch. For example, WebInspectorUI only builds for Mac _or_ installsrc, and the workspace schemes do not accurately model that logic.

> Makefile.shared:-49
> -ifeq ($(findstring UseNewBuildSystem,$(ARGS)),)
> -	CAN_USE_XCBUILD = $(shell perl -I$(SCRIPTS_PATH) -Mwebkitdirs -e 'print canUseXCBuild()')
> -	ifeq ($(CAN_USE_XCBUILD),1)
> -		# Temporarily disable default use of XCBuild until issues with it are ironed out.
> -		#XCODE_OPTIONS += -UseNewBuildSystem=YES
> -		XCODE_OPTIONS += -UseNewBuildSystem=NO
> -	else
> -		XCODE_OPTIONS += -UseNewBuildSystem=NO
> -	endif
> -endif
> -

Removed because we don't need it. *All* non-workspace builds with use the legacy build system, as they currently do, and this flag's behavior changed in rdar://86872452.

> WebKit.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings:-6
> -	<key>BuildSystemType</key>
> -	<string>Original</string>

This only affects builds started from the workspace, so EWS + WebKit devs should continue to see PBXBuild for the time being.

[Radar WebKit Bug Importer]

<rdar://problem/87798139>

[Alexey Proskuryakov]

Comment on attachment 449529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449529&action=review

> Makefile.shared:153
> +ifeq ($(SKIP_INSTALLSRC),YES)
> +	@true
> +else

I don't understand why this is needed. `make installsrc` is only invoked in very specific circumstances, and when it is, it's really necessary. So it's not clear to me how it would be invoked from the root makefile with workspace.

[Elliott Williams]

(In reply to Alexey Proskuryakov from comment #5)
> > Makefile.shared:153
> > +ifeq ($(SKIP_INSTALLSRC),YES)
> > +	@true
> > +else
> 
> I don't understand why this is needed. `make installsrc` is only invoked in
> very specific circumstances, and when it is, it's really necessary. So it's
> not clear to me how it would be invoked from the root makefile with
> workspace.

Tools/Makefile is the only place that sets this, to match its current `installsrc` rule. If we'd never actually try to installsrc from Tools/, maybe it's safe to delete this?

[Alexey Proskuryakov]

I'm not sure what directory it gets invoked from.

[Elliott Williams]

Created attachment 449545 [details]
Patch

[Elliott Williams]

(In reply to Elliott Williams from comment #6) 
> Tools/Makefile is the only place that sets this, to match its current
> `installsrc` rule. If we'd never actually try to installsrc from Tools/,
> maybe it's safe to delete this?

(In reply to Alexey Proskuryakov from comment #7)
> I'm not sure what directory it gets invoked from.

Turns out installsrc is impossible to invoke from a workspace build, anyways. If you try to, you get:

    xcodebuild: error: Cannot use the installsrc action with -scheme

It wouldn't be logical for a user or bot to run `make installsrc USE_WORKSPACE=YES`, so it seems fine to not handle this case, and let any errant invocation fall through to the xcodebuild error.

[Alexey Proskuryakov]

Comment on attachment 449545 [details]
Patch

r=ews

[EWS]

Committed r288316 (246232@main): <https://commits.webkit.org/246232@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449545 [details].