Option to skip CoS copying of class files when the target is already newer, such as from an external compilation

[jglick]

Related to #227791 and linked issues. This is a robustness fix: in NetBeans currently, when the built-in compiler gets confused during classpath scanning and writes out broken class files due to various subtle bugs, as happens pretty frequently in my experience on big complex projects, CoS-mode execution (e.g., running of individual unit tests in quick mode) is broken because the test JVM loads bytecode full of errors (“uncompilable source code” etc.). This is irritating because the errors are almost always from classes deep in the dependency tree and unrelated to what you are actually editing and testing, yet the problem blocks you from using CoS mode and so you must fall back to much slower full builds.

With this patch, NetBeans will just trust the external build tool preferentially, based on *.class timestamp. Thus, if you notice some error during a CoS test case run which was caused by an IDE problem, you have a simple workaround: just do a command-line rebuild of the module containing the class mentioned in the error. You can still make edits to unrelated (source or test) files and rerun the test to pick up those changes. I have been running with this patch for over a year and it seems to work well; I no longer curse in frustration at nbjavac bugs, just note them and move on.

java.source.base is giving me 29 test failures after 20m (!) run time, but I picked one at random and confirmed that it was also failing in master, so at least that one is not a regression. When was the last time mvn -f whatever.module test actually worked?

[JaroslavTulach]

Robustness is needed. Btw. @jlahoda provided me with a patch that works the other way around: e.g. it can adopt the existing .class files internally. That would be useful enhancement as well.

If this change doesn't clash with the future copy instead of parse enhancement, let's go for it.

[eirikbakke]

just do a command-line rebuild of the module containing the class mentioned in the error

I assume this also works with maven builds invoked via "Clean and Build" from the project context menu, rather than from the command-line?

Thanks for working on CoS robustness! One of my big annoyances with NB9 has been when NetBeans erroneously marks source files as having errors. Sometimes a clean build has not been enough to resolve this--I've had to restart NetBeans or even delete the cache directory. If a clean build can now resolve these cases, it makes many other bugs less serious.

[jglick]

a patch that works the other way around: e.g. it can adopt the existing .class files internally

If that works, it would be great, as you would be able to clear bogus error badges too. They might reappear after some kinds of edits, but doing a CLI build is a natural gesture to fix that.

I assume this also works with maven builds invoked via "Clean and Build" from the project context menu, rather than from the command-line?

Yes, that is what I normally do.

Sometimes a clean build has not been enough to resolve this--I've had to restart NetBeans or even delete the cache directory.

Yes, I have done the same on many occasions.

If a clean build can now resolve these cases, it makes many other bugs less serious.

The intent is indeed to mitigate the impact of other bugs—they still need to be fixed once reproduction steps can be nailed down.

To be clear, this patch (unlike the one supposedly developed by @jlahoda) does not fix anything from the IDE’s perspective. You may still have error badges, broken code completion, that sort of thing. Its sole purpose is to allow CoS actions (typically running or debugging a single test case) to work, under the assumption that the erroneous classes have no particular relationship to what you are actually working on—the test and the classes being tested—other than that some of the code marked as erroneous happens to get run somewhere deep in the call stack.

As an example: if I am working in jenkinsci/jenkins on some test that creates a Jenkins project with paraneters and then runs a build step which I just wrote, to see if the build step properly handles parameterized variables, the files I am likely to be editing iteratively are core/src/main/java/pkg/SomeBuildStep.java and test/src/test/java/pkg/SomeBuildStepTest.java as I work out the kinks. In the course of a test run, hundreds of other classes will get loaded and used, including for example Job (superclass of all projects) and ParametersAction (captures parameters passed to one project build). Yet before https://github.com/jenkinsci/jenkins/pull/2595 was merged, certain versions of NetBeans would mark both those sources as erroneous, even though mvn compile using JDK 8 worked fine. Without this patch, I would be unable to use CoS mode, and would need to fall back to a shell:

mvn -pl core,test -Dtest=SomeBuildStepTest#smokes -DfailIfNoSpecifiedTests=false test

which runs all sorts of unrelated goals and is much slower.

[jlahoda]

On Tue, May 8, 2018 at 3:57 AM, Jesse Glick notifications@github.com wrote:

@jglick commented on this pull request.

In java.source.base/src/org/netbeans/modules/java/source/usages/ BuildArtifactMapperImpl.java https://github.com/apache/incubator-netbeans/pull/504#discussion_r186599280 :

