[Netbeans-5734] Quick fix to make NB 12.4+ run Glassfish 6.x also on any 11+ JDK version.

[nicolaken]

First of all Netbeans 12.4 had the V6 config file but was not calling it, now it does.
It also adds all JDKs to the available JDK runtimes.
Should instead differentiate between 6.0 and 6.1+, but I still don't understand the codebase very well.
I also don't know where it's more correct to put the --add-opens paramters in StartTask.java.

[pepness]

I think this should be in netbeans.conf. See PR-1581

[matthiasblaesing]

@nicolaken the question is: are the opens directives targeted at the glassfish process or at the netbeans process

@pepness I would not be too surprised, if an application server uses reflection to change the JDK internals, so it is plausible from my perspective, that the module openes are required for the glassfish to work correctly.

[nicolaken]

This is not needed to run the Netbeans IDE, it's only needed to launch Glassfish.
I have not tested, but because it spawn a new process, it should not inherit the Netbeans IDE arguments, and thus not work.

[nicolaken]

I have just seen that this PR should be rewritten and the applied only after applying #2902 .
In the comment of that PR I see that the author is working on adding the functionality of this PR to #2902, without knowing it's existence.
What 's the best way forward?

[juneau001]

I have just seen that this PR should be rewritten and the applied only after applying #2902 .
In the comment of that PR I see that the author is working on adding the functionality of this PR to #2902, without knowing it's existence.
What 's the best way forward?

Hi @nicolaken thanks for working on this PR! Please feel free to rewrite as needed and apply this PR after 2902. I did not modify the code for PR 2902 to incorporate GlassFish 6.2 support. I left it as-is. Thanks again and sorry for the delay in response.

[nicolaken]

Hi @juneau001, thank you!
I don't know if I have time for doing 6.2 ATM, but I will surely check that 6.1 works with JDK16 with this patch.

[ebarboni]

@nicolaken @juneau001 , would be nice to have this PR ready even if not for 6.2. 6.2 may wait 12.6. Wanna cut branch soon to make preparation for 12.5

[nicolaken]

@ebarboni @juneau001 I have updated the PR and it seems to work as intended with GF6.1 on JDK16.

Now possible JDKs for GF 6.1 are only 1.8, 11 and 16, removed all other intermediate JDKs.
Officially only JDK 11 is supported on GF 6.1, but 16 mainly works so JDK 16 is there too.
I'm not sure if GF 6.1 works on 1.8 as it's not clear from the documentation, but included nevertheless.

Controlled that GF 6.1 correctly launches from NetBeans running on JDK 16 with Glassfish platform set to JDK 16 and that it does not add the --add-opens to JDKs less than 9. Did not do further testing.

[ebarboni]

make sense to squash ? or every commit matter ? Not sure why github action is down but travis looks ok

[nicolaken]

Commit d6d1a2a fixes two line endings in a file that does not belong to this PR. Other than that squashing is fine.

[ebarboni]

how may I filter this ? could you do that from your side @nicolaken ?

[nicolaken]

Unfortunately I'm away in these days, will be back with a computer the 6th of August. Basically the last two commits descrive everything.

[matthiasblaesing]

This is a good change for glassfish, but I left to inline questions regarding IMHO unnessary changes. I verified, that this change indeed makes it possible to:

a) configure glassfish at all again (property files were broken by a prior change)
b) I could start glassfish 6.1 from a fresh netbeans build

The whole changeset should be squashed before merging.

[matthiasblaesing]

Why is this necessary? I downloaded Glassfish 6.1 from github and was able to start it standalone with JDK 11. This lead me to have a look at the domain.xml and I found this:

        <jvm-options>--add-opens=jdk.management/com.sun.management.internal=ALL-UNNAMED</jvm-options>
        <jvm-options>--add-opens=java.base/sun.net.www.protocol.jrt=ALL-UNNAMED</jvm-options>
        <jvm-options>--add-opens=java.base/java.lang=ALL-UNNAMED</jvm-options>
        <jvm-options>--add-opens=java.base/java.util=ALL-UNNAMED</jvm-options>
        <jvm-options>--add-opens=java.rmi/sun.rmi.transport=ALL-UNNAMED</jvm-options>

The jvm-options are picked up by:

https://github.com/apache/netbeans/blob/master/enterprise/glassfish.tooling/src/org/netbeans/modules/glassfish/tooling/server/ServerTasks.java#L162
and
https://github.com/apache/netbeans/blob/master/enterprise/glassfish.tooling/src/org/netbeans/modules/glassfish/tooling/server/parser/JvmConfigReader.java#L187-L189
https://github.com/apache/netbeans/blob/master/enterprise/glassfish.tooling/src/org/netbeans/modules/glassfish/tooling/server/parser/JvmConfigReader.java#L134-L139

As the server start standalone, I don't see why netbeans needs the addtional opens. Can you shed some light on this?

[nicolaken]

