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

Unstable framework versions are used when $(AndroidUselatestPlatformSDK)=true #1511

Closed
pjcollins opened this Issue Apr 4, 2018 · 9 comments

Comments

Projects
None yet
6 participants
@pjcollins
Member

pjcollins commented Apr 4, 2018

Steps to Reproduce

  1. Copy the v8.1 framework to v108.1.99

  2. Edit v108.1.99\AndroidApiInfo.xml, and set <Id/> to "Z", set <Level/> to 127, set <Version/> to v108.1.99, and <Stable/> to False:

    <AndroidApiInfo>
      <Id>Z</Id>
      <Level>127</Level>
      <Name>Z Preview</Name>
      <Version>v108.1.99</Version>
      <Stable>False</Stable>
    </AndroidApiInfo>
  1. Copy $(AndroidSdkDirectory)\platforms\android-27 to $(AndroidSdkDirectory)\platforms\android-Z.

  2. Update a new or existing project so that $(TargetFrameworkVersion)=v8.0, and $(AndroidUseLatestPlatformSdk)=true

Alternatively, a recent build from master which has early P preview support can be used.

Expected Behavior

Using a project with a $(TargetFrameworkVersion) lower than the max stable version, and setting $(AndroidUseLatestPlatformSdk)=true, I would expect my project to build against the latest stable version that is installed. In the case mentioned above, this would be android-27 (v8.1).

Actual Behavior

A project with a $(TargetFrameworkVersion) lower than the max stable version, and setting $(AndroidUseLatestPlatformSdk)=true builds against my fake unstable framework version, android-Z.

Version Information

Xamarin.Android.Sdk-OSS-8.3.99.91_HEAD_33822ad.vsix

Log File

http://xqa.blob.core.windows.net/gist/log-83bd0e75034940db9ae887a8f5c03d98.txt

VS bug #594908

@pjcollins pjcollins added this to the d15-7 milestone Apr 4, 2018

@pjcollins pjcollins added the vs-sync label Apr 4, 2018

jonpryor added a commit to xamarin/xamarin-android-tools that referenced this issue Apr 16, 2018

