Bug 205353 – Force `yasm` to be built for host OS

• Component: WebRTC
• Status: RESOLVED WONTFIX

[Keith Rollin]

When using `make` to build for the iOS Simulator, yasm would also get built for that platform, due to the specification of SDKROOT=iphonesimulator on the command line. However, yasm is supposed to be built for the current build host so that it can later be used in the build process. Using yasm on the build host even though it was built for iOS Simulator seems to have worked so far, but that may not always be the case. Fix this by forcing yasm to be built for the host as opposed to whatever it specified in SDKROOT on the `make` command-line.

[Radar WebKit Bug Importer]

<rdar://problem/58018752>

[Keith Rollin]

Created attachment 385913 [details]
Patch

[Jonathan Bedard]

Comment on attachment 385913 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Makefile:9
> +OVERRIDE_ARCHS = default

While this is generally the correct way to do this, I'd like to better understand why this is the case.

The only other place we have precedence of doing this right now is ImageDiff, and we only do it for embedded builds (ie, we conditionality it to iOS, watchOS and tvOS), and that's because we need the binary for testing. I would expect that libwebrtc is built for embedded platforms, is it not?

[Tim Horton]

(In reply to Jonathan Bedard from comment #3)
> Comment on attachment 385913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385913&action=review
> 
> > Source/ThirdParty/libwebrtc/Makefile:9
> > +OVERRIDE_ARCHS = default
> 
> While this is generally the correct way to do this, I'd like to better
> understand why this is the case.
> 
> The only other place we have precedence of doing this right now is
> ImageDiff, and we only do it for embedded builds (ie, we conditionality it
> to iOS, watchOS and tvOS), and that's because we need the binary for
> testing. I would expect that libwebrtc is built for embedded platforms, is
> it not?

This is a tool used to generate code that is then subsequently built for simulator. The tool runs on Mac; it's the same kind of problem as ImageDiff.

[Keith Rollin]

...and my change affects all of libwebrtc, when I wanted it to affect only yasm. This needs more fixing.

[Jonathan Bedard]

(In reply to Tim Horton from comment #4)
> (In reply to Jonathan Bedard from comment #3)
> > Comment on attachment 385913 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=385913&action=review
> > 
> > > Source/ThirdParty/libwebrtc/Makefile:9
> > > +OVERRIDE_ARCHS = default
> > 
> > While this is generally the correct way to do this, I'd like to better
> > understand why this is the case.
> > 
> > The only other place we have precedence of doing this right now is
> > ImageDiff, and we only do it for embedded builds (ie, we conditionality it
> > to iOS, watchOS and tvOS), and that's because we need the binary for
> > testing. I would expect that libwebrtc is built for embedded platforms, is
> > it not?
> 
> This is a tool used to generate code that is then subsequently built for
> simulator. The tool runs on Mac; it's the same kind of problem as ImageDiff.

libwebrtc is a tool? That's confusing, I would expect it to be a library based on the name. Should it be included in build archives? Or is it just used in the build process?

I think we should do the same thing we did with ImageDiff:

ifneq (,$(findstring iphone,$(SDKROOT)))
	OVERRIDE_SDKROOT = default
	OVERRIDE_ARCHS = default
endif
ifneq (,$(findstring watch,$(SDKROOT)))
	OVERRIDE_SDKROOT = default
	OVERRIDE_ARCHS = default
endif
ifneq (,$(findstring tv,$(SDKROOT)))
	OVERRIDE_SDKROOT = default
	OVERRIDE_ARCHS = default
endif

that way if the caller is overriding the Mac SDK, we build with the overwridden Mac SDK

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #6)
> ....
> libwebrtc is a tool? That's confusing, I would expect it to be a library
> based on the name. Should it be included in build archives? Or is it just
> used in the build process?

Keith just answered by question
...

[Keith Rollin]

I've got a solution in place. However, it's a little ugly in that it re-invokes xcodebuild in order to build just 'yasm' for macOS.

I'm talking with a Developer Tools guy who thinks that there's a cleaner solution. It works for him but it doesn't work for me. I'm going to try to get his project to see how it differs from libwebrtc's.

[Keith Rollin]

One Developer Tools guy said "Hey, do <this>". I said "I tried that and it didn't work. May I see your project?" To which he said "Hmm, my approach doesn't seem to be working any more...".

Another Developer Tools guy said "Xcode can't build host content simultaneously with device content. You will fight this battle forever and lose." I'm marking this as the selected answer.

A third said "Your scripted build should be able to use a -destination argument instead of overriding the SDKROOT build setting to build for different run destinations (aka operating systems etc.)" The "scripted build" refers to building from the command-line as opposed to from within the IDE. I may look into this -destination-based solution down the road, but right now I'm worried that it might be more trouble than it's worth. For instance, it seems to be workspace/scheme based rather than project-based, as we operate now. Also, some of the destinations we'd want to specify seem hands-off:

% xcodebuild -showdestinations -workspace WebKit.xcworkspace -scheme 'All Source'
...
	Ineligible destinations for the "All Source" scheme:
		{ platform:iOS, id:dvtdevice-DVTiPhonePlaceholder-iphoneos:placeholder, name:Generic iOS Device }
		{ platform:iOS Simulator, id:dvtdevice-DVTiOSDeviceSimulatorPlaceholder-iphonesimulator:placeholder, name:Generic iOS Simulator Device }
...

So I'll test that my current approach (which re-invokes xcodebuild to build yasm) works and upload that for review.

[Keith Rollin]

Created attachment 386167 [details]
Patch

[Jonathan Bedard]

Comment on attachment 386167 [details]
Patch

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

> Source/ThirdParty/libwebrtc/libwebrtc.xcodeproj/project.pbxproj:15944
> +			shellScript = "ARGS=(\n    -project \"${PROJECT_FILE_PATH}\"\n    -target yasm\n    -sdk macosx\n    -arch \"${NATIVE_ARCH_ACTUAL}\"\n    -configuration Release\n    OBJROOT=${PROJECT_TEMP_DIR}/Build_yasm/Intermediates.noindex\n    SYMROOT=${PROJECT_TEMP_DIR}/Build_yasm/Products\n )\n\nxcodebuild \"${ARGS[@]}\"\n\nYASM_PATH=$(xcodebuild \"${ARGS[@]}\" -showBuildSettings | grep -w BUILT_PRODUCTS_DIR | sed -Ee 's/[^=]* = (.*)/\\1/')\nYASM_NAME=$(xcodebuild \"${ARGS[@]}\" -showBuildSettings | grep -w EXECUTABLE_NAME | sed -Ee 's/[^=]* = (.*)/\\1/')\n\nSRC=\"${YASM_PATH}/${YASM_NAME}\"\nDEST=\"${BUILT_PRODUCTS_DIR}/yasm\"\n\n[[ -e \"${SRC}\" ]] || exit 1\n[[ -e \"${DEST}\" ]] && { cmp \"${SRC}\" \"${DEST}\" && exit 0; }\ncp \"${SRC}\" \"${DEST}\" || exit 1\n\nexit 0\n";

I'm pretty confused where this is actually triggered.

I applied the patch and I can't find this script in Xcode.

[Keith Rollin]

I guess I don't know why you can't find it. It's 'a custom build step in the "vpx" target'. If you look at the build steps for vpx, you should see it as the second build step, called "Build yasm for macOS".

Regardless, this is all moot. Youenn is taking a different approach in Bug 205491, where yasm is no longer needed for non-macOS platforms. I'll be closing this bug as NTBF once that one lands.