Skip to content

Conversation

@dellis1972
Copy link
Contributor

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=51507

If a user has an older verison of Java installed they currently get
this kind of helpful error message.

"Unsupported major.minor version 52.0"

While for the experienced user this might mean "Ah I need Java 1.8".
For the new users this is confusing.

This commit adds a new property in the Xamarin.Android.Common.props
called

`$(MinimumRequiredJavaVersion)`

This will define the Minimum required version of Java we need.
It can be overridden on the command line or in the project is needed.

If this mimimum version is NOT met, we will error out. While it
might be nicers to issue a warning, this will eventually end up with
the above error anyway. So we might as well tell the user exactly what
is wrong.

@jonpryor
Copy link
Contributor

This fixes a problem, but isn't elegant.

For example, see the System Requirements section of our release notes:

  • JDK 1.8 - up to API 24+
  • JDK 1.7 - up to API 23
  • JDK 1.6 - up to API 20

We know this.

The problem with this PR is that it adds a new $(MinimumRequiredJavaVersion) property that nobody is going to know about. Sure, we provide a default value, but the result is this: if someone has a development environment today which:

  1. Has JDK 1.7 installed, and
  2. Writes a project which Targets API-23 or earlier

then their currently working system will no longer work after upgrade. We will instead flag an error.

This is, shall we say, not desirable. :-)

What should instead be done is we should do in code what we currently have in the release notes: associate a minimum required JDK version which each $(TargetFrameworkVersion), and check the current java -version output against the associated minimum version. This will allow existing development environments to continue working as they have been.


