Share file system caches via the EvaluationContext by cdmihai · Pull Request #5546 · dotnet/msbuild

cdmihai · 2020-07-25T02:12:40Z

Made IFileSystem public. Added some extra methods to it from QuickBuild's file system interface. This interface will also be passed to the future project cache interface so that caches can reuse IO from evaluation and between themselves.
EvaluationContext can take an IFileSystem. This enables QuickBuild and VisualStudio to share their file system caches with msbuild evaluations.

Possible Next steps (future PRs):

Pass the interface to sdk resolvers in a non breaking manner. AFAIK they do a lot of IO
I saw some evaluation bits that do IO without passing through the interface, so propagating the interface to those places as well seems like a potential perf gain.

cdmihai · 2020-07-25T02:18:13Z

@lifengl @dfederm

cdmihai · 2020-07-28T02:17:33Z

@Forgind, @rainersigwald
The PR fails this curious looking test:

msbuild/src/Build.OM.UnitTests/NugetRestoreTests.cs

Lines 21 to 26 in e8338f5

    
                   // This NuGet version cannot locate other assemblies when parsing solutions at restore time. This includes localized strings required in debug mode. 
        
                   // NuGet version 4.1.0 was somewhat arbitrarily chosen. 3.5 breaks with an unrelated error, and 4.8.2 does not fail when a new dependency is introduced. This is a safe middle point. 
        
           #if !DEBUG 
        
                   [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp)] 
        
                   [Fact] 
        
                   public void TestOldNuget()

What's the idea behind the test? We can't ever break that buggy old nuget version? :)

The change that breaks it is that on master, IFileSystem is included in every MSBuild dll, but in this PR I am making it public, so I have to put it in only a single place as opposed to inserting it in every dll, otherwise C# fails with ambiguous typing errors. This breaks the old nuget version, but the latest nuget works.

How should I go around it? Can I just drop a binding redirect file next to the old nuget.exe to make it bind all msbuild assemblies to the correct ones (copying msbuild's binding redirect next to it makes it work)?

dfederm · 2020-07-28T02:48:22Z

In general you seem to just have taken Quickbuild's impl even the parts which make no sense for MSBuild.

You should cater MSBuild's interface for MSBuild alone. Adapters will be used to bridge the gap.

dfederm · 2020-07-28T02:43:55Z

src/MSBuildTaskHost/FileSystem/MSBuildTaskHostFileSystem.cs

            return NativeMethodsShared.FileOrDirectoryExists(path);
        }

+        public void ClearCaches() { }


Why is this part of the interface if the impl is empty and it's never called?

Same with the write state method below

I also wasn't sure about this one, so left it for review comments :)

Having a method to clear caches would be useful in situations where replacing the entire IFileSsytem object is not possible (the msbuild xml cache suffers from this pain). But this is speculative, since no particular situation comes to mind.

I'm fine with leaving it or removing it, but if you leave it, I'd prefer for it to be implemented. I don't know if the intention is for a user to be able to call this via IFileSystem, but if so, they would be confused that it doesn't do anything. (And I would be, too, if I saw the method existed and tried to call it but couldn't figure out what exactly it did until I looked at it.)

dfederm · 2020-07-28T02:45:25Z

src/MSBuildTaskHost/FileSystem/MSBuildTaskHostFileSystem.cs

+        public void ClearCaches() { }
+        public void WriteStatistics(TextWriter writer) { }
+
+        public FileAttributes GetAttributesNoMissingException(string path)


This seems like an awkward API to expose

Yea, it's pretty funky. I'll remove the part with the exception and leave it a simple wrapper over File.GetAttributes

dfederm · 2020-07-28T02:46:11Z

ref/Microsoft.Build.Framework/net/Microsoft.Build.Framework.cs

+    public partial interface IFileSystem
+    {
+        void ClearCaches();
+        bool DirectoryEntryExists(string path);


What's a "directory entry" vs just a directory?

An entry is either a file or a directory. Took the naming from this BCL method. Is FileOrDirectoryExists better? Longer but explicit.

dfederm · 2020-07-28T02:46:53Z

ref/Microsoft.Build.Framework/net/Microsoft.Build.Framework.cs

+        System.IO.FileAttributes GetAttributesNoMissingException(string path);
+        System.IO.Stream GetFileStream(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share);
+        System.IO.TextReader ReadFile(string path);
+        byte[] ReadFileAllBytes(string path);


This doesn't seem like something MSBuild needs?

But it does seem useful for sdk resolvers and cache plugins. Both read files, and probably both will re-read the same files over and over. For example the .net sdk keeps cracking open the same json over and over again AFAIK. The cache plugins that hash files will also probably re-read some files.

Forgind · 2020-07-28T03:40:10Z

@Forgind Nathan Mytelka FTE, @rainersigwald Rainer Sigwald FTE
The PR fails this curious looking test:

msbuild/src/Build.OM.UnitTests/NugetRestoreTests.cs

Lines 21 to 26 in e8338f5

// This NuGet version cannot locate other assemblies when parsing solutions at restore time. This includes localized strings required in debug mode.

// NuGet version 4.1.0 was somewhat arbitrarily chosen. 3.5 breaks with an unrelated error, and 4.8.2 does not fail when a new dependency is introduced. This is a safe middle point.

#if !DEBUG

[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp)]

[Fact]

public void TestOldNuget()

What's the idea behind the test? We can't ever break that buggy old nuget version? :)