• LOG.log(Level.FINER, "#227791: ignoring non-file-based source {0}", approximateSource);
• return false;
• }
• if (!target.isFile()) {
• LOG.log(Level.FINER, "#227791: {0} does not even exist", target);
• return false;
• }
• long targetLastMod = target.lastModified();
• File mockSrc;
• try {
• mockSrc = BaseUtilities.toFile(approximateSource.toURI());
• } catch (URISyntaxException x) {
• LOG.log(Level.FINER, "#227791: cannot convert " + approximateSource, x);
• return false;
• }
• File src = new File(mockSrc.getParentFile(), mockSrc.getName().replaceFirst("([$].+)*[.]sig$", ".java"));

-uncomment the line in CosTest, wait 5+seconds -go to Dep, uncomment the method in Dep

Are you saving the sources here, or letting the save be done by F6? If the latter, that does sound like a race condition here: you would want to let all newly saved sources be scanned

Yes, I am saving, sorry for not being explicit. The steps would be:

-do a clean build -(CoS) run -uncomment the line in CosTest, save CosTest, wait 5+seconds -go to Dep, uncomment the method in Dep -save Dep, wait a few seconds, (CoS) run

into sig files before the run starts, which the IDE normally ensures, and perhaps that is getting broken by this change? Or if the former (manual saves in reverse order), then, well, do not do that. Either way, you can simply touch CosTest again to work around.

So the users need to do dependency tracking themselves. So far, this was IDE's job.

[JaroslavTulach]

I am not following your discussions fully, but the picture I got goes again to the question whether NetBeans is an IDE for DevOps (e.g. those who understand build system) or normal developers that only want to use compile on save...

[jglick]

users need to do dependency tracking themselves

I guess? Really it just seemed reflexive that if you were bothering to explicitly save at all, you would save the upstream part which defined an API before working on, and saving, the downstream part which called it. But maybe there is some way to salvage this situation if people rely on it; I would have to study it.

At any rate, I like the idea of stripping the sig files down to, well, signatures and putting the CoS functionality into a more superficial layer. Sounds like it would be more intuitive and predictable: the IDE would just be performing the final compilation of recently modified sources not covered by the last external build. Deserves a prototype, certainly a bigger project than this PR which was intended as a stopgap measure.

[jglick]

I realized that in the case of Maven there is a way to combine CoS-like recompilation and test run (not sure about main class run) into a single tool execution: create an ephemeral POM in a temp dir listing the open projects with snapshot relationships as <modules>, then

mvn resource:copy compiler:compile surefire:test -f /tmp/pom123.xml -DignoreMissingTests -Dtest=MyTest#case

or similar. Compared to using javax.tools.JavaCompiler this is a bit safer still, since even POM misinterpretations by the IDE would only affect editor functions, not test runs; and I think the overhead would be modest. (Maven CoS already runs mvn.)

TBD what is best for (managed or freeform) Ant- or Gradle-based builds, but this reinforces the notion that the project type can keep CoS functionality closer to the build system.

[geertjanw]

Any reason not to merge this? Or what is the current status of this and the way forward?

[jglick]

While I continue to find the behavior in this patch preferable on balance to the stock behavior, it is sufficiently controversial that it should just be considered on hold until I can find time to spend on a more ambitious alternate approach. I did not see an “experimental” label or similar, so just using the closest applicable label.

[eirikbakke]

I have been running with this patch for a couple of weeks now. I think it has removed a lot of previously very annoying errors, or at least saved me from doing a complete clean build every time they occur ("Annotation: An error occurred during parsing", "Absent Code attribute in method", "The bytes do not represent a valid class" etc.)

Does this patch impact "Apply Code Changes" in any way? (That feature has always been very flaky... yet it is so incredibly useful that I use it anyway.)

[jglick]

Does this patch impact "Apply Code Changes" in any way?

Not sure. I do use that feature (and also find it flaky).

[eirikbakke]

It would be really nice to get this patch integrated. It makes a huge difference. Here's my experience:

After upgrading from NetBeans 8 to NetBeans 9, my multi-module NetBeans platform maven project would constantly need to be recompiled after errors like these:

• java.lang.ClassFormatError: Duplicate method name&signature in class file
• ClassFormatError: Absent Code attribute in method that is not native or abstract
• NoClassDefFoundError: Could not initialize class

I then applied this patch in September, on top of NetBeans 9, and had no more problems with these errors.

When NetBeans 10 was released, I started using it without the patch for two weeks. The errors above started appearing daily again. It was a huge annoyance. I'm have now applied the patch again, and the errors are gone.

