main.yml: removed java-8 runner image #1916
Conversation
|
I converted this to draft until it is completed. |
| - jdk: 11 | ||
| deploy: false |
There was a problem hiding this comment.
Please revert this, and instead remove the - jdk: 17.
Also, please update this comment on line 83: # Build the engine, we only deploy from ubuntu-latest jdk8 to refer jdk17 instead of jdk8.
There was a problem hiding this comment.
By the way, what will happen if we just set deploy: true on jdk 17 instead of removing it from "include". Will it deploy with all OS?
The include config seems a bit confusing to me!
There was a problem hiding this comment.
The include just adds additional flags for each runner image.
There was a problem hiding this comment.
I am going to explicitly evaluate the jdk11 to deploy: false and jdk17 to deploy: true and update the comments, I think it will be much better if we specify flags explicitly, do we agree on these changes ?
There was a problem hiding this comment.
and jdk17 to deploy: true
I wonder if it will also deploy with mac and windows as well if you set it to true? I need to take a look at the docs to see how include works.
There was a problem hiding this comment.
And I will also create deploy: false explicitly for other systems (Mac and windows) to avoid confusion.
[mac:deploy=false, jdk17: deploy=true]
[windows:deploy=false, jdk17: deploy=true]
May the deploy defined for jdk overwrite the one in os?
There was a problem hiding this comment.
On the current master, this flag sets all the Ubuntu images (knowing that ubuntu images = 3, jdk-8/jdk-11/jdk-17) to true deploy, so all jdks should deploy unless one of them overrides the flag:
jmonkeyengine/.github/workflows/main.yml
Line 96 in 5664d59
While deploy: false overrides the true on particularly jdk11 and jdk17 Ubuntu images.
There was a problem hiding this comment.
Long story short, we shouldn't set deploy: true on a jdk runner because it will override the previously defined flag.
There was a problem hiding this comment.
I see.
So I believe we should keep this
- jdk: 11
deploy: false
and remove this
- jdk: 17
deploy: false
to make it deploy with java 17.
| osName: windows | ||
| - os: macOS-latest | ||
| osName: mac |
There was a problem hiding this comment.
I think we need to also explicitly specify deploy: false on windows and mac os.
|
Finished this in #1922 |
FYI, closing others PR without their knowledge may lead to a bad impact on the engine contributors on the long run (one will think why on earth would i open a PR when someone else will do and everything will start to get teared down in the knowledge of others will do all the needed tasks, but this is not the fact), for me, it won't, but it may perceived from others as disrespect, and/or they may have another benefit thoughts, ideas or tasks to add on a WIP PR that could improve the engine productivity..... I don't think that this applies to this PR anyway, since we are just improving things related to a secondary build script, but anyway you got my point. With your permission, i am going to review your merged PR again to give out loud my suggestions, if they are not good, no problem at all, i will keep the ideas for my projects. |
|
@Scrappers-glitch, I'm sorry about this! I agree with you that I must have asked before I wanted to close this and submit another PR, my fault. Sorry again!
Sure, please let me know if I need to improve #1922. |
This PR attempts to remove the java-8 runner images since we are using gradle-7.6 with a release option 8:
For more refer to the gradle docs on building jvm applications:
https://docs.gradle.org/current/userguide/building_java_projects.html#sec:java_cross_compilation