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

VS 16.3.0 regression: Inconsistent <AndroidStoreUncompressedFileExtensions> response. #3675

Closed
plynkus opened this issue Sep 24, 2019 · 21 comments · Fixed by #3690
Closed

VS 16.3.0 regression: Inconsistent <AndroidStoreUncompressedFileExtensions> response. #3675

plynkus opened this issue Sep 24, 2019 · 21 comments · Fixed by #3690
Assignees

Comments

@plynkus
Copy link

plynkus commented Sep 24, 2019

Steps to Reproduce

(Working on a viable compact test case.)

Expected Behavior

In all cases, specified AndroidStoreUncompressedFileExtensions contents (either in app project or from NuGet package .targets files that amend the property) are properly honored at APK creation time.

Actual Behavior

As of VS 16.3.0, we now see (in the same APK) cases where assets are stored (as requested) and others deflated (despite being specified for uncompressed storage).

Version Information

Xamarin.Android SDK 10.0.0.43 (d16-3/8af1ca8)
Xamarin.Android Reference Assemblies and MSBuild support.
Mono: mono/mono@7af64d1ebe9
Java.Interop: xamarin/java.interop@5836f58
LibZipSharp: grendello/LibZipSharp/d16-3@71f4a94
LibZip: nih-at/libzip@b95cf3f
ProGuard: xamarin/proguard@905836d
SQLite: xamarin/sqlite@8212a2d
Xamarin.Android Tools: xamarin/xamarin-android-tools@cb41333

@plynkus
Copy link
Author

plynkus commented Sep 24, 2019

I'll be looking to identify how to provoke this in a simple test case, but wanted to get the issue on record in case it rings a bell with anyone.

Additionally, what would be easier for us (feature request, ultimately) is an ability to specify uncompressed storage requests on a per-AndroidAsset basis vs. having to accomplish this by adopting file extension conventions that we then have to amend AndroidStoreUncompressedFileExtensions to honor. We have NuGet packages that perform asset conditioning that result in additional AndroidAsset additions to the client project---it'd be more natural to simply add these with an additional field (e.g. Uncompressed="True") vs. having to constrain the paths to a known extension we then register for raw storage. The set of uncompressed assets stored in the APK would then be the union of those that match AndroidStoreUncompressedFileExtensions contents and those explicitly specified individually (no matter their extension).

In the regression case, we have identical patterns of NuGet package-registered assets (and corresponding AndroidStoreUncompressedFileExtensions amendments) in multiple packages in use by the client and yet see both stored (proper) and deflated (improper) outcomes at the same time in the resulting APK. A puzzle.

@plynkus
Copy link
Author

plynkus commented Sep 24, 2019

Example partial listing of an affected packaged_resources (partial token anonymization):

. . .
   336292  Stored    336292   0% 1980-12-31 17:00 1af3589c  resources.arsc
264517680  Stored 264517680   0% 1980-12-31 17:00 18a3c79d  assets/APP.artifacts
    75229  Defl:N     41844  44% 1980-12-31 17:00 0876a722  assets/APP.artifacts.xyz
. . .

...for an aggregate AndroidStoreUncompressedFileExtensions value of ";.artifacts;.xyz".

Both the APP.artifacts and APP.artifacts.xyz are generated by tasks from separate NuGet packages, each package containing .targets that add their artifacts to the build as AndroidAsset entries and register their custom (arbitrary) extension by appending AndroidStoreUncompressedFileExtensions.

Note that the first asset is stored and the second deflated. The second is incorrect (and a consequence of the VS 16.3 upgrade, apparently).

@plynkus
Copy link
Author

plynkus commented Sep 24, 2019

After failing to get any asset to store uncompressed in even a simplified form, I find even the following fails to do what's expected:

  • Create a new Android project (blank).
  • Change the build action for the template-provided AboutAssets.txt file to AndroidAsset.
  • Change the project settings leave ".txt" files uncompressed.
  • Build and inspect the resulting APK.

Result: The text file is deflated in the APK.

Is it possible that the leave-uncompressed feature is broken or changed in what it covers?

@pjcollins
Copy link
Member

I do also see a change in compression behavior between d16-2 and d16-3 with the simple repro steps mentioned above. I'm attaching a repro project and binlogs from d16-2 and d16-3:

App61.zip

@dellis1972
Copy link
Contributor

Looks like we are missing UncompressedFileExtensions on the Aapt2 calls in the targets. I'll get this fixed up