(More usage background: I always use Compile-on-Save, and often also Apply Code Changes in the debugger. In my own project, the "start state" is always a clean maven build--but with the patch applied, I can go for several days without needing to do a clean build again. And when I do run a clean build, it's usually because I need to for an unrelated reason.)

While I continue to find the behavior in this patch preferable on balance to the stock behavior, it is sufficiently controversial that it should just be considered on hold until I can find time to spend on a more ambitious alternate approach.

What is the main "controversial" aspect of this patch? Are there downsides to it?

[geertjanw]

Well, there are conflicting files now. Once they're fixed, since you're a committer, you should be able to commit this.

[eirikbakke]

The conflicts are just due to the java.source.base and maven modules being moved into the top-level "java" folder.

I've tested the patch, but not reviewed the actual code (it would require a cup of coffee and a spare hour sometime--I'm not familiar with CoS inner workings). If someone else already did review this, they'd be in a better position than me in determining whether it's safe to apply the patch or not.

[jglick]

Are there downsides to it?

Probably yes, depending on your usage; there is a long and somewhat technical discussion previously in the PR. Reviewing the code patch in isolation is probably not meaningful without understanding the history of design trade-offs in NetBeans’ Java “compile-on-save” mode.

[eirikbakke]

I have now started using NetBeans 11 as my daily development IDE. As before, without this patch applied, I get many of the previously mentioned errors (ClassFormatError etc.). When I apply this patch, those errors go away.

Unfortunately, even with this patch applied, I now very frequently have other problems with Compile-on-Save, e.g. symbols that are not indexed (for "Find Usages" or "Go to Symbol"), red error badges on files that won't go away unless I turn CoS off and do a clean build etc. I am imagining that these latest errors were not as frequent in NetBeans 10 (with the patch in this PR applied).

I see that Jaroslav's patch https://github.com/apache/netbeans/pull/1123 was merged into NetBeans 11. How does that patch relate to the one in this PR? Is it meaningful to apply them both in the same build?

[jglick]

even with this patch applied, I now very frequently have other problems with Compile-on-Save, e.g. symbols that are not indexed (for "Find Usages" or "Go to Symbol"), red error badges on files that won't go away

Well there have been various bugs since the inception of the Java parser. This PR merely attempts to make them not be fatal to CoS execution.

I had not seen #1123 until now, though I suppose it grew out of https://github.com/apache/netbeans/pull/504#issuecomment-381851991. Clearly it is related, but I think independent. This (#504) patch is applicable to any Java project, not a specific build technology, and should only affect the behavior of CoS execution. #1123 actually switches what the editor considers to be the authoritative source of dependencies for various purposes. If I understand it correctly, it would be similar to what for example the Maven project type already did when it detects that the Shade plugin is in use: bypass the normal binary → source linkage and just trust the build product instead.

Much as I loathe to do so, I wonder if this is important enough to integrate as a user option (off by default). I recall that there used to be some GUI in the global Options dialog to select different modes of CoS, but I cannot find it now. There is just the per-project Compile on Save checkbox.

[eirikbakke]

I am tempted to try to dive into the CoS logic and see if I can help fix, or at least make reproducible, some of the bugs that occur intermittently in my own workflow.

The problem is, if I'm running NetBeans with non-standard CoS logic enabled, it will be hard for me to understand whether certain behaviors apply to the standard NetBeans distro, or whether they are caused by the patch. Thus debugging becomes harder--doubling the testing surface, so to speak. It would be a lot easier if CoS worked in one "official" way.

At the moment, NetBeans is hard to work with without this patch enabled, and so any debugging efforts I do now would apply to "NetBeans-with-Jesse's-patch-applied" rather than the vanilla NetBeans 11.0. And I will need to use NetBeans as a working IDE over several days in order to start narrowing down these intermittent bugs.

[jglick]

There are certain limitations that are just going to be inherent in the mode proposed in this patch. For example, if you refer to a nonexistent method in a caller class, save it, then define the method in the API class, then run a test, you will get an error. (Worked around by just going back to the caller and resaving it.) But these should be pretty deterministic. I suspect that the intermittent bugs you are talking about are not bugs in CoS per se, but general bugs in the Java parser, which this patch would not affect except insofar as it may prevent them from breaking CoS execution.

Certainly if you can track down any of these with reproducible test cases, the NetBeans community would be graceful. I do see obvious bugs in the form of uncaught exceptions from javac from time to time, but historically just reporting the stack trace has not always led to a resolution: the response was often that nothing could be done without a test case. In principle the reaction should be to figure out what critical diagnostic information was missing from the stack trace and improve the code somewhere in that stack to do better next time, whether with a richer exception message, earlier assertions, etc.

