Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[msbuild] Provide the correct value for the operating system for tvOS and watchOS to a few tasks. Fixes #6200. #7226

Merged
merged 5 commits into from
Oct 16, 2019
Merged

[msbuild] Provide the correct value for the operating system for tvOS and watchOS to a few tasks. Fixes #6200. #7226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce33c84
[msbuild] Provide the correct value for the operating system for tvOS…
rolfbjarne Oct 14, 2019
11a3b8c
[tests] Fix mtouch build and fix a minor annoyance as well.
rolfbjarne Oct 14, 2019
8982a4c
Remove debug spew.
rolfbjarne Oct 15, 2019
ab00a42
Avoid some code duplication.
rolfbjarne Oct 15, 2019
e5c2a12
Fix build/typo.
rolfbjarne Oct 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions msbuild/Xamarin.Mac.Tasks/Tasks/CompileSceneKitAssets.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,5 @@ namespace Xamarin.Mac.Tasks
{
public class CompileSceneKitAssets : CompileSceneKitAssetsTaskBase
{
protected override string OperatingSystem {
get { return "osx"; }
}
}
}
4 changes: 0 additions & 4 deletions msbuild/Xamarin.Mac.Tasks/Tasks/Metal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@ namespace Xamarin.Mac.Tasks
{
public class Metal : MetalTaskBase
{
protected override string OperatingSystem {
get { return "macosx"; }
}

#if !MTOUCH_TESTS
protected override string MinimumDeploymentTargetKey {
get { return ManifestKeys.LSMinimumSystemVersion; }
Expand Down
3 changes: 0 additions & 3 deletions msbuild/Xamarin.Mac.Tasks/Tasks/ScnTool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,5 @@ namespace Xamarin.Mac.Tasks
{
public class ScnTool : ScnToolTaskBase
{
protected override string OperatingSystem {
get { return "osx"; }
}
}
}
3 changes: 3 additions & 0 deletions msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ Copyright (C) 2014 Xamarin. All rights reserved.
IntermediateOutputPath="$(IntermediateOutputPath)"
AppManifest="$(_AppManifest)"
ProjectDir="$(MSBuildProjectDirectory)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
ResourcePrefix="$(XamMacResourcePrefix)"
SdkDevPath="$(_SdkDevPath)"
SdkRoot="$(_SdkRoot)"
Expand Down Expand Up @@ -588,6 +589,7 @@ Copyright (C) 2014 Xamarin. All rights reserved.
SdkRoot="$(_SdkRoot)"
SdkDevPath="$(_SdkDevPath)"
SdkVersion="$(MacOSXSdkVersion)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
IntermediateOutputPath="$(IntermediateOutputPath)"
InputScene="%(_ColladaAssetWithLogicalName.Identity)"
OutputScene="$(IntermediateOutputPath)%(_ColladaAssetWithLogicalName.LogicalName)">
Expand Down Expand Up @@ -675,6 +677,7 @@ Copyright (C) 2014 Xamarin. All rights reserved.
ToolPath="$(CopySceneKitAssetsPath)"
SceneKitAssets="@(SceneKitAsset)"
IntermediateOutputPath="$(IntermediateOutputPath)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
ProjectDir="$(MSBuildProjectDirectory)"
ResourcePrefix="$(XamMacResourcePrefix)"
SdkPlatform="MacOSX"
Expand Down
17 changes: 17 additions & 0 deletions msbuild/Xamarin.MacDev.Tasks.Core/PlatformFramework.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,22 @@ public static PlatformFramework GetFramework (string targetFrameworkIdentifier)
throw new InvalidOperationException ("Unknown TargetFrameworkIdentifier: " + targetFrameworkIdentifier);
}
}

public static string GetOperatingSystem (string targetFrameworkIdentifier)
{
var framework = PlatformFrameworkHelper.GetFramework (targetFrameworkIdentifier);
switch (framework) {
case PlatformFramework.WatchOS:
return "watchos";
case PlatformFramework.TVOS:
return "tvos";
case PlatformFramework.MacOS:
return "osx";
case PlatformFramework.iOS:
return "ios";
default:
throw new InvalidOperationException (string.Format ("Unknown target framework {0} for target framework identifier {2}.", framework, targetFrameworkIdentifier));
}
}
}
}
9 changes: 7 additions & 2 deletions msbuild/Xamarin.MacDev.Tasks.Core/Tasks/CompileSceneKitAssetsTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ public abstract class CompileSceneKitAssetsTaskBase : Task
[Required]
public string SdkVersion { get; set; }

[Required]
public string TargetFrameworkIdentifier { get; set; }