@dellis1972
Copy link
Contributor

I think we need this

diff --git a/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Aapt2.targets b/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Aapt2.targets
index 9fb8aaa6..179904e8 100644
--- a/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Aapt2.targets
+++ b/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Aapt2.targets
@@ -175,6 +175,7 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
       ExtraArgs="$(AndroidAapt2LinkExtraArgs)"
       ToolPath="$(Aapt2ToolPath)"
       ToolExe="$(Aapt2ToolExe)"
+      UncompressedFileExtensions="$(AndroidStoreUncompressedFileExtensions)"
   />
   <ItemGroup>
     <FileWrites Include="$(IntermediateOutputPath)R.txt" Condition=" '$(_AndroidUseAapt2)' == 'True' And Exists ('$(IntermediateOutputPath)R.txt') " />
@@ -239,6 +240,7 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved.
       ExtraArgs="$(AndroidAapt2LinkExtraArgs)"
       ToolPath="$(Aapt2ToolPath)"
       ToolExe="$(Aapt2ToolExe)"
+      UncompressedFileExtensions="$(AndroidStoreUncompressedFileExtensions)"
   />
 </Target>

@plynkus
Copy link
Author

plynkus commented Sep 25, 2019

I'd also suggest a unit test of some kind to ensure no future regression.

@dellis1972
Copy link
Contributor

We have a unit test, not sure it is running against aapt2.

@dellis1972
Copy link
Contributor

Since its not the full default at the moment afaik

jonpryor pushed a commit that referenced this issue Oct 1, 2019
Fixes: #3675

`<Aapt2Link>` didn't use `$(AndroidStoreUncompressedFileExtensions)`
at all 🤦‍♂.  Consequently, if `$(AndroidUseAapt2)`=True and
`$(AndroidStoreUncompressedFileExtensions)` was set, files with the
specified file extensions would *not* be stored uncompressed within
the resulting `.apk`.

This was presumably overlooked and missed when
[`aapt2` was enabled by default in d16-3][0].

Update the `<Aapt2Link>` invocation so that
`$(AndroidStoreUncompressedFileExtensions)` is provided, which in
turn allows `aapt2` output to *not* compress files with the specified
file extensions.

Check that this works by updating
`PackagingTest.CheckIncludedNativeLibraries()`, and requiring that
`.so` files be stored uncompressed when
`$(AndroidStoreUncompressedFileExtensions)`=`.so`.

[0]: https://docs.microsoft.com/en-us/xamarin/android/release-notes/9/9.4#aapt2-enabled-by-default-for-new-projects
jonpryor pushed a commit that referenced this issue Oct 1, 2019
Fixes: #3675

`<Aapt2Link>` didn't use `$(AndroidStoreUncompressedFileExtensions)`
at all 🤦‍♂.  Consequently, if `$(AndroidUseAapt2)`=True and
`$(AndroidStoreUncompressedFileExtensions)` was set, files with the
specified file extensions would *not* be stored uncompressed within
the resulting `.apk`.

This was presumably overlooked and missed when
[`aapt2` was enabled by default in d16-3][0].

Update the `<Aapt2Link>` invocation so that
`$(AndroidStoreUncompressedFileExtensions)` is provided, which in
turn allows `aapt2` output to *not* compress files with the specified
file extensions.

Check that this works by updating
`PackagingTest.CheckIncludedNativeLibraries()`, and requiring that
`.so` files be stored uncompressed when
`$(AndroidStoreUncompressedFileExtensions)`=`.so`.

[0]: https://docs.microsoft.com/en-us/xamarin/android/release-notes/9/9.4#aapt2-enabled-by-default-for-new-projects
@brendanzagaeski
Copy link
Member

Release status update

A new Preview version has now been published that includes the fix for this item. The fix is not yet included in a Release version. I will update this item again when a Release version is available that includes the fix.

Fix included in Xamarin.Android 10.1.0.1.

Fix included on Windows in Visual Studio 2019 version 16.4 Preview 2. To try the Preview version that includes the fix, check for the latest updates in Visual Studio Preview.

Fix included on macOS in Visual Studio 2019 for Mac version 8.4 Preview 1. To try the Preview version that includes the fix, check for the latest updates on the Preview updater channel.

@wolpers
Copy link

wolpers commented Nov 21, 2019

Any Update on when this appears in a release? We want to release our app but prefer to work with the Release update channel, and this bug breaks our app.

