Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Jan 13, 2022

Context: dotnet/java-interop#727

Add an MSBuild equivalent to the <ns-replace> metadata feature added in dotnet/java-interop#727.

This takes namespace transforms like:

<ItemGroup>
  <AndroidNamespaceReplacement Include='Androidx' Replacement='AndroidX' />
  <AndroidNamespaceReplacement Include='Com' Replacement='' />
  <AndroidNamespaceReplacement Include='Com.Google.' Replacement='Google' />
</ItemGroup>

And places them into a generated msbuild-metadata.xml file that is passed to generator:

<metadata>
  <ns-replace source='Androidx' replacement='AndroidX' />
  <ns-replace source='Com' replacement='' />
  <ns-replace source='Com.Google.' replacement='Google' />
</metadata>

Today, this only supports <ns-replace> metadata, but it could be expanded in the future if we feel that allowing other metadata is desirable. (I am personally skeptical 😁.)

@jpobst jpobst force-pushed the namepsace-replacements branch 5 times, most recently from b1ba3b9 to 3776da5 Compare January 20, 2022 17:15
@jpobst jpobst marked this pull request as ready for review January 20, 2022 22:23
@jonpryor
Copy link
Contributor

The RemoveEventHandlerResolution failures are fixed by 599fffd.

// ex: obj/Debug/generated/msbuild-metadata.xml
var transform_file = Path.Combine (OutputDirectory, "..", "msbuild-metadata.xml");

var xml = new XDocument ();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you need the XDocument at all? XElement.WriteTo() also works…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@jonpryor
Copy link
Contributor

@jpobst: the new item group should also be documented in: https://github.com/xamarin/xamarin-android/blob/main/Documentation/guides/building-apps/build-items.md

xml.Add (root);

foreach (var nt in NamespaceTransforms)
xml.Root.Add (new XElement ("ns-replace", new XAttribute ("source", nt.ItemSpec), new XAttribute ("replacement", nt.GetMetadata ("replacement"))));
Copy link
Member

Choose a reason for hiding this comment

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

Should we emit a warning/error if %(Replacement) is blank? I'm just thinking if someone put Replace="AndroidX" on accident, they won't know what's wrong?

Copy link
Contributor Author

@jpobst jpobst Jan 20, 2022

Choose a reason for hiding this comment

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

%(Replacement) being blank is valid and useful. I guess the question is should we require users to provide the blank attribute Replacement=""? Perhaps we should?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and documented new error.

{
var cmd = GetCommandLineBuilder ();

if (NamespaceTransforms?.Any () == true) {
Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine, but feels weird. Would this be just as good?

Suggested change
if (NamespaceTransforms?.Any () == true) {
if (NamespaceTransforms != null && NamespaceTransforms.Length > 0) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, those are equivalent. I think Any and null-check operator is generally considered the modern "best practice".

Copy link
Member

Choose a reason for hiding this comment

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

I generally avoid using System.Linq;, but there are some cases where OrderBy() is ok.

This is fine, though.

@jpobst jpobst force-pushed the namepsace-replacements branch 3 times, most recently from d5e6aca to 476d740 Compare January 25, 2022 15:53
@jpobst jpobst force-pushed the namepsace-replacements branch from 476d740 to 3607637 Compare January 31, 2022 16:19
This reverts commit 19ffad4.

lol, conflicts w/ main!
4a71aa6 renamed the `android-aot` workload to `android`, which
appears to have broken some unit tests.

Merge and pray for the best.
@jonpryor jonpryor merged commit 398afd4 into main Feb 2, 2022
@jonpryor jonpryor deleted the namepsace-replacements branch February 2, 2022 01:59
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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.

5 participants