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.

Sign up for GitHub

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

Jump to bottom

CI: Test on all current Java LTS versions (8, 11 ,17, and 21) #4938

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CI: Test on all current Java LTS versions (8, 11 ,17, and 21) #4938

wants to merge 1 commit into from

Conversation

allentiak
Copy link
Member

@allentiak allentiak commented Apr 1, 2024 •
edited
Loading

Tests on JDK 17 and 21 are optional (since they currently fail).

The very existence of these tests will increase the awareness on the importance of code compatibility with newer Java versions, thus enabling future transitions to be implemented gradually.

Making the code run on JDK 11 took a lot of hacking. This will reduce the effort for eventual future transitions.

@allentiak
Test on all current Java LTS versions (8, 11 ,17, and 21)
f970524 
Tests on JDK 17 and 21 are optional (since they currently fail).

The very existence of these testes will increase the awareness on the
importance of code compatibility with newer Java versions, thus enabling
future transitions to be implemented gradually.

Making the code run on JDK 11 took a lot of hacking.
This will reduce the effort for eventual future transitions.
@tool4ever
Copy link
Contributor

Looks like it's TestNG causing problems?
Would they even offer a version that can support 4 JDK simultaneously... 🤔
Otherwise I think seeing all PR fail (partly) is too noisy and confusing for contributors.

@allentiak
Copy link
Member Author

allentiak commented Apr 2, 2024 •
edited
Loading

Looks like it's TestNG causing problems? Would they even offer a version that can support 4 JDK simultaneously... 🤔 Otherwise I think seeing all PR fail (partly) is too noisy and confusing for contributors.

Well, it seems this PR is already achieving its aim of raising awareness on things to improve...
What about accepting this pain as an incentive to

  1. deprecate TestNG?, and
  2. move existing TestNG tests to JUnit 5?

Like this PR, for example: #4942

@allentiak allentiak changed the title Test on all current Java LTS versions (8, 11 ,17, and 21) CI: Test on all current Java LTS versions (8, 11 ,17, and 21) Apr 7, 2024
@tehdiplomat
Copy link
Contributor

Don't you need to run your test with add-opens?

forge/forge-gui-desktop/src/main/config/forge.sh

Lines 44 to 52 in 152ad0b

if [[ $v -ge 17 ]]
then
java --add-opens java.desktop/java.beans=ALL-UNNAMED --add-opens java.desktop/java.awt.color=ALL-UNNAMED --add-opens java.desktop/javax.swing.border=ALL-UNNAMED --add-opens java.desktop/javax.swing.event=ALL-UNNAMED --add-opens java.desktop/sun.awt.image=ALL-UNNAMED --add-opens java.desktop/sun.swing=ALL-UNNAMED --add-opens java.desktop/javax.swing=ALL-UNNAMED --add-opens java.desktop/java.awt=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.lang.reflect=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.desktop/java.awt.font=ALL-UNNAMED --add-opens java.desktop/java.awt.image=ALL-UNNAMED --add-opens java.base/jdk.internal.misc=ALL-UNNAMED --add-opens java.base/sun.nio.ch=ALL-UNNAMED --add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED -Dio.netty.tryReflectionSetAccessible=true $SHAREDPARAMS
elif [[ $v -ge 11 ]]
then
java --illegal-access=permit $SHAREDPARAMS
else
java $SHAREDPARAMS
fi

@allentiak
Copy link
Member Author

allentiak commented Apr 13, 2024 •
edited
Loading

Don't you need to run your test with add-opens?

Wouldn't this hide the fact that the current code does not compile with JDK 17+?

I think that it is important to be explicitly aware of this...
(aka "Bring the pain forward")
https://medium.com/continuousdelivery/if-it-hurts-do-it-more-often-f5a00cc12ffa

If we are not even aware of this problem, then no one will see an incentive to fix it...

As I said above, I think that just this awareness in itself makes merging this worth it...

@Hanmac
Copy link
Contributor

Hanmac commented Apr 13, 2024

Wouldn't this hide the fact that the current code does not compile with JDK 17+?

it would. if using the right ADD-OPENS parameters

@github-actions GitHub Actions
Copy link

This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed

@Hanmac Hanmac added keep no stale and removed no-pr-activity labels May 29, 2024
@allentiak
Copy link
Member Author

Well, being explicit about that the code won't compile unless that hack is used is part of the "awareness raising" rationale I mentioned when I first opened this PR...

@allentiak
Copy link
Member Author

allentiak commented Jun 9, 2024 •
edited
Loading

I mean, the first step to solve a problem is to accept its existence, right?
https://jhall.io/archive/2022/06/11/if-it-hurts-do-it-more/

@allentiak allentiak mentioned this pull request Jun 9, 2024
Closed
@Hanmac
Copy link
Contributor

Hanmac commented Jun 10, 2024

I mean, the first step to solve a problem is to accept its existence, right? https://jhall.io/archive/2022/06/11/if-it-hurts-do-it-more/

The "pain" you are referring too is JAVA 17+ messing up our Code we depend on it, and can't really update

@tehdiplomat
Copy link
Contributor

Here's my suggestion for this PR. Instead of having these failing (optional) tests run every single PR which is a lot of noise and Github Actions CPU cycles. And potentially very noisy for new people who don't understand that this is expected behavior, why don't we associate this change only during release builds that way we can keep it in mind if there's a future way to avoid add-opens, but its not just noise for the sake of noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
keep no stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@allentiak @tool4ever @tehdiplomat @Hanmac