search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.64k stars 849 forks source link

nb-javac: "Eat your own dog food" testing #5609

Closed jtulach closed 1 year ago

jtulach commented 1 year ago

TL;DR

This PR doesn't change anything on the way NetBeans is built by default. This PR offers an an opt-in support to build NetBeans with a special compiler provided as a set of JAR files. One needs to provide additional -Dnbjavac.class.path property. Having such support in allows nb-javac project (see https://github.com/JaroslavTulach/nb-javac/pull/17) as well as NetBeans CI (part of this PR) to verify nb-javac can built the NetBeans codebase.

Details

With #5550 in, there is a time to start "consuming our own dog food". NetBeans relies on nb-javac to provide WYSIWYG Java editing experience supporting the latest Java language features regardless the JDK the IDE runs on. Reliable, compatible and well tested nb-javac JARs are essential for delivering such an experience and ensuring the same behavior as when running NetBeans IDE on the latest JDK without nb-javac.

There are many ways to ensure nb-javac is compatible with JDK's javac including its unique automatic generation process. However nothing can beat a real world experience: Let's use nb-javac to build some real world project! Let's use it to build NetBeans! NetBeans is a huge, long running, well established software project using various aspects of the Java language - by building it with nb-javac we can proof the compiler is good enough (at least for building NetBeans).

To build NetBeans with this PR use:

