-
Notifications
You must be signed in to change notification settings - Fork 108
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
Version bumps to fix build and resolve CVE-2021-37136 and CVE-2021-37137 #578
Conversation
@davidwheeler123 I was going to make this pull request when I saw you had done it already. The CI logs are no longer available. Have you any idea why CI was failing? |
It looked completely unrelated to the change, that's all I remember |
I get the impression this project has become inactive 😟 |
@davidwheeler123 Please push a new (or empty) commit to restart actions. |
- Java target to v11 - Kotlin to 1.9.10 (latest supported by Gradle) - Gradle to 8.4 (latest) - Netty to 4.1.100.Final (latest - to resolve CVEs) - asciidoctorJ to 2.5.10 (latest) - asciiDoctorPlugin to 3.3.2 (latest - for JDK11 support)
@kdubb Any direction on how to resolve this? |
@davidwheeler123 Glad to see you got this working! I had it open and was starting to look at it when I got pulled away |
@kdubb Not sure it's working yet - I just (force) pushed some changes that reduced the number of failures on my local - it seemed to fix all the SSL test failures, but I'm still getting 3 failures |
Oooh looks like it worked on Github! @kdubb I've disabled the Java 8 builds because there was some dependency not available in java 8 class format, but github still seems to be waiting for them. Is there some github setting you need to update to correspond? Would you like me to enable another java version also? |
I think the Java 8 thing was what I was seeing. Can you post an error message from the Java 8 build? |
You can see it here: https://github.com/impossibl/pgjdbc-ng/actions/runs/6758368037/job/18369978501
|
Yeah that looks familiar... Not sure how dependencies are disappearing from Central but it's a plugin for documentation generation (which shouldn't care about what Java version is used to build it). We could increase the Java version to 11 for that module to get it working again. |
I'm all for dropping Version 8. I had planned to move the minimum to 11 quite a long time ago but it should be done in a planned way and a major release. |
That's a good idea. We still wouldn't be able to run that build in a java 8 JDK though, even if we produce java 8 compatible results |
Nevermind that wouldn't work the actual java-version is set in the action workflow. I thought it was being selected via Gradle toolchains. |
You could try using toolchains in the Gradle build to select the JDK (it should download the required JDK if setup correctly). This might slow down the build a bit but that can be mititagated by using the |
Then change the workflow to always use Java 17 or something. |
I think it's using both actually. I've just set target vers back to 1.8 and it seems to be working on my local (building with jdk 17) |
Ahh I missed your change to the minimum Java version. Yeah that needs to be Java 11 until we really remove Java 8. |
I'm gonna sign off for a while, I think this is ready for a review. LMK what you think |
Did you add Java 8 back to the build matrix? |
Nope. Do you think I should? It's still producing Java 8 compatible output, so I don't see why we need to build with Java 8 |
@kdubb i have some time today. Anything else you'd like me to do on this pr? |
@davidwheeler123 Sorry, I thought I responded to this 🤦♂️ It may be a bit pedantic but I think we should stick to building with Java 8 until we update to a newer LTS release (at least 11, if not 17). |
I can't work out how to do it. The issue at hand is that |
I suppose one way to do it would be to have separate github jobs to publish the maven artifacts vs the documentation. Does that sound ok to you? |
@kdubb Build is now passing, but it's not super tidy because I actually have to exclude the |
BTW I tried running the tests against postgres 14-16, but the tests are failing. I'll tackle that as a separate PR once this one gets merged |
you should be able to build on java 17 and release on java 8 if that helps |
Yeah I know, that's how I had it but @kdubb preferred to build with Java 8. Any thoughts on how it looks now @kdubb ? |
@kdubb , javac 8 has known issues and it might easily produce ill bytecode. I would suggest using Java 17 as Gradle toolchain and use Here's a sample: pgjdbc/pgjdbc#3026 Of course, it might require slightly more changes, however, I am sure the best approach is to use Java 17 toolchain for build (both running Gradle and use javac 17), and then target whatever you need (e.g. 8). |
@davidwheeler123 My apologies for my absence. I'm fine with checking this in as is but now that @davecramer and @vlsi have weighed in it seems your original idea might be the correct course. It's up to you at this point. Let me know what you want to do.. revert to your original plan or check it in as is. |
I'll revert back to how it was I think, it's a bit simpler to maintain |
@kdubb I've reverted to how it was before: building with JDK 11 with target set to java 8. I can see the resulting jar has produced files compatible with java 8
Happy with that? Apologies for taking so long with this, it's been a busy few months. |
Oh I think there must be some github settings to say wait for the JDK 8 builds to pass before allowing merge. Are you able to correct that? |
@kdubb Just checking you saw that this PR is ready. |
@davidwheeler123 I removed the 8 build requirements so this is passing now. |
@davidwheeler123 https://github.com/impossibl/pgjdbc-ng/actions/runs/9310946886 The test certificate that passed for this PR appears to be out of date now. |
Oh that's weird I thought I made 10yr certs. I'll have a look when I can |
Actually it can't load it but the error is
|
Ahh, I reckon we might need to regenerate the certs with rsa:2048 rather than rsa:1024 pgjdbc-ng/driver/src/test/resources/certdir/README Lines 24 to 38 in 1ef3e9c
https://stackoverflow.com/a/67754773 |
Can we bump the netty to 4.1.109.Final which is now the latest and what all my other 3rd-party dependencies are looking for? |
@crowmagnumb I don't see why not. |
Need a PR to clear up this cert issue first |
Bump some versions to get it to build and resolve CVEs CVE-2021-37136 and CVE-2021-37137
Tests still failing on my local, but passing on github