Enable symlinks in ProjFS-Managed #73

narasamdya · 2021-10-16T06:53:29Z

In this PR we want to enable symlinks in ProjFS-Managed. This symlink feature is needed by our build system to properly treat symlinks used/produced during builds.

narasamdya · 2021-10-18T17:29:58Z

scripts/BuildProjFS-Managed.bat


 IF "%1"=="" (SET "Configuration=Debug") ELSE (SET "Configuration=%1")
-IF "%2"=="" (SET "ProjFSManagedVersion=0.2.173.2") ELSE (SET "ProjFSManagedVersion=%2")
+IF "%2"=="" (SET "ProjFSManagedVersion=1.2.19351.1") ELSE (SET "ProjFSManagedVersion=%2")


This change was only for testing.

erikmav · 2021-10-18T22:24:53Z

ProjectedFSLib.Managed.API/IDirectoryEnumerationResults.h

+    /// <c>true</c> if the entry was successfully added to the enumeration buffer, <c>false</c> otherwise.
+    /// </para>
+    /// </returns>
+    bool Add2(


I recommend just 'Add', the additional param should differentiate the overloads. #Closed

erikmav · 2021-10-18T22:26:13Z

ProjectedFSLib.Managed.props

    <!-- The real version is determined by the official build process.
         This version is a "built on dev box" special version. -->
-    <ProjFSManagedVersion>0.2.173.2</ProjFSManagedVersion>
+    <ProjFSManagedVersion>1.2.19351.1</ProjFSManagedVersion>


Should revert this, version number is updated by Windows team CI pipeline #Closed

erikmav · 2021-10-18T22:27:58Z

simpleProviderManaged/SimpleProviderManaged.csproj

    <CodeAnalysisRuleSet>MinimumRecommendedRules.ruleset</CodeAnalysisRuleSet>
  </PropertyGroup>

+  <PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|netcoreapp3.1|AnyCPU'">


Seems like all these can be consolidated into an true in the main property list #Closed

(tag formatting fail) Consolidate to single property in the main PropertyGroup without all these conditions

erikmav · 2021-10-18T22:30:16Z

ProjectedFSLib.Managed.API/DirectoryEnumerationResults.h

+        System::DateTime changeTime,
+        System::String^ symlinkTargetOrNull) sealed
+    {
+        if (System::String::IsNullOrEmpty(fileName))


Move all validation code common between original Add() and this overload into a helper method to avoid duplication #Closed

Note I do not recommend combining the 2 overloads into one method to avoid the data-dependent branch below for symlinkTargetOrNull

…s creation

erickulcyk · 2021-10-21T20:43:52Z

ProjectedFSLib.Managed.API/DirectoryEnumerationResults.h

+        System::DateTime lastAccessTime,
+        System::DateTime lastWriteTime,
+        System::DateTime changeTime,
+        System::String^ symlinkTargetOrNull) sealed


I wonder if we want an arg for link type, so eventually we can add directory symlink, hard link, etc... And then error for now if anything but symlink

The underlying ProjFS API has an extension point for other link types but is not filled in. I would minimize the API size for now, as a 3rd overload can always be added to deal with that if it happens.

As of now, we can use ProjFS API for file and directory symlinks. For other types of reparse point, like junction, the extended info provided in ProjFS API doesn't have any field to specify that.

erickulcyk · 2021-10-21T20:46:37Z

ProjectedFSLib.Managed.API/DirectoryEnumerationResults.h

+        System::DateTime lastAccessTime,
+        System::DateTime lastWriteTime,
+        System::DateTime changeTime,
+        System::String^ symlinkTargetOrNull) sealed


System::String^

what is the reason for some of these being pass by reference and not others?

Is it not the others are primitive types or structs? So pass-by-value is more appropriate, unless you need to modify them.

String instances in managed C++ are always refs since String is a class. The "by ref" here is passing the pointer to the heap String.

erickulcyk · 2021-10-21T20:48:56Z

simpleProviderManaged/FileSystemApi.cs

+    public sealed class FileSystemApi
+    {
+        [Flags]
+        public enum SymbolicLinkTarget : uint


SymbolicLinkTarget

Seems like the name here doesn't describe the vales. SymbolicLinkOptions?

erickulcyk · 2021-10-21T20:52:02Z

simpleProviderManaged/FileSystemApi.cs

+
+namespace SimpleProviderManaged
+{
+    public sealed class FileSystemApi


FileSystemApi

Was this code copied from somewhere? If so, should there be a central locaiton to use it from?

I think it resembles some code in BuildXL but not sure if we can consume it from there.
https://github.com/microsoft/BuildXL/blob/master/Public/Src/Utilities/Native/IO/Windows/FileSystem.Win.cs

@narasamdya is it correct? I guess I could trim it down a bit (remove enum entries not used in SimpleProvider) so it becomes just a file helper class for SimpleProvider. I'm not sure if we should take a dependency on BuildXL repo from this repo here

I cherry-picked it from BuildXL.

And, no, don't take any dependency from BuildXL here.

erickulcyk

narasamdya · 2021-10-25T19:34:56Z

ProjectedFSLib.Managed.Test/BasicTests.cs

            Assert.That("RandomNonsense", Is.Not.EqualTo(line));
        }

+        // We start the virtualization instance in each test case, so that exercises the following


/

May be add the following kind of test that involve both file and directory symlink:

├── Dir.lnk->Dir ├── Dir │ ├── file.lnk -> file.txt │ └── file.txt └── file.lnk -> Dir.lnk\file.lnk

Accessing file.lnk should result in Dir\file.txt.

narasamdya · 2021-10-25T19:36:08Z

simpleProviderManaged/ProjectedFileInfo.cs

    {
        public ProjectedFileInfo(
            string name,
+            string fullName,


fullName

Why do we need a fullName here?

We should be able to get the full path from the layerPath.

Name only contains the file name (missing the subpath relative to the layer root). We need to have either that subpath or the full path in order to pass them to TryGetTargetIfReparsePoint. I could also calculate and use that relative path (in the places where ProjectedFileInfo is initialized) instead of this property if that makes more sense?

narasamdya · 2021-10-25T19:46:49Z

simpleProviderManaged/SimpleProvider.cs

+                }
+                else if (Path.IsPathRooted(targetPath))
+                {
+                    string targetRelativePath = FileSystemApi.TryGetPathRelativeToRoot(this.layerRoot, targetPath, fileInfo.IsDirectory);


string targetRelativePath = FileSystemApi.TryGetPathRelativeToRoot(this.layerRoot, targetPath, fileInfo.IsDirectory);

Why do we need to get the relative path to root?

Do we have tests with rooted path target?

Yes, both of the existing symlink tests exercise this code path. I could also add more tests with un-rooted target paths (by updating Helpers.CreateVirtualSymlink).

yerudako · 2021-11-30T01:07:26Z

Status update - We are currently trying to see if we need a pool update or whether we have to add conditional compilation checks to make it build on the old build agent.

cgallred · 2021-11-30T01:51:29Z

ProjectedFSLib.Managed.API/DirectoryEnumerationResults.h

+            pin_ptr<const WCHAR> targetPath = PtrToStringChars(symlinkTargetOrNull);
+            extendedInfo.Symlink.TargetName = targetPath;
+
+            hr = ::PrjFillDirEntryBuffer2(m_dirEntryBufferHandle,


This now results in a linker error. Your header that defines the signatures and structures for the new APIs lets this compile. But this isn't being called through a dynamic import and ProjectedFSLib.Managed.props still references C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0 in several places instead of 19041.

Thanks! I reverted the conditional changes for now (in case this will work with a new agent pool). I also updated the props as you suggested. What is interesting, my Visual Studio is showing that it uses 10.0.22000.0

Except when I'm dealing with this project (which is not often), I don't use Visual Studio as anything other than a nice editor; everything I work on is in the OS repo and built with sources files and Razzle commands. So I don't know enough about how VS works to explain why it would show 22000 in your UI but 18362 in the props file.

SDK version 22000 is the Win11 SDK. My bet is that the worker pool you're using has moved to VS2022

(Or really your local VS is 2022 with the Win11 SDK installed as default)

…and adding conditional symbol declarations." This reverts commit 4f1ff1f.

yerudako · 2021-11-30T21:26:58Z

Another status update - in order to not mix too many changes into this PR, I opened #77 to track the build agent upgrade. I believe that unfortunately this PR will have to wait until the build agent runs with the new OS.

…nfo2Wrapper # Conflicts: # scripts/BuildProjFS-Managed.bat

This reverts commit f71b4d2.

cgallred · 2021-12-03T19:08:15Z

ProjectedFSLib.Managed.API/ApiHelper.cpp

        this->_PrjMarkDirectoryAsPlaceholder = reinterpret_cast<t_PrjMarkDirectoryAsPlaceholder>(::GetProcAddress(projFsLib,
                                                                                                                  "PrjMarkDirectoryAsPlaceholder"));
+        bool environmentSupportsSymlinks = false;
+        if (::GetProcAddress(projFsLib, "PrjWritePlaceholderInfo2") != nullptr)


Similar treatment needed for PrjFillDirEntryBuffer2. Then DirectoryEnumerationResults::Add needs to call the dynamically loaded version.

Ohhhh good catch!! I didn't notice that. Thank you Chris!

That might be the reason providers don't run on 2019. Let me try that.

I added conditional code to the enumeration, seems to work - although I am not 100% I did it the best way (not sure if I should call LoadLibrary there). Christian - could you please take a look and let me know what you think and if there's a better way to do that I will be happy to use that.

I have some changes that consolidate the conditional API use into the ApiHelper object, and that fix the bug in SimpleProvider that was the actual cause of the InsufficientBuffer error on .NET Framework but not .NET Core. I don't have permission to push to your branch though.

Oh sorry for that - I am not sure why GitHub does not allow it - I can merge your branch into this branch or cherry-pick the commit if you could send their hashes? Thanks Christian!

@cgallred, do you know why, with the current implementation of SimpleProvider, TestEnumerationInVirtualizationRoot that includes directory enumeration, but not symlinks, passes in both .NET core and framework?

I'd guess that the case doesn't hit the problem scenario. It may be able to fit all the enumeration entries in a single buffer, so it doesn't provoke a repeat call to the callback. Enumeration that includes symlinks takes up more space in the enumeration buffer.

…ion callbacks

Signed-off-by: yerudako <yerudako@gmail.com>

This reverts commit 6d632b5.

yerudakoms and others added 7 commits October 15, 2021 12:53

Define types

e1ff2f1

Add some wrapper code

d2732f3

Fix some bugs

1872d50

Update to make the version appear correct

63470a3

Changes need to play symlink in SimpleProvider

8783182

Removed unused function

3d8322e

Merge pull request #1 from narasamdya/user/yerudako/AddPrjInfo2Wrapper

1354f2b

narasamdya commented Oct 18, 2021

View reviewed changes

erikmav suggested changes Oct 19, 2021

View reviewed changes

yerudakoms added 2 commits October 20, 2021 20:10

Fixes for CR comments: rename and refactor DirectoryEnumerationResult…

d280d37

…s creation

CR fixes: consolidate SimpleProviderManaged properties

d86fce1

erikmav approved these changes Oct 21, 2021

View reviewed changes

yerudakoms added 3 commits October 21, 2021 10:26

Add a draft of a symlink test and some test helpers

95e678f

Add FileApi code for relative path calculation

d37b45a

Fix the TestCanReadSymlinksThroughVirtualizationRoot

76c52e5

erickulcyk reviewed Oct 21, 2021

View reviewed changes

erickulcyk approved these changes Oct 21, 2021

View reviewed changes

yerudakoms added 3 commits October 21, 2021 17:30

Fix symlink enumeration for subdirectories

744775b

Add symlink directory tests

aa8b624

Minor cleanup

2a7bedd

narasamdya commented Oct 25, 2021

View reviewed changes

yerudakoms and others added 2 commits October 25, 2021 17:21

Add more tests to ensure proper symlink coverage

ded1c29

Final cleanup

c609a20

narasamdya marked this pull request as ready for review November 4, 2021 17:04

narasamdya changed the title ~~[DRAFT] Enable symlink in ProjFS-Managed~~ Enable symlink in ProjFS-Managed Nov 4, 2021

Try including VS 2022 into the product selector

6f65d6c

cgallred requested changes Nov 30, 2021

View reviewed changes

yerudakoms and others added 7 commits November 29, 2021 18:06

Revert "Fix the build agent failures by making the tests conditional …

7aec0cb

…and adding conditional symbol declarations." This reverts commit 4f1ff1f.

Fix versions in props

64de528

Add a simple script to install .net sdk 461

55e0cb8

Revert the script change

0a200fc

Add .net 461 reference assemblies

61db53a

Install the Reference Assemblies before Managed project compiles

ca6b60f

Cherry-pick f156306 in order to unblock VS version checks

ff7cd45

yerudako mentioned this pull request Nov 30, 2021

The build agent needs an upgrade to build against the new ProjFS API #77

Closed

yerudako added 8 commits November 30, 2021 17:33

Fix the directory enumeration logic in tests

711c3d8

Merge remote-tracking branch 'origin/main' into user/yerudako/AddPrjI…

2882373

…nfo2Wrapper # Conflicts: # scripts/BuildProjFS-Managed.bat

Revert "Add .net 461 reference assemblies"

de03965

This reverts commit f71b4d2.

Add SimpleProvider's handling of non-symlink environments

b095608

Add code to ignore symlink tests on envs not supporting symlinks

e7697ae

Simplify the test exclusion code

9ffe50a

Condition _PrjWritePlaceholderInfo2 out of the ApiHelper initialization

b0c34da

Minor changes

0f1758b

yerudako mentioned this pull request Dec 3, 2021

DirectoryEnumerationResults.Add symlink override is incompatible with EnumerateFileSystemInfos on .NET Framework #81

Closed

Add a link to an issue about .net framework incompatibility

0bdde8a

cgallred reviewed Dec 3, 2021

View reviewed changes

yerudako and others added 5 commits December 3, 2021 11:56

Add conditional _PrjFillDirEntryBuffer2 pointing code to the enumerat…

c3b8376

…ion callbacks

Consolidate API version handling

6d632b5

Signed-off-by: yerudako <yerudako@gmail.com>

Revert "Consolidate API version handling"

34e010f

This reverts commit 6d632b5.

(cgallred) Consolidate-API-version-handling

3417d3a

(cgallred) Fix-enumeration-handling

12a50c8

cgallred approved these changes Dec 6, 2021

View reviewed changes

cgallred merged commit 5d047db into microsoft:main Dec 6, 2021

Enable symlinks in ProjFS-Managed #73

Enable symlinks in ProjFS-Managed #73

Uh oh!

Conversation

narasamdya commented Oct 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikmav Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikmav Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikmav Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikmav Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narasamdya Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erickulcyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narasamdya Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yerudako Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yerudako commented Nov 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

narasamdya commented Oct 16, 2021 •

edited

Loading

erikmav Oct 18, 2021 •

edited

Loading

erikmav Oct 18, 2021 •

edited

Loading

erikmav Oct 18, 2021 •

edited

Loading

erikmav Oct 18, 2021 •

edited

Loading

narasamdya Oct 25, 2021 •

edited

Loading

narasamdya Oct 25, 2021 •

edited

Loading

yerudako Oct 25, 2021 •

edited

Loading

narasamdya Dec 6, 2021 •

edited

Loading