[Xamarin.Android.Tools.AndroidSdk] Unstable framework versions are us…
…ed (#25)

Context: xamarin/xamarin-android#1511
Context: xamarin/xamarin-android#1512

Within xamarin-android, when building with
`$(AndroidUseLatestPlatformSdk)`=True -- which is supposed to use the
latest *stable* API level -- builds would instead use the latest API
level, [*even if unstable*][0]:

[0]: http://xqa.blob.core.windows.net/gist/log-83bd0e75034940db9ae887a8f5c03d98.txt

	ResolveSdksTask: (TaskId:7)
	  AndroidApiLevel:  (TaskId:7)
	  TargetFrameworkVersion: v8.0 (TaskId:7)
	  UseLatestAndroidPlatformSdk: True (TaskId:7)
	  ...
	ResolveSdksTask Outputs: (TaskId:7)
	  AndroidApiLevel: 127 (TaskId:7)
	  AndroidApiLevelName: Z (TaskId:7)
	  TargetFrameworkVersion: v108.1.99 (TaskId:7)
	  SupportedApiLevel: 127 (TaskId:7)
	  ...

Note that in the repro, API-127 ("Z") is *Unstable* (as per
`AndroidApiInfo.xml`), yet it's used for the build.

The cause is due to our logic to calculate
`AndroidVersions.MaxStableVersion`. The first time through the
`AndroidVersions.LoadVersions()` method the
`AndroidVersions.MaxStableVersion` property will *always* be `null`.
As a result the *first* `version` value would *always* be assigned to
`AndroidVersions.MaxStableVersion`, even if not stable!

	// Old and busted
	foreach (var version in versions) {
	  installedVersions.Add (version);
	  if (MaxStableVersion == null || (version.Stable && MaxStableVersion.TargetFrameworkVersion < version.TargetFrameworkVersion)) {
	    MaxStableVersion = version;
	  ...

Whether or not `AndroidVersions.MaxStableVersion` *could* be unstable
was dependent on the order of directory traversal, which isn't sorted
(nor should it be!), meaning behavior could *also be random*. (Fun!)

If `AndroidVersions.MaxStableVersion` *were* an unstable API level,
xamarin-android builds when `$(AndroidUseLatestPlatformSdk)`=True
would be unpredictable, as an API binding with no API compatibility
requirements could be used.

The fix is to completely ignore unstable apis. This way the first
stable api we find will be the first value we assign.

jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Apr 16, 2018

Bump to xamarin-android-tools/master/50af063b
Fixes: xamarin#1511
Fixes: xamarin#1512

Check `$HOME/Library/Android/sdk` for default Android SDK location.

Add `AndroidSdkInfo.AllAndroidSdkPaths` property.

jonpryor added a commit that referenced this issue Apr 17, 2018

Bump to xamarin-android-tools/master/50af063b (#1562)
Fixes: #1511
Fixes: #1512

Check `$HOME/Library/Android/sdk` for default Android SDK location.

Add `AndroidSdkInfo.AllAndroidSdkPaths` property.

dellis1972 added a commit to dellis1972/xamarin-android that referenced this issue Apr 17, 2018

[Xamarin.Android.Build.Test] Rework CreateFauxAndroidSdkDirectory to …
…take ApiInfo[].

Context xamarin#1511

For most tests we already define an ApiInfo[] to contain
apis we want to test against. This commit cleans up the helper
methods to make use of the same ApiInfo[] arrays to create
the `android-{Id}` directories.

It also adds unit tests to conver the fix made for xamarin#1511.

@pjcollins pjcollins added the verified label Apr 17, 2018

@pjcollins

This comment has been minimized.

Member

pjcollins commented Apr 17, 2018

This is no longer reproducible using xamarin-android/master/efe81f5.

jonpryor added a commit to xamarin/xamarin-android-tools that referenced this issue Apr 17, 2018

[Xamarin.Android.Tools.AndroidSdk] Unstable framework versions are us…
…ed (#25)

Context: xamarin/xamarin-android#1511

Within xamarin-android, when building with
`$(AndroidUseLatestPlatformSdk)`=True -- which is supposed to use the
latest *stable* API level -- builds would instead use the latest API
level, [*even if unstable*][0]:

[0]: http://xqa.blob.core.windows.net/gist/log-83bd0e75034940db9ae887a8f5c03d98.txt

	ResolveSdksTask: (TaskId:7)
	  AndroidApiLevel:  (TaskId:7)
	  TargetFrameworkVersion: v8.0 (TaskId:7)
	  UseLatestAndroidPlatformSdk: True (TaskId:7)
	  ...
	ResolveSdksTask Outputs: (TaskId:7)
	  AndroidApiLevel: 127 (TaskId:7)
	  AndroidApiLevelName: Z (TaskId:7)
	  TargetFrameworkVersion: v108.1.99 (TaskId:7)
	  SupportedApiLevel: 127 (TaskId:7)
	  ...

Note that in the repro, API-127 ("Z") is *Unstable* (as per
`AndroidApiInfo.xml`), yet it's used for the build.

The cause is due to our logic to calculate
`AndroidVersions.MaxStableVersion`. The first time through the
`AndroidVersions.LoadVersions()` method the
`AndroidVersions.MaxStableVersion` property will *always* be `null`.
As a result the *first* `version` value would *always* be assigned to
`AndroidVersions.MaxStableVersion`, even if not stable!

	// Old and busted
	foreach (var version in versions) {
	  installedVersions.Add (version);
	  if (MaxStableVersion == null || (version.Stable && MaxStableVersion.TargetFrameworkVersion < version.TargetFrameworkVersion)) {
	    MaxStableVersion = version;
	  ...

Whether or not `AndroidVersions.MaxStableVersion` *could* be unstable
was dependent on the order of directory traversal, which isn't sorted
(nor should it be!), meaning behavior could *also be random*. (Fun!)

If `AndroidVersions.MaxStableVersion` *were* an unstable API level,
xamarin-android builds when `$(AndroidUseLatestPlatformSdk)`=True
would be unpredictable, as an API binding with no API compatibility
requirements could be used.

The fix is to completely ignore unstable apis. This way the first
stable api we find will be the first value we assign.

jonpryor added a commit that referenced this issue Apr 17, 2018

@xamarin-release-manager

This comment has been minimized.

Collaborator

xamarin-release-manager commented Apr 18, 2018

[sync] [VS-8] Comment by Bill Holmes

 Opening bug again for the 15-7 P5 release pipeline.  

jonpryor added a commit that referenced this issue Apr 23, 2018

[Xamarin.Android.Build.Test] Rework CreateFauxAndroidSdkDirectory to …
…take ApiInfo[]. (#1566)

Context: #1511

For most tests we already define an `ApiInfo[]` to contain apis we
want to test against. This commit cleans up the helper methods to
make use of the same `ApiInfo[]` arrays to create the
`android-{Id}` directories.

It also adds unit tests to conver the fix for #1511 in efe81f5.
@pjcollins

This comment has been minimized.

Member

pjcollins commented May 21, 2018

While this was seemingly fixed for me against a previous Xamarin OSS master build, I am again able to reproduce this against monodroid/master. The unstable .dll and .jar files are still used:

FilteredAssemblies=C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v108.1.99\Mono.Android.dll
MonoPlatformJarPath=C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\ReferenceAssemblies\Microsoft\Framework\MonoAndroid\v108.1.99\mono.android.jar
JavaPlatformJarPath=C:\Program Files (x86)\Android\android-sdk\platforms\android-Z\android.jar

XA version: 8.3.99.12 (monodroid/HEAD/de662f58d)

Diagnostic build output:
http://xqa.blob.core.windows.net/gist/log-fd74d75ecc3c41b0a59c4cbfc3532bfb.txt

@pjcollins pjcollins reopened this May 21, 2018

@pjcollins pjcollins modified the milestones: d15-7, d15-8 May 21, 2018

@dellis1972

This comment has been minimized.

Contributor

dellis1972 commented May 21, 2018

@pjcollins

This comment has been minimized.

Member

pjcollins commented May 30, 2018

This is fixed again when using XA 8.3.99.20 (monodroid/master/80386193a), but I'm having a bit of difficulty tracking the fix as it seems like it should have also been included in 8.3.99.12 (monodroid/master/de662f58d)?

@jonathanpeppers

This comment has been minimized.

Contributor

jonathanpeppers commented May 31, 2018

@pjcollins I think it's because we have multiple copies of xamarin-android-tools floating around.

I suspect monodroid/c03f3b4 fixed this from bumping androidtools, which has its own copy of xamarin-android-tools for some reason...

@jonpryor

This comment has been minimized.

Contributor

jonpryor commented Jun 1, 2018

which has its own copy of xamarin-android-tools for some reason...

Everything references xamarin-android-tools. It's turning into quite the leaf node... You wouldn't believe the fun I've had bumping all the intermediate references...

@pjcollins

This comment has been minimized.

Member

pjcollins commented Jun 20, 2018

This is no longer reproducible from command line on both Mac and Windows using d15-8/8.4.0.6.

As for in IDE experiences however, we're a bit out of sync. VS removes and ignores the AndroidUseLatestPlatformSdk flag entirely in 15.8, so this is no longer a valid scenario there.

This is still an issue when running inside of VS Mac however. I've opened https://devdiv.visualstudio.com/DevDiv/_workitems/edit/636136 to track that, and will and close this one out.

@pjcollins pjcollins closed this Jun 20, 2018

@MagicAndre1981

This comment has been minimized.

MagicAndre1981 commented Aug 20, 2018

@pjcollins

is there any documentation about the AndroidUseLatestPlatformSdk removal in 15.8? I only see it gets removed (changed local files in source control) when I open a project in 15.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment