Xamarin.Build.Download package id validation causes build errors

[AdamEssenmacher]

I upgraded my Xamarin.Android project to Xamarin.Firebase.Storage to v120.x.x.x to take advantage of the newer firebase emulator APIs, which caused the build error "Invalid item ID firebaseappcheckinterop-16.0.0-beta01".

Related issues reported on StackOverflow:
https://stackoverflow.com/questions/68599578/xamarin-build-download-targets-invalid-id
https://stackoverflow.com/questions/69458719/i-get-the-error-invalid-item-id-firebaseappcheckinterop-16-0-0-beta01-during-b

Digging in, I've determined that the root cause is in Xamarin.Build.Download.DownloadUtils.ParseDownloadItems(..), which is highly opinionated on what should be considered a valid package id. Specifically, it expects that the version component of a package id (in this case, 16.0.0-beta01) should only use periods as separators. Since this version component uses a dash - before the beta01 postfix, the id check fails.

To confirm this, I cloned the XamarinComponents repo and modified the offending method to exclude the id check:

public IEnumerable<XamarinBuildDownload> ParseDownloadItems (ITaskItem[] items, bool allowUnsecureUrls)
		{
			if (items == null || items.Length <= 0)
				return new List<XamarinBuildDownload> ();
			
			var results = new List<XamarinBuildDownload> ();

			foreach (var item in items) {
				var xbd = new XamarinBuildDownload ();

				xbd.Id = item.ItemSpec;
				// !!!! THIS IS CAUSING BUILDS TO FAIL
				//if (!ValidateId (xbd.Id)) {
				//	Log.LogCodedError (ErrorCodes.XbdInvalidItemId, "Invalid item ID {0}", xbd.Id);
				//	continue;
				//}
				xbd.Url = item.GetMetadata ("Url");
......

After packing the modified code and copying the output to my local nuget cache (replacing xamarin.build.download v0.10.0), my Xamarin.Android project compiled and ran without issue.

My recommendations for options to fix:

1. Convince Google to stop including preview dependencies in non-preview packages 🙄
2. Modify the id-checking logic in DownloadUtils to permit dash - delimiters OR
3. Remove the id-checking logic in DownloadUtils entirely. Unless there's a good reason for this that isn't apparent to me (which is totally possible likely), it seems odd that Xamarin.Build.Download would assume responsibility for enforcing any specific package id/version format.

[moljac]

Thanks for your feedback

My recommendations for options to fix:

1. Convince Google to stop including preview dependencies in non-preview packages 🙄

Nice try, but this is utopistic. The number of artifacts depending on previews will most likely grow.

See

https://stackoverflow.com/questions/68723859/ml-kit-stable-artifacts-depending-on-preview-package-com-google-android-odml-im

2. Modify the id-checking logic in DownloadUtils to permit dash - delimiters OR

firebaseappcheckinterop-16.0.0-beta01 is not official artifactId, but seems some internal representation in XBD (most likely to shorten the path in order not to exceed MAX_PATH in Windows, but not 100% sure here).

To me it smells more like semver parsing (problem with previews).

3. Remove the id-checking logic in DownloadUtils entirely. Unless there's a good reason for this that isn't apparent to me (which is totally possible likely), it seems odd that Xamarin.Build.Download would assume responsibility for enforcing any specific package id/version format.

I work on new bindings tooling and will try to fix this with few utilities from that effort and as soon as possible. Checks have good intention, but can cause problems if not all edge cases are covered (like in this case).

[moljac]

Why not submitting PR?

[moljac]

Notes to myself:

source + unit tests (tests are in source/ folde)

https://github.com/xamarin/XamarinComponents/tree/main/Util/Xamarin.Build.Download/source

[moljac]

possibly related

#1296

at least merge together

[AdamEssenmacher]

Why not submitting PR?

@moljac I'd be happy to submit a PR to either remove the check or modify it to allow for the -alpha42 style pre-release suffixes as documented here: https://docs.microsoft.com/en-us/nuget/concepts/package-versioning#pre-release-versions

I didn't want to open a PR right away without fully understanding why that check was put there in the first place. I tried a git blame to maybe get more context, but it looks like that code has been there since before it was open-sourced. Maybe someone from Microsoft can look back further?

Also, I stumbled across #1248 which looks like the same issue. These could probably be merged.