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

Rename the Vineflower properties ? #37332

Closed
agoncal opened this issue Nov 27, 2023 · 10 comments · Fixed by #37445
Closed

Rename the Vineflower properties ? #37332

agoncal opened this issue Nov 27, 2023 · 10 comments · Fixed by #37445
Labels
area/core kind/enhancement New feature or request
Milestone

Comments

@agoncal
Copy link
Contributor

agoncal commented Nov 27, 2023

Description

In previous versions of Quarkus, to get the decompiled code, we had to set the Fernflower property:

quarkus.package.fernflower.enabled=true

Since Quarkus 3.6, Fernflower has been dropped, and instead it uses Vineflower with the following properties:

quarkus.package.vineflower.enabled
quarkus.package.vineflower.version
quarkus.package.vineflower.jar-directory

This being an implementation detail, the property is not very easy to find. What about renaming it with decompile or something like that ?

Implementation ideas

No response

@agoncal agoncal added the kind/enhancement New feature or request label Nov 27, 2023
@gsmet
Copy link
Member

gsmet commented Nov 28, 2023

I'm a bit undecided on it. I can see why you would want that but then we would have a hard time making sense of quarkus.package.vineflower.version?

Maybe we should have some generic options to enable the decompiler (I would go with quarkus.package.decompiler.*) that most people would use as they don't care about the tool and some advanced options specific to the decompiler (e.g. quarkus.package.vineflower.version).

Now not sure it's worth the bikeshedding, I will let @geoand decide.

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 28, 2023

/cc @Sanne (core), @aloubyansky (core), @geoand (core), @radcortez (core), @stuartwdouglas (core)

@agoncal
Copy link
Contributor Author

agoncal commented Nov 28, 2023

It was @maxandersen who asked me to create an issue.

As you pointed out, it's not a big of an issue. But for some advanced case, finding the property fernflower and now vineflower is tricky. You really need to remember it. And in the future you change the implementation to sunflower or yetanotherflower you would have to change the property name.

And what about ?

quarkus.package.decompiler.enabled
quarkus.package.decompiler.vineflower.version
quarkus.package.decompiler.vineflower.jar-directory
quarkus.package.decompiler.fernflower.version
quarkus.package.decompiler.fernflower.jar-directory
quarkus.package.decompiler.sunflower.version
quarkus.package.decompiler.sunflower.jar-directory

@geoand
Copy link
Contributor

geoand commented Nov 28, 2023

I can see why you would want that but then we would have a hard time making sense of quarkus.package.vineflower.version

That's exactly why I had a specific version.

However, it turns out it never needs to be changed, so the decompiler name is almost always useless - not to mention harder to type.

In retrospect, I should have used the simple name proposed here and not doing it only added confusion.

So +1 for the change

@mkouba
Copy link
Contributor

mkouba commented Nov 28, 2023

+1 for quarkus.package.decompiler.enabled, quarkus.package.decompiler.jar-directory and removing the version completely.

I wonder if it would make sense to support different decompilers though. Probably not.

@gsmet
Copy link
Member

gsmet commented Nov 28, 2023

I wonder if it would make sense to support different decompilers though. Probably not.

I wouldn't spend time on that except if there is a very good reason for it.

@maxandersen
Copy link
Member

Can we avoid break users again by removing properties :) just have décompiler.enabled and let the others be the default value?

@geoand
Copy link
Contributor

geoand commented Nov 28, 2023

Yeah, I intend to have a transition period

@geoand
Copy link
Contributor

geoand commented Dec 1, 2023

I am going to do what @mkouba suggests and simplify and just remove the version configuration property altogether as there is very very little use in setting it. I'll only keep enabled and jar-directory.

For now, I'll allow both quarkus.package.decompile.* and quarkus.package.vineflower.*

geoand added a commit to geoand/quarkus that referenced this issue Dec 1, 2023
This is meant to be used instead of
quarkus.package.vineflower.
Furthermore, this removes the configurable
version property which was never being used.

Closes: quarkusio#37332
@geoand
Copy link
Contributor

geoand commented Dec 1, 2023

#37445 takes care of it

geoand added a commit to geoand/quarkus that referenced this issue Dec 1, 2023
This is meant to be used instead of
quarkus.package.vineflower.
Furthermore, this removes the configurable
version property which was never being used.

Closes: quarkusio#37332
geoand added a commit to geoand/quarkus that referenced this issue Dec 5, 2023
This is meant to be used instead of
quarkus.package.vineflower.
Furthermore, this removes the configurable
version property which was never being used.

Closes: quarkusio#37332
geoand added a commit to geoand/quarkus that referenced this issue Dec 18, 2023
This is meant to be used instead of
quarkus.package.vineflower.
Furthermore, this removes the configurable
version property which was never being used.

Closes: quarkusio#37332
geoand added a commit to geoand/quarkus that referenced this issue Dec 20, 2023
This is meant to be used instead of
quarkus.package.vineflower.
Furthermore, this removes the configurable
version property which was never being used.

Closes: quarkusio#37332
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 20, 2023
geoand added a commit that referenced this issue Dec 20, 2023
Introduce quarkus.package.decompiler config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants