Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change decompiler from quiltflower to vineflower #35610

Merged
merged 1 commit into from Sep 20, 2023

Conversation

manofthepeace
Copy link
Contributor

QuiltFlower became VineFlower https://github.com/Vineflower
Where's Quiltflower?

In short, Vineflower is Quiltflower- created by the same team and having the same goals, just under a new name. The reason for this shift was not due to any drama or falling out with the Quilt team, instead from a desire to expand past a focus on mainly Minecraft and create a more general Java decompiler. Quiltflower grew significantly since its inception and had always been doing its own thing on the side, and this is a natural progression of the project.

Last version of quiltflower was 1.9.0 (https://github.com/Vineflower/vineflower/releases/tag/1.9.0)

see: https://quiltmc.org/en/blog/2023-07-09-quiltflower-is-now-vineflower/

This is a 'somewhat' breaking change as it removes the possibility of using quiltflower/older versions of quiltflower

@github-actions
Copy link

github-actions bot commented Aug 28, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Aug 29, 2023

@yrodiere the failure above is an OOM for metaspace so it looks like we are not done with it, unfortunately...

@yrodiere
Copy link
Member

Oh we certainly aren't... I just hoped the problem was mitigated just enough that we don't see it anymore. But indeed it seems we can still see it, so I'll have to have another look... someday...

@geoand
Copy link
Contributor

geoand commented Aug 29, 2023

Thanks for this.

Have you tried it to make sure it decompiles the code?

@manofthepeace
Copy link
Contributor Author

I have downloaded the code.quarkus.io basic app to test it and it worked.

@geoand
Copy link
Contributor

geoand commented Aug 29, 2023

I assume you mean both with 3.3 and with the PR, correct?

@manofthepeace
Copy link
Contributor Author

I only tested with the pr i.e. 999-SNAPSHOT. I would assume that 3.3 wont work without the pr as the repo url changed, unless you already have it downloaded in your ~/.quarkus

@geoand
Copy link
Contributor

geoand commented Aug 29, 2023

That's fine, thanks.

Quarkus Documentation automation moved this from To do to Reviewer approved Aug 29, 2023
@gsmet
Copy link
Member

gsmet commented Aug 29, 2023

I would assume that 3.3 wont work without the pr as the repo url changed

I checked and there is a redirection between the old project and the new one. If we are following the redirects when downloading it should be good.

Now I think we should probably move to a Maven Central URL such as https://repo.maven.apache.org/maven2/org/vineflower/vineflower/1.9.2/ to download the jar as we are sure these URLs are immutables.

@manofthepeace
Copy link
Contributor Author

Now I think we should probably move to a Maven Central URL such as https://repo.maven.apache.org/maven2/org/vineflower/vineflower/1.9.2/ to download the jar as we are sure these URLs are immutables.

Done and retested.

*/
@ConfigItem
public QuiltFlowerConfig quiltflower;
public VineFlowerConfig vineflower;
Copy link
Contributor

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? decompiler perhaps?

Copy link
Contributor Author

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=true maybe 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

Copy link
Contributor

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=true is cleaner (to me at least) 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit what I could gather from the code in #25729 where both fernflower and quiltflower could coexist

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.quiltflower in favor of quarkus.package.vineflower may be better in this case

Copy link
Contributor

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 :)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@@ -1589,7 +1589,7 @@ public boolean downloadIfNecessary() {
return true;
}
String downloadURL = String.format(
"https://github.com/QuiltMC/quiltflower/releases/download/%s/quiltflower-%s.jar",
"https://repo.maven.apache.org/maven2/org/vineflower/vineflower/%s/vineflower-%s.jar",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gastaldi
Copy link
Contributor

This is a 'somewhat' breaking change as it removes the possibility of using quiltflower/older versions of quiltflower

If no future versions of quiltflower are expected, then this is okay.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 8, 2023

Failing Jobs - Building d044e9b

Status Name Step Failures Logs Raw logs Build scan
✔️ Maven Tests - JDK 11
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good and we can merge it, thanks.

@gsmet gsmet merged commit adeab40 into quarkusio:main Sep 20, 2023
51 of 53 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Sep 20, 2023
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 20, 2023
@manofthepeace manofthepeace deleted the vineflower branch October 3, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants