microsoft / MSBuildSdks

MSBuild project SDKs
MIT License
460 stars 82 forks source link

Minimize source diffs between MSBuild Copy.cs and CoW Copy.cs #496

Closed erikmav closed 1 year ago

erikmav commented 1 year ago

And update warning and error logging to send MSBxxxx code to allow configured suppression or level changes.

Fixes #493, #494 as well as https://github.com/microsoft/CopyOnWrite/issues/32

jeffkl commented 1 year ago

FYI, I'm finally fixing the tests: https://github.com/microsoft/MSBuildSdks/pull/497

erikmav commented 1 year ago

@jeffkl Thanks for fixing tests - I have a draft that I've been stuck on for the same reason

KirillOsenkov commented 1 year ago

Looks good from a cursory glance. Have you tested that the various scenarios are identical between standard Copy and this? Primarily I can think of:

  1. access denied because of admin
  2. file locked (you can use https://github.com/KirillOsenkov/Misc/blob/main/FileLocker.cs to test)
  3. overwrite a hardlinked destination (this is subtle, need to make sure we're doing exactly the same thing as the original, otherwise it'll be a nightmare to debug)

In general, the more we test now, the less painful our debugging in the future ;)

NickCraver commented 1 year ago

Overall, this regressed the fix in #454, causing us to once again error if any drive is unavailable (in my case: bitlocker locked). Can we please restore this error handling?

Building on the latest release:

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061: The "Copy" task could not be instantiated from "C:\Users\nickcraver\.nuget\packages\microsoft.build.copyonwrite\1.0.282\Sdk\..\build\netstandard2.0\Microsoft.Build.CopyOnWrite.dll".  [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061: System.TypeInitializationException: The type initializer for 'Microsoft.Build.Tasks.Copy' threw an exception. ---> System.ComponentModel.Win32Exception: Failed retrieving volume information for D:\ with winerror -2144272384 [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at Microsoft.CopyOnWrite.Windows.NativeMethods.ThrowSpecificIoException(Int32 lastErr, String message) in C:\CoW\lib\Windows\NativeMethods.cs:line 30 [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at Microsoft.CopyOnWrite.Windows.VolumeInfoCache.GetVolumeInfo(VolumePaths volumePaths) in C:\CoW\lib\Windows\VolumeInfoCache.cs:line 151 [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at Microsoft.CopyOnWrite.Windows.VolumeInfoCache.BuildFromCurrentFilesystem() in C:\CoW\lib\Windows\VolumeInfoCache.cs:line 36 [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at Microsoft.CopyOnWrite.CopyOnWriteFilesystemFactory.Create() in C:\CoW\lib\CopyOnWriteFilesystemFactory.cs:line 48 [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at System.Lazy`1.CreateValue() [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at System.Lazy`1.LazyInitValue() [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    at Microsoft.Build.Tasks.Copy..cctor() [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\Bin\amd64\Microsoft.Common.CurrentVersion.targets(5232,5): error MSB4061:    --- End of inner exception stack trace --- [C:\ms\Antares\dev\src\Hosting\RsFilter\CustomFileDownloader\CustomFileDownloader.csproj]

It's the same error/issue, just a new path to it. IMO, this overall scenario needs to be handle more gracefully upstream.

jeffkl commented 1 year ago

@erikmav ^

erikmav commented 1 year ago

FVE_E_LOCKED_VOLUME is handled in the CoW code: https://github.com/microsoft/CopyOnWrite/blob/001ad162f22289228d37b163aaad8d3c112bafc9/lib/Windows/VolumeInfoCache.cs#L135 added in CopyOnWrite package version 0.3.4 (current is 0.3.7). Can you verify what version of the CopyOnWrite.dll assembly are on-disk?

I've been mulling inverting the logic in that cache to just return null on error, but then there's no information channel to tell the user that a volume was ignored. Hence once of the sources of new errors in the underlying logic is adding to that list of error codes.

NickCraver commented 1 year ago

I tried this with every SDK version in the last 6 months (hoping to unblock with a release when the fix was active, but no joy). Checked the version and indeed it's at 0.3.7 (and reflected code shows handled), and the error message pathing purports to be the latest SDK...but I think we can disregard my report here. Looks like I was bitten again by "long-running MSBuild had (an old copy of) the types loaded", so despite me upgrading and trying various SDKs, I really wasn't using an updated CopyOnWrite.dll at any point during this.

@erikmav @jeffkl I apologize for the noise, and thanks for the quick follow-up! I totally agree this has ultimately been fixed - I was just not actually testing latest :( Clearing everything out and trying 1.0.282 SDK is all good even with a locked drive.

For anyone else finding this: be sure to kill all MSBuild processes between SDK upgrades - since it's a dependency DLL the error messages and paths can not be what's actually in play.

erikmav commented 1 year ago

@NickCraver no problem, been bitten there myself. I'll still take a look at making this code path less prone to throw on something unexpected.

erikmav commented 1 year ago

@NickCraver JFYI: https://devblogs.microsoft.com/engineering-at-microsoft/copy-on-write-in-win32-api-early-access/