Stop using FileSystem and Common submodules #4536

jeffkl · 2022-03-31T15:32:22Z

Bug

Fixes: NuGet.Commands embedded copy of FileSystemGlobbing is now public Home#2345

Regression? Last working version:

Description

Most of this was done by @nkolev92, I just continued his work. Removes the FileSystem and Common submodules in favor of package references. This gets rid of the classes being compiled into our assemblies so they don't conflict with other types.

PR Checklist

nkolev92

Love it.

Did RPS pass?
Not sure if we need to do anything special to ensure source build is good beyond the build passing given that we're adding new assemblies.

cc @crummel

cc @zivkan We're deleting lots of public APIs here, but I think this is a risk we're 100% willing to take?

.gitmodules

crummel

source-build already includes MS.Extensions so this should be fine by us. Thanks for the ping!

zivkan

We're deleting lots of public APIs here, but I think this is a risk we're 100% willing to take?

I agree. Removing the submodules will unavoidably break ABI compat, but since file globbing isn't a primary feature of nuget, we can just hope that very few people took that dependency.

Unfortunately namespaces changed, so it also breaks API compat. But that was the ASP.NET team's choice when they changed the namespace of these APIs.

src/NuGet.Clients/NuGet.VisualStudio.Client/.vsixinclude

aortiz-msft · 2022-03-31T20:29:02Z

So cool. Out of curiosity, does this reduce the build time, both locally and on our CI? If yes, by how much?

jeffkl · 2022-04-04T16:10:13Z

RESOLVED

Does anyone know why only this one test is failing? Or at least how to figure out why its failing? Thanks in advance!

NetCoreToolsVSandMSBuildNoOp

