Сurrent directory virtualization for RAR task #6316

AR-May · 2021-04-01T09:18:28Z

Context

This PR is part of RAR as a service user story #3139. The current design supposes RAR tasks run concurrently in threads. However, the RAR task also depends on the current directory which is shared between the threads.

Changes Made

In the PR we implement current directory virtualization.
For all the inputs we either absolutize the paths at the start of the RAR task or if we are unable to recognize the input's type, absolutize an entry when we use it, i.e. when we know the type of the entry (path/assembly name/etc).

Testing

There already were unit tests for relative inputs in Miscellaneous.cs, we leverage them to check that the virtualization of the current directory works well.

Notes

…asing on his changes)

…section.

This reverts commit 7c398f1.

This reverts commit 2e5e3b6.

…s for this pr, remove it later.

… ci tests for this pr, remove it later." This reverts commit b1a8473.

…omments.

AR-May · 2021-04-13T13:17:45Z

src/Tasks/AssemblyDependency/TaskExecutionContextExtension.cs

+                // However, if the combined path consists of different path separators (both windows and unix style),
+                // then the behavior of Path.GetFullPath differs in windows and unix systems, as in Windows both Windows and Unix style separators works and in Unix - not.
+                // Windows' function eleminates the internal "./" and "../", Unix's function does not. We are using FixFilePath to remove windows-style separators when on unix machine.
+                return Path.GetFullPath(Path.Combine(taskExecutionContext.StartupDirectory, FileUtilities.FixFilePath(path)));


