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.

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

added java module flag list. #3658

Merged
merged 1 commit into from
Mar 4, 2022
Merged

added java module flag list. #3658

merged 1 commit into from
Mar 4, 2022

Conversation

mbien
Copy link
Member

@mbien mbien commented Feb 22, 2022

this PR is in search for a repository :)
(see apache/netbeans-jenkins-lib#53, apache/netbeans-mavenutils-archetype-nbm-archetype#2)

JVMs of JDK 16+ require this list to run NetBeans. This means we need it in netbeans.conf (already there), in CI configs, local junit tests and in maven archetypes and/or project templates.

Having it in the repo is most likely the best place for it, it has to evolve with the code + everything can refer to it.

This list should hopefully shrink over time when less and less encapsulation breaches are necessary - some of it is likely to stay however.

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Platform [ci] enable platform tests (platform/*) labels Feb 22, 2022
@mbien mbien added this to the NB14 milestone Feb 22, 2022
@mbien mbien added the CI Continuous Integration label Feb 22, 2022
@ebarboni
Copy link
Contributor

for me is ok but we should have another PR to make nbbuild/build.xml alter nb/ide.launcher/netbeans.conf cli add-**** with the content of this file. To keep synchro

@neilcsmith-net
Copy link
Member

OK with me, although my preference would be for the code to template nb/ide.launcher/netbeans.conf to be a second commit in this PR.

@lkishalmi
Copy link
Contributor

Well shall we create one for each cluster? I mean, I do not think that a platform application needs to packages related to javac.

@mbien
Copy link
Member Author

mbien commented Feb 22, 2022

Well shall we create one for each cluster? I mean, I do not think that a platform application needs to packages related to javac.

if its an IDE plugin it does need the list in the maven config of the unit tests for example. We can make this arbitrarily granular, e.g add a perfect list to every single module but i don't have the time for that unfortunately. Lets start somewhere.

OK with me, although my preference would be for the code to template nb/ide.launcher/netbeans.conf to be a second commit in this PR.

I agree. Would be good to have at least one place synced up with this PR. I take a look.

@mbien
Copy link
Member Author

mbien commented Feb 23, 2022

@lkishalmi I split the list into four categories, I think this could be a good compromise, what do you think?

Btw the only reason why I didn't make it a properties file is because i think a simple list makes it easier to parse outside of a java program.

@mbien
Copy link
Member Author

mbien commented Feb 23, 2022

@ebarboni @neilcsmith-net done

@matthiasblaesing
Copy link
Contributor

I'm not sure what we gain from splitting this into multiple file instead of using real property files with comments. In the end we will need a real solution, where the platform allows opening packages at runtime (java.base might be problematic). @jlahoda has a prototype here:

#291

I have looking into that on my "when I find time" list....

As a stop gap ok.

@mbien
Copy link
Member Author

mbien commented Feb 23, 2022

I'm not sure what we gain from splitting this into multiple file instead of using real property files with comments.

properties need parsing, a text file needs only simple tokenizing which should simplify usage outside of java programs. CI going to need a horizontal line without -J (probably generated by bash), maven files (archetypes) might want a vertical version like here (probably generated by freemarker): https://github.com/apache/netbeans-mavenutils-archetype-nbm-archetype/pull/2/files#diff-976f3bd796e4d3a6bb6bdb1d39336860e7bc4d10b9ad93ebca24e01db902cc4f allows copy+paste for users too etc.

As a stop gap ok.

thats what it is yes. I don't think I have to remind anyone that NB has several JDK 17 compatible releases out by now, but not a single job runs on it so far. The maven archetypes for platform or plugins don't work either, starting NB dev builds from the IDE will also only work up to JDK 15.

@matthiasblaesing
Copy link
Contributor

I'm not sure what we gain from splitting this into multiple file instead of using real property files with comments.

properties need parsing, a text file needs only simple tokenizing which should simplify usage outside of java programs.

  • ASF requires copyright headers, so parsers would at least be able to skip comments list (which in turn raises the question how the comment format should look like)
  • There might be the need to add further information. We have seen that at different places in the codebase. If the binaries-list format (the list of external binary files) would have been a structured format it would be less of a hack to parse it and we'd not guess that the target is a maven reference and/or we could trivially add a different checksum.

@mbien
Copy link
Member Author

mbien commented Feb 24, 2022

ASF requires copyright headers, so parsers would at least be able to skip comments list (which in turn raises the question how the comment format should look like)

that is new to me. Even the FAQ says "It is also valuable to tag each of your source-code files in case they become detached from the LICENSE file." This isn't even source code, just a list of JVM flags. But if the NB repo has stricter rules, I am ok with making it a java properties file, but this will likely complicate use cases.

@neilcsmith-net
Copy link
Member

We don't need license headers here. We already have a bunch of RAT exceptions for files with no degree of creativity - https://www.apache.org/legal/src-headers.html#faq-exceptions These may need adding there though?

If we're going to have multiple files, maybe put them in a subfolder? Could include comments on how they're used and where in a short readme file instead?

@mbien
Copy link
Member Author

mbien commented Feb 25, 2022

moved files into new folder + added README as requested

I renamed the files and properties so that it won't be confused with module dependencies or similar declarations.

@mbien mbien force-pushed the add-opens branch 2 times, most recently from 68bf2a0 to efb719a Compare March 2, 2022 10:54
@mbien
Copy link
Member Author

mbien commented Mar 2, 2022

planning to merge this PR this weekend. Speak up if there are more change requests.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Mar 2, 2022

Looks good, but as I expected, they're failing RAT tests. The files (or file extension and/or folder) need adding in to the "no degree of creativity" section of the RAT exclusions - https://github.com/apache/netbeans/blob/master/nbbuild/rat-exclusions.txt#L56

 - see README for details
 - base.flags for base module flags
 - desktop.flags for UI related module flags
 - compiler.flags for java compiler integration
 - tools.flags for jdk.* module flags
@mbien
Copy link
Member Author

mbien commented Mar 4, 2022

@neilcsmith-net added it to the ignore list, everything is green -> merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants