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

Configure Java 8 as the minimum required version #741

Merged
merged 10 commits into from Sep 3, 2023
Merged

Configure Java 8 as the minimum required version #741

merged 10 commits into from Sep 3, 2023

Conversation

javadev
Copy link
Contributor

@javadev javadev commented May 29, 2023

No description provided.

@javadev
Copy link
Contributor Author

javadev commented May 29, 2023

image

@javadev javadev changed the title Setup java 11 as minimum version Configure Java 11 as the minimum required version May 29, 2023
@michael-o
Copy link
Contributor

michael-o commented Jun 7, 2023

This is really inapropriate for a low-level library. There is zero benefit from a user's perspective. Java 8 is more than enough.

@javadev
Copy link
Contributor Author

javadev commented Jun 7, 2023

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.

@michael-o
Copy link
Contributor

michael-o commented Jun 7, 2023

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.

@javadev
Copy link
Contributor Author

javadev commented Jun 7, 2023

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.

@michael-o
Copy link
Contributor

michael-o commented Jun 7, 2023

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...

@javadev
Copy link
Contributor Author

javadev commented Jun 7, 2023

We can choose to wait for years and continue to maintain Java 1.6 as the minimum required JDK version.

@michael-o
Copy link
Contributor

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.

@javadev
Copy link
Contributor Author

javadev commented Jun 7, 2023

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.

@stleary
Copy link
Owner

stleary commented Jun 7, 2023

I think it's reasonable to require Java 8 if there is a valid PR that uses it.
For example, #737 would be a good candidate to add an API method that works with Streams, which is only available from Java 8.
Once this is ready I can do a final v6 release, followed by a v8 build.
Will let the v8 build sit for a month or so in case there are concerns.
I don't think v11 is advisable since some projects, including my current and previous employers, are still stuck on v8.
I think Android is no longer an issue due to its good-enough support for v8.

@bowbahdoe bowbahdoe mentioned this pull request Aug 23, 2023
@johnjaylward
Copy link
Contributor

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.

@@ -11,19 +11,21 @@ on:

jobs:
# old-school build and jar method. No tests run or compiled.
build-1_6:
build-11:
Copy link
Contributor

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

@@ -42,14 +44,16 @@ jobs:
strategy:
matrix:
# build against supported Java LTS versions:
java: [ 8, 11 ]
java: [ 11, 17 ]
Copy link
Contributor

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>
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

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.

@javadev javadev changed the title Configure Java 11 as the minimum required version Configure Java 8 as the minimum required version Aug 27, 2023
@@ -11,19 +11,21 @@ on:

jobs:
# old-school build and jar method. No tests run or compiled.
build-1_6:
build-8:
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@johnjaylward johnjaylward left a 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.

@johnjaylward
Copy link
Contributor

Why are you pushing more commits after approval? You are making me feel like I'm wasting my time.

@javadev
Copy link
Contributor Author

javadev commented Aug 28, 2023

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.

@javadev
Copy link
Contributor Author

javadev commented Aug 28, 2023

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.

@johnjaylward
Copy link
Contributor

I got an email that looked like this:

@javadev requested your review on: #741 Configure Java 8 as the minimum required version.

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.

@stleary
Copy link
Owner

stleary commented Aug 29, 2023

The maven build works fine with Java 8, but this error with gradlew. Did something get missed?
image

@javadev
Copy link
Contributor Author

javadev commented Aug 29, 2023

The maven build works fine with Java 8, but this error with gradlew. Did something get missed? image

I have prepared a solution.

@stleary
Copy link
Owner

stleary commented Aug 29, 2023

What problem does this code solve?
Update builds to support Java 8 and above.
Java 6 and 7 users will need to stay with v20230618.

Risks
Moderate

Changes to the API?
No

Will this require a new release?
Yes

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
No

Review status
APPROVED
Starting 3-day comment window.
This change will be included in the next release, which will also need to include a useful feature that requires Java 8.

build.gradle Outdated
Copy link
Contributor

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

@javadev
Copy link
Contributor Author

javadev commented Sep 1, 2023

johnjaylward reviewed 3 days ago

@stleary
Copy link
Owner

stleary commented Sep 3, 2023

Will merge when we have a feature that requires Java 8, e.g. #737

@johnjaylward
Copy link
Contributor

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.

@stleary
Copy link
Owner

stleary commented Sep 3, 2023

Good point @johnjaylward

@stleary stleary merged commit c29d488 into stleary:master Sep 3, 2023
5 checks passed
nathan454 pushed a commit to Rookout/JSON-java that referenced this pull request Oct 25, 2023
This reverts commit c29d488, reversing
changes made to 7c1b653.
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.

None yet

4 participants