Skip to content

Commit

Permalink
[release/6.0.3xx] [dotnet] Accept invalid runtime identifiers for Res…
Browse files Browse the repository at this point in the history
…tore. (#15490)

Validating that projects are only restored using valid runtime identifiers
have turned out to be an interesting rabbit hole.

Nobody seems to dispute the fact that it's a correct validation, the problem
is that it apparently turns out quite complicated to not do the wrong thing
for projects with multiple target frameworks.

In some scenarios apparently the Restore target might want to restore all
target frameworks, which breaks down if whatever the user wants to do requires
setting a runtime identifier, because then that runtime identifier is set for
all target frameworks.

So for the sake of simplicity, we're going to try removing this validation for
the Restore target (we're keeping it for the Build target).

There are thus two potential scenarios:

1. The Restore succeeds using invalid runtime identifiers. This shouldn't
   affect valid builds (since those should be completely separate due to using
   different runtime identifiers).
2. Something else breaks. At worst the user will be given a less
   comprehensible error message. Time will tell if this is better or worse
   than the current experience.

Ref: NuGet/Home#11653
Ref: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1558247


Backport of #15357

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
  • Loading branch information
1 parent 363ecca commit c23dbe9
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 20 deletions.
2 changes: 1 addition & 1 deletion dotnet/targets/Xamarin.Shared.Sdk.targets
Original file line number Diff line number Diff line change
Expand Up @@ -1737,7 +1737,7 @@

<Target Name="_ValidateRuntimeIdentifier"
Condition="'$(RuntimeIdentifier)' != '' And '$(_RuntimeIdentifierValidation)' != 'false' And '$(_RuntimeIdentifierIsRequired)' == 'true'"
BeforeTargets="Restore;Build;ResolvedFrameworkReference;ResolveRuntimePackAssets;ProcessFrameworkReferences">
BeforeTargets="Build;ResolvedFrameworkReference;ResolveRuntimePackAssets">
<PropertyGroup>
<_IsValidRuntimeIdentifier Condition="@(_XamarinValidRuntimeIdentifier->WithMetadataValue('Platform', '$(_PlatformName)')->WithMetadataValue('Filename', '$(RuntimeIdentifier)')->Count()) &gt; 0">true</_IsValidRuntimeIdentifier>
</PropertyGroup>
Expand Down
6 changes: 6 additions & 0 deletions tests/common/DotNet.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public static ExecutionResult AssertPublishFailure (string project, Dictionary<s
return rv;
}

public static ExecutionResult AssertRestore (string project, Dictionary<string, string> properties = null)
{
return Execute ("restore", project, properties, true);
}

public static ExecutionResult AssertBuild (string project, Dictionary<string, string> properties = null)
{
return Execute ("build", project, properties, true);
Expand Down Expand Up @@ -103,6 +108,7 @@ public static ExecutionResult Execute (string verb, string project, Dictionary<s
case "build":
case "pack":
case "publish":
case "restore":
var args = new List<string> ();
args.Add (verb);
args.Add (project);
Expand Down
60 changes: 41 additions & 19 deletions tests/dotnet/UnitTests/ProjectTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -470,24 +470,24 @@ public void BuildAndExecuteNativeReferencesTestApp (string project, ApplePlatfor
}

[Test]
[TestCase (ApplePlatform.iOS, "ios-x64")] // valid RID in a previous preview (and common mistake)
[TestCase (ApplePlatform.iOS, "iossimulator-x84")] // it's x86, not x84
[TestCase (ApplePlatform.iOS, "iossimulator-arm")] // we don't support this
[TestCase (ApplePlatform.iOS, "helloworld")] // random text
[TestCase (ApplePlatform.iOS, "osx-x64")] // valid RID for another platform
[TestCase (ApplePlatform.TVOS, "tvos-x64")] // valid RID in a previous preview (and common mistake)
[TestCase (ApplePlatform.TVOS, "tvossimulator-x46")] // it's x64, not x46
[TestCase (ApplePlatform.TVOS, "tvossimulator-arm")] // we don't support this
[TestCase (ApplePlatform.TVOS, "helloworld")] // random text
[TestCase (ApplePlatform.TVOS, "osx-x64")] // valid RID for another platform
[TestCase (ApplePlatform.MacOSX, "osx-x46")] // it's x64, not x46
[TestCase (ApplePlatform.MacOSX, "macos-arm64")] // it's osx, not macos
[TestCase (ApplePlatform.MacOSX, "helloworld")] // random text
[TestCase (ApplePlatform.MacOSX, "ios-arm64")] // valid RID for another platform
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-x46")] // it's x64, not x46
[TestCase (ApplePlatform.MacCatalyst, "helloworld")] // random text
[TestCase (ApplePlatform.MacCatalyst, "osx-x64")] // valid RID for another platform
public void InvalidRuntimeIdentifier (ApplePlatform platform, string runtimeIdentifier)
[TestCase (ApplePlatform.iOS, "ios-x64", false)] // valid RID in a previous preview (and common mistake)
[TestCase (ApplePlatform.iOS, "iossimulator-x84", true)] // it's x86, not x84
[TestCase (ApplePlatform.iOS, "iossimulator-arm", true)] // we don't support this
[TestCase (ApplePlatform.iOS, "helloworld", true)] // random text
[TestCase (ApplePlatform.iOS, "osx-x64", false)] // valid RID for another platform
[TestCase (ApplePlatform.TVOS, "tvos-x64", false)] // valid RID in a previous preview (and common mistake)
[TestCase (ApplePlatform.TVOS, "tvossimulator-x46", true)] // it's x64, not x46
[TestCase (ApplePlatform.TVOS, "tvossimulator-arm", true)] // we don't support this
[TestCase (ApplePlatform.TVOS, "helloworld", true)] // random text
[TestCase (ApplePlatform.TVOS, "osx-x64", false)] // valid RID for another platform
[TestCase (ApplePlatform.MacOSX, "osx-x46", true)] // it's x64, not x46
[TestCase (ApplePlatform.MacOSX, "macos-arm64", true)] // it's osx, not macos
[TestCase (ApplePlatform.MacOSX, "helloworld", true)] // random text
[TestCase (ApplePlatform.MacOSX, "ios-arm64", false)] // valid RID for another platform
[TestCase (ApplePlatform.MacCatalyst, "maccatalyst-x46", true)] // it's x64, not x46
[TestCase (ApplePlatform.MacCatalyst, "helloworld", true)] // random text
[TestCase (ApplePlatform.MacCatalyst, "osx-x64", false)] // valid RID for another platform
public void InvalidRuntimeIdentifier (ApplePlatform platform, string runtimeIdentifier, bool notRecognized)
{
var project = "MySimpleApp";
Configuration.IgnoreIfIgnoredPlatform (platform);
Expand All @@ -499,7 +499,29 @@ public void InvalidRuntimeIdentifier (ApplePlatform platform, string runtimeIden
var errors = BinLog.GetBuildLogErrors (rv.BinLogPath).ToArray ();
var uniqueErrors = errors.Select (v => v.Message).Distinct ().ToArray ();
Assert.AreEqual (1, uniqueErrors.Length, "Error count");
Assert.AreEqual ($"The RuntimeIdentifier '{runtimeIdentifier}' is invalid.", uniqueErrors [0], "Error message");
string expectedError;
if (notRecognized) {
expectedError = $"The specified RuntimeIdentifier '{runtimeIdentifier}' is not recognized.";
} else {
expectedError = $"The RuntimeIdentifier '{runtimeIdentifier}' is invalid.";
}
Assert.AreEqual (expectedError, uniqueErrors [0], "Error message");
}

[Test]
[TestCase (ApplePlatform.iOS, "win10-x86")]
[TestCase (ApplePlatform.TVOS, "win10-x64")]
[TestCase (ApplePlatform.MacOSX, "win10-arm")]
[TestCase (ApplePlatform.MacCatalyst, "win10-arm64")]
public void InvalidRuntimeIdentifier_Restore (ApplePlatform platform, string runtimeIdentifier)
{
var project = "MySimpleApp";
Configuration.IgnoreIfIgnoredPlatform (platform);

var project_path = GetProjectPath (project, platform: platform);
Clean (project_path);
var properties = GetDefaultProperties (runtimeIdentifier);
DotNet.AssertRestore (project_path, properties);
}

[Test]
Expand Down

5 comments on commit c23dbe9

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

📋 [CI Build] API Diff 📋

API Current PR diff

✅ API Diff (from PR only) (no change)

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

Generator diff

Generator Diff (no change)

Pipeline on Agent XAMMINI-057.Monterey
Hash: c23dbe920ec74daae9ef8e9ac4d44a6bfe5385b9

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

📚 [CI Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMMINI-054.Monterey'
Hash: c23dbe920ec74daae9ef8e9ac4d44a6bfe5385b9

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

💻 [CI Build] Tests on macOS Mac Catalina (10.15) passed 💻

All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: c23dbe920ec74daae9ef8e9ac4d44a6bfe5385b9

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

❌ [CI Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

  • monotouch-test

Pipeline on Agent
Hash: c23dbe920ec74daae9ef8e9ac4d44a6bfe5385b9

@vs-mobiletools-engineering-service2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅ [CI Build] Tests passed on VSTS: simulator tests iOS. ✅

Tests passed on VSTS: simulator tests iOS.

🎉 All 234 tests passed 🎉

Pipeline on Agent XAMBOT-1162.Monterey'
[release/6.0.3xx] [dotnet] Accept invalid runtime identifiers for Restore. (#15490)

Please sign in to comment.