Skip to content
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

Use the correct runtime to validate the JVM versions between the debugger and debuggee #353

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

testforstephen
Copy link
Contributor

When attaching to an existing debuggee program, its runtime version may be different with the one that the debugger used to build your project. Although the debugger still works when the JDK mismatches. But stepping into the JDK system library, the native library source line may be not consistent with the remote one and cause the line mismatch during stepping. For such case, the debugger will print a warning message when runtime is not matched in the DEBUG CONSOLE view.

@@ -181,6 +187,37 @@ public String getSourceFileURI(String fullyQualifiedName, String sourcePath) {
return null;
}

private static final Pattern JAVA_VERSION_PATTERN = Pattern.compile("version \"(.*)\"");
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI that the pattern could not cover some corner case (though we haven't figure out why), see adoptium/temurin-build#1610

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info. I plan to reuse the JDT logic to get the Java version, where it uses release file under the Java home to get the version. I assume it's more stable regarding the jdt user base.

	public synchronized String readReleaseVersion(File javaHome) {

		String version = ""; //$NON-NLS-1$

		if (Files.notExists(Paths.get(javaHome.getAbsolutePath(), RELEASE_FILE))) {
			return version;
		}
		try (Stream<String> lines = Files.lines(Paths.get(javaHome.getAbsolutePath(), RELEASE_FILE), Charset.defaultCharset()).filter(s -> s.contains(JAVA_VERSION))) {
			Optional<String> hasVersion = lines.findFirst();
			if (hasVersion.isPresent()) {
				String line = hasVersion.get();
				version = line.substring(14, line.length() - 1); // length of JAVA_VERSION + 2 in JAVA_VERSION="9"
			}
		}
		catch (UncheckedIOException | IOException e) {
			LaunchingPlugin.log(e);
		}

		return version;

	}

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@testforstephen release file looks great! I tried several, one distribution of AdoptOpenJDK uses major version while others use full version. But it's the same value with java -version output.

Zulu:

IMPLEMENTOR="Azul Systems, Inc."
IMPLEMENTOR_VERSION="Zulu14.28+21-CA"
JAVA_VERSION="14.0.1"

Adopt:

IMPLEMENTOR="AdoptOpenJDK"
IMPLEMENTOR_VERSION="AdoptOpenJDK"
JAVA_VERSION="14"    // <-------------HERE
IMPLEMENTOR="AdoptOpenJDK"
IMPLEMENTOR_VERSION="AdoptOpenJDK"
JAVA_VERSION="13.0.2"

Oracle:

BUILD_TYPE="commercial"
IMPLEMENTOR="Oracle Corporation"
JAVA_VERSION="12.0.2"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YES. In real world, maybe 99% cases will contain release file. You could find the Java version from the release file first. If it doesn't exist, then fall back to java -version command.

Eskibear
Eskibear previously approved these changes Sep 24, 2020
@testforstephen testforstephen merged commit 24a7260 into microsoft:master Sep 24, 2020
@testforstephen testforstephen deleted the jinbo_runtime branch September 24, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants