Fix the -jvm-debug and -h flags in the ash template

[qwe2]

This should fix issues like #1523

[lightbend-cla-validator]

At least one commit author ([email protected], [email protected]) is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

[lightbend-cla-validator]

At least one commit author ([email protected], [email protected]) is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

[lightbend-cla-validator]

Hi @qwe2,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

[qwe2]

Interestingly the addDebugger syntax from bash-template is only compatible with Java 9+, but the GH workflow is still pulling JDK 1.8. java_version_check also checks for Java 8, not 9. Not sure if this is intentional...

[muuki88]

Thanks for tackling this 💪

I'll be on vacation . Maybe @dwickern can take a look 😊

[qwe2]

I've been a bit busy and wasn't sure what to do about Java 8 compat, but I ended up adding some checks so that -jvm-debug works on both ash and bash for Java 8 as well.

[muuki88]

Hi @qwe2

Sorry for the waiting - I'll try to find some time to take a deeper look!

[dwickern]

Re: Java 8 compatibility, I found the relevant change in the JDK 9 release notes (bold mine)

JDWP socket connector accept only local connections by default

The JDWP socket connector has been changed to bind to localhost only if no ip address or hostname is specified on the agent command line. A hostname of asterisk (*) may be used to achieve the old behavior which is to bind the JDWP socket connector to all available interfaces; this is not secure and not recommended.
JDK-8041435

IMO we should keep the secure defaults and let users decide what address to bind to:
-jvm-debug 5005 - listen on all addresses (Java 8) or only localhost (Java 9+)
-jvm-debug localhost:5005 - works the same on all Java versions
-jvm-debug '*:5005' - listen on all addresses for Java 9+ (quotes needed in some shells to prevent * expansion)

That means we can remove all the Java version detection. We should also revert #1546 to keep bash and ash consistent.

[qwe2]

Sounds good, but this would be a breaking change then:

-jvm-debug 5005 - listen on all addresses (Java 8) or only localhost (Java 9+)

This currently means *:5005 on JDK 9+, but going forward, it would mean localhost:5005 instead. Is that acceptable?

[muuki88]

I'm with @dwickern on the secure setting by default and also reflecting what the JVM does. If you are not using sbt-native-packager and you use -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005 it will also break when you upgrade the JVM.

After all, upgrading a major version of the JVM must be tested and I think it's better if sbt-native-packager doesn't try to magically add a migration path.

[qwe2]

I've reverted #1546 and removed the major version checks. Note that this would be a breaking change for anyone currently using -jvm-debug 5005 on JDK 9+. They would have to change it to -jvm-debug '*:5005' to match the current behavior.

[muuki88]

Thanks for you patience @qwe2 and thanks for stating the explicit breaking change.

Note that this would be a breaking change for anyone currently using -jvm-debug 5005 on JDK 9+

I have never used this flag before. My assumption would be, that this is only used in testing environments and not in production, right?

[qwe2]

that this is only used in testing environments and not in production, right?

Yeah, I think that nobody should be using this flag in production. (Unfortunately that doesn't necessarily mean that nobody does 😞 )

[muuki88]

Thanks for clarifying 😄 . Then I'm willing to accept than change that some cowboy devops folks may not be able to start there instance when upgrading sbt-native-packager and need to do a small change to their build.sbt 😁

[muuki88]

@dwickern do you have any remarks? Otherwise I would merge and release this change 🎉

[dwickern]

LGTM 🎉