-
Notifications
You must be signed in to change notification settings - Fork 560
[Xamarin.Android.Build.Tasks] only warn about failed deletion of temp dirs #4444
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
[Xamarin.Android.Build.Tasks] only warn about failed deletion of temp dirs #4444
Conversation
… dirs Context: https://build.azdo.io/3575078 Context: https://build.azdo.io/3574952 Our CI builds have been hitting random failures such as: (_UpdateAndroidResgen target) -> error XARDF7024: System.IO.IOException: The directory is not empty. error XARDF7024: error XARDF7024: at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data) error XARDF7024: at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost) error XARDF7024: at Xamarin.Android.Tasks.RemoveDirFixed.RunTask() Or in another target: (_CreateBaseApk target) -> error XARDF7024: System.IO.IOException: The directory is not empty. error XARDF7024: error XARDF7024: at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data) error XARDF7024: at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost) error XARDF7024: at Xamarin.Android.Tasks.RemoveDirFixed.RunTask() In both of these places, we have the pattern: <CreateTemporaryDirectory> <Output TaskParameter="TemporaryDirectory" PropertyName="AaptTemporaryDirectory" /> </CreateTemporaryDirectory> <!-- Then later on --> <RemoveDirFixed Directories="$(AaptTemporaryDirectory)" /> The temporary directory is created in C# via: TemporaryDirectory = Path.Combine (Path.GetTempPath (), Path.GetRandomFileName ()); Directory.CreateDirectory (TemporaryDirectory); I think a solution here is to make the `<RemoveDirFixed/>` call set to `ContinueOnError="WarnAndContinue"`. I don't think we need to *fail* the overall build in these scenarios. The user is going to have leftover files in `%TEMP%`, and I don't think there is much we can do. Changing this from a build *error* to a *warning* is a general improvement if we are unable to delete the directory. The error code will also be preserved. I don't know if this will help our users at all, but should help our CI.
I agree that we should do this fix, but I think we should also look and see if something changed recently that has caused this to start happening. Presumably something is still holding these files open that previously was not. |
@jpobst the only thing that would hold these directories open is I have no explanation, unless you have an idea. We haven't changed aapt2, except for this PR that isn't merged yet: #4190 Maybe its PR builds could break other PRs running on the same machine? |
This PR is nearly green, but we get a network issue:
I'm going to merge, as it seems to help the Windows side. |
… dirs (#4444) Context: https://build.azdo.io/3575078 Context: https://build.azdo.io/3574952 Our CI builds have been hitting random failures such as: (_UpdateAndroidResgen target) -> error XARDF7024: System.IO.IOException: The directory is not empty. error XARDF7024: error XARDF7024: at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data) error XARDF7024: at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost) error XARDF7024: at Xamarin.Android.Tasks.RemoveDirFixed.RunTask() Or in another target: (_CreateBaseApk target) -> error XARDF7024: System.IO.IOException: The directory is not empty. error XARDF7024: error XARDF7024: at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data) error XARDF7024: at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost) error XARDF7024: at Xamarin.Android.Tasks.RemoveDirFixed.RunTask() In both of these places, we have the pattern: <CreateTemporaryDirectory> <Output TaskParameter="TemporaryDirectory" PropertyName="AaptTemporaryDirectory" /> </CreateTemporaryDirectory> <!-- Then later on --> <RemoveDirFixed Directories="$(AaptTemporaryDirectory)" /> The temporary directory is created in C# via: TemporaryDirectory = Path.Combine (Path.GetTempPath (), Path.GetRandomFileName ()); Directory.CreateDirectory (TemporaryDirectory); I think a solution here is to make the `<RemoveDirFixed/>` call set to `ContinueOnError="WarnAndContinue"`. I don't think we need to *fail* the overall build in these scenarios. The user is going to have leftover files in `%TEMP%`, and I don't think there is much we can do. Changing this from a build *error* to a *warning* is a general improvement if we are unable to delete the directory. The error code will also be preserved. I don't know if this will help our users at all, but should help our CI.
Draft release note Although this issue hasn't been reported by users, I'll plan to include a tiny note for it just to be thorough. Here's what I'm thinking:
To suggest something different or to recommend excluding it from the release notes, feel free to reply in this PR or add a |
Context: https://build.azdo.io/3575078
Context: https://build.azdo.io/3574952
Our CI builds have been hitting random failures such as:
Or in another target:
In both of these places, we have the pattern:
The temporary directory is created in C# via:
I think a solution here is to make the
<RemoveDirFixed/>
call set toContinueOnError="WarnAndContinue"
. I don't think we need to failthe overall build in these scenarios.
The user is going to have leftover files in
%TEMP%
, and I don'tthink there is much we can do. Changing this from a build error to a
warning is a general improvement if we are unable to delete the
directory. The error code will also be preserved.
I don't know if this will help our users at all, but should help our CI.