public string ToolExe {
get { return toolExe ?? ToolName; }
set { toolExe = value; }
Expand All @@ -67,8 +70,10 @@ static string ToolName {
get { return "copySceneKitAssets"; }
}

protected abstract string OperatingSystem {
get;
protected virtual string OperatingSystem {
get {
return PlatformFrameworkHelper.GetOperatingSystem (TargetFrameworkIdentifier);
}
}

string DeveloperRootBinDir {
Expand Down
21 changes: 19 additions & 2 deletions msbuild/Xamarin.MacDev.Tasks.Core/Tasks/MetalTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ public abstract class MetalTaskBase : ToolTask
[Required]
public ITaskItem SourceFile { get; set; }

[Required]
public string TargetFrameworkIdentifier { get; set; }

#endregion

[Output]
Expand All @@ -48,8 +51,22 @@ protected abstract string MinimumDeploymentTargetKey {
get;
}

protected abstract string OperatingSystem {
get;
protected virtual string OperatingSystem {
get {
switch (PlatformFrameworkHelper.GetFramework (TargetFrameworkIdentifier)) {
case PlatformFramework.WatchOS:
return "watchos";
case PlatformFramework.TVOS:
return "tvos";
case PlatformFramework.MacOS:
return "macosx";
case PlatformFramework.iOS:
return "ios";
default:
Log.LogError ($"Unknown target framework identifier: {TargetFrameworkIdentifier}.");
return string.Empty;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That block is repeated 3 times (not DRY)
Can it be moved into a (new) base type ? or an helper method ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - but now you have to explain why this one is different ;-) and not shared
Curious minds must know :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we use c# 8? Switch expressions!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I thought (jumped) to the default case, i.e. String.Empty vs Exception, and overlooked the mac string :)

}

protected abstract string DevicePlatformBinDir {
Expand Down
9 changes: 7 additions & 2 deletions msbuild/Xamarin.MacDev.Tasks.Core/Tasks/ScnToolTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,19 @@ public string SdkDevPath {
}
}

[Required]
public string TargetFrameworkIdentifier { get; set; }

#endregion

string DevicePlatformBinDir {
get { return Path.Combine (SdkDevPath, "usr", "bin"); }
}

protected abstract string OperatingSystem {
get;
protected virtual string OperatingSystem {
get {
return PlatformFrameworkHelper.GetOperatingSystem (TargetFrameworkIdentifier);
}
}

protected override string ToolName {
Expand Down
3 changes: 0 additions & 3 deletions msbuild/Xamarin.iOS.Tasks.Core/Tasks/CompileSceneKitAssetsTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,5 @@
{
public abstract class CompileSceneKitAssetsTaskBase : Xamarin.MacDev.Tasks.CompileSceneKitAssetsTaskBase
{
protected override string OperatingSystem {
get { return "ios"; }
}
}
}
4 changes: 0 additions & 4 deletions msbuild/Xamarin.iOS.Tasks.Core/Tasks/MetalTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@ namespace Xamarin.iOS.Tasks
{
public abstract class MetalTaskBase : Xamarin.MacDev.Tasks.MetalTaskBase
{
protected override string OperatingSystem {
get { return "ios"; }
}

protected override string MinimumDeploymentTargetKey {
get { return ManifestKeys.MinimumOSVersion; }
}
Expand Down
3 changes: 0 additions & 3 deletions msbuild/Xamarin.iOS.Tasks.Core/Tasks/ScnToolTaskBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,5 @@
{
public abstract class ScnToolTaskBase : Xamarin.MacDev.Tasks.ScnToolTaskBase
{
protected override string OperatingSystem {
get { return "ios"; }
}
}
}
3 changes: 3 additions & 0 deletions msbuild/Xamarin.iOS.Tasks.Core/Xamarin.iOS.Common.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,7 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
IntermediateOutputPath="$(DeviceSpecificIntermediateOutputPath)"
AppManifest="$(_AppManifest)"
ProjectDir="$(MSBuildProjectDirectory)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
ResourcePrefix="$(IPhoneResourcePrefix)"
SdkDevPath="$(_SdkDevPath)"
SdkRoot="$(_SdkRoot)"
Expand Down Expand Up @@ -1220,6 +1221,7 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
SdkRoot="$(_SdkRoot)"
SdkDevPath="$(_SdkDevPath)"
SdkVersion="$(MtouchSdkVersion)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
IntermediateOutputPath="$(DeviceSpecificIntermediateOutputPath)"
InputScene="%(_ColladaAssetWithLogicalName.Identity)"
OutputScene="$(DeviceSpecificIntermediateOutputPath)%(_ColladaAssetWithLogicalName.LogicalName)">
Expand Down Expand Up @@ -1333,6 +1335,7 @@ Copyright (C) 2013-2016 Xamarin. All rights reserved.
ToolPath="$(CopySceneKitAssetsPath)"
SceneKitAssets="@(SceneKitAsset)"
IntermediateOutputPath="$(DeviceSpecificIntermediateOutputPath)"
TargetFrameworkIdentifier="$(TargetFrameworkIdentifier)"
ProjectDir="$(MSBuildProjectDirectory)"
ResourcePrefix="$(IPhoneResourcePrefix)"
IsWatchApp="$(IsWatchApp)"
Expand Down
7 changes: 5 additions & 2 deletions tests/mtouch/ToolTasksBinPathTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ namespace Xamarin.MacDev.Tasks {

public abstract class MetalTaskBase {

protected abstract string OperatingSystem {
get;
protected virtual string OperatingSystem {
get {
throw new NotImplementedException ();
}
}

protected abstract string DevicePlatformBinDir {
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that change is not mentioned in the commit message

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description has been updated.

var proc = Process.Start (psi);

string output = proc.StandardOutput.ReadToEnd ();
Expand Down