In short, yes. Some enterprise customers rely on an older version of NuGet that can't follow dependencies as effectively. They use reflection to load a solution as part of restore, so even if building works, it will throw an error before it gets there.

The change that breaks it is that on master, IFileSystem is included in every MSBuild dll, but in this PR I am making it public, so I have to put it in only a single place as opposed to inserting it in every dll, otherwise C# fails with ambiguous typing errors. This breaks the old nuget version, but the latest nuget works.

How should I go around it? Can I just drop a binding redirect file next to the old nuget.exe to make it bind all msbuild assemblies to the correct ones (copying msbuild's binding redirect next to it makes it work)?

The old NuGet version didn't know that it needed to load Framework at all (for the restore—it clearly knows how to load it for building), so a binding redirect won't help. There can't be any reference to anything in Framework as part of loading a solution.

Forgind · 2020-07-28T04:00:48Z

Nothing jumped out at me as obviously the point at which restore needed IFileSystem, but it's used in so many places, it's probably indirect. I was surprised FileUtilities isn't directly called from SolutionFile, but there's probably somewhere in ProjectInstance or SolutionFile that indirectly calls a relevant FileUtilities method or similar.

I don't know nearly enough about this to know if this even makes sense, but is there some way you can leave IFileSystem where it is but pass as a parameter to its constructor a delegate with the file system cache from visual studio/quickbuild baked into it?

cdmihai · 2020-07-28T17:48:50Z

@dfederm

In general you seem to just have taken Quickbuild's impl even the parts which make no sense for MSBuild.
You should cater MSBuild's interface for MSBuild alone. Adapters will be used to bridge the gap.

I think having a reasonably complete interface (enumerations, existence checks, reading, file attributes) would be good here:

The interface won't be used by MSBuild alone. The new project cache plugins will use it, and I was thinking to also pass it to sdk resolvers which do a lot of IO.
IFileSystem is relatively new to MSBuild. My intent was / is to slowly move as much msbuild IO as possible under this interface. Initially I mostly did evaluation (the convenient parts, and the profiling hot paths). So even if MSBuild does not use it much today, my intent is to have it use it more and more.

But I agree there's a fine line between what's useful and what's not.

cdmihai · 2020-07-28T18:31:24Z

@Forgind

Some enterprise customers rely on an older version of NuGet that can't follow dependencies as effectively. They use reflection to load a solution as part of restore, so even if building works, it will throw an error before it gets there.

The simplest solution here is to just break the things that reflect into non-public APIs :). I would say the contract for reflecting into private state is that one shall expect and react to breaking changes whenever they happen. The alternative is code freezing the entire code base over the long run (might be a good way to deprecate MSBuild though ...)

Is there some telemetry or other data we can use to learn how many users still use the old nuget? If they are inside Microsoft, we could push them to update. If they are outside, maybe they've updated in the meantime? Do you know a good contact to make such a query?

The old NuGet version didn't know that it needed to load Framework at all (for the restore—it clearly knows how to load it for building), so a binding redirect won't help. There can't be any reference to anything in Framework as part of loading a solution.

The sdk resolver interfaces were put in Framework as well. They should have also broken the old nuget scenario. I say it's a good precedent to break it again :)

Possible ways to workaround this:

