Change decompiler from quiltflower to vineflower #35610
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -603,17 +603,17 @@ private JarBuildItem buildThinJar(CurateOutcomeBuildItem curateOutcomeBuildItem, | |
| Path decompiledOutputDir = null; | ||
| boolean wasDecompiledSuccessfully = true; | ||
| Decompiler decompiler = null; | ||
| if (packageConfig.vineflower.enabled) { | ||
| decompiledOutputDir = buildDir.getParent().resolve("decompiled"); | ||
| FileUtil.deleteDirectory(decompiledOutputDir); | ||
| Files.createDirectory(decompiledOutputDir); | ||
| if (packageConfig.vineflower.enabled) { | ||
| decompiler = new Decompiler.VineflowerDecompiler(); | ||
| Path jarDirectory = Paths.get(packageConfig.vineflower.jarDirectory); | ||
| if (!Files.exists(jarDirectory)) { | ||
| Files.createDirectory(jarDirectory); | ||
| } | ||
| decompiler.init(new Decompiler.Context(packageConfig.vineflower.version, jarDirectory, decompiledOutputDir)); | ||
| decompiler.downloadIfNecessary(); | ||
| } | ||
| } | ||
|
|
@@ -1572,15 +1572,15 @@ public Context(String versionStr, Path jarLocation, Path decompiledOutputDir) { | |
|
|
||
| } | ||
|
|
||
| class VineflowerDecompiler implements Decompiler { | ||
|
|
||
| private Context context; | ||
| private Path decompilerJar; | ||
|
|
||
| @Override | ||
| public void init(Context context) { | ||
| this.context = context; | ||
| this.decompilerJar = context.jarLocation.resolve(String.format("vineflower-%s.jar", context.versionStr)); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -1589,7 +1589,7 @@ public boolean downloadIfNecessary() { | |
| return true; | ||
| } | ||
| String downloadURL = String.format( | ||
| "https://repo.maven.apache.org/maven2/org/vineflower/vineflower/%s/vineflower-%s.jar", | ||
|
There was a problem hiding this comment. Can we somehow use the Maven resolver APIs here? I fear that this may not work in environments with a firewall, but maybe it can be revisited in a future PR There was a problem hiding this comment. I am not exactly sure what that means as I have never used it. I wouldn't mind looking into it in another PR to have less risks in that one. For my comprehension, would it be about fetching the jar during build so it can be used form the MAVEN_HOME when needed? Or like currently download as needed via something like shrinkwrap/resolver (this does not seem to help with firewall though)? There was a problem hiding this comment. I would argue this is exactly similar to what it used to be. So let's not make @manofthepeace 's life harder than it needs to be. |
||
| context.versionStr, context.versionStr); | ||
| try (BufferedInputStream in = new BufferedInputStream(new URL(downloadURL).openStream()); | ||
| FileOutputStream fileOutputStream = new FileOutputStream(decompilerJar.toFile())) { | ||
|
|
@@ -1600,7 +1600,7 @@ public boolean downloadIfNecessary() { | |
| } | ||
| return true; | ||
| } catch (IOException e) { | ||
| log.error("Unable to download Vineflower from " + downloadURL, e); | ||
| return false; | ||
| } | ||
| } | ||
|
|
@@ -1633,7 +1633,7 @@ public boolean decompile(Path jarToDecompile) { | |
| } | ||
|
|
||
| if (exitCode != 0) { | ||
| log.errorf("Vineflower decompiler exited with error code: %d.", exitCode); | ||
| return false; | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming it to something future-proof?
decompilerperhaps?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's somewhat meant to be futureproof in the sense you could have support for multiple decompilers for example the config is
quarkus.package.vineflower.enabled=truemaybe there could be support for another one this way?quarkus.package.my-other-decompiler.enabled=true. I may be wrong though!a bit what I could gather from the code in #25729 where both fernflower and quiltflower could coexist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I ask is that I never know the config to enable the compiler without looking at the docs.
quarkus.package.decompiler.enabled=trueis cleaner (to me at least) 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, and I believe this is the reason why @geoand made it that way. I'm afraid that removing the config option would make existing users a bit annoyed, so perhaps deprecating (for removal)
quarkus.package.quiltflowerin favor ofquarkus.package.vineflowermay be better in this caseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I am still leaning towards having
quarkus.package.decompiler.enabled, given I doubt anybody cares about the decompiler used in practice :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is there a consensus here? :)
In my opinion, having both config, with the deprecated one is more or less useful as there isn't extreme adaptation to be made. On quarkus update, when looking at the migration it would be quite clear, vs deprecating, it, then doing it in some other release, not like if a behaviour changed underneath. As per
quarkus.package.decompiler.enabled, I do like it, but definitely not my call! I can adapt to what you guys decide.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep the decompiler name for the time being.