-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Configure Java 8 as the minimum required version #741
Conversation
This is really inapropriate for a low-level library. There is zero benefit from a user's perspective. Java 8 is more than enough. |
I understand your perspective, but I respectfully disagree. While Java 8 may be sufficient for some use cases, it is important to consider security implications when choosing a minimum secure version for a low-level library. Java 11 introduced several security enhancements and bug fixes compared to Java 8. It includes updates to the Java Security Architecture, improvements to the TLS protocol, and the introduction of the HTTP/2 protocol, among other security-related features. By requiring a minimum secure version like Java 11, developers ensure that their library takes advantage of the latest security improvements and patches. This can help prevent potential vulnerabilities and protect users' systems from security threats. Furthermore, as time progresses, older Java versions may become more susceptible to security risks as new vulnerabilities are discovered. Requiring a more recent version ensures that the library remains compatible with modern security standards. While there might be an initial cost associated with upgrading to a newer version, the long-term benefits of improved security outweigh the inconvenience. It's essential to prioritize security in software development, especially when developing low-level libraries that could be utilized in various contexts. Ultimately, the decision of the minimum required version depends on the specific needs and considerations of the library and its users. However, from a security standpoint, Java 11 or a newer version would be a more appropriate choice than Java 8 for a low-level library. |
What? How does all this matter for a library which parses JSON? TLS? HTTP/2? Totally unrelated. OpenJDK 8 receives all necessary security fixes until 2030. At the end, as a library developer I should not impose something which isn't required from a technical PoV. I don't understand the justification here. The users sill can run a newer Java version, if necessary for their need, but bytecode is Java 8 here. |
Twistlock, a security analysis platform, has flagged Java 8 as insecure despite its long-term support and security fixes until 2030. While Java 8 may still receive critical security updates, it's worth considering the recommendations provided by Twistlock to ensure optimal security for your low-level library. Ultimately, as a library developer, it's important to weigh the technical requirements, user needs, and security concerns when determining the minimum required Java version. |
...and you believe everything you are told by commercial, closed source entities? Department of health says also that smoking is lethal, doesn't stop many. I just can't believe this... |
We can choose to wait for years and continue to maintain Java 1.6 as the minimum required JDK version. |
It'd be advisable to release a last 1.6 version with last fixes and move to Java 8 to have a maintained stack with a new (at least) minor version. Here, I doubt that people will object Java 8. |
This pull request was specifically created for Java 11, indicating that it targets the features and functionality available in that version. Whether the pull request will be closed, merged, or remain open depends on the project maintainers and their evaluation of its compatibility and relevance. While it's possible that some individuals may prefer Java 8, the decision ultimately lies with the project maintainers and their consideration of the pull request's alignment with the project's goals and requirements. |
I think it's reasonable to require Java 8 if there is a valid PR that uses it. |
I really don't see a need to drop Java8 at this time. The library is not today using any features above Java 1.6, and form a Bytecode perspective, there isn't a good reason to limit to Java11+. As stated elsewhere in this thread and other issues, Java8 will be around for a long time. Just because there are valid Application use-cases for using Java11+, this application really does not have a use case for forcing that JVM restriction on downstream developers at this time. I would not recommend merging this unless the v8 build is put back in with the source and target outputs being v8. Also, the pipeline.yml can get cleaned up quite a bit. There is no reason to keep the section that was used for the old 1.6 code. Simply using the section dedicated to JVM 8+ would be advisable. I would also not build/test against every JVM version out there. Sticking with the LTS versions (8,11,17) only is a valid approach for libraries. |
.github/workflows/pipeline.yml
Outdated
@@ -11,19 +11,21 @@ on: | |||
|
|||
jobs: | |||
# old-school build and jar method. No tests run or compiled. | |||
build-1_6: | |||
build-11: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole pipeline file needs a lot of cleanup. The entire old "build-1_6" (original lines 14 through 39) section should just be deleted.
Keep just the "Build" section
.github/workflows/pipeline.yml
Outdated
@@ -42,14 +44,16 @@ jobs: | |||
strategy: | |||
matrix: | |||
# build against supported Java LTS versions: | |||
java: [ 8, 11 ] | |||
java: [ 11, 17 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add 8 back in here.
pom.xml
Outdated
<configuration> | ||
<source>1.6</source> | ||
<target>1.6</target> | ||
<source>11</source> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source and target should both be set back to 1.8.
The changes from #761 will also need to be incorporated once merged to master
@@ -620,10 +620,6 @@ public void jsonObjectByBean1() { | |||
assertTrue("expected h\be\tllo w\u1234orld!", "h\be\tllo w\u1234orld!".equals(jsonObject.query("/escapeStringKey"))); | |||
assertTrue("expected 42", Integer.valueOf("42").equals(jsonObject.query("/intKey"))); | |||
assertTrue("expected -23.45e7", Double.valueOf("-23.45e7").equals(jsonObject.query("/doubleKey"))); | |||
// sorry, mockito artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we aren't going to validate the Mockito artifacts here, then I see no reason to use Mockito for this test case at all. It adds no benefit if we remove the assertions against it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand these checks don't work against the new version of Mockito which is required for Java 17+, If you can, please try to find a solution that will do similar validations.
.github/workflows/pipeline.yml
Outdated
@@ -11,19 +11,21 @@ on: | |||
|
|||
jobs: | |||
# old-school build and jar method. No tests run or compiled. | |||
build-1_6: | |||
build-8: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole build-8
section still needs to be deleted. All it does is waste time in the pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lines 13-41 should be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this set of changes as long as @stleary agrees with the direction.
Why are you pushing more commits after approval? You are making me feel like I'm wasting my time. |
I noticed that the master branch isn't currently up to date. Therefore, I took the initiative to rebase the branch with the most current version. |
I apologize if my actions have caused any inconvenience. The reason for pushing additional commits after approval is to ensure the code's integrity and address any potential issues that may have been missed earlier. Please know that your time and contributions are valued, and I'm committed to delivering the best quality work. If you prefer a different approach or have specific concerns, please feel free to share them, and I'll do my best to accommodate your preferences. |
I got an email that looked like this:
So I re-reviewed and approved, then you pushed again... then I got another email... If it's automated by GitHub, then there isn't much you could have done about it I suppose, but if there was a button you hit to re-request review, I'd ask that you refrain from hitting the button before finalizing your changes. The other option would be to place the PR in draft mode, so that reviewers know you are still working out changes. |
What problem does this code solve? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status |
build.gradle
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worthwhile to change the sourceCompatability line from 1.7
to 1.8
johnjaylward reviewed 3 days ago |
Will merge when we have a feature that requires Java 8, e.g. #737 |
I think that's putting the horse (v8 support) after the cart (v8 features). Merging this will allow version 8 features to be implemented. I think this should be merged before features. Also, if this waits too long it will become stale and a pain to maintain if there are conflicting changes. |
Good point @johnjaylward |
No description provided.