-
Notifications
You must be signed in to change notification settings - Fork 528
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] Prevent $(AndroidUseLatestPlatformSdk)
from using Unstable APIs
#1228
Conversation
build |
The macOS+xbuild PR Build is failing:
|
build |
520e940
to
653786b
Compare
653786b
to
3cea623
Compare
var userSelected = MonoAndroidHelper.SupportedVersions.GetApiLevelFromFrameworkVersion (TargetFrameworkVersion); | ||
// overwrite using user version only if it is | ||
// above the maxStableApi and a valid apiLevel. | ||
if (userSelected != null && userSelected > maxSupported && userSelected <= maxInstalled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of this make sense. :-(
The use case is this: "soon", Google will presumably release API-P in alpha form, for release as API-28 this fall.
When we provide a binding, it will probably be $(TargetFrameworkVersion)
=v8.99.
Assuming the user has v8.99
installed -- how that's done is "elsewhere" -- the user should be able to opt in to using it by setting $(TargetFrameworkVersion)
=v8.99.
Thus, !string.IsNullOrWhiteSpace (TargetFrameworkVersion)
(line 433) will be true
.
userSelected
(line 434), meanwhile, will be null
, as there is no API level for unstable APIs. Relatedly, MonoAndroidHelper.SupportedVersions.GetIdFromFrameworkVersion("v8.99")
will return "P"
.
Similarly, maxInstalled
comes from int.TryParse()
comes from GetMaxInstalledApiLevel()
, which only looks at values which can be parsed into an int
. As such, API-P will not be present or used, meaning -- for our current exercise -- AndroidApiLevel
is ~useless. Consequently, maxInstalled
is likewise useless.
In short, I don't see how this will do what we want it to do. :-(
new ApiInfo () { Id = 23, Level = 23, Name = "Marshmallow", FrameworkVersion = "v6.0", Stable = true }, | ||
new ApiInfo () { Id = 26, Level = 26, Name = "Oreo", FrameworkVersion = "v8.0", Stable = true }, | ||
new ApiInfo () { Id = 27, Level = 27, Name = "Oreo", FrameworkVersion = "v8.1", Stable = false }, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the test case, we need to add:
new ApiInfo () { Id = "P", Level = 28, Name = "P", FrameworkVersion="v8.99", Stable = false },
(This is why AndroidApiInfo.xml
differentiates Id
from Level
. They're the same for all stable API levels, but for preview API levels they are not.)
if [ -z "$XBUILD_FRAMEWORK_FOLDERS_PATH" ]; then | ||
export XBUILD_FRAMEWORK_FOLDERS_PATH="$TARGETS_DIR-frameworks" | ||
else | ||
export XBUILD_FRAMEWORK_FOLDERS_PATH="$XBUILD_FRAMEWORK_FOLDERS_PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the environment have $XBUILD_FRAMEWORK_FOLDERS_PATH
already exported, and why should we be using it in preference to the computed value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be able to override it to the Faux location as part of the unit tests. If we don't it will pick up the bin\Debug
path which we don't want. Is there a better way of doing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I can't think of a better way, though this will only work with xbuild
, not msbuild
.
Perhaps a "better" (or just different?) way would be to define a new environment variable, e.g. $__MSBUILD_FRAMEWORK_FOLDERS_PATH_OVERRIDE
, and use that if it's set. We'd also want to update the msbuild
-based codepath to follow suit, perhaps via setting $(TargetFrameworkRootPath)
?
One problem with the current xabuild
script, though: the msbuild
-specific logic on line 133 happens after we exec xabuild.exe
on line 86, meaning line 133/etc. is superfluous. This should be corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msbuild will use the TargetFrameworkRootPath
as a parameter so msbuild support works. xbuild ignores or doesn't use TargetFrameworkRootPath
so thats why I needed to use the envvar.
if we merge #1321 I can add actual tests for this logic :) |
e576b14
to
042b1dd
Compare
@jonpryor Given the following bindings are available can you confirm the correct behaviour (assuming the required java platforms are installed).
|
@dellis1972: that table mostly looks correct. However, this line seems weird:
If |
…` from using Unstable APIs Fixes dotnet#1221 Currently when a user sets `$(AndroidUseLatestPlatformSdk)` it will automatically use the Max ApiLevel. This is not always a stable ApiLevel. It is highly possible that it will be unstable. Users will generally want to keep to the Max Stable in this senario. This commit alters the code so that we use the latest Max Stable ApiLevel when using `$(AndroidUseLatestPlatformSdk)`. However we should still allow users to use the unstable ones if they really want to. This can be achieved in a number of ways 1. Turn off `$(AndroidUseLatestPlatformSdk)` and set the `$(TargetFrameworkVersion)` directly. 2. Leave `$(AndroidUseLatestPlatformSdk)` on and set the `$(TargetFrameworkVersion)` directly. So effectviely, just manually target the unstable Api. But it is something the user will need to think about and do manaully. By default we will only use to the latest stable ApiLevel.
@jonpryor if you take a look at [1]. In ValidateApiLevels if |
7b63603
to
35b248f
Compare
…TABLE levels (#1228) Fixes: #1221 Currently when a user sets `$(AndroidUseLatestPlatformSdk)` it will automatically use the maximum known API level. This is not always a stable ApiLevel, e.g. when a preview API level has been bound, and the binding has been installed. Users will generally want to use the maximum *stable* API level when `$(AndroidUseLatestPlatformSdk)` is True. Alters things so that we use the maximum Stable API level when `$(AndroidUseLatestPlatformSdk)` is True. However, allow users to use unstable API levels if desired. This can be achieved by setting `$(TargetFrameworkVersion)` to the desired unstable framework version. (For example, when API-O was in preview, this would be `v7.99`. When API-P is presumably announced, it will be bound as `v8.99`.) This will require a *manual* change to the `.csproj`, as we do not anticipate the IDEs showing unstable API levels (though this could plausibly be done with sufficient warnings...).
Context: #6353 Changes: xamarin/monodroid@07a99d7...9b3a37a * xamarin/monodroid@9b3a37af1: [tools/msbuild] <GetPrimaryCpuAbi/> should not select 32-bit ABIs (#1228) Update `MonoAndroidExportTest.MonoAndroidExportReferencedAppStarts()` under .NET 6 to add `x86_64`, e.g. `$(AndroidSupportedAbis)`=`armeabi-v7a;x86;x86_64`. This is needed because the API-31 emulator only supports x86_64, not x86.
Context: #6353 Changes: xamarin/monodroid@07a99d7...9b3a37a * xamarin/monodroid@9b3a37af1: [tools/msbuild] <GetPrimaryCpuAbi/> should not select 32-bit ABIs (#1228) Update `MonoAndroidExportTest.MonoAndroidExportReferencedAppStarts()` under .NET 6 to add `x86_64`, e.g. `$(AndroidSupportedAbis)`=`armeabi-v7a;x86;x86_64`. This is needed because the API-31 emulator only supports x86_64, not x86.
Fixes #1221
Currently when a user sets
$(AndroidUseLatestPlatformSdk)
it willautomatically use the Max ApiLevel. This is not always a stable
ApiLevel. It is highly possible that it will be unstable.
Users will generally want to keep to the Max Stable in this senario.
This commit alters the code so that we use the latest Max Stable
ApiLevel when using
$(AndroidUseLatestPlatformSdk)
.However we should still allow users to use the unstable ones if
they really want to. This can be achieved in a number of ways
Turn off
$(AndroidUseLatestPlatformSdk)
and set the$(TargetFrameworkVersion)
directly.Leave
$(AndroidUseLatestPlatformSdk)
on and set the$(TargetFrameworkVersion)
directly.So effectviely, just manually target the unstable Api. But it
is something the user will need to think about and do manaully.
By default we will only use to the latest stable ApiLevel.