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

[JENKIKNS-54355] Dynamically load java.sql classes #55

Merged
merged 4 commits into from
Feb 1, 2019

Conversation

alecharp
Copy link
Member

@alecharp alecharp commented Jan 9, 2019

JENKINS-54355

Tag.java has references to classes which are not longer available in Java 11, without a module addition. This could cause problem when running this plugin on a Jenkins with Java 11.

@jenkinsci/java11-support

} catch (ClassNotFoundException e) {
LOG.log(Level.INFO, "java.sql.Date and / or java.sql.Timestamp are not available. " +
"You are probably on Java 11, if so, it's ok.", e);
}
Copy link
Member

Choose a reason for hiding this comment

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

Upside: this looks like you got the right fix :-) https://bitbucket.org/asomov/snakeyaml/src/d318aef91b9df07b369eba6c553a8457b05e0726/src/main/java/org/yaml/snakeyaml/nodes/Tag.java?at=default&fileviewer=file-view-default#Tag.java-64:70

Downside: I actually think the right fix would be to upgrade snakeyaml. But seems like this is not yet released. So this looks fine.

I commented in https://bitbucket.org/asomov/snakeyaml/issues/422/provide-a-stripped-down-library-for-just to ask for a release.

I think in the meantime this fix looks correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Upgrading snakeyaml should be done in a dedicated PR imo, even if it was released with the fix.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but then upgrading snakeyaml would be the fix. But as it's not released, that's fine.

Copy link
Member

Choose a reason for hiding this comment

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

Also, got feedback from the snakeyaml maintainer in the issue linked above: release with this fix is reflected in February.

Jenkinsfile Outdated
[ platform: "linux", jdk: "8", jenkins: null ],
[ platform: "windows", jdk: "8", jenkins: null ],
[ platform: "linux", jdk: "8", jenkins: "2.150.1", javaLevel: "8" ],
[ platform: "windows", jdk: "8", jenkins: "2.150.1", javaLevel: "8" ],
Copy link
Member

Choose a reason for hiding this comment

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

❓ The addition below for JDK11 does make sense.

But the two lines above adding jenkins.version=2.150.1 seem unrelated. I mean, why not, but it looks slightly out of scope here.

Copy link
Member Author

Choose a reason for hiding this comment

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

anyway, I think this is not working as the build only ran on linux/windows 8, nothing in 11.

Copy link
Member

Choose a reason for hiding this comment

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

I've changed the Jenkinsfile after the merge so that it only runs the jdk 11 configurations if anyone of you guys are the PR author.
Too many test failures :)

@batmat
Copy link
Member

batmat commented Jan 28, 2019

Given since this was filed, we understood a bit more and didn't actually see this surface in a normal Jenkins environment (i.e. not in test mode, PCT, JenkinsfileRunner, etc.), I'm inclined to now say we should close it (until at least we possibly see this surface actually in normal running conditions).

@alecharp
Copy link
Member Author

I have no hard feelings about that.

@rsandell rsandell merged commit fd96bed into jenkinsci:master Feb 1, 2019
rsandell added a commit that referenced this pull request Feb 1, 2019
[JENKIKNS-54355] Dynamically load java.sql classes
@alecharp alecharp deleted the JENKINS-54355 branch February 5, 2019 10:14
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