-
-
Notifications
You must be signed in to change notification settings - Fork 182
fix: add Java 25 as minimum JVM version when using compact source files #2143
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
Conversation
| {/for} | ||
| {#if dependencies.isEmpty()}// //DEPS <dependency1> <dependency2>{/if} | ||
| {#if compactSourceFiles} | ||
| //JAVA 25+ |
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 is not symmetric fix - what if user chose a different java version?
the logic here is that if you run init with Java 25+ wehter available or chosen we generate for that - if anything it should generate //JAVA but its not something we can do reliably.
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.
But isn't it true that it should be at least Java 25? Otherwise it just won't work, right?
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.
Without //JAVA 25+ the generated code does not run on a system with a default JVM of version 24 or less.
If a JVM of version for example 26 is installed, then it will be used to run the script because it matches the JAVA specification of 25+.
Maybe not a perfect solution, but it is better than not having //JAVA 25+.
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.
It also works on java22 with preview flags.
I'm just saying we also don't add java 17 when deps require java 17... Should we ?
I'm not super against it but just saying until now we haven't explicitly added //JAVA unless explicitly user asking for 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'm happy to abide by what you decide.
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.
It also works on java22 with preview flags.
The compactSourceFiles is only true for Java 25+
I'm just saying we also don't add java 17 when deps require java 17... Should we ?
Do we have templates that use deps that require Java 17? Then I would say yes we should. But only if we have templates where we have explicitly added those deps. I'm not suggesting we should do any detection or anything. Only straightforward things like have templates or examples where we know the minimum version required.
until now we haven't explicitly added //JAVA
Until now we haven't used any special Java features in our templates, haven't we? In some of our examples we do, but AFAIK those have //JAVA lines.
|
No, but we also never added additional flags for your environment. we only do it when some explicit ask. Like I would expect if I do |
|
lgtm - though i'm not a fan adding lines which aren't needed by default ;) |
Yes, probably. Still, I'm not saying we should just go add this to templates, but I can imagine that the templates we provide do this, yes. |
|
The turtle.java example program specifies the Java version as
in PR jbangdev/jbang-examples#14. Is that not enough reason to accept the current PR as specified? |
|
the examples are different in the sense they are pretty static in point in time and can put it when its needed. I would have it much better with this PR if it added the requested java version if/when available so it would not be tied on java 25. which reminds me another reason why i'm hesitant to add this - the whole point of the naked mains is that it requires less code - and we then adding the /// root jbang line + //JAVA 25+ feels like unnecessary noise for newbies to me. |
|
OK, then I think it is best to close this pull request and call it a day. Was a good discussion. |
A newbie running a source file like this without the I would say it's very useful now, when 25 is so very new that not many people will be running it. And then in a couple of years we could remove the
I'd rather remove the /// line than the //JAVA one :-) |
|
newbie wont get that line unless he actually runs that init with java 25. as said, i'm fine adding it but we should be adding the actual requested java version not hardcode it to 25. |
That's assuming the user is creating the file themselves. If it's created by someone else and then added to a GH repo it's anyone's guess what JDK users will use to try to run it. Which, to me, means that to make life easy on people you should always use a //JAVA line when using features that are newer then Java 8. It encourages a way of working with JBang that "it always just works".
but it depends on the feature! if somebody used a Java 25 to run It's the developer that wrote the template that knows that this specific feature needs 25+ and therefore they should add that version explicitly, always. |
|
I'm not disagreeing but I'll argue that if you put 'jbang init --java 26 herewego.java' it's a big if we then put //JAVA 25+ and not //JAVA 26 |
|
True, but I'm not sure But that means that if the template developer did not add this section then the |
Closes #2142