try putting the interface in Utilities
rename the public interface to something else (IMSBuildFileSystem) and write an adapter to the now legacy IFileSystem
drop / postpone the PR. This isn't a blocker to project cache plugins as far as I can tell. But would have been a nice thing to have.

is there some way you can leave IFileSystem where it is but pass as a parameter to its constructor a delegate with the file system cache from visual studio/quickbuild baked into it?

The delegate would have to reference a new type from Framework right? And the old nuget requirement is that one cannot add new apis to Framework.

Forgind · 2020-07-28T18:47:41Z

Is there some telemetry or other data we can use to learn how many users still use the old nuget? If they are inside Microsoft, we could push them to update. If they are outside, maybe they've updated in the meantime? Do you know a good contact to make such a query?

I think they were external, but @rrelyea would know more.

The sdk resolver interfaces were put in Framework as well. They should have also broken the old nuget scenario. I say it's a good precedent to break it again :)

If they weren't required for restore, and they weren't referenced in a method required by restore, I think it was ok. There are certainly some things that can be in Framework and used by other parts of MSBuild that don't affect it.

Possible ways to workaround this:

try putting the interface in Utilities

rename the public interface to something else (IMSBuildFileSystem) and write an adapter to the now legacy IFileSystem

drop / postpone the PR. This isn't a blocker to project cache plugins as far as I can tell. But would have been a nice thing to have.

I don't remember if Utilities was loaded by the old NuGet, but it's worth a try. Option 2 is probably the easiest and most likely to work, but it's also a bit messy...

The delegate would have to reference a new type from Framework right? And the old nuget requirement is that one cannot add new apis to Framework.

That isn't necessarily problematic, but I imagine you could use something like string -> object and cast the output appropriately. That should work without needing to refer to any MSBuild types in relevant code paths.

cdmihai · 2020-07-28T21:17:16Z

I tried option 1 but that still broke the old nuget.exe, so I did option 2.

If they weren't required for restore, and they weren't referenced in a method required by restore, I think it was ok. There are certainly some things that can be in Framework and used by other parts of MSBuild that don't affect it.

Sdk resolvers are required during restore because they are called during evaluation.

something like string -> object and cast the output appropriately. That should work without needing to refer to any MSBuild types in relevant code paths.

IMHO that would leak too much implementation detail, raising maintenance pain in the future.

rrelyea · 2020-07-29T02:02:56Z

@skofman1 and her team run nuget.org and could give data about how many users use different nuget.exe versions.

rainersigwald · 2020-07-29T18:47:56Z

It's not just "how many people use old NuGet.exe". It's also "who are those people and do they know they're doing it?". The bug that caused that test to be written caused an Azure DevOps outage, because AzDO defaulted to an old version of NuGet. In addition, several customers had slightly-funky workflows like "restore all packages online, then disable the network and build stuff" that broke and didn't have an easy way to update NuGet versions.

I don't think we can break that version of NuGet/that test. Maybe for 17.0.

cdmihai · 2020-07-29T20:23:34Z

It's also "who are those people and do they know they're doing it?"

Well, at some point they are going to break no matter what, so I'd rather have a slow trickle of breakages to keep the systems fit, well tested, and proactive. It's for their own good :). Joke aside, the workaround turned out to not be that horrible, as long as people follow the instructions in the interface comments and don't let them diverge (which realistically means that given enough time the interfaces will diverge, but that's a problem for future us).

cdmihai · 2020-08-06T01:16:28Z

@dfederm Removed the obviously awkward APIs. Remaining ones seem reasonable to me for a file system API.

cdmihai · 2020-08-06T21:24:33Z

/azp run

azure-pipelines · 2020-08-06T21:24:38Z

No pipelines are associated with this pull request.

cdmihai · 2020-08-06T21:27:46Z

/azp run

azure-pipelines · 2020-08-06T21:27:51Z

No pipelines are associated with this pull request.

cdmihai · 2020-08-10T17:51:10Z

/azp run

azure-pipelines · 2020-08-10T17:51:16Z

No pipelines are associated with this pull request.

cdmihai · 2020-08-10T17:51:49Z

/azp run

azure-pipelines · 2020-08-10T17:51:55Z

No pipelines are associated with this pull request.

Forgind · 2020-08-10T17:55:05Z

@cdmihai, our CIBuild isn't working for some reason for any PR. @benvillalobos is looking into it.

benvillalobos · 2020-08-10T20:40:33Z

Closing to reopen and trigger a CI build. See #5646