Need to consider possible switch from Path.GetFullPath -> PathInternal.RemoveRelativeSegments to avoid extra actions in Path.GetFullPath. Unfortunately, RemoveRelativeSegments is not yet available in the Path (dotnet/runtime#2162), so it might only be code duplication.

AR-May · 2021-04-13T13:23:14Z

src/Tasks/AssemblyDependency/TaskExecutionContextExtension.cs

+
+namespace Microsoft.Build.Tasks
+{
+    public static class TaskExecutionContextExtension


Do not need public here?

I don't think so, at least as long as these methods are fixed rather than user-manipulatable. Should this explicitly extend TaskExecutionContext?

yes, there was no need for "?" mark in my comment, actually, this has to be private. That is a reminder for myself to fix this together with the other changes.

Forgind

In the issue, you mentioned both normalizing and denormalizing as necessary. I saw the first but not the second. Did you determine it's unnecessary?

Forgind · 2021-04-18T15:14:12Z

...Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceEnvironmentVirtualizationTests.cs

@@ -0,0 +1,190 @@
+using System;


copyright header

Forgind · 2021-04-18T15:16:16Z

...Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceEnvironmentVirtualizationTests.cs

+
+            string testPath = Path.Combine(Path.GetTempPath(), @"RawFileNameRelative");
+
+            Directory.CreateDirectory(testPath);


Creating a temporary directory can be a little cleaner with TestEnvironment, though that might be overkill here.

Forgind · 2021-04-18T15:16:40Z

...Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceEnvironmentVirtualizationTests.cs

+        [Fact]
+        [Trait("Category", "mono-osx-failing")]
+        [Trait("Category", "netcore-osx-failing")]
+        [Trait("Category", "netcore-linux-failing")]


Why is this failing on linux/mac?

There is a problem I recently discovered with unit tests for RAR in unix. ResolveAssemblyReferenceTestFixture does not work correctly with unix paths. A lot of tests currently are just excluded for unix/mac because of that (well, that is my suggestion). I managed to fix ResolveAssemblyReferenceTestFixture for all other tests I introduced except this one. This one (and other tests that are currently just do not run in unix) need much more fixes in ResolveAssemblyReferenceTestFixture. I think we should fix it in another PR.

so, the problem is in the testing system, with file system's mocking, not with RAR.

I tried to glance through that to see if it was a quick fix, and it's clear that some parts were designed with cross-plat in mind...but that's a really big class, and a lot of it wasn't. Sounds like a lot of work for not enough return on investment to me.

hmm, so far I saw the only problem with this class which prevents it to be cross-plat: the comparison of paths (well, at least tests I saw were failing because of that). Yes, this is a big class and you need to fix it in a lot of places, that's why I did not want to do this in this PR. This is very technical fix and I did not think it will take so much time. But maybe you are right, there also might be other changes needed to make it cross platform, I did not thought much about that.

Forgind · 2021-04-18T15:17:01Z

...Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceEnvironmentVirtualizationTests.cs

+
+                Execute(rarTask);
+
+                Assert.Single(rarTask.ResolvedFiles);


nit: please use Shouldly for asserts.

Forgind · 2021-04-18T15:17:56Z

src/Tasks.UnitTests/HintPathResolver_Tests.cs

@@ -1,4 +1,5 @@
-using Microsoft.Build.Shared;
+using Microsoft.Build.Framework;


copyright header

Forgind · 2021-04-18T15:25:16Z

src/Tasks/AssemblyDependency/ReferenceTable.cs

@@ -438,7 +445,7 @@ string assemblyFileName

            if (!Path.IsPathRooted(assemblyFileName))
            {
-                reference.FullPath = Path.GetFullPath(assemblyFileName);
+                reference.FullPath = Path.GetFullPath(_executionContext.GetFullPath(assemblyFileName));


This is calling GetFullPath twice. Simplify? (Same for other occurrences.)

Nope, _executionContext.GetFullPath sometimes, if we do not have any execution context, should not absolutize the path. This is to keep previous behavior as an escape hatch when we do not use RAR as a service. Maybe I should rename the func to clarify this.

Maybe renaming to _executionContext.MapPath can imply it is not just FullPath what can happen.

Forgind · 2021-04-18T15:26:39Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

+        /// Execution context used when task is supposed to run concurrently in multiple threads.
+        /// If null hosting process do not run this task concurrently and set it execution context on process level.
+        /// </summary>
+        public TaskExecutionContext ConcurrencyExecutionContext


Can this be an auto-property?

Forgind · 2021-04-18T15:56:22Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

@@ -3049,5 +3074,60 @@ public override bool Execute()
        }

        #endregion
+
+        void IConcurrentTask.ConfigureForConcurrentExecution(TaskExecutionContext executionContext)


nit: private

Forgind · 2021-04-18T16:09:26Z

src/Tasks/AssemblyDependency/TaskExecutionContextExtension.cs

+        /// <returns></returns>
+        public static string GetFullPath(this TaskExecutionContext taskExecutionContext,  string path)
+        {
+            if (String.IsNullOrEmpty(taskExecutionContext.StartupDirectory) || String.IsNullOrEmpty(path))


This is almost identical to FileUtilities.GetFullPath, though that does some extra escaping/unescaping. Do you think any of that is necessary here, and if so, should we just call that?

FileUtilities.GetFullPath take path and if it is relative absolutize it based on running process 'current directory' while
method takes path and if it is relative absolutize it based on taskExecutionContext.StartupDirectory.
@Forgind What escaping do you have on mind on particular?

FileUtilities.GetFullPath gives absolute path using the wrong current directory, without virtualization. Here we use taskExecutionContext.StartupDirectory as our current directory to complete the relative path (if it exists).

FileUtilities has an overload to let you specify currentDirectory:

msbuild/src/Shared/FileUtilities.cs

Lines 661 to 680 in 369631b

internal static string GetFullPath(string fileSpec, string currentDirectory)

{

// Sending data out of the engine into the filesystem, so time to unescape.

fileSpec = FixFilePath(EscapingUtilities.UnescapeAll(fileSpec));

// Data coming back from the filesystem into the engine, so time to escape it back.

string fullPath = EscapingUtilities.Escape(NormalizePath(Path.Combine(currentDirectory, fileSpec)));

if (NativeMethodsShared.IsWindows && !EndsWithSlash(fullPath))

{

if (FileUtilitiesRegex.IsDrivePattern(fileSpec) ||

FileUtilitiesRegex.IsUncPattern(fullPath))

{

// append trailing slash if Path.GetFullPath failed to (this happens with drive-specs and UNC shares)

fullPath += Path.DirectorySeparatorChar;

}

}

return fullPath;

}

Ohh. I wasn't aware of that. I believe this method is to be used during evaluation or other processing on unescaped data. Its name should be GetFullPathEscaped. Since changed code under review deals with already unescaped Task inputs, we can't use FileUtilities.GetFullPath for that matter.

Forgind · 2021-04-18T16:10:27Z

src/Tasks/AssemblyDependency/TaskExecutionContextExtension.cs

+
+namespace Microsoft.Build.Tasks
+{
+    public static class TaskExecutionContextExtension


I don't think so, at least as long as these methods are fixed rather than user-manipulatable. Should this explicitly extend TaskExecutionContext?

AR-May · 2021-04-19T15:31:42Z

In the issue, you mentioned both normalizing and denormalizing as necessary. I saw the first but not the second. Did you determine it's unnecessary?

I currently think denormalizing is not necessary, but I would like to ask about that more. It is my current understanding that all output is currently supposed to be absolute.
In the exp branch I also checked if we get any problems if we normalize all the output and insertion VS tests does not show any.

AR-May · 2021-04-19T17:25:28Z

Moved this PR to draft. We should merge this only with RAR as a Service feature PR, as it is meaningless to have this code otherwise. The future of RAR as a Service feature (when/if it will be merged) is currently unclear.

rokonec · 2021-04-15T10:14:45Z

src/Framework/TaskExecutionContext.cs

+        }
+    }
+
+    // TODO: move to own file


Suggested change

// TODO: move to own file

Lets keep it here.

rokonec · 2021-04-15T12:35:30Z

src/Tasks/AssemblyDependency/DirectoryResolver.cs

+            string directory = FullSearchPath;
+            string resolvedPath = ResolveFromDirectory(assemblyName, isPrimaryProjectReference, wantSpecificVersion, executableExtensions, directory, assembliesConsideredAndRejected);


Suggested change

string directory = FullSearchPath;

string resolvedPath = ResolveFromDirectory(assemblyName, isPrimaryProjectReference, wantSpecificVersion, executableExtensions, directory, assembliesConsideredAndRejected);

string resolvedPath = ResolveFromDirectory(assemblyName, isPrimaryProjectReference, wantSpecificVersion, executableExtensions, directory: FullSearchPath, assembliesConsideredAndRejected);

On the second though, I think we can have it even more readable by above changes:

rokonec · 2021-04-18T23:40:46Z

src/Tasks/AssemblyDependency/TaskExecutionContextExtension.cs

+        /// <returns></returns>
+        public static string GetFullPath(this TaskExecutionContext taskExecutionContext,  string path)
+        {
+            if (String.IsNullOrEmpty(taskExecutionContext.StartupDirectory) || String.IsNullOrEmpty(path))


FileUtilities.GetFullPath take path and if it is relative absolutize it based on running process 'current directory' while
method takes path and if it is relative absolutize it based on taskExecutionContext.StartupDirectory.
@Forgind What escaping do you have on mind on particular?

rokonec · 2021-04-20T18:54:55Z

src/Tasks/AssemblyDependency/TaskExecutionContextExtension.cs

+        /// <returns></returns>
+        public static string GetFullPath(this TaskExecutionContext taskExecutionContext,  string path)
+        {
+            if (String.IsNullOrEmpty(taskExecutionContext.StartupDirectory) || String.IsNullOrEmpty(path))


Ohh. I wasn't aware of that. I believe this method is to be used during evaluation or other processing on unescaped data. Its name should be GetFullPathEscaped. Since changed code under review deals with already unescaped Task inputs, we can't use FileUtilities.GetFullPath for that matter.

rokonec · 2021-05-03T14:38:51Z

src/Tasks/AssemblyDependency/RawFilenameResolver.cs

+
+                rawFileNameCandidate = executionContext.GetFullPath(rawFileNameCandidate);
+


Suggested change

rawFileNameCandidate = executionContext.GetFullPath(rawFileNameCandidate);

rawFileNameCandidate = executionContext.GetFullPath(rawFileNameCandidate);

NIT: unnecessary empty lines

rokonec · 2021-05-03T14:51:24Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

+                    }
+                    else
+                    {
+                        _concurrencyExecutionContext = new TaskExecutionContext();


Please comment here the intent of using default TaskExecutionContext behavior i.e. using 'process current directory' for relative paths.

rokonec · 2021-05-03T23:34:06Z

src/Tasks/AssemblyDependency/ReferenceTable.cs

@@ -438,7 +445,7 @@ string assemblyFileName

            if (!Path.IsPathRooted(assemblyFileName))
            {
-                reference.FullPath = Path.GetFullPath(assemblyFileName);
+                reference.FullPath = Path.GetFullPath(_executionContext.GetFullPath(assemblyFileName));


Maybe renaming to _executionContext.MapPath can imply it is not just FullPath what can happen.

rokonec · 2021-05-03T23:35:15Z

src/Tasks/AssemblyDependency/ReferenceTable.cs

@@ -1266,8 +1273,7 @@ private void ResolveReference
        (
            AssemblyNameExtension assemblyName,
            string rawFileNameCandidate,
-            Reference reference
-        )
+            Reference reference)


Suggested change

Reference reference)

Reference reference

)

Forgind · 2021-06-01T21:09:45Z

Is the future of RARaaS more clear? Wondering if we should close this temporarily until we're ready to go back to it.

AR-May · 2021-06-02T12:17:07Z

Work on "RAR as a service" was cut off, I close this PR.

AR-May added 4 commits March 31, 2021 22:28

add TaskExecutionContext to rar (from rokonec, revert later, when reb…

c229ca8

…asing on his changes)

current directory virtualization: processing inputs - in work

6ae2474

add unit tests

2e5e3b6

fixing failing unit test: fix cache (de)serialization

79c7b5e

AR-May marked this pull request as draft April 1, 2021 09:19

AR-May added 4 commits April 1, 2021 14:59

fix name, remove unused functions, move absolutization under the try …

a8310dc

…section.

remove unnecessary unit tests, fix unit tests.

7c398f1

Revert "remove unnecessary unit tests, fix unit tests."

5f2bec0

This reverts commit 7c398f1.

Revert "add unit tests"

5db2554

This reverts commit 2e5e3b6.

AR-May force-pushed the current-diretory-virtualization-3 branch from 5765e1c to 5db2554 Compare April 5, 2021 15:15

AR-May added 8 commits April 5, 2021 18:03

testing absolutization for inputs. this commit is for running ci test…

b1a8473

…s for this pr, remove it later.

Fix path absolutization in TaskExecutionContext

032bcba

Revert "testing absolutization for inputs. this commit is for running…

4e9a5da

… ci tests for this pr, remove it later." This reverts commit b1a8473.

add unit tests

3e64191

current directory virtualization: processing inputs - fixing bugs

82ab5c0

Small fixes: renames, etc.

68653c1

Fix paths' comparison in ResolveAssemblyReferenceTestFixture

dd32c79

fix GetFullPath function so it works for unix

d4f259c

AR-May added the WIP label Apr 9, 2021

AR-May added 4 commits April 9, 2021 15:36

Fix the file name of unit tests.

6c875b3

add comments

0510d9d

Turn on unit tests on unix.

935bdab

Moving functions from TaskExecutionContext to its extension, fixing c…

a2a35f0

…omments.

AR-May changed the title ~~[DRAFT] current directory virtualization for RAR task~~ Сurrent directory virtualization for RAR task Apr 13, 2021

AR-May marked this pull request as ready for review April 13, 2021 12:12

AR-May commented Apr 13, 2021

View reviewed changes

AR-May removed the WIP label Apr 15, 2021

Forgind reviewed Apr 18, 2021

View reviewed changes

AR-May marked this pull request as draft April 19, 2021 15:34

Small fix to a comment.

0c375f8

rokonec reviewed May 3, 2021

View reviewed changes

AR-May closed this Jun 2, 2021

AR-May deleted the current-diretory-virtualization-3 branch March 1, 2024 15:30


		string testPath = Path.Combine(Path.GetTempPath(), @"RawFileNameRelative");

		Directory.CreateDirectory(testPath);

		@@ -1,4 +1,5 @@
		using Microsoft.Build.Shared;
		using Microsoft.Build.Framework;

	internal static string GetFullPath(string fileSpec, string currentDirectory)
	{
	// Sending data out of the engine into the filesystem, so time to unescape.
	fileSpec = FixFilePath(EscapingUtilities.UnescapeAll(fileSpec));

	// Data coming back from the filesystem into the engine, so time to escape it back.
	string fullPath = EscapingUtilities.Escape(NormalizePath(Path.Combine(currentDirectory, fileSpec)));

	if (NativeMethodsShared.IsWindows && !EndsWithSlash(fullPath))
	{
	if (FileUtilitiesRegex.IsDrivePattern(fileSpec) \|\|
	FileUtilitiesRegex.IsUncPattern(fullPath))
	{
	// append trailing slash if Path.GetFullPath failed to (this happens with drive-specs and UNC shares)
	fullPath += Path.DirectorySeparatorChar;
	}
	}

	return fullPath;
	}

		string directory = FullSearchPath;
		string resolvedPath = ResolveFromDirectory(assemblyName, isPrimaryProjectReference, wantSpecificVersion, executableExtensions, directory, assembliesConsideredAndRejected);


		rawFileNameCandidate = executionContext.GetFullPath(rawFileNameCandidate);

Сurrent directory virtualization for RAR task #6316

Сurrent directory virtualization for RAR task #6316

Conversation

AR-May commented Apr 1, 2021 • edited Loading

Context

Changes Made

Testing

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AR-May Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AR-May commented Apr 19, 2021

AR-May commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Jun 1, 2021

AR-May commented Jun 2, 2021

AR-May commented Apr 1, 2021 •

edited

Loading

AR-May Apr 20, 2021 •

edited

Loading