Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Oct 23, 2023

Conversation

erikmav
Copy link
Collaborator

@erikmav erikmav commented Oct 23, 2023

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

Fixes #493, #494 as well as microsoft/CopyOnWrite#32

…date warning and error logging to send MSBxxxx code to allow configured suppression or level changes.
src/CopyOnWrite/Copy.cs Outdated Show resolved Hide resolved
@jeffkl
Copy link
Contributor

jeffkl commented Oct 23, 2023

FYI, I'm finally fixing the tests: #497

@erikmav
Copy link
Collaborator Author

erikmav commented Oct 23, 2023

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

@KirillOsenkov
Copy link
Member

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 ;)

@erikmav erikmav merged commit dddcbe2 into microsoft:main Oct 23, 2023
5 checks passed
@erikmav erikmav deleted the dev/erikmav/cowMsbParity branch October 23, 2023 20:35
@NickCraver
Copy link
Member

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
Copy link
Contributor

jeffkl commented Nov 9, 2023

@erikmav ^

@erikmav
Copy link
Collaborator Author

erikmav commented Nov 9, 2023

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
Copy link
Member

NickCraver commented Nov 9, 2023

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
Copy link
Collaborator Author

erikmav commented Nov 9, 2023

@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
Copy link
Collaborator Author

erikmav commented Nov 9, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CopyOnWrite: make MSB3021 suppressable
4 participants