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

Fix accidental config breakage of quarkus.package.decompiler.* properties #40277

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

dmlloyd
Copy link
Member

@dmlloyd dmlloyd commented Apr 25, 2024

This was missed in #39295. Fixes #40272.

/cc @mkouba

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Oh, I didn't even know about the existence of ConfigCompatibility class!

This is intended to be a temporary and evolutionary solution to be replaced by generative remapping.

I think that it would deserve more attention even though it's a temporary solution.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Apr 26, 2024

@dmlloyd slightly related to that, I'm not very happy about how we deprecated the package config properties:

  • the warning is not really actionable: you get a list of various config properties, it's hard to understand what you really need to do to make the warning quiet. I can see how it might not be practical to put all the details there but see below.
  • we need entries in the migration guide to explain how to fix the setup (especially on how to fix our default project template if you have generated your project pre-3.10 also for the decompiler properties): https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.10
  • we need something in quarkus-updates to update the projects. I'm going to have a look in the next few days, hopefully in time for the 3.10 release.

@galderz
Copy link
Member

galderz commented Apr 29, 2024

I guess this fixes this right?

[INFO] --- quarkus-maven-plugin:999-SNAPSHOT:build (default) @ getting-started-reactive ---
[WARNING] [io.quarkus.deployment.configuration] Configuration property 'quarkus.package.vineflower.enabled' has been deprecated and replaced by: [quarkus.package.decompiler.enabled]
[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.package.decompiler.enabled" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 29, 2024

@dmlloyd slightly related to that, I'm not very happy about how we deprecated the package config properties:

  • the warning is not really actionable: you get a list of various config properties, it's hard to understand what you really need to do to make the warning quiet. I can see how it might not be practical to put all the details there but see below.

I agree to an extent, but other than creating a custom message for each detected usage I wasn't sure how I could do much more. Also imagine that this could be automated some day; how would we want to approach that? Do you want a custom message for each case, and if so, what should the message say? Now, it tells you which property is deprecated, and what properties to set in its place, so at least that gives a good starting point for looking in the docs.

OK, I can look into this.

  • we need something in quarkus-updates to update the projects. I'm going to have a look in the next few days, hopefully in time for the 3.10 release.

OK, I'll see what I can figure out here.

@dmlloyd
Copy link
Member Author

dmlloyd commented Apr 29, 2024

I guess this fixes this right?

[INFO] --- quarkus-maven-plugin:999-SNAPSHOT:build (default) @ getting-started-reactive ---
[WARNING] [io.quarkus.deployment.configuration] Configuration property 'quarkus.package.vineflower.enabled' has been deprecated and replaced by: [quarkus.package.decompiler.enabled]
[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.package.decompiler.enabled" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo

Yes this change hides the second message.

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 29, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 21ce678.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/infinispan-cache/deployment

io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

  • expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)

@gsmet gsmet merged commit f97eca3 into quarkusio:main Apr 30, 2024
51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 30, 2024
@dmlloyd dmlloyd deleted the fix-40272 branch April 30, 2024 18:30
@gsmet gsmet modified the milestones: 3.11 - main, 3.10.1 May 10, 2024
dmlloyd added a commit to dmlloyd/quarkus that referenced this pull request May 29, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this pull request Jun 4, 2024
Fixes quarkusio#40874. quarkusio#40277 had a mistake in it.

(cherry picked from commit a9c04c4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config: reflect a breaking change of package config in the docs
4 participants