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

Issue/27 #28

Merged
merged 2 commits into from
Dec 14, 2018
Merged

Issue/27 #28

merged 2 commits into from
Dec 14, 2018

Conversation

MarkusTiede
Copy link
Member

@MarkusTiede MarkusTiede commented Dec 14, 2018

Fixes #27

Copy link
Collaborator

@bsolar17 bsolar17 left a comment

Choose a reason for hiding this comment

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

Rationale to set useSystemClassLoader to false is not clear: I understand it fixes the ForkedBooter class loading failure, but why? I tried to follow the linked CircleCI thread but didn't find this information quickly enough.

@MarkusTiede
Copy link
Member Author

MarkusTiede commented Dec 14, 2018

I'm not sure whether digging into the why of classloading in maven / surefire is really necessary.

This PR already shows that it's effective:

image

As far as I understand it's valid (widely) used workaround for this issue. What's a / the solution you'd prefere @bsolar17 ? In my opinion the other thing would be

  1. no longer build (until resolved) on circle CI
  2. disable surefire test execution in that environment
  3. downgrade the openjdk base container used for building at circle CI

Some details are also available on a corresponding debian bug report.

@bsolar17
Copy link
Collaborator

Ok, reading the Debian issue, if I understand correctly their Java8 package has a bug which makes JAR checking fail and they have backported a fix from Java11 to correct it. This backport is relatively recent and I guess the new version of the Java8 package is not being used yet in CircleCI, leading to the need for a workaround which is the useSystemClassLoader=false setting.

This is the kind of information I'd like to have together with a change: not only whether it's effective but also why it's needed in the first place (and why it might not be needed anymore in the future, e.g. when the fixed Java8 package is deployed).

@bsolar17 bsolar17 merged commit 3d3d128 into master Dec 14, 2018
@MarkusTiede MarkusTiede deleted the issue/27 branch December 14, 2018 16:10
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.

2 participants