Skip to content

[tests] fix failures related to AndroidSdkDirectory property #977

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2017
Merged

[tests] fix failures related to AndroidSdkDirectory property #977

merged 1 commit into from
Oct 25, 2017

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Oct 25, 2017

Context in #910

When switching to xabuild.exe, two mistakes were made:

  • xabuild.exe shouldn’t set AndroidSdkDirectory or
    AndroidNdkDirectory
  • Xamarin.Android.Build.Tests should pass these values to xabuild

This restores some of the code in Builder.cs removed in f9d15dd

@jonathanpeppers
[tests] fix failures related to AndroidSdkDirectory property
40d881b 
Context in dotnet#910

When switching to `xabuild.exe`, two mistakes were made:
- `xabuild.exe` shouldn’t set `AndroidSdkDirectory` or
`AndroidNdkDirectory`
- Xamarin.Android.Build.Tests should pass these values to xabuild

This restores some of the code in `Builder.cs` removed in f9d15dd
@dnfclas
Copy link

dnfclas commented Oct 25, 2017

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@jonpryor jonpryor merged commit 61b3235 into dotnet:master Oct 25, 2017
@jonpryor jonpryor mentioned this pull request Oct 25, 2017
Merged
@jonathanpeppers jonathanpeppers deleted the xabuild-androidsdk branch October 25, 2017 19:49
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Oct 27, 2017
@jonathanpeppers
[xabuild.exe] handle AndroidSdkDirectory like the script
4744d8c 
Context:
https://github.com/xamarin/xamarin-android/blob/master/tools/scripts/xabuild#L105

`xabuild`, the shell script, has some logic to set MSBuild properties
for `AndroidSdkDirectory` and `AndroidNdkDirectory`. The issue here is
on Windows, `xabuild.exe` is not invoked from this shell script. So we
need to replicate the logic in C# in `xabuild.exe`, but not break what
was fixed in dotnet#977.