@jonathanpeppers
Copy link
Member

@wolpers a workaround would be to use <AndroidUseAapt2>False</AndroidUseAapt2> until 16.4 hits stable. I don't think there are public release dates for Visual Studio.

@brendanzagaeski
Copy link
Member

Release status update

A new Release version has now been published on Windows that includes the fix for this item. The fix is not yet published in a Release version on macOS. I will update this item again when a Release version is available on macOS that includes the fix.

Fix included in Xamarin.Android 10.1.0.30.

Fix included on Windows in Visual Studio 2019 version 16.4. To get the new version that includes the fix, check for the latest updates or install the latest version from https://visualstudio.microsoft.com/downloads/.

(Fix also included on macOS in Visual Studio 2019 for Mac version 8.4 Preview 2.1 and higher. To try the Preview version that includes the fix, check for the latest updates on the Preview updater channel.)

@plynkus
Copy link
Author

plynkus commented Dec 4, 2019

I'm still seeing the issue described originally above (mix of stored vs. deflated despite both extensions in play) under 10.1.0.30. I'll try and get additional information.

@jonathanpeppers
Copy link
Member

@plynkus could you be hitting? #3968

Do your list of extensions have a . character included?

@plynkus
Copy link
Author

plynkus commented Dec 4, 2019

Confirmed that this still does not appear to be fully fixed.

DeflatedAssets3675.zip

I've attached a simple project that has the conditions originally described (multiple extensions set for raw storage).

After building under 10.1.0.30, with AndroidUseAapt2 set to true (the default)...

$ unzip -lv bin/Debug/com.companyname.deflatedassets3675.apk | grep Foo
     642  Stored      642   0% 1980-12-31 16:00 50f5aca8  assets/Foo.artifacts
     642  Defl:N      381  41% 1980-12-31 16:00 50f5aca8  assets/Foo.artifacts.xyz

...where with AndroidUseAapt2 to false instead results in:

$ unzip -lv bin/Debug/com.companyname.deflatedassets3675.apk | grep Foo
     642  Stored      642   0% 1980-12-31 16:00 50f5aca8  assets/Foo.artifacts
     642  Stored      642   0% 1980-12-31 16:00 50f5aca8  assets/Foo.artifacts.xyz

The correct outcome is both files Stored in the APK.

Note that the extensions list in this case has a leading semicolon---this is due to AndroidStoreUncompressedFileExtensions appending being done by nuget packages employed by our real application. (The app itself has no uncompressed extensions, but the two get added via package targets.) EDIT: And FWIW it looks like that leading semicolon has no bearing on the root cause here---still broken if removed.

@jonathanpeppers
Copy link
Member

@dellis1972 you should try this ^^

It's weird it seems like it doesn't like the .xyz extension and aapt(1) works???

@dellis1972
Copy link
Contributor

dellis1972 commented Dec 5, 2019

The docs for aapt2 say

-0 extension | Specifies the extensions of files that you do not want to compress.

which suggests we are passing it the correct arguments (in this case -0 .artifacts -0 .xyz). It kinda hints that this might be an issue in aapt2 itself. I'll see if I can repo the issue directly with aapt2.

@dellis1972
Copy link
Contributor

we are passing the correct arguments to aapt2 in the case of the -0 .artifacts -0 .xyz. It looks like aapt2 is finding the first period (.) and then using the remaining text as the extension. So to get this to work in that case you need to use

<AndroidStoreUncompressedFileExtensions>;.artifacts;.artifacts.xyz</AndroidStoreUncompressedFileExtensions>

Which is weird and odd behaviour. But its within aapt2 itself. So there isn't much we can do about that particular case :/

@plynkus
Copy link
Author

plynkus commented Dec 5, 2019

Good catch. Specifying full compound extensions will certainly work for us. Thanks again for the overall fix.

@brendanzagaeski
Copy link
Member

Release status update

A new Release version has now been published on macOS that includes the fix for this item, as was published earlier on Windows.

Fix included in Xamarin.Android 10.1.1.0.

Fix included on macOS in Visual Studio 2019 for Mac version 8.4. To get the new version that includes the fix, check for the latest updates on the Stable updater channel.

(Fix also included on Windows in Visual Studio 2019 version 16.4 and higher. To get the new version that includes the fix, check for the latest updates or install the latest version from https://visualstudio.microsoft.com/downloads/.)

@xamarin xamarin locked as resolved and limited conversation to collaborators Jun 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants