Add support for symlink files embedding to binlog #8213

JanKrivanek · 2022-12-02T14:50:38Z

Fixes #6773

Context

Symlinked files were not embedded into binlog

Changes Made

Add detection of symlinked files and switch to their target
Support not added for full FW

Testing

Unit test added, attempting to embed symlinked file (failing without the fix)

src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs

…inks

Forgind · 2022-12-09T19:17:44Z

src/Framework/NativeMethods.cs

+
+        WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();
+
+        return NativeMethods.GetFileAttributesEx(fullPath, 0, ref data) &&


Can this be simplified? I think you can check the reparse point on any framework and platform, and it's just as fast, right? Like you should be able to do this everywhere:
(File.GetAttributes(destFile) & FileAttributes.ReparsePoint) != 0

The FileInfo takes care about proper handling the logic on other platforms (e.g. https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs#L653). So I believe simplification is not possible without impact on functionality.
Please let me know if I misunderstood.

But - there can be another simplification

I realized and verified that FileStream ctor already does the proper 'see through' thing with the symlinks.
So the change can be limited to just verifying that FileInfo has nonzero content or it's a symlink.... Updated.

Forgind · 2022-12-09T20:11:54Z

src/Framework/NativeMethods.cs

+
+        if (IsSymLink(filePath))
+        {
+            using SafeFileHandle handle = OpenFileThroughSymlinks(filePath);


For these sorts of errors, you should be able to add [SupportedOSPlatform("windows")] to the relevant methods.

Forgind · 2022-12-15T18:53:29Z

documentation/wiki/Building-Testing-and-Debugging-on-Full-Framework-MSBuild.md

@@ -25,6 +25,10 @@ To run the unit tests from Visual Studio:

 To build MSBuild and run all unit tests from the command line, use `.\build.cmd -test`.

+Some tests are creating symlinks to test associated functionality - in order for them to succeed you have two options:
+* Run those tests elevated


Is there still a test that fails if you run elevated then run not-elevated? If so, might want to mention that and how to fix it.

Creating symlinks require elevation or developer mode on each run.

Would you suggest different wording informing about this behavior?

I'd put the option to enable developer mode first, and I'd add a warning for running tests elevated that if you then run not elevated, you have to do the following steps (and list out any tests that fail with running elevated --> not elevated along with an explanation of how to fix it—I think there was just one, and it could be fixed by deleting some directory...or you could fix the test to clean up properly.)

Switched the order of options.

I'm not aware of issues with tests when switching elevation mode. The newly added test - BinaryLoggerShouldEmbedSymlinkFilesViaTaskOutput - uses transient files. The pre-existing test that uses symlinks - DoNotFollowRecursiveSymlinks cleans-up after itself as well.
Am I missing any other case?
If we have any - I'd definitely prefer to do a proper cleanup in tests rather than scare reader by extra steps in wiki. Please point me :-)

@Forgind I've run all tests elevated and then without elevation and I do not observe any issues. Did you have any specific test or code issue in mind?
If not - do you feel there is any further improvement that can be made to the touched code or doc?

TestCache2 was the one I had in mind that had failed recently due to admin --> not admin running of the test. I believe it specifically failed if a particular folder did not exist prior to the test's execution, then you ran the test as an admin, then you ran the test as a normal user. It's possible that's been fixed since that came up, though; it was a while ago.

This seems to be addressed here: #8122

Do you feel there are any further improvement that can be made in scope of this PR?

Yup; I forgot about that. That's all I can think of.

src/Framework/NativeMethods.cs

KalleOlaviNiemitalo · 2023-01-04T10:22:10Z

src/Framework/NativeMethods.cs

+
+        return NativeMethods.GetFileAttributesEx(fileInfo.FullName, 0, ref data) &&
+               (data.fileAttributes & NativeMethods.FILE_ATTRIBUTE_DIRECTORY) == 0 &&
+               (data.fileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == FILE_ATTRIBUTE_REPARSE_POINT;


This could incorrectly return true for a file that is a reparse point but not a symbolic link.

One could use either GetFileInformationByHandleEx FILE_ATTRIBUTE_TAG_INFO or DeviceIoControl FSCTL_GET_REPARSE_POINT for reading the attributes and the reparse tag, and then compare to IO_REPARSE_TAG_SYMLINK; PowerShell uses FSCTL_GET_REPARSE_POINT. However, these functions would require first opening the file.

To get the file attributes and reparse tag without opening the file, one can use either FindFirstFileW or, starting from Windows 10 version 1709, NtQueryInformationByName FILE_STAT_INFORMATION. I assume that NtQueryInformationByName would be the most efficient of these, because it doesn't require opening and closing a handle to the parent directory.

@KalleOlaviNiemitalo Thank you for pointing to this and for the detailed suggestion!

I'm thinking about keeping the attribute check to filter out 'normal files' scenarios and in case of detecting reparse point querying further (like by obtaining reparse tag via DeviceIoControl - similarly how this is done in runtime: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs#L574-L609).

The FindFirstFile seems to be very neat way to get this info in single shot. This feels unnecessary heavy though. I haven't done any measuring though.

The methods in PowerShell and .NET Runtime use FSCTL_GET_REPARSE_POINT which also retrieves the target string of the symbolic link. If you don't need that, I suspect GetFileInformationByHandleEx FILE_ATTRIBUTE_TAG_INFO could be lighter weight as it could avoid the string copy.

KalleOlaviNiemitalo · 2023-01-04T14:35:16Z

src/Framework/NativeMethods.cs

+        else
+        {
+            symbolicLinkCreated = symlink(exitingFileName, newFileName) == 0;
+            errorMessage = symbolicLinkCreated ? null : "The link() library call failed with the following error code: " + Marshal.GetLastWin32Error();


It's the symlink call, not the link call. Not a new bug, though.

Very valid. Linked this to a PR taking care of localizing the message

Fixes #6773 Context Supersedes #8213 and #8282 Symlinked files were not embedded into binlog - previous solution was too much focused on symlinks and hence requried nontrivial code to properly distinguish aymlinks with available contnet. Alternate approach - proceed with adding file as soon as it has available content Testing Preexisting unit test is excercising the scenario. Added a case for empty file - that still should not be added.

Add support for symlink files embedding to binlog

f3a9a69

Forgind reviewed Dec 2, 2022

View reviewed changes

src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs Outdated Show resolved Hide resolved

rainersigwald reviewed Dec 5, 2022

View reviewed changes

src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs Outdated Show resolved Hide resolved

src/Build/Logging/BinaryLogger/ProjectImportsCollector.cs Outdated Show resolved Hide resolved

JanKrivanek added 2 commits December 9, 2022 12:10

Merge remote-tracking branch 'upstream/main' into proto/bl-embed-syml…

8b6341a

…inks

Make symlink functionality available for FullFW as well

296ba00

JanKrivanek requested review from rainersigwald and Forgind December 9, 2022 16:56

Add null check

c1e837b

Forgind reviewed Dec 9, 2022

View reviewed changes

Simplify the change, add doc

884fde8

JanKrivanek requested a review from Forgind December 13, 2022 14:12

JanKrivanek and others added 3 commits December 13, 2022 20:45

Fix wording

202a929

Merge branch 'main' into proto/bl-embed-symlinks

95706ab

Fix platform constraint

870155c

JanKrivanek force-pushed the proto/bl-embed-symlinks branch from 4ab113f to 870155c Compare December 14, 2022 17:44

Clarify wiki

93504d7

Forgind reviewed Dec 15, 2022

View reviewed changes

JanKrivanek requested a review from Forgind December 16, 2022 17:21

Add support for recursive symlinks

b94eadd

JanKrivanek force-pushed the proto/bl-embed-symlinks branch from 31c2402 to b94eadd Compare December 16, 2022 17:36

Switched options in doc

ee5fc55

Forgind approved these changes Dec 27, 2022

View reviewed changes

JanKrivanek merged commit ac0911a into dotnet:main Jan 3, 2023

JanKrivanek deleted the proto/bl-embed-symlinks branch January 3, 2023 16:16

KalleOlaviNiemitalo reviewed Jan 4, 2023

View reviewed changes

This was referenced Jan 4, 2023

Localize strings of link error messages on non-Windows #8238

Merged

Fix NativeMethods.IsSymLink #8282

Closed

JanKrivanek mentioned this pull request Jan 12, 2023

Handle symlinks embedding into binlogs - approach #2 #8306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for symlink files embedding to binlog #8213

Add support for symlink files embedding to binlog #8213

JanKrivanek commented Dec 2, 2022

Forgind Dec 9, 2022

JanKrivanek Dec 13, 2022

JanKrivanek Dec 13, 2022

Forgind Dec 9, 2022

Forgind Dec 15, 2022

JanKrivanek Dec 16, 2022

Forgind Dec 16, 2022

JanKrivanek Dec 19, 2022 •

edited

Loading

JanKrivanek Dec 21, 2022

Forgind Dec 22, 2022

JanKrivanek Dec 23, 2022

Forgind Dec 27, 2022

KalleOlaviNiemitalo Jan 4, 2023

JanKrivanek Jan 4, 2023

KalleOlaviNiemitalo Jan 4, 2023

KalleOlaviNiemitalo Jan 4, 2023

JanKrivanek Jan 4, 2023


		WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();

		return NativeMethods.GetFileAttributesEx(fullPath, 0, ref data) &&

Add support for symlink files embedding to binlog #8213

Add support for symlink files embedding to binlog #8213

Conversation

JanKrivanek commented Dec 2, 2022

Context

Changes Made

Testing

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

JanKrivanek Dec 19, 2022 • 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

JanKrivanek Dec 19, 2022 •

edited

Loading