There is one problem with the above approach: It is entirely plausible that, at some point in time, Android's build-tools SDK component (or whatever) will require JDK 1.8 in order to execute. Should that happen, we'll need a "global" java version anyway, and the approach of this PR may be more appropriate.

} catch (Exception ex) {
Log.LogWarningFromException (ex);
Log.LogWarning ($"Failed to get the Java SDK version. Please ensure you have Java {minRequiredJavaVersion} or above installed.");
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't Dispose() of the Process instance. You should wrap the new Process() in a using block to ensure we don't keep a process handle open longer than necessary.

@jonpryor
Copy link
Contributor

Additionally, in the case of Bug #51507, this PR is not relevant at all.

...probably because the commentary is wrong or otherwise non-sensical. :-/

The original issue in that bug is:

java.lang.UnsupportedClassVersionError: com/android/dx/command/Main : Unsupported major.minor version 52.0

This error isn't coming from javac, nor is it coming from java. Checking the java version number will not help in any way with this error.

This error is coming from the dx.jar invocation in the <CompileToDalvik/> task, because an older build-tools version is being used -- a version which doesn't support Java 1.8 bytecode (back in the days that the Jack & Jill toolchain seemed like a good idea).

I forget which build-tools version is required to support JDK 1.8 bytecode. :-(

Given that, it seems that it might be a reasonable idea (?) to associate a build-tools version alongside a JDK version for each API level, to ensure that we can reasonably handle the project.

@dellis1972 dellis1972 added the do-not-merge PR should not be merged. label Aug 1, 2017
@jonpryor
Copy link
Contributor

jonpryor commented Aug 2, 2017

Just so we don't lose track, the <ResolveSdks/> task needs to gain a JavaToolExe property so that java/java.exe isn't hardcoded.

SequencePointsMode="$(_AndroidSequencePointsMode)"
BuildingInsideVisualStudio="$(BuildingInsideVisualStudio)"
JavaSdkPath="$(JavaSdkDirectory)"
MonoAndroidBinPath="$(MonoAndroidBinDirectory)"
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the following line should not be present.

Version required = requiredJavaForFrameworkVersion > requiredJavaForBuildTools ? requiredJavaForFrameworkVersion : requiredJavaForBuildTools;

var proc = new Process ();
proc.StartInfo.FileName = Path.Combine (JavaSdkPath, "bin", OS.IsWindows ? "java.exe" : "java");
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should be using MonoAndroidHelper.RunProcess()? That should be a bit simpler.

{
Version buildTools;
if (!Version.TryParse (buildToolsVersion, out buildTools)) {
return new Version (1, 7);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why default to Java 1.7 if we can't parse buildToolsVersion? Shouldn't we default to LatestSupportedJavaVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is LatestSupportedJavaVersion defined? I can't see it anywhere?

…equired but not installed

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=51507

If a user has an older verison of Java installed they currently get
this kind of helpful error message.

	"Unsupported major.minor version 52.0"

While for the experienced user this might mean "Ah I need Java 1.8".
For the new users this is confusing.

This commit adds a new property in the Xamarin.Android.Common.props
called

	`$(MinimumRequiredJavaVersion)`

This will define the Minimum required version of Java we need.
It can be overridden on the command line or in the project is needed.

If this mimimum version is NOT met, we will error out. While it
might be nicers to issue a warning, this will eventually end up with
the above error anyway. So we might as well tell the user exactly what
is wrong.
@jonpryor jonpryor merged commit 099ab6a into dotnet:master Aug 8, 2017
jonpryor pushed a commit that referenced this pull request Aug 10, 2017
#698)

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=51507

If a user has an older verison of Java installed they currently get
this kind of helpful error message.

	Unsupported major.minor version 52.0

While for the experienced user this might mean "Ah I need Java 1.8".
For the new users this is confusing.

This commit adds a new property in the Xamarin.Android.Common.props
called

	$(MinimumRequiredJavaVersion)

This will define the Minimum required version of Java we need.
It can be overridden on the command line or in the project is needed.

If this mimimum version is NOT met, we will error out. While it
might be nicers to issue a warning, this will eventually end up with
the above error anyway. So we might as well tell the user exactly what
is wrong.
jonpryor pushed a commit that referenced this pull request Sep 2, 2020
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1139590
Context: dotnet/java-interop@afbc5b3

Changes: dotnet/java-interop@dcaf794...afbc5b3

  * dotnet/java-interop@afbc5b32: [C++] Get rid of compiler warnings (#703)
  * dotnet/java-interop@e295b498: [generator] Fix localization error. (#702)
  * dotnet/java-interop@5a0e37e0: [generator] Support 'managedOverride' metadata (#701)
  * dotnet/java-interop@f6c12ba3: [Xamarin.Android.Tools.Bytecode] Hide Kotlin synthetic constructors (#700)
  * dotnet/java-interop@0334f424: [generator] Hide Java.Lang.Object infrastructure (#698)
  * dotnet/java-interop@c0fcc435: [jcw-gen] Make MSBuild warning/error strings localizable. (#695)

Fixes a handful of warnings reported by GCC and clang when building
for different platforms.

`-Wshadow` warnings:

	monodroid-glue.cc:1609:8: warning: declaration shadows a static data member of 'xamarin::android::internal::MonodroidRuntime' [-Wshadow]
	void *api_dso_handle = nullptr;
	              ^
	monodroid-glue.cc:102:25: note: previous declaration is here
	void *MonodroidRuntime::api_dso_handle = nullptr;

Fix by renaming the local variable.  No functionality changes result
from this fix.

MinGW builds report:

	xa-internal-api.cc: In member function ‘virtual mono_bool xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_up_state(const char*, mono_bool*)’:
	xa-internal-api.cc:24:86: warning: unused parameter ‘ifname’ [-Wunused-parameter]
	   24 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_up_state (const char *ifname, mono_bool *is_up)
	      |                                                                          ~~~~~~~~~~~~^~~~~~
	xa-internal-api.cc:24:105: warning: unused parameter ‘is_up’ [-Wunused-parameter]
	   24 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_up_state (const char *ifname, mono_bool *is_up)
	      |                                                                                              ~~~~~~~~~~~^~~~~
	xa-internal-api.cc: In member function ‘virtual mono_bool xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_supports_multicast(const char*, mono_bool*)’:
	xa-internal-api.cc:34:96: warning: unused parameter ‘ifname’ [-Wunused-parameter]
	   34 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_supports_multicast (const char *ifname, mono_bool *supports_multicast)
	      |                                                                                    ~~~~~~~~~~~~^~~~~~
	xa-internal-api.cc:34:115: warning: unused parameter ‘supports_multicast’ [-Wunused-parameter]
	   34 | MonoAndroidInternalCalls_Impl::monodroid_get_network_interface_supports_multicast (const char *ifname, mono_bool *supports_multicast)
	      |                                                                                                        ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
	xa-internal-api.cc: In member function ‘virtual int xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_get_dns_servers(void**)’:
	xa-internal-api.cc:44:66: warning: unused parameter ‘dns_servers_array’ [-Wunused-parameter]
	   44 | MonoAndroidInternalCalls_Impl::monodroid_get_dns_servers (void **dns_servers_array)
	      |                                                           ~~~~~~~^~~~~~~~~~~~~~~~~
	xa-internal-api.cc: In member function ‘virtual int xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_getifaddrs(_monodroid_ifaddrs**)’:
	xa-internal-api.cc:54:82: warning: unused parameter ‘ifap’ [-Wunused-parameter]
	   54 | MonoAndroidInternalCalls_Impl::monodroid_getifaddrs (struct _monodroid_ifaddrs **ifap)
	      |                                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
	xa-internal-api.cc: In member function ‘virtual void xamarin::android::internal::MonoAndroidInternalCalls_Impl::monodroid_freeifaddrs(_monodroid_ifaddrs*)’:
	xa-internal-api.cc:64:82: warning: unused parameter ‘ifa’ [-Wunused-parameter]
	   64 | MonoAndroidInternalCalls_Impl::monodroid_freeifaddrs (struct _monodroid_ifaddrs *ifa)
	      |                                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

Fix by ignoring the unused parameters only during Windows builds
by using the C++17 attribute `[[maybe_unused]]`

MinGW also reports:

	embedded-assemblies.cc: In static member function ‘static ssize_t xamarin::android::internal::EmbeddedAssemblies::do_read(int, void*, size_t)’:
	embedded-assemblies.cc:663:26: warning: conversion from ‘size_t’ {aka ‘long long unsigned int’} to ‘unsigned int’ may change value [-Wconversion]
	  663 |   ret = ::read (fd, buf, count);
	      |

It is caused by **read**(2) declared in MinGW headers with the third
parameter using an `unsigned int` type instead of the standard
`size_t`.  Fixed by casting properly on Windows builds.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

do-not-merge PR should not be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants