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

Do not include jdk.jshell in test mod list by default. #4013

Merged
merged 1 commit into from May 3, 2022
Merged

Do not include jdk.jshell in test mod list by default. #4013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion nbbuild/default.xml
View file
Open in desktop
Expand Up @@ -246,6 +246,7 @@
</filterchain>
</loadfile>
<property name="metabuild.jms-flags" value="${jms-base.flags}${jms-desktop.flags}${jms-compiler.flags}${jms-tools.flags}"/>
<property name="metabuild.jms-flags.jvm" value="${jms-base.flags.jvm}${jms-desktop.flags.jvm}${jms-compiler.flags.jvm}${jms-tools.flags.jvm}"/>
<!-- Excluding jms-tools.flags, as nbjavac usually overlaps with tools dependencies -->
<property name="metabuild.jms-flags.jvm" value="${jms-base.flags.jvm}${jms-desktop.flags.jvm}${jms-compiler.flags.jvm}"/>
Comment on lines -249 to +250
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would mean that running NB from the IDE would set fewer JMS flags than running a NB dist started with the typical netbeans.conf. Do we really want to do that?

Copy link
Member

@mbien mbien Apr 24, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in other words: the only difference between metabuild.jms-flags and metabuild.jms-flags.jvm is that one has a -J prefix for each flag the other has not. IMO: both flag list should not diverge in content.

Maybe we shouldn't have two properties, it might be better to have just one, and build a second one (taking the first as input) just before its inserted into netbeans.conf to make the relationship more clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbien yes we do want that: with real module system in action (as it is the case at runtime), the correct classes (from NBJavac) are loaded despite the JDK modules present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdedic the problem is that metabuild.jms-flags.jvm will be pasted into maven templates too in future, while the other property with -J is pasted to the conf.

example: apache/netbeans-mavenutils-archetype-nbm-archetype#9

Do we need a third property only for testing? Originally, there was only supposed to be one property, the only reason there are two is the -J prefix - this already is a fairly ugly workaround IMO.

one for CI, one for nb.conf, one for maven run config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbien what purpose is/will be this "maven run config" used for: this 'special exclusion' should affect surefire plugin, but not exec plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdedic it is intended to be used as an initial set of flags which allow users to press play on a newly created platform project and it is going to work even if they added a few features. Users can then optimize the list and only include what is actually required to run their application.

If surefire will need a different set of flags than the exec plugin... well that complicates things even further. I hoped we could use one big list for everything - as initial default. How is the user supposed to know what to remove for the surefire plugin list?

I guess we could setup two lists by subtracting a few items from the first list? Maybe we could add the -J just before text substitution so that we don't need a third list.

as mentioned in the readme (https://github.com/apache/netbeans/blob/master/nbbuild/jms-config/README). "There should be hopefully fewer flags over time, devs are encouraged to shrink the list if possible."
I hope the list won't grow at least :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, the current state is fundamentally broken: it can not add jdk.jshell module while using --limit-modules at the same to exclude jdk.compiler and other jdk.jshell depenendencies. The set of flags is different already: the --limit-modules is only used for runnign tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like we need another list for tests which use --limit-modules, or as mentioned, it might be possible to simply remove jdk.shell flags from the list before use in tests, assuming its the only special case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but looking at the actual list https://github.com/apache/netbeans/blob/master/nbbuild/jms-config/tools.flags

its probably ok to exclude it by default, most platform apps won't notice anyway. Lets merge this PR :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"--add-modules=jdk.jshell" shouldn't technically be in the list anyway, since its a list of encapsulation exceptions, not a list of dependencies.

</target>
</project>
5 changes: 3 additions & 2 deletions nbbuild/templates/projectized.xml
View file
Open in desktop
Expand Up @@ -289,7 +289,8 @@
</target>
<target name="test-lib-init" depends="-init-bootclasspath-prepend,init,-build-libs.junit4">
<path id="test.unit.lib.cp"/>
<property name="test.run.args" value="-ea -Xmx700m ${metabuild.jms-flags.jvm} -XX:+IgnoreUnrecognizedVMOptions"/>
<property name="test.jms.flags" value=""/>
<property name="test.run.args" value="-ea -Xmx700m ${metabuild.jms-flags.jvm} ${test.jms.flags} -XX:+IgnoreUnrecognizedVMOptions"/>
<property name="extra.test.libs.dir" location="${test.dist.dir}/extralibs"/>
<macrodef name="test-init-nborg">
<attribute name="test.type"/>
Expand Down Expand Up @@ -886,4 +887,4 @@
</exec>
</target>

</project>
</project>