Initially I saw the same thing, and tried running without this part. I am using Liberica Jdk16, and downloaded and installed a fresh GF 6.1 directly from Netbeans, without running Derby. I ran it and got explicit errors that said that opening was needed. I did not investigate further for lack of time.

[matthiasblaesing]

The issue is here:

netbeans/enterprise/glassfish.tooling/src/org/netbeans/modules/glassfish/tooling/server/ServerTasks.java

Lines 304 to 368 in 19be548

	private static void appendOptions(StringBuilder argumentBuf,
	List<String> optList, Map<String, String> varMap) {
	final String METHOD = "appendOptions";
	HashMap<String, String> keyValueArgs = new HashMap<>();
	LinkedList<String> keyOrder = new LinkedList<>();
	String name, value;
	// first process optList aquired from domain.xml
	for (String opt : optList) {
	// do placeholder substitution
	opt = Utils.doSub(opt.trim(), varMap);
	int splitIndex = opt.indexOf('=');
	// && !opt.startsWith("-agentpath:") is a temporary hack to
	// not touch already quoted -agentpath. Later we should handle it
	// in a better way.
	if (splitIndex != -1 && !opt.startsWith("-agentpath:")) {
	// key=value type of option
	name = opt.substring(0, splitIndex);
	value = Utils.quote(opt.substring(splitIndex + 1));
	LOGGER.log(Level.FINER, METHOD,
	"jvmOptVal", new Object[] {name, value});
	} else {
	name = opt;
	value = null;
	LOGGER.log(Level.FINER, METHOD, "jvmOpt", name);
	}
	if (!keyValueArgs.containsKey(name)) {
	keyOrder.add(name);
	}
	keyValueArgs.put(name, value);
	}
	// override the values that are found in the domain.xml file.
	// this is totally a copy/paste from StartTomcat...
	final String[] PROXY_PROPS = {
	"http.proxyHost", // NOI18N
	"http.proxyPort", // NOI18N
	"http.nonProxyHosts", // NOI18N
	"https.proxyHost", // NOI18N
	"https.proxyPort", // NOI18N
	};
	boolean isWindows = OsUtils.isWin();
	for (String prop : PROXY_PROPS) {
	value = System.getProperty(prop);
	if (value != null && value.trim().length() > 0) {
	if (isWindows && "http.nonProxyHosts".equals(prop)) {
	// enclose in double quotes to escape the pipes separating
	// the hosts on windows
	value = "\"" + value + "\""; // NOI18N
	}
	keyValueArgs.put(JavaUtils.systemPropertyName(prop), value);
	}
	}
	// appending key=value options to the command line argument
	// using the same order as they came in argument - important!
	for (String key : keyOrder) {
	argumentBuf.append(' ');
	argumentBuf.append(key);
	if (keyValueArgs.get(key) != null) {
	argumentBuf.append("="); // NOI18N
	argumentBuf.append(keyValueArgs.get(key));
	}
	}
	}

This takes all parameters, splits them into key-value parts, stuffs them into a map, modifies them and serializes them into the command line. The problem is, that putting the options into a map breaks all options, that can be set multiple times., as "add-opens" can. Only one value survives this procedure. It is just luck, that that trick works at all. To me that function needs to be fixed (a multimap?).

[matthiasblaesing]

For me running glassfish on 6.1 on 8 failed. Are you sure 8 is still supported?

[pepness]

Glassfish 6.1 and 6.2 correspond with Jakarta 9.1 which runs on JDK 11, I understand that it should run on JDK 11 or higher.
https://github.com/eclipse-ee4j/glassfish/releases

[nicolaken]

I saw conflicting info in different places. It basically says that Jdk11 Is supported, but I do not see it mandatory. Also JakartaEE 10 will require 11, as of it was a new requirement. @juneau001 PR #2902 did not require 11. On the other hand I was not able to launch with 1.8 from Jdk16, it did not understand rmi opens from Glassfish. Maybe it needs to launch from a Netbeans running on 1.8?

[matthiasblaesing]

With the domain.xml of glassfish 6.1 it is not possible to run it on JDK 8 as the add-opens calls will always clash. So I put my work where my mouth was and this is my suggestion:

https://github.com/matthiasblaesing/netbeans/tree/pr-3056

The two last commits are the suggested changes. With that applied glassfish comes up without exceptions, in contrast to the situation in this PR where exceptions are generated about inaccessibles classes. Given that the domain.xml opens more packages, than the change proposed here, this looks sane.

[nicolaken]

With the domain.xml of glassfish 6.1 it is not possible to run it on JDK 8 as the add-opens calls will always clash. So I put my work where my mouth was and this is my suggestion:

https://github.com/matthiasblaesing/netbeans/tree/pr-3056

The two last commits are the suggested changes. With that applied glassfish comes up without exceptions, in contrast to the situation in this PR where exceptions are generated about inaccessibles classes. Given that the domain.xml opens more packages, than the change proposed here, this looks sane.

I cannot do any testing from my side, but it seems to really solve the issue nicely. Thank you @matthiasblaesing !

[ebarboni]

We should close this PR as mentioned in #3085