Solve #140 #142

xamde · 2022-03-09T11:46:33Z

Explicitly setting the PLATFORM_CLASSPATH seems key for proper shading of native JRE with emulate JRE classes.
However, this shading is not desired, when compiling dependencies. So a switch was needed, depending on who calls Javac.

Fixes #140

niloc132 · 2022-03-09T18:07:02Z

Thanks for this - can you add an integration test with the sample class that demonstrates the issue, and reduce the logging to DEBUG (as users generally won't need to see this)?

There's a good chance that this isn't going to be the long-term fix for this, but it will depend on how good j2cl's error messages are for members/classes that don't exist, but I think this will be good for now.

niloc132 · 2022-03-09T18:07:57Z

j2cl-maven-plugin/src/main/java/com/vertispan/j2cl/mojo/AbstractBuildMojo.java

@@ -255,6 +255,7 @@ private Project buildProject(MavenProject mavenProject, Artifact artifact, boole
                        throw new ProjectBuildingException(p.getId(), "Failed to resolve this project's artifact file", e);
                    }

+                    getLog().info("Building dependency "+p.getGroupId()+":"+p.getArtifactId()+":"+p.getVersion());


So it doesn't help your infinite loop problem, but there is already debug logging at line 99 that draws a rough tree diagram of the "dependency graph"

i think you can remove this? or at the very least demote it to debug, but then we still have duplicate log lines with line 99

If the build hangs, it won't print the tree later on. I've put it behind isDebugEnabled and set it to log.debug.

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Javac.java

xamde

Two new IT tests test the behaviour with JRE emulation.
In "-verbose" mode, javac shows where code is loaded from. If loaded from some /798A/...sig file, its from JDK implementation. If it lists some other path containing /java.base/ in the path and does not end in .sig, it's loaded from the JRE emulation.

The exact parameters to make set up the classpath in the right order are a bit tricky and differ completely between Java < 9 and Java >= 9. But I seems to work now.

Would be happy to get this PR in one of the next plugin versions.
Tests green.

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Javac.java

niloc132

Preliminary review, will take a deeper dive this next week.

niloc132 · 2022-03-10T13:40:48Z

...t/shared-lib-externalizable-reactor/lib/src/main/java/example/sharedlib/GwtIncompatible.java

+@Retention(RetentionPolicy.CLASS)
+@Target({ElementType.TYPE, ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.FIELD})
+@Documented
+public @interface GwtIncompatible {


It is safe to depend on gwt-core to get this dependency

Also, you don't need to copy the entire class, only the name and retention is necessary, not the value() method nor target, documented.

I slimmed down the annotation class, but would prefer to keep using a small hand-written GwtIncompatible class as this a simpler setup, e.g. no additional dependency which needs to be migrated from google to gwtproject and version upgrades.

build-caching/src/main/java/com/vertispan/j2cl/build/task/BuildLog.java

niloc132 · 2022-03-26T20:06:54Z

j2cl-maven-plugin/src/it/shared-lib-externalizable-reactor/pom.xml

+        <module>app</module>
+    </modules>
+
+    <repositories>


unnecessary, please remove

niloc132 · 2022-03-26T20:07:26Z

j2cl-maven-plugin/src/it/shared-lib-reactor/app/src/main/webapp/WEB-INF/web.xml

@@ -0,0 +1 @@
+<web-app></web-app>


add trailing newline

ok, but why?

It is a git convention, but I didn't realize that until you asked that it is actually a C convention that git holds on to. Basically, from looking at a diff or a file in the file viewer on github, you can't tell if a file ends in a newline or has a blank line after it, so git always flags files that don't end in newlines with red. In C, it is apparently required to end with a newline, so git always encourages it.

https://stackoverflow.com/a/5813359/860630

j2cl-maven-plugin/src/it/shared-lib-reactor/app/src/main/webapp/index.html

niloc132 · 2022-03-26T20:07:42Z

j2cl-maven-plugin/src/it/shared-lib-reactor/pom.xml

+        <module>app</module>
+    </modules>
+
+    <repositories>


unnecessary

This would be our only reactor test with a pure Java shared lib -- which is my main project setup (just 30 libs instead of one). I removed the "reactor" part from the "externalizable" test, but would like to keep this one reactor-ized.

My comment might have been unclear - only the tag is unnecessary, along with its contents.

niloc132 · 2022-03-26T20:08:30Z

j2cl-maven-plugin/src/main/java/com/vertispan/j2cl/mojo/AbstractBuildMojo.java

@@ -255,6 +255,7 @@ private Project buildProject(MavenProject mavenProject, Artifact artifact, boole
                        throw new ProjectBuildingException(p.getId(), "Failed to resolve this project's artifact file", e);
                    }

+                    getLog().info("Building dependency "+p.getGroupId()+":"+p.getArtifactId()+":"+p.getVersion());


i think you can remove this? or at the very least demote it to debug, but then we still have duplicate log lines with line 99

niloc132 · 2022-03-26T20:09:44Z

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Javac.java

+        fileManager = compiler.getStandardFileManager(listener, null, StandardCharsets.UTF_8);
+        javacOptions = new ArrayList<>(Arrays.asList("-encoding", "utf8", "-implicit:none"));
+        if (log.isDebugEnabled()) {
+            javacOptions.add("-verbose");


Hmm, I don't care for this - it means that the cached logs will stay non-verbose, even if you do a verbose log execution.

Instead it would be nicer to just not write the logs out if we can, but always ask for them?

Right, so I leave the "-verbose" here always on? It helped a lot in debugging. On the other hand, the build certainly runs faster without it. Ideally, we would run mvn with "-X" and have a detailed log and run without "-X" and have a fast build.
Hmm. Leaving it in, for now.

niloc132 · 2022-03-26T20:10:42Z

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Javac.java

-            //java 9+
-            javacOptions.add("--release=8");
+            // Java version >= 9
+            //javacOptions.add("--release=8");


if the line isn't used, please remove it

Why was it there? release=8 is incompatible with --add-reads, therefore I had to remove it. But maybe setting release=8 had some other intention? If I understood j2cl compiler correctly, it reads the sources of the current project and the classes (type descriptions) from Java dependencies. So what is the highest Java class format it can read? What trouble could be caused by removing release=8?

From memory, this had to be added so that changing the current jvm wouldnt invalidate your cache - without this, a v17 javac will emit java 17 bytecode, and if you then switched to java 11, you would no longer be able to compile against that code. By forcing all compilers to build bytecode to the only currently supported version, we don't have to think farther than "does it build", and don't have to key the cache entries with details about your compiler.

--

If there we aren't somehow solving that any more, we need to pass in the javac version as part of the cache key to avoid this problem. I'll test this PR our to be sure we don't break in this case.

niloc132 · 2022-03-26T20:15:20Z

In general I am a bit concerned about this complexity - we might need to test some caching a little more in depth to be sure that changing the JVM doesn't produce unusable code or something. With that said, this entire branch should go away if #150 pans out, since we won't use javac at all to produce gwtincompatible-stripped bytecode for j2cl any more.

If #150 does work out, we will still have these integration tests to ensure that we don't lose this functionality, even if we do end up tossing the code.

xamde

Ok, 2nd iteration done.

xamde mentioned this pull request Mar 9, 2022

Is stripped bytecode using the right JRE copy? #140

Open

niloc132 reviewed Mar 9, 2022

View reviewed changes

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Javac.java Outdated Show resolved Hide resolved

niloc132 added this to the 0.20 milestone Mar 9, 2022

xamde commented Mar 26, 2022

View reviewed changes

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Javac.java Outdated Show resolved Hide resolved

niloc132 reviewed Mar 26, 2022

View reviewed changes

xamde commented Mar 27, 2022

View reviewed changes

niloc132 removed this from the 0.20 milestone Jul 14, 2022

xamde closed this Jan 12, 2023

xamde force-pushed the main branch from 350d7a8 to 174ae56 Compare January 12, 2023 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solve #140 #142

Solve #140 #142

xamde commented Mar 9, 2022 •

edited by niloc132

Loading

niloc132 commented Mar 9, 2022

niloc132 Mar 9, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022 •

edited

Loading

xamde left a comment

niloc132 left a comment

niloc132 Mar 10, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022

niloc132 Apr 7, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022

niloc132 Apr 7, 2022

niloc132 Mar 26, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022

niloc132 Mar 26, 2022

xamde Mar 27, 2022

niloc132 Apr 7, 2022

niloc132 commented Mar 26, 2022 •

edited

Loading

xamde left a comment

		@@ -0,0 +1 @@
		<web-app></web-app>

Solve #140 #142

Solve #140 #142

Conversation

xamde commented Mar 9, 2022 • edited by niloc132 Loading

niloc132 commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xamde Mar 27, 2022 • edited Loading

Choose a reason for hiding this comment

xamde left a comment

Choose a reason for hiding this comment

niloc132 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niloc132 commented Mar 26, 2022 • edited Loading

xamde left a comment

Choose a reason for hiding this comment

xamde commented Mar 9, 2022 •

edited by niloc132

Loading

xamde Mar 27, 2022 •

edited

Loading

niloc132 commented Mar 26, 2022 •

edited

Loading