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

[Xamarin.Android.Build.Tasks] Allow override of uncompressedGlob. #7965

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

dellis1972
Copy link
Contributor

Users reported that when they added a custom uncompressedGlob entry to the $(AndroidBundleConfigurationFile), it was ignored. This is because the MergeArrayHandling option we were using in the BuildAppBundle task was set to MergeArrayHandling.Replace.

As a result we would just overwrite the user config with our own. We should be using the MergeArrayHandling.Union option instead. This with merge the arrays , but will remove duplicates. This should allow users to provide additional uncompressedGlob options.

A unit test has been added to test this new behaviour.

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

LGTM, but a lot of CI failed due to a random policy thing.

We'll probably have to rerun when that is fixed.

@dellis1972
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pjcollins
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

jonpryor commented Apr 19, 2023

Context: https://www.newtonsoft.com/json/help/html/P_Newtonsoft_Json_Linq_JsonMergeSettings_MergeArrayHandling.htm
Context: https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_Linq_MergeArrayHandling.htm

The MergeArrayHandling enum has .Concat, .Union, .Replace (what we previously used), and .Merge.

What the docs don't say is what those necessarily mean. .Concat seems simple enough -- concatenate the contents of the latter array to the former -- but how do .Union and .Merge differ from each other?

Merge Merge array items together, matched by index

I don't know what this means.

For example, what happens if we merge */*.mp3 with the default list? https://github.com/xamarin/xamarin-android/blob/e31afeec350db8550fb1073ef1b87f7af6c0d288/src/Xamarin.Android.Build.Tasks/Tasks/BuildAppBundle.cs#L22-L60

*/*.mp3 isn't identical to **/*.mp3, so…what would happen? Which comes first? Does that matter?

Is .Union the right value here?

@jonpryor
Copy link
Member

@dellis1972: please rebase this PR; the unit test failures are "weird", and I don't see how this PR could cause them.

@dellis1972
Copy link
Contributor Author

Merge Merge array items together, matched by index

I'm not sure what this means either. I suspect it means by index into the array ... so item 3 in the source array will be overwritten by item 3 if the new array (if it exists). That sounds a bit silly to me.

Users reported that when they added a custom `uncompressedGlob` entry to
the `$(AndroidBundleConfigurationFile)`, it was ignored.
This is because the `MergeArrayHandling` option we were using in
the `BuildAppBundle` task was set to `MergeArrayHandling.Replace`.

As a result we would just overwrite the user config with our own.
We should be using the `MergeArrayHandling.Union` option instead. This
with merge the arrays , but will remove duplicates. This should allow
users to provide additional `uncompressedGlob` options.

A unit test has been added to test this new behaviour.
@jonpryor jonpryor merged commit d035a32 into dotnet:main Jun 16, 2023
@dellis1972 dellis1972 deleted the aabuncompressed branch June 16, 2023 19:07
grendello added a commit to grendello/xamarin-android that referenced this pull request Jun 19, 2023
* main:
  [Xamarin.Android.Build.Tasks] Allow override of `uncompressedGlob` (dotnet#7965)
@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.

4 participants