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.
Looking again at this I realize that this check doesn't make any sense and it will essentially always be true.
@radcortez is there any way I can check if the configuration generation was successful or not (assuming that logging will only work if it was successful) other than checking for
ConfigurationException
orConfigValidationException
?If not, I am thinking that we will need to patch SmallryeConfig to catch more exceptions (currently looking for
IOException
s) in https:/smallrye/smallrye-config/blob/main/implementation/src/main/java/io/smallrye/config/AbstractLocationConfigSourceLoader.java to handle #42084WDYT?
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.
Maybe by checking
quarkus/core/runtime/src/main/java/io/quarkus/runtime/configuration/QuarkusConfigFactory.java
Line 13 in 133cbd9
We can certainly improve
AbstractLocationConfigSourceLoader
, but from theSmallRyeConfig
side, we only provideConfigValidationException
for validation errors that the user can fix. I'm not sure if should be wrappingIOException
.Alternatively, we can probably add some sort of flag into the runtime-generated config class that can tell us if the config was started successfully.
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 tried checking
QuarkusConfigFactory
but that doesn't work either, since in native executables the config is always set (done at build time).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.
Hum, can you confirm the repro steps of #42084? I've tried the steps, but I couldn't reproduce it. Maybe I'm missing something?
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.
Probably only for Mandrel?
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.
@radcortez I can no longer reproduce the silent failure on
main
, but the exceptions I am getting are still hiding the actual root cause.On linux I am getting:
which gets fixed in #42794. If you try building Quarkus with this fix you will be able to reproduce #42084 on linux. Unfortunately on MacOS I am getting an NCDFE which I still didn't have the time to fix.
What is the output of the following in your case?
If the build succeeds and the binary invocation exits without any messages then that's the reproducer. The desired behaviour is to fail with an error message (and ideally with a stacktrace as well) not silently.
If it prints the help message then something is wrong with the reproducer and I will need more info about your setup.
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.
Yes, I do get the NCDFE:
Could not initialize class io.smallrye.common.net.HostName
.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.
@radcortez I created #42810 to resolve the macOS issue but that requires graalvm/graalvm-community-jdk21u#9 being merged and released in the next Mandrel 23.1.x release to work.
Unfortunately with Mandrel 24.0.x and #42810 the issue is not reproducible on macOS (so you need to reproduce in linux)