System.Management.Automation.ItemNotFoundException: Cannot find path 'C:\Users\TestUser\.nuget\packages\.tools\Microsoft.VisualStudio.Web.CodeGeneration.Tools\1.0.0-msbuild3-final\netcoreapp2.0\Microsoft.VisualStudio.Web.CodeGeneration.Tools.nuget.cache' because it does not exist.
   at System.Management.Automation.LocationGlobber.ExpandMshGlobPath(String path, Boolean allowNonexistingPaths, PSDriveInfo drive, ContainerCmdletProvider provider, CmdletProviderContext context)
   at System.Management.Automation.LocationGlobber.ResolveDriveQualifiedPath(String path, CmdletProviderContext context, Boolean allowNonexistingPaths, CmdletProvider& providerInstance)
   at System.Management.Automation.LocationGlobber.GetGlobbedMonadPathsFromMonadPath(String path, Boolean allowNonexistingPaths, CmdletProviderContext context, CmdletProvider& providerInstance)
   at System.Management.Automation.LocationGlobber.GetGlobbedProviderPathsFromMonadPath(String path, Boolean allowNonexistingPaths, CmdletProviderContext context, ProviderInfo& provider, CmdletProvider& providerInstance)
   at System.Management.Automation.SessionStateInternal.GetProperty(String[] paths, Collection`1 providerSpecificPickList, CmdletProviderContext context)
   at Microsoft.PowerShell.Commands.GetItemPropertyCommand.ProcessRecord()

https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5964223&view=ms.vss-test-web.build-test-results-tab&runId=36869364&resultId=100100&paneView=debug

nkolev92 · 2022-04-04T17:09:37Z

Did you try running the test locally?

There's some end to end instructions in the one note.

jeffkl · 2022-04-05T14:36:46Z

Did you try running the test locally?

There's some end to end instructions in the one note.

Thanks for the pointer, I was able to run them locally after some trial and error, turns out we were missing a binding redirect entry in the pkgdef. That fixed the problem locally, I've pushed the change.

test/TestUtilities/Test.Utility/TestDotnetCLiUtility.cs

src/NuGet.Clients/NuGet.VisualStudio.Client/.vsixinclude

jeffkl · 2022-04-07T14:55:38Z

So cool. Out of curiosity, does this reduce the build time, both locally and on our CI? If yes, by how much?

@aortiz-msft It will reduce the build time by a little since its not fetching these submodules during build anymore and no longer compiling the sources into our assemblies. I doubt it's going to save minutes of time though. This change is more about reducing the overhead of cloning and building our repository. There's also a bug where the types from these classes are public and can mess up consumers of our assemblies.

jeffkl · 2022-04-07T15:32:27Z

@nkolev92 or @zivkan do you know if I need to update this?

NuGet.Client/setup/Microsoft.VisualStudio.NuGet.BuildTools.swr

Lines 7 to 29 in 5b0ee25

    
           folder InstallDir:\Common7\IDE\CommonExtensions\Microsoft\NuGet  
        
                   file source=$(NuGetTargetsBasePath)NuGet.targets 
        
                   file source=$(NuGetTargetsBasePath)NuGet.props 
        
                   file source=$(NuGetTargetsBasePath)NuGet.RestoreEx.targets 
        
                   file source=$(ReferenceOutputPath)Microsoft.Web.XmlTransform.dll 
        
                   file source=$(ReferenceOutputPath)Microsoft.Build.NuGetSdkResolver.dll 
        
                   file source=$(NewtonsoftJsonPath)Newtonsoft.Json.dll 
        
                   file source=$(ReferenceOutputPath)NuGet.Build.Tasks.dll 
        
                   file source=$(ReferenceOutputPath)NuGet.Build.Tasks.Console.exe 
        
                   file source=$(ReferenceOutputPath)NuGet.Build.Tasks.Console.exe.config   
        
                   file source=$(ReferenceOutputPath)NuGet.Commands.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Common.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Configuration.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.DependencyResolver.Core.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Frameworks.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.LibraryModel.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.PackageManagement.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Packaging.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.ProjectModel.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Protocol.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Resolver.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Versioning.dll   
        
                   file source=$(ReferenceOutputPath)NuGet.Credentials.dll

nkolev92 · 2022-04-07T16:55:11Z

@jeffkl Yep.

That VSIX is for the build tools SKU and it needs restore to work.

jeffkl · 2022-04-07T18:36:47Z

That VSIX is for the build tools SKU and it needs restore to work.

@nkolev92 do you know if Apex, end-to-end tests, or anything else do anything with BuildTools SKU? Or do I just need to manually test it?

nkolev92 · 2022-04-07T19:04:07Z

They don't, but there are manual vendors tests for this, but they are not super detailed.

ghost · 2022-04-18T22:00:42Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

jeffkl · 2022-04-19T17:15:29Z

@NuGet/nuget-client please review

nkolev92

🚢

nkolev92 · 2022-04-21T16:02:24Z

🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳 🥳

This reverts commit cebb690.

Revert "Revert "Stop using FileSystem and Common submodules (#4536)" (#4595)" This reverts commit 5ac16d7.

jeffkl requested a review from a team as a code owner March 31, 2022 15:32

jeffkl changed the title ~~Dev jeffkl stop using submodules~~ Stop using FileSystem and Common submodules Mar 31, 2022

nkolev92 previously approved these changes Mar 31, 2022

View reviewed changes

.gitmodules Show resolved Hide resolved

erdembayar previously approved these changes Mar 31, 2022

View reviewed changes

crummel previously approved these changes Mar 31, 2022

View reviewed changes

zivkan previously approved these changes Mar 31, 2022

View reviewed changes

src/NuGet.Clients/NuGet.VisualStudio.Client/.vsixinclude Show resolved Hide resolved

jeffkl dismissed stale reviews from zivkan, crummel, erdembayar, and nkolev92 via bd3e501 April 5, 2022 14:34

jeffkl force-pushed the dev-jeffkl-stop-using-submodules branch from 95352cb to bd3e501 Compare April 5, 2022 14:34

nkolev92 previously approved these changes Apr 5, 2022

View reviewed changes

zivkan previously approved these changes Apr 5, 2022

View reviewed changes

test/TestUtilities/Test.Utility/TestDotnetCLiUtility.cs Outdated Show resolved Hide resolved

src/NuGet.Clients/NuGet.VisualStudio.Client/.vsixinclude Show resolved Hide resolved

jeffkl dismissed stale reviews from zivkan and nkolev92 via f2d4d50 April 7, 2022 15:01

jeffkl added 6 commits April 11, 2022 13:32

Remove FileSystem and Common submodules

aa3e39e

Always run "LocValidation: Publish Logs as an artifact"

2550a8a

Use NuGetValidator version 2.0.5

c148b59

Add assemblies to binding redirects and codebase in devenv.exe.config

324e8ea

Add comments to TestDotnetCLiUtility.cs

8c761d4

Update BuildTools SKU

46873a1

jeffkl force-pushed the dev-jeffkl-stop-using-submodules branch 2 times, most recently from 464e17a to 46873a1 Compare April 11, 2022 21:50

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Apr 18, 2022

jeffkl removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Apr 19, 2022

nkolev92 approved these changes Apr 19, 2022

View reviewed changes

jeffkl merged commit cebb690 into dev Apr 21, 2022

jeffkl deleted the dev-jeffkl-stop-using-submodules branch April 21, 2022 13:32

erdembayar added a commit that referenced this pull request Apr 27, 2022

Revert "Stop using FileSystem and Common submodules (#4536)"

130dc5c

This reverts commit cebb690.

nkolev92 pushed a commit that referenced this pull request Apr 27, 2022

Revert "Stop using FileSystem and Common submodules (#4536)" (#4595)

5ac16d7

This reverts commit cebb690.

jeffkl added a commit that referenced this pull request May 2, 2022

***NO_CI***

f566189

Revert "Revert "Stop using FileSystem and Common submodules (#4536)" (#4595)" This reverts commit 5ac16d7.

TheCakeIsNaOH mentioned this pull request Jun 21, 2022

(#508) Uplift NuGet library chocolatey/choco#2740

Merged

46 tasks

jeffkl added a commit that referenced this pull request Nov 1, 2022

***NO_CI***

b80ad32

Revert "Revert "Stop using FileSystem and Common submodules (#4536)" (#4595)" This reverts commit 5ac16d7.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using FileSystem and Common submodules #4536

Stop using FileSystem and Common submodules #4536

jeffkl commented Mar 31, 2022

nkolev92 left a comment

crummel left a comment

zivkan left a comment

aortiz-msft commented Mar 31, 2022

jeffkl commented Apr 4, 2022 •

edited

Loading

nkolev92 commented Apr 4, 2022

jeffkl commented Apr 5, 2022

jeffkl commented Apr 7, 2022

jeffkl commented Apr 7, 2022

nkolev92 commented Apr 7, 2022

jeffkl commented Apr 7, 2022

nkolev92 commented Apr 7, 2022

ghost commented Apr 18, 2022

jeffkl commented Apr 19, 2022

nkolev92 left a comment

nkolev92 commented Apr 21, 2022

Stop using FileSystem and Common submodules #4536

Stop using FileSystem and Common submodules #4536

Conversation

jeffkl commented Mar 31, 2022

Bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

crummel left a comment

Choose a reason for hiding this comment

zivkan left a comment

Choose a reason for hiding this comment

aortiz-msft commented Mar 31, 2022

jeffkl commented Apr 4, 2022 • edited Loading

RESOLVED

NetCoreToolsVSandMSBuildNoOp

nkolev92 commented Apr 4, 2022

jeffkl commented Apr 5, 2022

jeffkl commented Apr 7, 2022

jeffkl commented Apr 7, 2022

nkolev92 commented Apr 7, 2022

jeffkl commented Apr 7, 2022

nkolev92 commented Apr 7, 2022

ghost commented Apr 18, 2022

jeffkl commented Apr 19, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 commented Apr 21, 2022

jeffkl commented Apr 4, 2022 •

edited

Loading