[Xamarin.Android.Build.Tasks] add feature flag for Google Manifest Merger #4022
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
My "concern" with In this context, "managed" would be the bad version. "managed" also precludes us from doing a different implementation from C# in the future which follows the Android Studio semantics. I thus propose the alternate names:
This "nicely" has a clear "old and busted" value, and if (when?) Google comes out with a different native tool, we'll be able to reasonably distinguish between them. |
Contributor
|
Using |
…rger Context: dotnet#3918 In 2c6f5cd, we added support for the official Google/Android Manifest Merger. After further QA testing, it seems that a reasonable number of existing apps require code changes to *appease* the new manifest merger. This is a similar situation we were in compared to `aapt2`, where the suggested changes are good (and usually problems). I think we should add a feature flag, so we can gradually turn this on. We could add the feature off by default for one release, and then turn it on by default for the next. This PR adds: * `$(AndroidManifestMerger)` with enum-style values of `legacy` and `manifestmerger.jar`. * Updated a few tests to leverage both code paths.
jonathanpeppers
force-pushed
the
manifest-merger-feature-flag
branch
from
5f495d3 to
40946ef
Compare
pjcollins
reviewed
Dec 12, 2019
Member
pjcollins
left a comment
pjcollins
left a comment
There was a problem hiding this comment.
Looking at 2c6f5cd, I think this may also need changes in
src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Designer.targets?
We likely need to add the MergedManifestDocuments parameter back (and maybe rely on the new path property) here:
https://github.com/xamarin/xamarin-android/blob/2c6f5cd16fb5087731107c1defa13ebb56cf63df/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Designer.targets#L79
I think we also need to add a condition here:
Condition=" '$(AndroidManifestMerger)' == 'manifestmerger.jar' "
Contributor
|
👍 to I like the idea that we would in the future use whatever the new tooling name is as the replacement to Add a 👍 to the current proposal from me. |
Member
Author
|
I think the failures here are ignorable, this one has started popping up again randomly: |
garuma
approved these changes
Dec 17, 2019
pjcollins
approved these changes
Dec 17, 2019
Member
pjcollins
left a comment
pjcollins
left a comment
There was a problem hiding this comment.
Updated naming schema and latest changes LGTM 👍
jonathanpeppers
added a commit
to jonathanpeppers/xamarin-android
that referenced
this pull request
Dec 17, 2019
…rger (dotnet#4022) Context: xamarin/QualityAssurance@a01b7d9 In commit 2c6f5cd, we added support for using the official Google/Android [Manifest Merger][0]. After further QA testing, it seems that a reasonable number of existing apps require code changes to *appease* the new manifest merger: error AMM0000: …/QualityAssurance/Samples/android/XAQA.Bug28816/obj/Release/AndroidManifest.xml:4:3-42 Error: error AMM0000: uses-sdk:minSdkVersion 10 cannot be smaller than version 14 declared in library …/QualityAssurance/Samples/android/XAQA.Bug28816/obj/Release/lp/6/jl/AndroidManifest.xml as the library might be using APIs not available in 10 error AMM0000: Suggestion: use a compatible library with a minSdk of at most 10, error AMM0000: or increase this project's minSdk version to at least 14, error AMM0000: or use tools:overrideLibrary="android.support.v4" to force usage (may lead to runtime failures) This is a similar situation we were in compared to `aapt2`, where the suggested changes are good (and usually fix problems). I think we should add a feature flag, so we can gradually turn this on. We could add the feature off by default for one release, and then turn it on by default for the next. Add a new `$(AndroidManifestMerger)` MSBuild property to control which `AndroidManifest.xml` merger is used. The values are: * `legacy`: Merge `AndroidManifest.xml` files in the same way we did before commit 2c6f5cd. * `manifestmerger.jar`: Use Android's Manifest Merger. The default value for `$(AndroidManifestMerger)` is `legacy`. This will change in a future release. [0]: https://developer.android.com/studio/build/manifest-merge
jonathanpeppers
added a commit
to jonathanpeppers/xamarin-android
that referenced
this pull request
Dec 17, 2019
…rger (dotnet#4022) Context: xamarin/QualityAssurance@a01b7d9 In commit 2c6f5cd, we added support for using the official Google/Android [Manifest Merger][0]. After further QA testing, it seems that a reasonable number of existing apps require code changes to *appease* the new manifest merger: error AMM0000: …/QualityAssurance/Samples/android/XAQA.Bug28816/obj/Release/AndroidManifest.xml:4:3-42 Error: error AMM0000: uses-sdk:minSdkVersion 10 cannot be smaller than version 14 declared in library …/QualityAssurance/Samples/android/XAQA.Bug28816/obj/Release/lp/6/jl/AndroidManifest.xml as the library might be using APIs not available in 10 error AMM0000: Suggestion: use a compatible library with a minSdk of at most 10, error AMM0000: or increase this project's minSdk version to at least 14, error AMM0000: or use tools:overrideLibrary="android.support.v4" to force usage (may lead to runtime failures) This is a similar situation we were in compared to `aapt2`, where the suggested changes are good (and usually fix problems). I think we should add a feature flag, so we can gradually turn this on. We could add the feature off by default for one release, and then turn it on by default for the next. Add a new `$(AndroidManifestMerger)` MSBuild property to control which `AndroidManifest.xml` merger is used. The values are: * `legacy`: Merge `AndroidManifest.xml` files in the same way we did before commit 2c6f5cd. * `manifestmerger.jar`: Use Android's Manifest Merger. The default value for `$(AndroidManifestMerger)` is `legacy`. This will change in a future release. [0]: https://developer.android.com/studio/build/manifest-merge
jonpryor
pushed a commit
that referenced
this pull request
Dec 18, 2019
…rger (#4022) Context: https://github.com/xamarin/QualityAssurance/commit/a01b7d9d6a724523878236e49dbb8f69c6dfedc8 In commit 2c6f5cd, we added support for using the official Google/Android [Manifest Merger][0]. After further QA testing, it seems that a reasonable number of existing apps require code changes to *appease* the new manifest merger: error AMM0000: …/QualityAssurance/Samples/android/XAQA.Bug28816/obj/Release/AndroidManifest.xml:4:3-42 Error: error AMM0000: uses-sdk:minSdkVersion 10 cannot be smaller than version 14 declared in library …/QualityAssurance/Samples/android/XAQA.Bug28816/obj/Release/lp/6/jl/AndroidManifest.xml as the library might be using APIs not available in 10 error AMM0000: Suggestion: use a compatible library with a minSdk of at most 10, error AMM0000: or increase this project's minSdk version to at least 14, error AMM0000: or use tools:overrideLibrary="android.support.v4" to force usage (may lead to runtime failures) This is a similar situation we were in compared to `aapt2`, where the suggested changes are good (and usually fix problems). I think we should add a feature flag, so we can gradually turn this on. We could add the feature off by default for one release, and then turn it on by default for the next. Add a new `$(AndroidManifestMerger)` MSBuild property to control which `AndroidManifest.xml` merger is used. The values are: * `legacy`: Merge `AndroidManifest.xml` files in the same way we did before commit 2c6f5cd. * `manifestmerger.jar`: Use Android's Manifest Merger. The default value for `$(AndroidManifestMerger)` is `legacy`. This will change in a future release. [0]: https://developer.android.com/studio/build/manifest-merge
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #3918
In 2c6f5cd, we added support for the official Google/Android Manifest
Merger. After further QA testing, it seems that a reasonable number of
existing apps require code changes to appease the new manifest
merger.
This is a similar situation we were in compared to
aapt2, where thesuggested changes are good (and usually problems). I think we should
add a feature flag, so we can gradually turn this on. We could add the
feature off by default for one release, and then turn it on by default
for the next.
This PR adds:
$(AndroidManifestMerger)with enum-style values oflegacyandmanifestmerger.jar.