The same may apply to situations which clearly represent a bug in the product, such as the editor tab displaying an error badge but the editor pane not showing any specific errors. One thing that I have long wished for is a simple UI gesture to tell the IDE to throw out all its Java parser caches for a given source root and start over—sometimes deleting the var/cache/ directory and restarting NetBeans fixes a problem, but this is rather drastic as it forces everything to be rebuilt.

[JaroslavTulach]

I had not seen #1123 until now, though I suppose it grew out of #504 (comment).

True.

bypass the normal binary → source linkage and just trust the build product instead.

Right.

[eirikbakke]

One thing that I have long wished for is a simple UI gesture to tell the IDE to throw out all its Java parser caches for a given source root and start over

I'd like to start working on this feature. Is resetting the parser cache a trivial method call somewhere, or is there more to it that I need to investigate?

(Things like "Find Usages" not working properly are driving me mad... I rely on it heavily when reviewing my own code. Currently, "Find Usages" (and "Go To Symbol", and presumably "Refactor Rename" etc.) will silently omit results at random, only to display them again if I go to the Java file in question, insert a space, and invoke Save to force CoS. Again, this is with the patch in this PR applied, on top of NetBeans 11.0.)

[lkishalmi]

@eirikbakke How this patch serves? (I guess you run your own custom built NetBeans). Also to @jglick @JaroslavTulach and @jlahoda shall this one be closed or integrated after all? Honestly, I have not read all the comments you've made on this.

[eirikbakke]

@lkishalmi I have used this patch for two years. Every time a new NetBeans release comes around, I first try developing for some hours without the patch. Compile-on-Save is completely unusable without it on my particular project (which is a multi-module NetBeans Platform maven project).

I'm not sure what the best way forward is, since according to @jglick's comment above, this patch cannot just be applied in every case.

Even with this patch, the NetBeans index still frequently gets corrupted, with Find Usages, Go to Symbol, Refactor etc. not finding every case, and error badges showing for files without errors. Existing NetBeans, deleting the "index" directory in the NetBeans cache directory, starting NetBeans again, and waiting for a complete reindex is now unfortunately part of my daily routine.

[jglick]

I modified this patch to turn off the new behavior by default, enabled only via system property. That should make it safer to merge. Whether this should be promoted to a GUI option is another question.

[eirikbakke]

Good call with the system property. That prevents the patch from being lost, and shows more clearly what the difference in behavior is.

[jglick]

@eirikbakke if you can spare a bit of time, please try running with this patch with and without -J-Dorg.netbeans.modules.java.source.usages.BuildArtifactMapperImpl.COMPARE_TIMESTAMPS=true to verify that it continues to solve your CoS issues. (Yes the index can still get corrupted, leading to breakage of editor features.)

[eirikbakke]

Will do.

[eirikbakke]

Sorry for the delay... I'll be testing this patch in my working IDE for a few days now, with COMPARE_TIMESTAMPS first disabled and then enabled, to verify the latest behavior.

[eirikbakke]

@jglick could you remind me how to configure NetBeans to show the various FINE log messages in BuildArtifactMapperImpl? I want to convince myself that I am successfully managing to turn this feature on and off...

[jglick]

how to configure NetBeans to show the various FINE log messages in BuildArtifactMapperImpl

I used the org.netbeans.modules.logmanagement contrib module to display logs in the Services tab. There are probably other ways.

[eirikbakke]

Can confirm that the patch still works, and that it can be turned on or off. I now also finally understand what it does...

With the patch included in the IDE but without the COMPARE_TIMESTAMPS setting, I pretty quickly get the old "Absent Code attribute in method that is not native or abstract" exceptions that I've become used to in the past three years of NetBeans releases (reported on https://issues.apache.org/jira/browse/NETBEANS-430 ). When enabled (with "-J-Dorg.netbeans.modules.java.source.usages.BuildArtifactMapperImpl.COMPARE_TIMESTAMPS=true"), these exceptions go away except when the error is in the same class as is being edited. I've also examined the log messages to confirm that the functionality can be turned on and off.

Also looked at a diff between the old and the new version of the patch, and confirmed that it's just cleanup and the system property test.

(Figured the log part out... added "-J-Dorg.netbeans.modules.java.source.usages.BuildArtifactMapperImpl.level=100" to netbeans_default_options in netbeans.conf to get the messages showing.)

I think this patch should be safe to merge.

Thanks for this!

[lkishalmi]

It still can go into 12.2 if you able to rebase it on delivery branch by tomorrow EOB (PDT)

[lkishalmi]

oh and document the system property in the arch.xml

[eirikbakke]

No need to rush this patch into 12.2 as far as I'm concerned; it's a feature that's off by default and which has to be enabled by a command-line flag. It's just good to get it merged eventually so that the code isn't lost.