Changes:
- check for `ANDROID_SDK_PATH` or `ANDROID_NDK_PATH` environment
variables
- Run `Paths.targets` via a new MSBuild process, the same way as
`xabuild`, the script
- Set the `AndroidSdkDirectory` and `AndroidNdkDirectory` values in the
config file if they are not blank
- `AndroidSdkDirectory` and `AndroidNdkDirectory` can still be
overridden by a user, because incoming values supersede what is found
in the config file
@jonathanpeppers jonathanpeppers mentioned this pull request Oct 27, 2017
Merged
jonpryor pushed a commit that referenced this pull request Oct 27, 2017
@jonathanpeppers @jonpryor
[xabuild.exe] Handle AndroidSdkDirectory like xabuild (#988)
a5fba94 
Context: https://github.com/xamarin/xamarin-android/blob/master/tools/scripts/xabuild#L105

`xabuild`, the shell script, has some logic to set MSBuild properties
for `AndroidSdkDirectory` and `AndroidNdkDirectory`. The issue here is
on Windows, `xabuild.exe` is not invoked from this shell script. So we
need to replicate the logic in C# in `xabuild.exe`, but not break what
was fixed in #977.

Changes:

  * Check for `ANDROID_SDK_PATH` or `ANDROID_NDK_PATH` environment
    variables
  * Run `Paths.targets` via a new MSBuild process, the same way as
    `tools/scripts/xabuild`
  * Set the `AndroidSdkDirectory` and `AndroidNdkDirectory`
    properties in the config file if they are not blank
  * `AndroidSdkDirectory` and `AndroidNdkDirectory` can still be
    overridden by a user, because incoming values supersede what is
    found in the config file
Redth pushed a commit to Redth/xamarin-android that referenced this pull request Oct 30, 2017
@jonathanpeppers @Redth
[tests] fix failures related to AndroidSdkDirectory property (dotnet#977
0934d2a 
)

The introduction of `xabuild` in commit f9d15dd omitted a design
requirement: `xabuild` needs to be runnable on a machine which does
*does not* have a xamarin-android build environment. The idea is that
e.g. Windows developers could download a Jenkins-built
`oss-xamarin.android*.zip` file, extract it, and run
`bin\Debug\bin\xabuild.exe` to use the extract artifacts *without*
requiring system-wide installation via `setup-windows.exe`.

This not-very-well-stated requirement brought along two mistakes:

 1. `xabuild.exe` should *not* set the `$(AndroidSdkDirectory)` and
    `$(AndroidNdkDirectory)` MSBuild properties. It *can't*; it could
    be executed on ~any machine, in particular one which hasn't built
    xamarin-android, so setting these properties to "build-tree"
    values doesn't make sense and will fail.

 2. Even if (1) weren't the case, commit f9d15dd still allowed the
    `Xamarin.Android.Build.Tests` unit tests to use `xbuild` as the
    MSBuild engine, via the `tools/scripts/xabuild` script.
    This script *does not* always set `$(AndroidSdkDirectory)` or
    `$(AndroidNdkDirectory)`.
    
As a result of (2), many unit tests *fail*:

	Error executing task ResolveSdks: System.InvalidOperationException: Could not determine Android SDK location. Please provide `androidSdkPath`.
	  at Xamarin.Android.Tools.AndroidSdkInfo..ctor (System.Action`2[T1,T2] logger, System.String androidSdkPath, System.String androidNdkPath, System.String javaSdkPath) [0x0008e] in …/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs:28 
	  at Xamarin.Android.Tasks.MonoAndroidHelper.RefreshAndroidSdk (System.String sdkPath, System.String ndkPath, System.String javaPath) [0x00021] in …/xamarin-android/src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:111 
	  at Xamarin.Android.Tasks.ResolveSdks.RunTask () [0x001a0] in …/xamarin-android/src/Xamarin.Android.Build.Tasks/Tasks/ResolveSdksTask.cs:157 
	  at Xamarin.Android.Tasks.ResolveSdks.Execute () [0x00002] in …/xamarin-android/src/Xamarin.Android.Build.Tasks/Tasks/ResolveSdksTask.cs:130 
	  at Microsoft.Build.BuildEngine.TaskEngine.Execute () [0x00000] in …/mcs/class/Microsoft.Build.Engine/Microsoft.Build.BuildEngine/TaskEngine.cs:134 
	  at Microsoft.Build.BuildEngine.BuildTask.Execute () [0x0008d] in …/mcs/class/Microsoft.Build.Engine/Microsoft.Build.BuildEngine/BuildTask.cs:101 

Update `xabuild` to *never* explicitly set `$(AndroidSdkDirectory)`
and `$(AndroidNdkDirectory)`, and update `Xamarin.Android.Build.Tests`
to *always* explicitly provide these MSBuild properties. This allows
unit test execution to be more reliable, and will prevent 
`xabuild.exe` from looking at "random" non-existent directories.
Redth pushed a commit to Redth/xamarin-android that referenced this pull request Oct 30, 2017
@jonathanpeppers @Redth
[xabuild.exe] Handle AndroidSdkDirectory like xabuild (dotnet#988)
b3d7ab5 
Context: https://github.com/xamarin/xamarin-android/blob/master/tools/scripts/xabuild#L105

`xabuild`, the shell script, has some logic to set MSBuild properties
for `AndroidSdkDirectory` and `AndroidNdkDirectory`. The issue here is
on Windows, `xabuild.exe` is not invoked from this shell script. So we
need to replicate the logic in C# in `xabuild.exe`, but not break what
was fixed in dotnet#977.

Changes:

  * Check for `ANDROID_SDK_PATH` or `ANDROID_NDK_PATH` environment
    variables
  * Run `Paths.targets` via a new MSBuild process, the same way as
    `tools/scripts/xabuild`
  * Set the `AndroidSdkDirectory` and `AndroidNdkDirectory`
    properties in the config file if they are not blank
  * `AndroidSdkDirectory` and `AndroidNdkDirectory` can still be
    overridden by a user, because incoming values supersede what is
    found in the config file
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 5, 2022
@jonpryor
Bump to xamarin/Java.Interop/main@4787e017
9226926 
Changes: dotnet/java-interop@2a882d2...4787e01

  * dotnet/java-interop@4787e017: [Java.Base-Tests] Test Java-to-Managed invocations for Java.Base (dotnet#975)
  * dotnet/java-interop@59716252: [build] `main` *conceptually* targets .NET 7 (dotnet#978)
  * dotnet/java-interop@61cdb40d: [generator] Fix reserved keywords binary search (dotnet#977)
@jonpryor jonpryor mentioned this pull request May 5, 2022
Merged
jonpryor added a commit that referenced this pull request May 6, 2022
@jonpryor
Bump to xamarin/Java.Interop/main@843f3c78
4e233cb 
Changes: dotnet/java-interop@2a882d2...843f3c7

  * dotnet/java-interop@843f3c78: [Java.Base-Tests] Use $(UtilityOutputFullPath)/jcw-gen.dll (#979)
  * dotnet/java-interop@4787e017: [Java.Base-Tests] Test Java-to-Managed invocations for Java.Base (#975)
  * dotnet/java-interop@59716252: [build] `main` *conceptually* targets .NET 7 (#978)
  * dotnet/java-interop@61cdb40d: [generator] Fix reserved keywords binary search (#977)
jonpryor added a commit that referenced this pull request May 6, 2022
@jonpryor
Bump to xamarin/Java.Interop/main@843f3c78 (#6984)
a690ea9 
Changes: dotnet/java-interop@2a882d2...843f3c7

  * dotnet/java-interop@843f3c78: [Java.Base-Tests] Use $(UtilityOutputFullPath)/jcw-gen.dll (#979)
  * dotnet/java-interop@4787e017: [Java.Base-Tests] Test Java-to-Managed invocations for Java.Base (#975)
  * dotnet/java-interop@59716252: [build] `main` *conceptually* targets .NET 7 (#978)
  * dotnet/java-interop@61cdb40d: [generator] Fix reserved keywords binary search (#977)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dellis1972 dellis1972 dellis1972 approved these changes

@jonpryor jonpryor Awaiting requested review from jonpryor

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jonathanpeppers @dnfclas @dellis1972 @jonpryor