Allow copying across mounts on workload install #18482
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs
Outdated
Show resolved
Hide resolved
| @@ -271,5 +271,24 @@ private ManifestVersion GetInstalledManifestVersion(ManifestId manifestId) | |||
|
|
|||
| internal static PackageId GetManifestPackageId(SdkFeatureBand featureBand, ManifestId manifestId) => | |||
| new PackageId($"{manifestId}.Manifest-{featureBand}"); | |||
|
|
|||
| // Note: cannot use Directory.Move because it errors when copying across mounts | |||
| internal static void CopyDirectory(string sourcePath, string destPath) | |||
There was a problem hiding this comment.
It looks like this changes behavior from moving to copying. It's also no longer atomic, which I think was the reason for extracting to a temp folder and then moving in the first place.
So I think we should use Directory.Move when it's possible. When it's not possible, we probably want to do something more resilient / atomic, such as copying it to the metadata folder (which we can probably assume is on the same volume) and then moving it from there.
I'm not sure if there's a way to detect up-front whether Directory.Move will work or whether we have to catch the failure.
cc @wli3
There was a problem hiding this comment.
I agree on using Directory.Move when possible, I'll make that change. However I don't think we can always depend on the metadata dir being in the same mount- for example, when we're copying advertising manifests into the user home I believe that's on a different mount on linux by default.
There was a problem hiding this comment.
Is it possible that one of the target folders can be a symlink that crosses mount points?
There was a problem hiding this comment.
we kind of giving up the atomic move when we decided on using OS temp dir. So this is expected.
Although I am surprised Directory.Move will just fail
|
Should there be a new test to ensure that this doesn't regress? |
This line causes existing tests to use different mounts/ drives, which repros the issue: https://github.com/dotnet/sdk/pull/18482/files#diff-81861764e6652051f255fa90e0aba2c3bc22f8bb020931fb7de60a5c5650174cR433 |
Fixes #18450