[msbuild] Fix some issues with various types of resources when buildi…

…ng from Windows. (#21586)

While adding a test for a new feature, it turned out that the new test uncovered existing bugs. So here I'm adding that new test, and fixing those bugs.

The main part is that computing the virtual project path for resource items on Windows wasn't correct, in particular when referencing resources outside the current project directory. The existing code didn't even have enough information to compute the correct values...

Sidenote: the virtual project path is used to decide where in an app bundle a particular resource should go.

So:

* Add new metadata to resource items, for the path of the defining project + the path of the main project. This is required to compute the correct virtual project path.
* Completely rewrite the code to compute virtual project path, prioritizing making the code legible and easy to understand (in particular minimizing differences in code paths between macOS and Windows), and add lots of comments.
* Add unit tests for computing the virtual project path.
* Add a new .NET project test for all variations of resources we support.
* Run said new test on Windows as well, both in remote and local (Hot Restart) mode.

msbuild/Xamarin.Localization.MSBuild/MSBStrings.resx

@@ -1623,6 +1623,14 @@
         </comment>
     </data>
 
+    <data name="E7133" xml:space="preserve">
+        <value>The item '{0}' does not have a '{1}' value set.</value>
+        <comment>
+            {0}: a path to a file
+            {1}: the name of a MSBuild metadata
+        </comment>
+    </data>
+
     <data name="XcodeBuild_CreateNativeRef" xml:space="preserve">
         <value>Adding reference to Xcode project output: '{0}'. The '%(CreateNativeReference)' metadata can be set to 'false' to opt out of this behavior.</value>
         <comment>

msbuild/Xamarin.MacDev.Tasks/BundleResource.cs

@@ -1,12 +1,20 @@
+// #define TRACE
+
 using System;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Linq;
 using System.Collections.Generic;
 
 using Microsoft.Build.Framework;
+using Microsoft.Build.Utilities;
 
+using Xamarin.Localization.MSBuild;
 using Xamarin.Utils;
+using Xamarin.MacDev.Tasks;
+
+#nullable enable
 
 namespace Xamarin.MacDev {
 	public static class BundleResource {
@@ -48,74 +56,149 @@ public static bool IsIllegalName (string name, [NotNullWhen (true)] out string?
 			return false;
 		}
 
-		public static IList<string> SplitResourcePrefixes (string prefix)
+		public static IList<string> SplitResourcePrefixes (string? prefix)
 		{
+			if (prefix is null)
+				return Array.Empty<string> ();
+
 			return prefix.Split (new [] { ';' }, StringSplitOptions.RemoveEmptyEntries)
 				.Select (s => s.Replace ('\\', Path.DirectorySeparatorChar).Trim () + Path.DirectorySeparatorChar)
 				.Where (s => s.Length > 1)
 				.ToList ();
 		}
 
-		public static string GetVirtualProjectPath (string projectDir, ITaskItem item, bool isVSBuild)
+		[Conditional ("TRACE")]
+		static void Trace (Task task, string msg)
 		{
-			var link = item.GetMetadata ("Link");
+			task.Log.LogMessage (MessageImportance.Low, msg);
+		}
 
-			// Note: if the Link metadata exists, then it will be the equivalent of the ProjectVirtualPath
+		// Compute the path of 'item' relative to the project.
+		public static string GetVirtualProjectPath<T> (T task, ITaskItem item) where T : Task, IHasProjectDir, IHasSessionId
+		{
+			// If the Link metadata exists, use that, it takes precedence over anything else.
+			var link = item.GetMetadata ("Link");
 			if (!string.IsNullOrEmpty (link)) {
-				if (Path.DirectorySeparatorChar != '\\')
-					return link.Replace ('\\', '/');
-
+				// Canonicalize to use macOS-style directory separators.
+				link = link.Replace ('\\', '/');
+				Trace (task, $"BundleResource.GetVirtualProjectPath ({item.ItemSpec}) => Link={link}");
 				return link;
 			}
 
-			// HACK: This is for Visual Studio iOS projects
-			if (isVSBuild) {
-				if (item.GetMetadata ("DefiningProjectFullPath") != item.GetMetadata ("MSBuildProjectFullPath")) {
-					return item.GetMetadata ("FullPath").Replace (item.GetMetadata ("DefiningProjectDirectory"), string.Empty);
-				} else {
-					return item.ItemSpec;
-				}
-			}
+			// Note that '/' is a valid path separator on Windows (in addition to '\'), so canonicalize the paths to use '/' as the path separator.
 
 			var isDefaultItem = item.GetMetadata ("IsDefaultItem") == "true";
-			var definingProjectFullPath = item.GetMetadata (isDefaultItem ? "MSBuildProjectFullPath" : "DefiningProjectFullPath");
-			var path = item.GetMetadata ("FullPath");
-			string baseDir;
+			var localMSBuildProjectFullPath = item.GetMetadata ("LocalMSBuildProjectFullPath").Replace ('\\', '/');
+			var localDefiningProjectFullPath = item.GetMetadata ("LocalDefiningProjectFullPath").Replace ('\\', '/');
+			if (string.IsNullOrEmpty (localDefiningProjectFullPath)) {
+				task.Log.LogError (null, null, null, item.ItemSpec, 0, 0, 0, 0, MSBStrings.E7133 /* The item '{0}' does not have a '{1}' value set. */, item.ItemSpec, "LocalDefiningProjectFullPath");
+				return "placeholder";
+			}
+
+			if (string.IsNullOrEmpty (localMSBuildProjectFullPath)) {
+				task.Log.LogError (null, null, null, item.ItemSpec, 0, 0, 0, 0, MSBStrings.E7133 /* The item '{0}' does not have a '{1}' value set. */, item.ItemSpec, "LocalMSBuildProjectFullPath");
+				return "placeholder";
+			}
+
+			// * If we're not a default item, compute the path relative to the
+			//   file that declared the item in question.
+			// * If we're a default item (IsDefaultItem=true), compute
+			//   relative to the user's project file (because the file that
+			//   declared the item is our Microsoft.Sdk.DefaultItems.template.props file,
+			//   and the path relative to that file is certainly not what we want).
+			//
+			// We use the 'LocalMSBuildProjectFullPath' and
+			// 'LocalDefiningProjectFullPath' metadata because the
+			// 'MSBuildProjectFullPath' and 'DefiningProjectFullPath' are not
+			// necessarily correct when building remotely (the relative path
+			// between files might not be the same on macOS once XVS has
+			// copied them there, in particular for files outside the project
+			// directory).
+			//
+			// The 'LocalMSBuildProjectFullPath' and 'LocalDefiningProjectFullPath'
+			// values are set to the Windows version of 'MSBuildProjectFullPath'
+			// and 'DefiningProjectFullPath' when building remotely, and the macOS
+			// version when building on macOS.
+
+			// First find the absolute path to the item
+			var projectAbsoluteDir = task.ProjectDir;
+			var isRemoteBuild = !string.IsNullOrEmpty (task.SessionId);
+			string itemAbsolutePath;
+			if (isRemoteBuild) {
+				itemAbsolutePath = PathUtils.PathCombineWindows (projectAbsoluteDir, item.ItemSpec);
+			} else {
+				itemAbsolutePath = Path.Combine (projectAbsoluteDir, item.ItemSpec);
+			}
+			var originalItemAbsolutePath = itemAbsolutePath;
 
-			if (!string.IsNullOrEmpty (definingProjectFullPath)) {
-				baseDir = Path.GetDirectoryName (definingProjectFullPath);
+			// Then find the directory we should use to compute the result relative to.
+			string relativeToDirectory; // this is an absolute path.
+			if (isDefaultItem) {
+				relativeToDirectory = Path.GetDirectoryName (localMSBuildProjectFullPath);
 			} else {
-				baseDir = projectDir;
+				relativeToDirectory = Path.GetDirectoryName (localDefiningProjectFullPath);
 			}
+			var originalRelativeToDirectory = relativeToDirectory;
 
-			baseDir = PathUtils.ResolveSymbolicLinks (baseDir);
-			path = PathUtils.ResolveSymbolicLinks (path);
+			// On macOS we need to resolve symlinks before computing the relative path.
+			if (!isRemoteBuild) {
+				relativeToDirectory = PathUtils.ResolveSymbolicLinks (relativeToDirectory);
+				itemAbsolutePath = PathUtils.ResolveSymbolicLinks (itemAbsolutePath);
+			}
 
-			return PathUtils.AbsoluteToRelative (baseDir, path);
+			// Compute the relative path we want to return.
+			string rv;
+			if (isRemoteBuild) {
+				rv = PathUtils.AbsoluteToRelativeWindows (relativeToDirectory, itemAbsolutePath);
+			} else {
+				rv = PathUtils.AbsoluteToRelative (relativeToDirectory, itemAbsolutePath);
+			}
+			// Make it a mac-style path
+			rv = rv.Replace ('\\', '/');
+
+			Trace (task, $"BundleResource.GetVirtualProjectPath ({item.ItemSpec}) => {rv}\n" +
+					$"\t\t\tprojectAbsoluteDir={projectAbsoluteDir}\n" +
+					$"\t\t\tIsRemoteBuild={isRemoteBuild}\n" +
+					$"\t\t\tisDefaultItem={isDefaultItem}\n" +
+					$"\t\t\tLocalMSBuildProjectFullPath={localMSBuildProjectFullPath}\n" +
+					$"\t\t\tLocalDefiningProjectFullPath={localDefiningProjectFullPath}\n" +
+					$"\t\t\toriginalItemAbsolutePath={originalItemAbsolutePath}\n" +
+					$"\t\t\titemAbsolutePath={itemAbsolutePath}\n" +
+					$"\t\t\toriginalRelativeToDirectory={originalRelativeToDirectory}\n" +
+					$"\t\t\trelativeToDirectory={relativeToDirectory}\n");
+
+			return rv;
 		}
 
-		public static string GetLogicalName (string projectDir, IList<string> prefixes, ITaskItem item, bool isVSBuild)
+		public static string GetLogicalName<T> (T task, ITaskItem item) where T : Task, IHasProjectDir, IHasResourcePrefix, IHasSessionId
 		{
 			var logicalName = item.GetMetadata ("LogicalName");
 
+			// If an item has the LogicalName metadata set, return that.
 			if (!string.IsNullOrEmpty (logicalName)) {
-				if (Path.DirectorySeparatorChar != '\\')
-					return logicalName.Replace ('\\', '/');
-
-				return logicalName;
+				Trace (task, $"BundleResource.GetLogicalName ({item.ItemSpec}) => has LogicalName={logicalName.Replace ('\\', '/')} (original {logicalName})");
+				// Canonicalize to use macOS-style directory separators.
+				return logicalName.Replace ('\\', '/');
 			}
 
-			var vpath = GetVirtualProjectPath (projectDir, item, isVSBuild);
+			// Check if the start of the item matches any of the resource prefixes, in which case choose
+			// the longest resource prefix, and subtract it from the start of the item.
+			var vpath = GetVirtualProjectPath (task, item);
 			int matchlen = 0;
-
+			var prefixes = SplitResourcePrefixes (task.ResourcePrefix);
 			foreach (var prefix in prefixes) {
-				if (vpath.StartsWith (prefix, StringComparison.OrdinalIgnoreCase) && prefix.Length > matchlen)
-					matchlen = prefix.Length;
+				var mpath = vpath.Replace ('\\', '/');
+				var mprefix = prefix.Replace ('\\', '/');
+				if (mpath.StartsWith (mprefix, StringComparison.OrdinalIgnoreCase) && mprefix.Length > matchlen)
+					matchlen = mprefix.Length;
 			}
-
-			if (matchlen > 0)
+			if (matchlen > 0) {
+				Trace (task, $"BundleResource.GetLogicalName ({item.ItemSpec}) => LogicalName={vpath.Substring (matchlen)} (vpath={vpath} matchlen={matchlen} prefixes={string.Join (",", prefixes)})");
 				return vpath.Substring (matchlen);
+			}
 
+			// Otherwise return the item as-is.
+			Trace (task, $"BundleResource.GetLogicalName ({item.ItemSpec}) => LogicalName={vpath} (prefixes={string.Join (",", prefixes)})");
 			return vpath;
 		}
 	}

msbuild/Xamarin.MacDev.Tasks/Tasks/ACTool.cs

@@ -74,7 +74,7 @@ bool IsMessagesExtension {
 
 		protected override void AppendCommandLineArguments (IDictionary<string, string?> environment, CommandLineArgumentBuilder args, ITaskItem [] items)
 		{
-			var assetDirs = new HashSet<string> (items.Select (x => BundleResource.GetVirtualProjectPath (ProjectDir, x, !string.IsNullOrEmpty (SessionId))));
+			var assetDirs = new HashSet<string> (items.Select (x => BundleResource.GetVirtualProjectPath (this, x)));
 
 			if (!string.IsNullOrEmpty (XSAppIconAssets) && !string.IsNullOrEmpty (AppIcon)) {
 				Log.LogError (MSBStrings.E7129 /* Can't specify both 'XSAppIconAssets' in the Info.plist and 'AppIcon' in the project file. Please select one or the other. */);
@@ -257,7 +257,7 @@ public override bool Execute ()
 			var specs = new PArray ();
 
 			for (int i = 0; i < ImageAssets.Length; i++) {
-				var vpath = BundleResource.GetVirtualProjectPath (ProjectDir, ImageAssets [i], !string.IsNullOrEmpty (SessionId));
+				var vpath = BundleResource.GetVirtualProjectPath (this, ImageAssets [i]);
 
 				// Ignore MacOS .DS_Store files...
 				if (Path.GetFileName (vpath).Equals (".DS_Store", StringComparison.OrdinalIgnoreCase))
@@ -298,7 +298,7 @@ public override bool Execute ()
 				items.Clear ();
 
 				for (int i = 0; i < ImageAssets.Length; i++) {
-					var vpath = BundleResource.GetVirtualProjectPath (ProjectDir, ImageAssets [i], !string.IsNullOrEmpty (SessionId));
+					var vpath = BundleResource.GetVirtualProjectPath (this, ImageAssets [i]);
 					var clone = false;
 					ITaskItem item;
 
@@ -349,7 +349,7 @@ public override bool Execute ()
 
 			// Note: `items` contains only the Contents.json files at this point
 			for (int i = 0; i < items.Count; i++) {
-				var vpath = BundleResource.GetVirtualProjectPath (ProjectDir, items [i], !string.IsNullOrEmpty (SessionId));
+				var vpath = BundleResource.GetVirtualProjectPath (this, items [i]);
 				var path = items [i].GetMetadata ("FullPath");
 
 				// get the parent (which will typically be .appiconset, .launchimage, .imageset, .iconset, etc)

msbuild/Xamarin.MacDev.Tasks/Tasks/CollectBundleResources.cs

@@ -1,6 +1,7 @@
 using System;
 using System.IO;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 
 using Microsoft.Build.Framework;
 using Microsoft.Build.Utilities;
@@ -9,7 +10,7 @@
 using Xamarin.Utils;
 
 namespace Xamarin.MacDev.Tasks {
-	public class CollectBundleResources : XamarinTask, ICancelableTask {
+	public class CollectBundleResources : XamarinTask, ICancelableTask, IHasProjectDir, IHasResourcePrefix {
 		#region Inputs
 
 		public ITaskItem [] BundleResources { get; set; } = Array.Empty<ITaskItem> ();
@@ -64,7 +65,6 @@ public override bool Execute ()
 
 		bool ExecuteImpl ()
 		{
-			var prefixes = BundleResource.SplitResourcePrefixes (ResourcePrefix);
 			var bundleResources = new List<ITaskItem> ();
 
 			foreach (var item in BundleResources) {
@@ -73,7 +73,7 @@ bool ExecuteImpl ()
 				if (!string.IsNullOrEmpty (publishFolderType))
 					continue;
 
-				var logicalName = BundleResource.GetLogicalName (ProjectDir, prefixes, item, !string.IsNullOrEmpty (SessionId));
+				var logicalName = BundleResource.GetLogicalName (this, item);
 				// We need a physical path here, ignore the Link element
 				var path = item.GetMetadata ("FullPath");
 

msbuild/Xamarin.MacDev.Tasks/Tasks/CompileAppManifest.cs

@@ -13,7 +13,7 @@
 #nullable enable
 
 namespace Xamarin.MacDev.Tasks {
-	public class CompileAppManifest : XamarinTask, ITaskCallback, ICancelableTask {
+	public class CompileAppManifest : XamarinTask, IHasProjectDir, IHasResourcePrefix, ITaskCallback, ICancelableTask {
 		#region Inputs
 
 		// Single-project property that maps to CFBundleIdentifier for Apple platforms
@@ -212,10 +212,9 @@ void RegisterFonts (PDictionary plist)
 			// https://developer.apple.com/documentation/swiftui/applying-custom-fonts-to-text
 
 			// Compute the relative location in the app bundle for each font file
-			var prefixes = BundleResource.SplitResourcePrefixes (ResourcePrefix);
 			const string logicalNameKey = "_ComputedLogicalName_";
 			foreach (var item in FontFilesToRegister) {
-				var logicalName = BundleResource.GetLogicalName (ProjectDir, prefixes, item, !string.IsNullOrEmpty (SessionId));
+				var logicalName = BundleResource.GetLogicalName (this, item);
 				item.SetMetadata (logicalNameKey, logicalName);
 			}
 

msbuild/Xamarin.MacDev.Tasks/Tasks/CompileSceneKitAssets.cs

@@ -16,7 +16,7 @@
 #nullable disable
 
 namespace Xamarin.MacDev.Tasks {
-	public class CompileSceneKitAssets : XamarinTask, ICancelableTask {
+	public class CompileSceneKitAssets : XamarinTask, ICancelableTask, IHasProjectDir, IHasResourcePrefix {
 		string toolExe;
 
 		#region Inputs
@@ -130,7 +130,6 @@ public override bool Execute ()
 				return taskRunner.RunAsync (this).Result;
 			}
 
-			var prefixes = BundleResource.SplitResourcePrefixes (ResourcePrefix);
 			var intermediate = Path.Combine (IntermediateOutputPath, ToolName, AppBundleName);
 			var bundleResources = new List<ITaskItem> ();
 			var modified = new HashSet<string> ();
@@ -150,7 +149,7 @@ public override bool Execute ()
 
 				asset.RemoveMetadata ("LogicalName");
 
-				var bundleName = BundleResource.GetLogicalName (ProjectDir, prefixes, asset, !string.IsNullOrEmpty (SessionId));
+				var bundleName = BundleResource.GetLogicalName (this, asset);
 				var output = new TaskItem (Path.Combine (intermediate, bundleName));
 
 				if (!modified.Contains (scnassets) && (!File.Exists (output.ItemSpec) || File.GetLastWriteTimeUtc (asset.ItemSpec) > File.GetLastWriteTimeUtc (output.ItemSpec))) {
@@ -202,7 +201,7 @@ public override bool Execute ()
 
 			var tasks = new List<Task> ();
 			foreach (var item in items) {
-				var bundleDir = BundleResource.GetLogicalName (ProjectDir, prefixes, new TaskItem (item), !string.IsNullOrEmpty (SessionId));
+				var bundleDir = BundleResource.GetLogicalName (this, new TaskItem (item));
 				var output = Path.Combine (intermediate, bundleDir);
 
 				tasks.Add (CopySceneKitAssets (item.ItemSpec, output, intermediate));

msbuild/Xamarin.MacDev.Tasks/Tasks/CoreMLCompiler.cs

@@ -12,7 +12,7 @@
 #nullable disable
 
 namespace Xamarin.MacDev.Tasks {
-	public class CoreMLCompiler : XamarinTask, ICancelableTask {
+	public class CoreMLCompiler : XamarinTask, ICancelableTask, IHasProjectDir, IHasResourcePrefix {
 		string toolExe;
 
 		public string ToolName { get { return "coremlc"; } }
@@ -162,7 +162,6 @@ public override bool Execute ()
 				return new TaskRunner (SessionId, BuildEngine4).RunAsync (this).Result;
 
 			var coremlcOutputDir = Path.Combine (IntermediateOutputPath, "coremlc");
-			var prefixes = BundleResource.SplitResourcePrefixes (ResourcePrefix);
 			var mapping = new Dictionary<string, IDictionary> ();
 			var bundleResources = new List<ITaskItem> ();
 			var partialPlists = new List<ITaskItem> ();
@@ -171,7 +170,7 @@ public override bool Execute ()
 				Directory.CreateDirectory (coremlcOutputDir);
 
 				foreach (var model in Models) {
-					var logicalName = BundleResource.GetLogicalName (ProjectDir, prefixes, model, !string.IsNullOrEmpty (SessionId));
+					var logicalName = BundleResource.GetLogicalName (this, model);
 					var bundleName = GetPathWithoutExtension (logicalName) + ".mlmodelc";
 					var outputPath = Path.Combine (coremlcOutputDir, bundleName);
 					var outputDir = Path.GetDirectoryName (outputPath);

msbuild/Xamarin.MacDev.Tasks/Tasks/FindItemWithLogicalName.cs

@@ -8,7 +8,7 @@
 #nullable disable
 
 namespace Xamarin.MacDev.Tasks {
-	public class FindItemWithLogicalName : XamarinTask {
+	public class FindItemWithLogicalName : XamarinTask, IHasProjectDir, IHasResourcePrefix {
 		#region Inputs
 
 		[Required]
@@ -34,10 +34,8 @@ public class FindItemWithLogicalName : XamarinTask {
 		public override bool Execute ()
 		{
 			if (Items is not null) {
-				var prefixes = BundleResource.SplitResourcePrefixes (ResourcePrefix);
-
 				foreach (var item in Items) {
-					var logical = BundleResource.GetLogicalName (ProjectDir, prefixes, item, !string.IsNullOrEmpty (SessionId));
+					var logical = BundleResource.GetLogicalName (this, item);
 
 					if (logical == LogicalName) {
 						Log.LogMessage (MessageImportance.Low, MSBStrings.M0149, LogicalName, item.ItemSpec);