$ ant download-all-extbins
$ ant build -Dnbjavac.class.path=java/libs.javacapi/external/*.jar

Without specifying the nbjavac.class.path property the build runs as normal. When the nbjavac.class.path property is specified, it uses the provided location to find nb-javac JARs and load the compiler from there - a bit of ClassLoader magic is necessary to make sure the nb-javac takes precedence over JDK's javac and is cached between subsequent compiler invocations in the Ant nbbuild/build.xml script, but everything seems to work across JDKs.

The nb-javac JARs are intentionally specified via a property to simplify providing different version of nb-javac. Maintainers of nb-javac project (e.g. me and Dušan) are encouraged to include build of NetBeans into their own CI pipeline to verify new versions of nb-javac remain compatible with NetBeans sources.

Similarly the NetBeans CI shall be extended with a job to verify ant build -Dnbjavac.class.path=java/libs.javacapi/external/*.jar continues to work. Here I rely on @mbien guidance as the CI configuration changed a lot since the last time I played with it.

mbien commented 1 year ago

nb-javac allows the java editor to run on JDKs which are older than the JDK the editor modules were linked against. It is purely a bytecode level backport and is optional if NB is run on the right JDK (same version as the one nb-javac was based on). Community installers for example could easily bundle NB without nb-javac in it, even NB itself could download a runtime JDK and completely drop the nb-javac dependency some time in future.

I don't see what problem this PR is solving beside adding complexity and an indirection to the build process. Soon we will switch to building/testing on JDK 21 EA and part of the test is using the EA compiler too. The javac of the JDK works just fine and we make sure it builds on all LTS versions. Again: nb-javac is just a dependency for some modules, we don't have to bootstrap the whole project with it. NetBeans is more than just the java editor which currently relies on nb-javac.

nb-javac is currently also not in this repository (or even an apache project unless I miss something obvious), so I am not sure what you mean by "eat your own dog food".

I would not want to see this added to the project since I frankly don't see a reason for it. If the nb-javac project needs more tests, why can't nb-javac host the ant task (maven plugin?) and build a larger project there in CI?

We should rather concentrate on upgrading everything to JDK 11 which actually solves problems and has likely community consensus as I gather from the last thread about it. @neilcsmith-net drafted a proposal for this.

Further, a change like this (swapping out the compiler) would require more discussion on the dev list anyway - and a vote IMO.

matthiasblaesing commented 1 year ago

Further idea: My reading of the module system is, that it should be possible to selectively upgrade parts of the JDK runtime (see --upgrade-module-path option). That way "nb-javac" would just "be" jdk.compiler. No fiddling with the build environment necessary.

jtulach commented 1 year ago

Further idea: My reading of the module system is, that it should be possible to selectively upgrade parts of the JDK runtime (see --upgrade-module-path option). That way "nb-javac" would just "be" jdk.compiler. No fiddling with the build environment necessary.

The drawback is: thenb-javac is not used that way in NetBeans. NetBeans is using similar "classloader trick" with masking JDK classes. If we succeed with --upgrade-module-path option - it may not guarantee nb-javac works in NetBeans properly.

Btw. I wanted to "Eat our own dog food" with nb-javac@19, but I couldn't as https://github.com/JaroslavTulach/nb-javac/pull/11 - we wouldn't found that problem with --upgrade-module-path option.

While real JDK test is also useful check, for NetBeans I'd rather stick with old good "classloader trick".

neilcsmith-net commented 1 year ago

I share reservations of @mbien and @matthiasblaesing It's an interesting exercise, but could never be the default option, adds complexity to the build, and isn't really the best test of nb-javac IMO. Tests using modern language features and comparing output with the un-backported javac would seem more useful? Still, I'm -0 as long as it doesn't break things!

nb-javac is currently also not in this repository (or even an apache project unless I miss something obvious), so I am not sure what you mean by "eat your own dog food".

Well, nb-javac is not an Apache NetBeans project, and cannot be developed as an Apache NetBeans project*. Unless something significant changes in ASF rules, nb-javac is never going to be our "own dog food". Compiling NetBeans might be considered to be.

Whichever, the responsibility for development and the bulk of testing nb-javac needs to remain external. If this change helps with testing upstream, it could go in. We could even consider running that build test in CI here, but I'd probably limit that to nb-javac update PRs. We don't have unlimited capacity.

* one option to investigate, and discussed previously, might be running the backport javac build inside the NetBeans build, assuming no GPL+CPE sources required on our side. That would still need some checking with legal.

We should rather concentrate on upgrading everything to JDK 11 which actually solves problems and has likely community consensus as I gather from the last thread about it. @neilcsmith-net drafted a proposal for this.

+1 to that, but I don't think there's any overlap in the two things. nb-javac cannot help with our requirements to update minimum JDK, and upgrading the base to JDK 11 then 17 won't remove our reliance on nb-javac for Java editing.

mbien commented 1 year ago

I don't think there's any overlap in the two things. nb-javac cannot help with our requirements to update minimum JDK, and upgrading the base to JDK 11 then 17 won't remove our reliance on nb-javac for Java editing.

I didn't mean it like that, indeed bumping the bytecode level won't remove the requirement for nb-javac: it would bring us a tiny step closer though, while not bumping the version increases the distance with every single java release.

I hope that i am wrong but I do fear that this PR here is just bootstrapping the option for a future switch to frgaal with the argument: look we don't have to leave JDk 8 til 2030+ since we just made our own language with some of the language features of newer java versions.

This wouldn't fix any of the problems either btw (e.g the fact that we run out of supported libraries we can use - see that whole maven-indexer discussion and many other modules which want to use non JDK 8 libs). I am taking this purely from previous discussions on dev, and reading between lines (again: i hope that i am wrong).

I don't understand at all, why the netbeans project would have to run tests for nb-javac, this would have to be handled in the nb-javac repo. We also don't test OpenJDK or any other third party lib in CI of the cost of apache.

I read through the PR text a third time now and still can't come up with a single reason why we should try this except form a purely academical perspective.

But as someone who basically spend this weekend trying to find workarounds for maven-indexer, I am very close to -1 any compiler swap proposals until we solve some of our technical dept and the fact that we still cant use anything beyond JDK 8 in 2023 due to non-technical reasons.

jtulach commented 1 year ago

Thank you @neilcsmith-net:

Interesting exercise, ... adds complexity to the build,

The complexity is fairly isolated into its own NbJavacLoader class. Beyond that there are just three executable lines in CustomJavac task. I can refactor the NbJavacLoader to be a separate top level class, if that feels better.

and isn't really the best test of nb-javac IMO.

Real project test is always a good test. In fact there are already two cases where the "dog food test" helps nb-javac get better:

If this change helps with testing upstream, it could go in.

Yes, it does help. The testing is ready to go in: https://github.com/JaroslavTulach/nb-javac/pull/17

We could even consider running that build test in CI here,

Great, 7de1422 adds the GitHub Actions run and it has succeeded.

but I'd probably limit that to nb-javac update PRs. We don't have unlimited capacity.

OK, guidance to adjust 7de1422 welcomed.

jlahoda commented 1 year ago

My personal opinion only.

One advantage of having a firm compiler during build is less dependence on specifics of the compile-time JDK. This might be important if we e.g. wanted to compile some module with javac.source=19 or javac.source=20 - currently, that would require upgrading the build JDK to 19/20, and everyone building NB would need to use that. Also, there may be conflict between supported JDKs of Gradle that we use, and the required JDK to build e.g. javac.source=20 module.

Also, if we ever tried to make reproducible builds, having a firm, known, compiler might reduce the amount of problems to solve.

jtulach commented 1 year ago

One advantage of having a firm compiler during build is less dependence on specifics of the compile-time JDK.

Thank you Jan. While this PR demonstrates having such a firm compiler specification is possible, it is not the goal of this PR to make any such changes. My goal is to allow usage of nb-javac in the NetBeans build system and start testing such combo. Clearly there are issues in the nb-javac that shall be improved (for example the -Werror problem mentioned above). Improving them while having CI checks on NetBeans as well as nb-javac side is a solid engineering approach I am aiming at.

Hopefully the three line change to the regular build process is found safe enough to allow us to get it in and start evolving the NetBeans+nb-javac combo properly.

matthiasblaesing commented 1 year ago

One advantage of having a firm compiler during build is less dependence on specifics of the compile-time JDK. This might be important if we e.g. wanted to compile some module with javac.source=19 or javac.source=20 - currently, that would require upgrading the build JDK to 19/20, and everyone building NB would need to use that. Also, there may be conflict between supported JDKs of Gradle that we use, and the required JDK to build e.g. javac.source=20 module.

I think this is an academic discussion. If we can't build with the current JDK, it is a bug that must be fixed. Fixating the compiler will only suppress the pain and when it is finally resolved, it will be painful to fix. We see this in the codebase already:

The same will happen, once we introduce a fixed compiler. People will not fix problems with upstream compiler and code will bitrot. We will be forced to use a non-standard javac ad infinitum.

It is also a bit ironic to speak about javac.source=20 in the NetBeans codebase. It currently literally takes days to make a module, that can use current APIs (current as in "5 years ago"). The NetBeans codebase targets bytecode, that is 9(!) years old, so sorry, but that argument does not fly with me.

Also, if we ever tried to make reproducible builds, having a firm, known, compiler might reduce the amount of problems to solve.

It is trivial to install a JDK from a known vendor, if the javac is really the biggest problem. Contrary to some comments about using recent JDKs read as if it takes days to install a current JDK, in reality it is a matter of minutes.

jlahoda commented 1 year ago

One advantage of having a firm compiler during build is less dependence on specifics of the compile-time JDK. This might be important if we e.g. wanted to compile some module with javac.source=19 or javac.source=20 - currently, that would require upgrading the build JDK to 19/20, and everyone building NB would need to use that. Also, there may be conflict between supported JDKs of Gradle that we use, and the required JDK to build e.g. javac.source=20 module.

I think this is an academic discussion. If we can't build with the current JDK, it is a bug that must be fixed. Fixating the compiler will only suppress the pain and when it is finally resolved, it will be painful to fix. We see this in the codebase already:

* NetBeans forked javac in the past instead of working with upstream to stabilize it and make it usable over multiple java versions. We are still suffering from that failure.

* NetBeans used an ancient copy of graaljs and maybe even patched this. The two branches got so far apart, that now they have to maintained independently. The initial help of the external codebased turned into a problem.

The same will happen, once we introduce a fixed compiler. People will not fix problems with upstream compiler and code will bitrot. We will be forced to use a non-standard javac ad infinitum.

I think this is backwards: the compiler would be the up-to-date compiler. So, people would have to deal with any problems, and it would be more difficult to hide from problems related to the compiler. (It might be easier to hide solving non-compiler related problems, though.)

It is also a bit ironic to speak about javac.source=20 in the NetBeans codebase. It currently literally takes days to make a module, that can use current APIs (current as in "5 years ago"). The NetBeans codebase targets bytecode, that is 9(!) years old, so sorry, but that argument does not fly with me.

I am completely confused here: on purely technical level, I think I can add a optional module with javac.source=17 quite easily: https://github.com/jlahoda/netbeans/tree/test-module-17

There are, of course, some issues with that, among others:

The second point makes it unclear to me how to finalize a change like this - while this should not affect people running the resulting build, it will affect everyone building it, and will require upgrade of the build JDK. How does one do that?

Should we simply say that NB builds using the newest JDK only? (The newest JDK at the time of release, or something like this.) Because that's one of the very few alternatives I see for using newer sources?

(Here I'd like to point out that build JDK is not necessarily the runtime JDK. And, with the changes in past years, I believe it should be easily achievable to have build JDK newer than the runtime JDK, and the opposite direction was always the case.)

Also, if we ever tried to make reproducible builds, having a firm, known, compiler might reduce the amount of problems to solve.

It is trivial to install a JDK from a known vendor, if the javac is really the biggest problem. Contrary to some comments about using recent JDKs read as if it takes days to install a current JDK, in reality it is a matter of minutes.

dbalek commented 1 year ago

If I understand changes in this PR correctly, this PR only adds an option for people to build NB using nb-javac if they want to (it does not require them to do so) and it should not affect the standard NB build process (as written in the PR description: "Without specifying the nbjavac.class.path property the build runs as normal"). If this is so, I have nothing against it.

neilcsmith-net commented 1 year ago

but I'd probably limit that to nb-javac update PRs. We don't have unlimited capacity.

OK, guidance to adjust 7de1422 welcomed.

Add a GitHub label (eg. nb-javac) and only run those specific tests when the PR has that label, such as a nb-javac update PR. The same logic is used throughout the existing CI tests, to only run those relevant to the PR.

@mbien better to answer that question, should he be happy for this to merge at all.

matthiasblaesing commented 1 year ago

@jtulach the conflict is caused by overlapping changed from #5652. I put my words to work and cleaned the flags up. I suggest to remove the conflicting parts from 0112550f0a792fc6cfe0f26d74defc057a6d5cd0 and rebase this whole PR onto current master. That should clean this up.

sdedic commented 1 year ago

let me share my overall opinion on this. In the current state of the codebase, we need_ nb-javac. As JDK codebase continues to evolve, newer versions of nbjavac are eventually released. Given there's automated process for building nb-javac out of upstream sources, it's rather straightforward to prepare a new build. We are using nbjavac to analyse user's sources AND we +- support analysis by Javac itself given the IDE runs on the "correct" JDK.

I think we really want to ensure that "running on correct JDK" and "running on possible JDK" (that implies nb-javac to supply the targetted JDK behaviour) gives the same results, or at least does not fail, if the user switches nb-javac for javac (or vice-versa). From this point of view, I fully support integrating the PR and eventually running the tests when a new version of nb-javac is included. I would assume that maintainer will continue to update nbjavac as JDK team releases newer versions. While that cooperation works smothly, we can easily keep the test and benefit from it; it's in the best interest of the proponent(s) and nbjavac maintainers that nbjavac releases timely follows JDK ones.

There were several opinions voiced in the thread

@mbien: nb-javac is optional if NB is run on the right JDK

I need to remind: people who use NetBeans to produce code (as opposed to people developing NetBeans), and also people using NetBeans as tooling platform have very different business requirements that require them to run on just specific JDKs (even the IDE), and have hard requirements for the tooling platform's environment (i.e. build machines in company cluster may have some restrictions). I want to emphasize, is that it does not matter if that requirement (for those groups of our users) is 'technically sound' or not - they may exist for purely business, legal or security reasons - which may be even silly, but impossible to revert from the developer seat. If we require our users to "run on the right JDK" - such groups of people may be forced to simply drop NetBeans completely and look for other options. If we're lucky, mandating 'just the newest JDK' will be an unneccessary PITA. We do not have that much 'spare users' that we can just throw these groups over the board just because it makes NetBeans development more pleasant. Oracle's Graal/Cloud team may be also affected by such approach.

So the assumption that 'everyone can use any JDK' so NB "runs on the correct JDK" is IMHO wrong. I feel that the discussion emphasizes the importance of development "our common pet project" (Apache NetBeans) too much and optimizes for it, while deprioritizing our users. I happen to be

I believe that maintaining a bundled, well-tested compiler is a benefit for those user groups with "strange requirements" - as long as the nb-javac keeps releasing updated versions after its upstream releases. Now it does, so we should act accordingly. At the same time, we can just plan for an exit strategy, which may be fairly simple. We do not need to play what-if games now to block a thing that would, for example, alert us when a problematic change happens in some future JDK javac version.

@jlahoda : One advantage of having a firm compiler during build is less dependence on specifics of the compile-time JDK.

@matthiasblaesing: I think this is an academic discussion. If we can't build with the current JDK, it is a bug that must be fixed.

I don't think this is not an academic discussion: I agree that when we detect that our codebase is not compilable with new JDK, we should address that SOON. But I do not see that as a valid argument for or against the discussed change, rather as an unrelated test and policy that we should establish anyway. As jlahoda noted, usage of "most updated" but bundled javac allows modern optional modules to be written when the author decides he does not need (want) to support older JDKs. In particular modules that require e.g. JDK17 APIs to run. Doing so without nb-javac would require everyone working on NB codebase to upgrade to JDK17 ... which is not an option (see above).

@mbien : I do fear that this PR here is just bootstrapping the option for a future switch to frgaal with the argument

It might. But a 'fear that something might happen' should not cause to block a change just because "it might be extended in the future, maybe". You/we should be aware of such extension may happen. Rejecting a purely 'piecemeal' approach is a valid position, if it is the (only/major) supportive "argument" if such proposal comes in the future.

sdedic commented 1 year ago

By the way - we probably SHOULD move this to the mailing list ... there are several 'strategic' topics not related to or blocking this change involved that should be discussed (and probably voted on) in a forum rather than discussed in the PR.

neilcsmith-net commented 1 year ago

With all the Oracle people talking up the benefit of nb-javac here, you'd have thought that developing and delivering from Oracle and in an Oracle namespace, as originally agreed with ASF legal for us to distribute it, wouldn't have been that hard of a position to achieve! :wink:

A backported compiler is a positive thing (it's a shame javac isn't developed like this). While it remains "unofficial", nb-javac remains a problem and something there is always going to be some push to move away from, not deeper ingrain into the platform.

I need to remind: people who use NetBeans to produce code (as opposed to people developing NetBeans), and also people using NetBeans as tooling platform have very different business requirements that require them to run on just specific JDKs (even the IDE), and have hard requirements for the tooling platform's environment

The former "producing code" people I'm less convinced by, at least judging by IntelliJ's popularity! The latter "tooling platform" people I am, and why I keep reiterating the platform is the entire framework.

Agree a lot of this conversation should go on the mailing list though (and realise me adding to it is probably not helping! :smile:)

mbien commented 1 year ago

please clarify what this PR is supposed to achieve and how it benefits the project. PRs are supposed to improve the project.

if this intends to actually test nb-javac:

This is however is not what I see in this PR! This PR adds a CI workflow. If nb-javac stages a new build It makes absolutely no sense to test it here, check if NB builds and if not, fix something in nb-javac which is a completely different project. The NetBeans project is also not running maven tests here to check if something fails before upgrading maven - this already happened!

Regarding reproducible builds: 1) I don't see anything indicating that anyone wants to work on this. Interest in CI topics is in general fairly low, as long CI works, nobody cares about it. (and I completely understand, since it is just a tool and a solved problem). Why do I think that would be the case? I migrated travis to gh and the feedback threads I opened on the dev list are basically empty, I almost merged some of the PRs without being able to motivate anyone to approve them (only the usual suspects took a look (thanks again)), and yes I usually added everyone. Again: as long the build works nobody cares 2) reproducible builds in java do not require to separate the compiler from the JDK, I have helped with reproducible builds before in other projects and that wasn't even on the table there. The JDK is a convenient bundle you can depend on - its a feature not a bug 3) trying to achieve reproducible builds with ~450 ant projects going to be hell.

It might. But a 'fear that something might happen' should not cause to block a change just because "it might be extended in the future, maybe"

@sdedic I disagree. If there is no clear benefit to the project in the first place, it should be enough of a reason to not merge something (we have done that before). The argument to merge it is the exact same. Someone might find it potentially useful in hypothetical reproducible builds some time in future, just in case someone wanted to swap out a compiler. Again: if this is about a test case for nb-javac -> wrong repository.

regarding JDK 8:

It's dead Jim. For those who haven't migrated yet: maven central is full with NB versions which still support it (and there will be a few more, I am sure). If someone wants to participate, please do so and I am sure we will find a way to branch NB (assuming everyone agrees and we find a release manager) - lets get involved if you have specific requirements - now is the time.

nb-javac does not backport the java ecosystem to JDK 8

I have never been in or heard of a team who didn't allow intellij or android studio because it uses JDK 17 to run on (it wouldn't surprise me at all if they move to 21 shortly after it lands). No matter what the support contracts were. I am sure they exist somewhere but I don't think we have to take them into account in the minimum requirements (again: get involved).

yes this absolutely should have been discussed on the dev list, like I wrote in the first comment of this PR.

mbien commented 1 year ago

I also don't want to write books here and go into everything, but another point I should have had covered:

The PR advertises this as great achievement to be finally able to use javac 20, while we are doing this for months already in CI, long before nb-javac was ready. Once NB 18 branched we will likely build on 21 LTS too, PR #5653 is prepared and green. This gives us early warning without having to wait for nb-javac being ready - every too weeks a new ea build.

nb-javac is a dependency of a subset of all netbeans modules (the java editor specifically). Making it a build dependency for everything is not a feature in itself.

matthiasblaesing commented 1 year ago

So turning back to the PR itself and not the politics around it. I had some suggestions/thoughts that address my technical points and might also partially address @mbien concerns:

https://github.com/matthiasblaesing/netbeans/tree/BuildNetBeansWithNbJavac

The branch was rebased onto master to fix the merge conflicts. Changes:

@jtulach could you please have a look at this? Feel free to use the branch or cherry pick the additional commits.

@mbien could you check if that addresses some of you concerns? We can discuss whether nbjavac is really a good idea, but at this point in time it is an integral part of the core of Java editing in NetBeans and I think it might help catching bugs.

Edited: Updated the final commit and switch label from Java to nb-javac.

neilcsmith-net commented 1 year ago

Thanks @matthiasblaesing Couple of comments, in priority order.

Only run the nbjavac test job when the Java label is added to the PR.

Can we please add a nb-javac label and only run the test on that, not every Java labelled PR as per https://github.com/apache/netbeans/pull/5609#issuecomment-1476006761

loadClass is not synchronized and does not synchronize access to the caching HashMap. Instead of introducing synchronization, use a thread safe ConcurrentHashMap.

Also use computeIfAbsent()?

matthiasblaesing commented 1 year ago

Thanks @matthiasblaesing Couple of comments, in priority order.

Only run the nbjavac test job when the Java label is added to the PR.

Can we please add a nb-javac label and only run the test on that, not every Java labelled PR as per #5609 (comment)

Label created, added here and updated the final commit (and the comment summarizing the changes).

loadClass is not synchronized and does not synchronize access to the caching HashMap. Instead of introducing synchronization, use a thread safe ConcurrentHashMap.

Also use computeIfAbsent()?

Decided against it. findClass throws a checked exception. The mapping function then become more complex than the current straight implementation.

mbien commented 1 year ago

@mbien could you check if that addresses some of you concerns? We can discuss whether nbjavac is really a good idea, but at this point in time it is an integral part of the core of Java editing in NetBeans and I think it might help catching bugs.

@matthiasblaesing github workflows can't really trigger on particular labels unfortunately. I believe the easiest way to build using nb-javac when the label is set would be to simply paste the proposed job into the main.yaml.

copy this: https://github.com/matthiasblaesing/netbeans/blob/623a396c8ed341834d080eb7f4a1d2aa487e6b87/.github/workflows/nb-javac.yml#L6-L28 (rename linux to nb-javac for example, since that would be the job handle)

and paste it somewhere below base-build in main.yaml

having it as separate workflow is fine in theory, but it would startup and do nothing most of the time and create empty runs on that list: https://github.com/apache/netbeans/actions

i would also recommend to remove the || github.event_name != 'pull_request' and ci:all-tests conditions so that it doesn't have to run outside of PRs which set the label.

The cache should be used too so that we don't DDoS someone:

  - name: Restore Cache
    uses: actions/cache/restore@v3
    with:
      path: ~/.hgexternalcache
      key: ${{ runner.os }}-${{ hashFiles('*/external/binaries-list', '*/*/external/binaries-list') }}
      restore-keys: ${{ runner.os }}-

this should only restore the cache based on hash match or prefix as fallback but never store, other then that it works analog to the base-build job. (never used the restore action before, its a fairly new feature they added earlier this year)

For the record: I have nothing against nb-javac and do agree it is an important dependency of the java editor right now. However, the testing of nb-javac should be done in the nb-javac repository, not in a downstream repository in my opinion. What should we even do if that one job is red? Not merge a PR? Wait for nb-javac to fix something, upload a binary to central and test again?

matthiasblaesing commented 1 year ago

@mbien was this what you had in mind: https://github.com/apache/netbeans/pull/5724/commits/8745ca5b958f2246ae68a552b0b0b0b619badda0?

A test run is currently executing here: https://github.com/apache/netbeans/pull/5724

mbien commented 1 year ago

@mbien was this what you had in mind: 8745ca5?

A test run is currently executing here: #5724

yes almost :) I commented inline, I hope you can see it.

jtulach commented 1 year ago

Thank you for your comments. First and foremost: This PR doesn't change anything on the way NetBeans is built by default. An opt-in is needed to turn the functionality on. Description of the PR has been updated to highlight that.

Thanks @matthiasblaesing for your changes in https://github.com/apache/netbeans/pull/5609#issuecomment-1484133563 - I've accepted all of the ones for CustomJavac.java file. I haven't accepted anything in the GitHub Actions area - there are some mention of Rust and I got confused/scared without knowing what to take and what not? Let's make it subject of further discussion.

I have two approvals. @neilcsmith-net wrote:

If this change helps with testing upstream, it could go in.

@matthiasblaesing helped me polish this PR greatly - see 42ed77a. @mbien wrote:

it is ok to add a few lines to the ant task if they make it easier for the nb-javac project to run CI tests

I believe we are all able to accept this opt-in support for nb-javac compiler executed from its JARs. I am removing the "do not merge" label and I'd like to integrate my change. Once it is in, the https://github.com/JaroslavTulach/nb-javac/pull/17 will go in as well. Further improvements (CI, discovery of nb-javac, removal of -Xlint:classfile, etc.) to this PR are of course possible.

jtulach commented 1 year ago

Latest GitHub Actions run detected a violation in JDK API usage. java/java.j2seplatform claims support for JDK8 by having javac.source=8 (which implies javac.target=8) and yet it was using JDK9 API. Fixed in 8018019 - @mbien please review for code correctness.

neilcsmith-net commented 1 year ago

@jtulach see #5748

jtulach commented 1 year ago

I've just squashed all the commits into single one a44b3ed to remove noise from the history.

matthiasblaesing commented 1 year ago

@jtulach thank you for considering the suggestions and for the separation of the classloader. That indeed looks better readable.

For the modifications to the CI/CD script, I'd like to try to explain what happens there:

We have several tests, that are run only if certain labels are set on the corresponding PR. The idea here is, that github and Apache can't provide infitite processing power and we need to use it were it counts. Two big blocks are PHP and full java tests. Both run in the range of an hour and should not be run on all PRs, just PRs with the label PHP or Java.

We had already a split of the CI/CD runs into separate parts to allow parallel execution and @mbien added protection to the execution blocks, that checks whether the label is set:

https://github.com/apache/netbeans/blob/e1b594034bdce520dbfbc720a6cd7b7b894fc756/.github/workflows/main.yml#L791-L794

This block says:

The platform-modules-test1 job is only run if the label Platform or ci:all-tests is set. Additionally this will be run on non-PR events, for example the merge to master (the last part of the if).

Yes you see rust in the full CI/CD configuration, as the first steps for rust support were added by @vieiro, but are only build by the full cluster configuration, not by the release config. The line you referenced declares a variable test_rust, nothing more, apart from that it is identical to the guard shown above for Platform.

I hope the clears the idea a bit.

I just saw your comment over in my "test" PR:

Is it wise to run the job only when a special label is added? We wouldn't catch https://github.com/apache/netbeans/pull/5748 so quickly without running this job.

I'm in a yes/no situation. I agree, that the nbjavac run was helpful to catch this. On the other hand I can understand @mbien concerns regarding false positive failures from the nbjavac run. A valid middle ground may be, to change this:

https://github.com/apache/netbeans/blob/3027e4274aaa0e8a40c1ab700db67b954d76784c/.github/workflows/main.yml#L162

to:

    if: ${{ contains(github.event.pull_request.labels.*.name, 'nb-javac') || contains(github.event.pull_request.labels.*.name, 'ci:all-tests') || github.event_name != 'pull_request' }}

This is a compromise for both sides: On the one hand the nbjavac job will only be run on specially labeled PRs (nb-javac or ci:all-tests). On the other hand it will be run on merges to master (at least that is my understanding). So normal PRs will not yield more runtime, yet if the unlikely (it happend twice now) case happens, that an API is used which does not match the declared JDK, it can still be caught. The price is IMHO ok, my notebook needs 9 minutes for the NetBeans build and 3 minutes for the commit-validation run, on server class hardware I assume less.

@mbien @neilcsmith-net @jtulach what do you think?

neilcsmith-net commented 1 year ago

@matthiasblaesing agreed to running with same logic as most other labels for now (nb-javac or ci:all-tests labelled PRs, plus non-PR runs). Let's see how that goes - false positive failures or resource issues and we pull the non-PR tests during the release.

btw - there wasn't an "approval" from me, just not standing in the way as long as it doesn't cause issues. There are certainly other tests of nb-javac that I think would be more valuable.

jtulach commented 1 year ago

This PR has nb-javac label and I can see NetBeans on nb-javac (pull_request) job has been executed with 91951b2 changes. Good.