Skip to content

Conversation

@adinn
Copy link
Collaborator

@adinn adinn commented Jul 15, 2020

Fix for issue #2679

@adinn adinn force-pushed the debugjdkpathfix branch from 48897d7 to d360f25 Compare July 15, 2020 14:55
@adinn adinn requested a review from olpaw July 15, 2020 14:55
Copy link
Collaborator

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

FWIW it LGTM

@adinn adinn force-pushed the debugjdkpathfix branch from 0afe859 to ede8e3e Compare July 16, 2020 13:42

@Override
public Path checkCacheFile(Path filePath) {
if (!isJDK8()) {
Copy link
Member

Choose a reason for hiding this comment

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

Use JavaVersionUtil.JAVA_SPEC >= 11 (like elsewhere)


@Override
public Path tryCacheFile(Path filePath) {
if (!isJDK8()) {
Copy link
Member

Choose a reason for hiding this comment

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

*/
private static String javaSpecVersion = System.getProperty(JAVA_SPEC_VERSION_PROP);

public static boolean isJDK8() {
Copy link
Member

Choose a reason for hiding this comment

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

Remove

* A whitelist of packages prefixes used to pre-filter JDK runtime class lookups.
* An accept list of packages prefixes used to pre-filter JDK11+ runtime class lookups.
*/
public static final String[] JDK_SRC_PACKAGE_PREFIXES = {
Copy link
Member

@olpaw olpaw Jul 17, 2020

Choose a reason for hiding this comment

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

JDK_SRC_PACKAGE_PREFIXES = Stream.concat(Stream.of(JDK8_SRC_PACKAGE_PREFIXES), Stream.of("org.graalvm.compiler")).toArray(String[]::new);

Copy link
Member

@olpaw olpaw left a comment

Choose a reason for hiding this comment

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

Looks good. Please fix the little things. I should be able to get to merging early next week.
Thanks!

@adinn
Copy link
Collaborator Author

adinn commented Jul 17, 2020

Hi Paul,

Thanks for the review. I have made all changes as requested.

@olpaw
Copy link
Member

olpaw commented Jul 20, 2020

@adinn your PR is in the merge queue.
To save time I fixed up your commits to start with uppercase letters.
see #2396 (comment)
For future PRs please also try to pay attention to those little things.

@olpaw
Copy link
Member

olpaw commented Jul 21, 2020

Changes are on master. e17b00a
(I rebased the commits thus github does not seem to be able to detect that the changes are on master.)

@olpaw olpaw closed this Jul 21, 2020
@adinn
Copy link
Collaborator Author

adinn commented Jul 21, 2020

@olpaw Thanks for your help!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants