[msbuild] Provide the correct value for the operating system for tvOS and watchOS to a few tasks. Fixes #6200. #7226
Conversation
… and watchOS to a few tasks. Fixes xamarin#6200. The problem with xamarin#6200 was that we'd pass -mios-version-min=x.y to the metal tool even for tvOS apps. This fixes it so that now pass -mtvos-version-min. Fixes xamarin#6200.
rolfbjarne
requested review from
chamons,
emaf,
jstedfast,
kzu and
VincentDondain
as code owners
|
Build failure Test results1 tests failed, 88 tests passed.Failed tests
|
|
Build failure Test results1 tests failed, 88 tests passed.Failed tests
|
|
Test failure is unrelated
|
| @@ -82,6 +84,7 @@ public void CheckToolBinDir (string taskName, string binDirToCheck) | |||
| RedirectStandardError = true, | |||
| }; | |||
| psi.EnvironmentVariables.Add ("DEVELOPER_DIR", Configuration.xcode_root); | |||
| psi.EnvironmentVariables.Remove ("XCODE_DEVELOPER_DIR_PATH"); // VSfM sets XCODE_DEVELOPER_DIR_PATH, which confuses the command-line tools if it doesn't match xcode-select, so just unset it. | |||
There was a problem hiding this comment.
that change is not mentioned in the commit message
There was a problem hiding this comment.
Wouldn't that cause problems in VSM? We need to make sure it resets it if needed or maybe tell the VSM team to not set it at all.
There was a problem hiding this comment.
This problem occurs when running tests from within VSfM, and those tests inherit VSfM's environment, and this variable causes trouble when running Apple's command line tools if it doesn't match what xcode-select says - it will never be a problem for VSfM because that environment is not affected by this change.
It's already filed here: #3931
There was a problem hiding this comment.
PR description has been updated.
| Log.LogError ($"Unknown target framework identifier: {TargetFrameworkIdentifier}."); | ||
| return string.Empty; | ||
| } | ||
| } |
There was a problem hiding this comment.
That block is repeated 3 times (not DRY)
Can it be moved into a (new) base type ? or an helper method ?
There was a problem hiding this comment.
Actually it's not repeated 3 times, only 2 (and one version is different from the others). In any case I've moved the duplicated code into a helper method.
There was a problem hiding this comment.
Good point - but now you have to explain why this one is different ;-) and not shared
Curious minds must know :)
There was a problem hiding this comment.
Also, can we use c# 8? Switch expressions!
There was a problem hiding this comment.
@spouliot it's the mac value that's different (osx vs macosx). It was that way before my changes, and I didn't want to change anything, so I kept it that way.
There was a problem hiding this comment.
ah, I thought (jumped) to the default case, i.e. String.Empty vs Exception, and overlooked the mac string :)
| @@ -82,6 +84,7 @@ public void CheckToolBinDir (string taskName, string binDirToCheck) | |||
| RedirectStandardError = true, | |||
| }; | |||
| psi.EnvironmentVariables.Add ("DEVELOPER_DIR", Configuration.xcode_root); | |||
| psi.EnvironmentVariables.Remove ("XCODE_DEVELOPER_DIR_PATH"); // VSfM sets XCODE_DEVELOPER_DIR_PATH, which confuses the command-line tools if it doesn't match xcode-select, so just unset it. | |||
There was a problem hiding this comment.
Wouldn't that cause problems in VSM? We need to make sure it resets it if needed or maybe tell the VSM team to not set it at all.
This comment has been minimized.
This comment has been minimized.
|
Build failure Test results1 tests failed, 88 tests passed.Failed tests
|
|
Test failure is unrelated
|
… and watchOS to a few tasks. Fixes xamarin#6200. (xamarin#7226) The problem with xamarin#6200 was that we'd pass -mios-version-min=x.y to the metal tool even for tvOS apps. This fixes it so that now pass -mtvos-version-min. Fixes xamarin#6200.
#7991) * [msbuild] Provide the correct value for the operating system for tvOS and watchOS to a few tasks. Fixes #6200. (#7226) The problem with #6200 was that we'd pass -mios-version-min=x.y to the metal tool even for tvOS apps. This fixes it so that now pass -mtvos-version-min. Fixes #6200. * [msbuild] Add support for Metal in the simulator. Fixes #7392. (#7983)
The problem with #6200 was that we'd pass -mios-version-min=x.y to the metal
tool even for tvOS apps. This fixes it so that now pass -mtvos-version-min.
Also a minor improvement to some related tests so that their execution is not affected by VSfM's environment.
Fixes #6200.