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

Bump to google/bundletool/main@f17ce94a #8135

Merged
merged 6 commits into from
Jun 22, 2023

Conversation

jonathanpeppers
Copy link
Member

Context: https://github.com/google/bundletool/releases/tag/1.15.1
Changes: google/bundletool@1.8.1...1.15.1

We are seeing an error with API 34:

XABBA7024: Xamarin.Tools.Zip.ZipIOException: The file 'obj\Release\android\bin\base.zip' is not a ZIP archive.

We wonder if updating bundletool will help. It was last updated in 989dc07.

Context: https://github.com/google/bundletool/releases/tag/1.15.1
Changes: google/bundletool@1.8.1...1.15.1

We are seeing an error with API 34:

    XABBA7024: Xamarin.Tools.Zip.ZipIOException: The file 'obj\Release\android\bin\base.zip' is not a ZIP archive.

We wonder if updating `bundletool` will help. It was last updated in
989dc07.
Context: https://github.com/xamarin/xamarin-android/blob/f1d59181c8daaa8d2abcdfd151b592ece49155ca/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/ManifestTest.cs#L650-L651

The `ModifyManifest` test actually writes "too much" to the
`AndroidManifest.xml` file, removing the `<application>` element,
causing the latest version of `bundletool` to error with:

    BT One element <application> was expected, but none were found.

To solve this, let's just set `application:versionCode` in the test
instead.
Comment on lines 674 to 672
Value=""@(_Permissions)""
Query=""/manifest""
Value=""12345""
Query=""/manifest/@android:versionCode""
Copy link
Member Author

@jonathanpeppers jonathanpeppers Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was failing.

It actually writes "too much" to the AndroidManifest.xml file, removing the <application> element, causing the latest version of bundletool to error with:

BT One element <application> was expected, but none were found.

To solve this, let's just set android:versionCode in the test instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure this is the best solution. The test in question is replicating a situation a customer reported (and is using). So we should try to figure out why this is not working. I'm not sure how the XmPoke code would remove the application element... unless the behaviour of XmlPoke has changed somehow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it looks like the test was doing an XmlPoke into the manifest element, which would override the entire contents.

The test itself, seems like it is testing $(AfterGenerateAndroidManifest), so as long as we write something in the manifest with a custom target, and we can read the value at the end: test is the same?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the test should be appending the permissions to the manifest element and leaving the rest in place.
If its not doing that then it should be updated to do that somehow.

Having an example of adding permissions using XmlPoke would be a good thing if we can get it working. Since its a use case which a number of users have been doing.

@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Jun 22, 2023

Well, the test changes didn't work:

error MSB4012: The expression "@(_XmlNodes->'%(Identity)', ' ')@(_Permissions)" cannot be used in this context. Item lists cannot be concatenated with other strings where an item list is expected. Use a semicolon to separate multiple item lists. 

Which I guess would have worked with a random ; in the middle.

Let me actually try it locally...

Work around an MSBuild issue:

    error MSB4012: The expression "@(_XmlNodes->'%(Identity)', ' ')@(_Permissions)" cannot be used in this context. Item lists cannot be concatenated with other strings where an item list is expected. Use a semicolon to separate multiple item lists.
@jonathanpeppers jonathanpeppers merged commit 6e375d5 into dotnet:main Jun 22, 2023
@jonathanpeppers jonathanpeppers deleted the bundletools-1.15.1 branch June 22, 2023 19:05
grendello added a commit to grendello/xamarin-android that referenced this pull request Jun 23, 2023
* main:
  Bump to google/bundletool@f17ce94a (dotnet#8135)
  [Xamarin.Android.Build.Tasks] Handle IOException in Aapt2Daemon (dotnet#8130)
  [tests] don't set `/uses-sdk@android:targetSdkVersion=34` by default (dotnet#8138)
grendello added a commit to grendello/xamarin-android that referenced this pull request Jun 26, 2023
* main:
  Bump to google/bundletool@f17ce94a (dotnet#8135)
  [Xamarin.Android.Build.Tasks] Handle IOException in Aapt2Daemon (dotnet#8130)
  [tests] don't set `/uses-sdk@android:targetSdkVersion=34` by default (dotnet#8138)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
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 this pull request may close these issues.

2 participants