Forgind

LGTM!

src/Build.UnitTests/Evaluation/ItemSpec_Tests.cs

src/Build/Evaluation/Context/EvaluationContext.cs

src/Build/FileSystem/MSBuildFileSystemAdapter.cs

Forgind · 2020-08-14T19:49:10Z

src/MSBuildTaskHost/FileSystem/MSBuildTaskHostFileSystem.cs

            return NativeMethodsShared.FileOrDirectoryExists(path);
        }

+        public void ClearCaches() { }


I'm fine with leaving it or removing it, but if you leave it, I'd prefer for it to be implemented. I don't know if the intention is for a user to be able to call this via IFileSystem, but if so, they would be confused that it doesn't do anything. (And I would be, too, if I saw the method existed and tried to call it but couldn't figure out what exactly it did until I looked at it.)

src/Shared/FileSystem/IFileSystem.cs

src/Shared/FileSystem/MSBuildOnWindowsFileSystem.cs

ladipro

How likely do you think it is that in the future we'll find ourselves wishing the interface had more and/or different methods? If such a need arises, is the versioning plan simply to add IMSBuildFileSystem2, IMSBuildFileSystem3, ...?

src/Shared/FileSystem/WindowsFileSystem.cs

src/Shared/UnitTests/ObjectModelHelpers.cs

src/Build/FileSystem/MSBuildFileSystemAdapter.cs

cdmihai · 2020-08-18T20:43:59Z

How likely do you think it is that in the future we'll find ourselves wishing the interface had more and/or different methods? If such a need arises, is the versioning plan simply to add IMSBuildFileSystem2, IMSBuildFileSystem3, ...?

@ladipro I am hoping we'll be able to take advantage of the new C# default interface member feature. If that's not feasible (@rainersigwald?), then I would rather have them be abstract classes instead of interfaces.

src/Shared/UnitTests/ObjectModelHelpers.cs

ladipro

Looks good! We also talked about adding a method/methods to MSBuildFileSystemBase for retrieving file timestamps. Do you think it's worth doing now?

cdmihai · 2020-08-28T16:57:04Z

src/Shared/FileSystem/MSBuildOnWindowsFileSystem.cs

I'll replace this with WindowsFileSystem in a later PR when I actually hook this method up in the evaluator

rainersigwald · 2020-08-28T20:30:29Z

src/Build.UnitTests/Definition/ProjectEvaluationContext_Tests.cs

Discussed offline but for posterity:

It feels wrong to pass a filesystem to the context and pass the context in multiple places and have the filesystem used in only one of them. Can we instead error?

src/Build/Resources/Strings.resx

Deprecate IFileSystem to follow MSBuildFileSystemBase. Ideally we should remove IFileSystem but doing so breaks some old nuget.exe version.

…ility constraints (#6475) Progress towards #6068 Context MSBuildFileSystemBase was introduced as a replacement of IFileSystem in #5546. It was added as an abstract class so potentially with the same constraints as an interface: Versioning constraints. We can't add a new public member to the class, unless we make it non-abstract and provide a default implementation of new members. Usability constraints. It is not possible to use MSBuildFileSystemBase to intercept calls or override a subset of them because the default MSBuild-provided file system is not exposed. Changes Made To address the constraints above, I am making the class non-abstract and having it by default forward all calls to the default MSBuild file system. This way the caller may choose to override only a subset of methods (e.g. file enumeration and existence/metadata checks) leaving the rest as is (e.g. actual file reads). As a result I was able to delete both IFileSystemAdapter.cs and MSBuildFileSystemAdapter.cs because they're not needed anymore. The difference between IFileSystem methods and public MSBuildFileSystemBase methods was just one method having a different name. I have renamed the one on the internal interface, assuming that this does not cause any troubles with the old NuGet, as tested by TestOldNuget(). This enabled further simplification and reduce the number of the "adapter" call hops. With the changes in this PR, we have: An internal interface IFileSystem, kept around for compat reasons. A public class MSBuildFileSystemBase, which implements IFileSystem with virtual methods. The class is meant to be derived from and its methods overridden. If we find ourselves adding new methods to the contract, it won't break existing MSBuildFileSystemBase because the new calls will just call through to the default implementation. If the change we need to make requires an orchestrated behavior change, we can always add a virtual 'version' prop, overriding which the derived class tells us which version of the contract it supports. Testing Existing unit test coverage. Notes This change is meant to be non-breaking. I'm basically just making an abstract class non-abstract (would have to be done at some point anyways if we were to add a new method). The rest of the changes are internal.

cdmihai force-pushed the iSystemData branch from 2c8bfb7 to aa27ff7 Compare July 27, 2020 17:30

dfederm suggested changes Jul 28, 2020

View reviewed changes

cdmihai force-pushed the iSystemData branch from ebcfb97 to 9e2020d Compare August 6, 2020 18:35

cdmihai closed this Aug 6, 2020

cdmihai reopened this Aug 6, 2020

cdmihai closed this Aug 10, 2020

cdmihai reopened this Aug 10, 2020

benvillalobos closed this Aug 10, 2020

benvillalobos reopened this Aug 10, 2020

Forgind approved these changes Aug 14, 2020

View reviewed changes

ladipro reviewed Aug 18, 2020

View reviewed changes

src/Shared/FileSystem/WindowsFileSystem.cs Outdated Show resolved Hide resolved

src/Shared/UnitTests/ObjectModelHelpers.cs Outdated Show resolved Hide resolved

src/Build/FileSystem/MSBuildFileSystemAdapter.cs Outdated Show resolved Hide resolved

benvillalobos reviewed Aug 20, 2020

View reviewed changes

src/Shared/UnitTests/ObjectModelHelpers.cs Outdated Show resolved Hide resolved

benvillalobos reviewed Aug 20, 2020

View reviewed changes

src/Shared/UnitTests/ObjectModelHelpers.cs Outdated Show resolved Hide resolved

ladipro approved these changes Aug 28, 2020

View reviewed changes

cdmihai commented Aug 28, 2020

View reviewed changes

rainersigwald reviewed Aug 28, 2020

View reviewed changes

src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved

rainersigwald approved these changes Aug 28, 2020

View reviewed changes

cdmihai added 2 commits August 28, 2020 14:49

Expose MSBuildFileSystemBase public api

35379b2

Deprecate IFileSystem to follow MSBuildFileSystemBase. Ideally we should remove IFileSystem but doing so breaks some old nuget.exe version.

EvaluationContext can be passed in a user file system

fd3f643

cdmihai force-pushed the iSystemData branch from 109a5f9 to fd3f643 Compare August 28, 2020 21:53

cdmihai merged commit d58e2b7 into dotnet:master Aug 29, 2020

ladipro mentioned this pull request May 24, 2021

Make MSBuildFileSystemBase non-abstract to remove versioning and usability constraints #6475

Merged

Conversation

cdmihai commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdmihai commented Jul 25, 2020

Uh oh!

cdmihai commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfederm commented Jul 28, 2020

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!

cdmihai Jul 28, 2020 • 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!

cdmihai Jul 28, 2020 • 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!

Forgind commented Jul 28, 2020

Uh oh!

Forgind commented Jul 28, 2020

Uh oh!

cdmihai commented Jul 28, 2020

Uh oh!

cdmihai commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Forgind commented Jul 28, 2020

Uh oh!

cdmihai commented Jul 28, 2020

Uh oh!

rrelyea commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainersigwald commented Jul 29, 2020

Uh oh!

cdmihai commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdmihai commented Aug 6, 2020

Uh oh!

cdmihai commented Aug 6, 2020

Uh oh!

azure-pipelines bot commented Aug 6, 2020

Uh oh!

cdmihai commented Aug 6, 2020

Uh oh!

azure-pipelines bot commented Aug 6, 2020

Uh oh!

cdmihai commented Aug 10, 2020

Uh oh!

azure-pipelines bot commented Aug 10, 2020

Uh oh!

cdmihai commented Aug 10, 2020

Uh oh!

azure-pipelines bot commented Aug 10, 2020

Uh oh!

Forgind commented Aug 10, 2020

Uh oh!

benvillalobos commented Aug 10, 2020

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdmihai commented Jul 25, 2020 •

edited

Loading

cdmihai commented Jul 28, 2020 •

edited

Loading

cdmihai Jul 28, 2020 •

edited

Loading

cdmihai Jul 28, 2020 •

edited

Loading

cdmihai commented Jul 28, 2020 •

edited

Loading

rrelyea commented Jul 29, 2020 •

edited

Loading

cdmihai commented Jul 29, 2020 •

edited

Loading

cdmihai commented Aug 18, 2020 •

edited

Loading