Cleanup code base #33
Conversation
| @@ -5,7 +5,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| strategy: | |||
| matrix: | |||
| java: [9, 10, 11, 12, 13] | |||
| java: [11, 17, 18, 19, 20] | |||
There was a problem hiding this comment.
Java 8 is still getting security updates until 2030 and is used in some companies. We should ideally support it, if possible.
There was a problem hiding this comment.
Why does it fail to build with Java 9?
There was a problem hiding this comment.
Java 8 is still supported for consumers of this library (see https://github.com/kichik/pecoff4j/blob/master/pom.xml#L116), but not for building, because it includes JPMS support (module-info.java), which Java 8 does not understand.
Building with Java 9 would work, but this version is not supported and should not be used anymore. That's why I removed it. My suggestion would be to reflect that in the pom.xml as well, and set the minimal version to build it to Java 11. Again, the output artifact is still compatible with Java 8 (byte code version and JDK API baseline). IMO, that would be "good enough".
If we don't trust the Java compiler checking the JDK compatibility properly, we would have to abandon the multi-release jar approach. We could separate the different Java versions e.g. by using Maven profiles and release multiple artifacts (pecoff4j-0.4.1-jdk8, pecoff4j-0.4.1-jdk11). I'm not sure whether it's worth the effort...
There was a problem hiding this comment.
Thanks for explaining. Would it be possible for us to build on 11 but also test on 8 automatically here? I assume we would need need a separate job and an artifact passed around, but hopefully there is a simpler solution.
There was a problem hiding this comment.
There seems to be no nice and easy solution for this. https://stackoverflow.com/questions/70541340/setting-up-multi-release-jar-unit-tests provides some insights. I'd suggest to drop the corresponding commit from this PR (as it does not fit the cleanup topic, anyway) and create a dedicated ticket for that issue. OK for you?
There was a problem hiding this comment.
Separate PR is fine. I don't think there is a need to drop it since it wasn't testing on 8 before either, right?
There was a problem hiding this comment.
Not being able to build the project with Java 8, but nevertheless wanting to test it with this version makes it very complicated. I therefore decided to add building support for Java 8 again using a dedicated profile. It's not a perfect solution as we are not testing against the multi-release JAR, but the main goal should be to test the code base for Java 8 compatibility (and have confidence that maven and javac work as expected). I hope that you are OK with this pragmatic solution as well...
| public class RCEdit { | ||
| public static void main(String[] args) throws Exception { | ||
| launch(new String[0]); | ||
| launch(new String[] { "/I", "test/WinRun4J.exe", "test/eclipse.ico" }); |
There was a problem hiding this comment.
Do we have a test to replace this?
There was a problem hiding this comment.
Sadly, this class is only a stub. The printUsage method documents what the authors of WinRun4J planned to achieve and some methods actually load the executable, but do nothing more.
When I discovered this library, I was also disappointed when realizing that it does not hold its promises... ;-)
As announced in the previous PR, I did some cleanups (remove empty classes, separate unit tests from experiments (and remove some of them), remove unused test resources).
Some comments:
println(opcode);inAssemblyParser, because it uses some reflection that is not allowed anymore and was rather for debugging purposes.