-
Notifications
You must be signed in to change notification settings - Fork 525
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] Bump ZipFlushFilesLimit #7957
Conversation
public static int ZipFlushSizeLimit = 50 * 1024 * 1024; | ||
public static int ZipFlushFilesLimit = 50; | ||
public static int ZipFlushSizeLimit = 100 * 1024 * 1024; | ||
public static int ZipFlushFilesLimit = 512; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a private MSBuild property? $(_AndroidZipFlushSizeLimit)
and $(_AndroidZipFlushFilesLimit)
would give us the ability to tell customers about it. And see if that fixes an issue for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, that might be a good idea. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While contemplating that question, why are these fields read+write? We don't ever change them, so shouldn't they be const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea at the start was probably that we could alter the defaults in code. But we never did lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea at the start was probably that we could alter the defaults in code. But we never did lol
…but you are now, in e.g. AddEntryAndFlush()
. Which means we now have unprotected globally mutable data, which I am allergic to.
For example -- and because I cannot remember -- what happens in a "redux"/"variant" of 22f10b2 (our favorite issue around .GetRegisteredTaskObjectAssemblyLocal<T>()
now needing to include a directory as part of the key to prevent unexpected sharing of data between project builds) and we have 2 or more projects which just happen to hit <BuildApk/>
at the "same" time and "just happen" to provide different values for $(_ZipFlushFilesLimit)
?
It might be "fine" but I don't like it.
I would prefer it if these fields became instance values set at ZipArchiveEx
construction time:
Context https://i.azdo.io/1782014 On Windows customers are seeing the following error ``` Error XABLD7000: Xamarin.Tools.Zip.ZipException: Renaming temporary file failed: Permission denied at Xamarin.Tools.Zip.ZipArchive.Close() in /Users/runner/work/1/s/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs:line 939 ``` We have traced this to the use of the `MoveFileExW`/`MoveFileExA` API. When libzip is trying to move its temp file to the final location, Windows is raising this error. ``` Renaming temporary file failed: Permission denied ``` Turning off Anti Virus seems to help, however adding an exclusion does not. This is very confusing. So we are unsure why this error is being raised. The process has the correct permissions and the file being moved is in the same directory, so its not a TEMP folder issue. So perhaps its the number of temp files we create? Part of the `BuildApk` system is that as we add files we `Flush` the zip file to commit those changes to disk. This is partly to work around how libzip works. It does not write any data to the main file until `zip_close` is called. So to work around issues around too many files being open [1], we added this flush. The limit of 50 files was picked out of a hat. So lets try pushing the limit up a bit to see if that helps. [1] dotnet@9166e03
2940d2e
to
99e8a95
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Draft commit message: [Xamarin.Android.Build.Tasks] Bump ZipFlushFilesLimit (#7957)
Context: https://dev.azure.com/DevDiv/DevDiv/_workitems/edit/1782014
Context: 9166e0363417d6d121de37a765db8f2ba7cece7b
On Windows customers are seeing the following error:
error XABLD7000: Xamarin.Tools.Zip.ZipException: Renaming temporary file failed: Permission denied
at Xamarin.Tools.Zip.ZipArchive.Close() in /Users/runner/work/1/s/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs:line 939
We have traced this to the use of the [`MoveFileExW()`][0] /
`MoveFileExA()` API: when libzip is trying to move its temp file to
the final location, Windows is raising this error:
Renaming temporary file failed: Permission denied
Turning off Anti Virus seems to help, however adding an exclusion
does not. This is very confusing, so we are unsure why this error is
being raised. The process has the correct permissions and the file
being moved is in the same directory, so its not a TEMP folder issue.
Perhaps it's the number of temp files we create? Part of the
`<BuildApk/>` system is that as we add files we `Flush()` the zip
file to commit those changes to disk. This is partly to work around
how libzip works: it does not write any data to the main file until
[`zip_close()`][1] is called. To work around issues around too many
files being open (9166e036), we added this flush.
The limit of 50 files was picked out of a hat. Try pushing the limit
up a bit to see if that helps.
Additionally, introduce the following two (private!) MSBuild
properties:
* `$(_ZipFlushFilesLimit)`: Call `Flush()` after
`$(_ZipFlushFilesLimit)` files have been added to the `.apk`.
* `$(_ZipFlushSizeLimit)`: Call `Flush()` after
`$(_ZipFlushSizeLimit)` bytes of data have been added to the
`.apk`.
[0]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw
[1]: https://libzip.org/documentation/zip_close.html |
* main: [Xamarin.Android.Build.Tasks] enable ForceInterpretedInvoke switch (dotnet#7972) [Mono.Android] Bind API-UpsideDownCake Beta 1 (dotnet#7980) Bump to xamarin/xamarin-android-tools/main@8bc07503 (dotnet#7863) [automation] Add 'xaSourcePath' to yaml so they can be used from monodroid. (dotnet#7978) Bump to dotnet/installer@16c10f8115 8.0.100-preview.4.23218.1 (dotnet#7969) [docs] Add UnitTest.md (dotnet#7877) [ci] Suppress fork PR build warnings (dotnet#7973) [Xamarin.Android.Build.Tasks] Bump ZipFlushFilesLimit (dotnet#7957) Bump to dotnet/installer@3ca7ad1c79 8.0.100-preview.4.23211.1 (dotnet#7946) [CI] Allow passing xamarin-android checkout dir to nested templates. (dotnet#7961) [Xamarin.Android.Build.Tasks] Fix `-int.ToString()` for locales (dotnet#7941) [ci] Automatically retry failed apk-instrumentation tests. (dotnet#7963)
Context: https://dev.azure.com/DevDiv/DevDiv/_workitems/edit/1782014 Context: 9166e03 On Windows customers are seeing the following error: error XABLD7000: Xamarin.Tools.Zip.ZipException: Renaming temporary file failed: Permission denied at Xamarin.Tools.Zip.ZipArchive.Close() in /Users/runner/work/1/s/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs:line 939 We have traced this to the use of the [`MoveFileExW()`][0] / `MoveFileExA()` API: when libzip is trying to move its temp file to the final location, Windows is raising this error: Renaming temporary file failed: Permission denied Turning off Anti Virus seems to help, however adding an exclusion does not. This is very confusing, so we are unsure why this error is being raised. The process has the correct permissions and the file being moved is in the same directory, so its not a TEMP folder issue. Perhaps it's the number of temp files we create? Part of the `<BuildApk/>` system is that as we add files we `Flush()` the zip file to commit those changes to disk. This is partly to work around how libzip works: it does not write any data to the main file until [`zip_close()`][1] is called. To work around issues around too many files being open (9166e03), we added this flush. The limit of 50 files was picked out of a hat. Try pushing the limit up a bit to see if that helps. Additionally, introduce the following two (private!) MSBuild properties: * `$(_ZipFlushFilesLimit)`: Call `Flush()` after `$(_ZipFlushFilesLimit)` files have been added to the `.apk`. * `$(_ZipFlushSizeLimit)`: Call `Flush()` after `$(_ZipFlushSizeLimit)` bytes of data have been added to the `.apk`. [0]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw [1]: https://libzip.org/documentation/zip_close.html
Context https://i.azdo.io/1782014
On Windows customers are seeing the following error
We have traced this to the use of the
MoveFileExW
/MoveFileExA
API. When libzip is trying to move its temp file to the final location, Windows is raising this error.Turning off Anti Virus seems to help, however adding an exclusion does not. This is very confusing. So we are unsure why this error is being raised. The process has the correct permissions and the file being moved is in the same directory, so its not a TEMP folder issue.
So perhaps its the number of temp files we create? Part of the
BuildApk
system is that as we add files weFlush
the zip file to commit those changes to disk. This is partly to work around how libzip works. It does not write any data to the main file untilzip_close
is called. So to work around issues around too many files being open [1], we added this flush.The limit of 50 files was picked out of a hat. So lets try pushing the limit up a bit to see if that helps.
[1] 9166e03