Automatically Load Liquibase Resource Files for Native Image Build #41928
Conversation
|
Thanks for your pull request! Your pull request does not follow our editorial rules. Could you have a look?
This message is automatically generated by a bot. |
gcw-it
changed the title
|
Thanks! It's definitely a good idea as we have been struggling with it a lot. I'll have a look soon. |
| var jarUrlTemplate = "jar:%s!/"; | ||
| try { | ||
| var srcUrl = Objects.requireNonNull(Liquibase.class | ||
| .getProtectionDomain() | ||
| .getCodeSource() | ||
| .getLocation()); | ||
| var jarUrl = new java.net.URL(jarUrlTemplate.formatted(srcUrl.toString())).toURI(); | ||
|
|
||
| try (var ignored = FileSystems.newFileSystem(jarUrl, Collections.EMPTY_MAP)) { | ||
| var rootPath = Paths.get(jarUrl); | ||
| loadLiquibaseServiceProviderConfig(rootPath, services, reflective, runtimeInitialized); | ||
| loadLiquibaseRootProperties(rootPath, resource); | ||
| loadLiquibaseXsdResources(rootPath, resource); | ||
| } | ||
| } catch (URISyntaxException | IOException ex) { | ||
| throw new IllegalStateException(ex); | ||
| } |
There was a problem hiding this comment.
I'm not entirely sure about the implementation here.
I think, maybe using CurateOutcomeBuildItem to get the ApplicationModel would be better. Then you could get a ResolvedDependency from getDependencies that match Liquibase and use getContentTree(PathFilter).
There was a problem hiding this comment.
Thanks for the pointer. Done.
| .map(this::classFromString) | ||
| .filter(not(runtimeInitializationServices::contains)) |
There was a problem hiding this comment.
I think I would avoid transforming this to a class then a String again. You can make runtimeInitializationServices a set of strings and use liquibase.configuration.ConfigurationValueProvider.class.
There was a problem hiding this comment.
Done.
I realised the same happened for the static initialisation. I changed it there, too.
| BuildProducer<ReflectiveClassBuildItem> reflective, | ||
| BuildProducer<RuntimeInitializedClassBuildItem> runtimeInitialized) throws IOException { | ||
|
|
||
| List<Class<?>> runtimeInitializationServices = List.of( |
There was a problem hiding this comment.
For now it doesn't make a lot of difference as you only have one item but let's make it a HashSet in case it grows in the future.
This comment has been minimized.
This comment has been minimized.
|
@gcw-it I sent you an email on the email you use to commit (hopefully you read it!), let's try to figure out things there so that your setup works. |
| return dependency.getResolvedPaths().stream() | ||
| .map(Path::getFileName) | ||
| .map(Path::toString) | ||
| .anyMatch(s -> s.contains(LIQUIBASE_ARTIFACT_NAME)); |
There was a problem hiding this comment.
Wouldn't dependency.getArtifactId().equals(LIQUIBASE_ARTIFACT_NAME) work here?
There was a problem hiding this comment.
You're right, somehow I managed to overlook that possibility.
I will change this accordingly.
There was a problem hiding this comment.
Please do that and also test the groupId. Typically you could have someone having a my-groupid:liquibase-core in their app and you don't want to catch that.
Also I recommend using the CONSTANT.equals(variable) pattern instead of variable.equals(CONSTANT). In this case, the variables won't be null so it won't change anything but it's a good practice to avoid NPEs. Once you have this habit, you're a lot safer.
...s/liquibase/deployment/src/main/java/io/quarkus/liquibase/deployment/LiquibaseProcessor.java
Show resolved
Hide resolved
| return dependency.getResolvedPaths().stream() | ||
| .map(Path::getFileName) | ||
| .map(Path::toString) | ||
| .anyMatch(s -> s.contains(LIQUIBASE_ARTIFACT_NAME)); |
There was a problem hiding this comment.
Please do that and also test the groupId. Typically you could have someone having a my-groupid:liquibase-core in their app and you don't want to catch that.
Also I recommend using the CONSTANT.equals(variable) pattern instead of variable.equals(CONSTANT). In this case, the variables won't be null so it won't change anything but it's a good practice to avoid NPEs. Once you have this habit, you're a lot safer.
| // Register Precondition services, and the implementation class for reflection while also registering fields for reflection | ||
| consumeService(liquibase.precondition.Precondition.class, (serviceClass, implementations) -> { | ||
| services.produce(new ServiceProviderBuildItem(serviceClass.getName(), implementations.toArray(new String[0]))); | ||
| consumeService("liquibase.precondition.Precondition", (serviceClass, implementations) -> { |
There was a problem hiding this comment.
I suggested to use liquibase.precondition.Precondition.class.getName() (and same for the others, for instance in RUNTIME_INITIALIZATION_SERVICES). Keep in mind that we don't manage the Liquibase development so better get a compilation warning if they change something in this area.
It's a bit more nuanced than that because we sometimes use the strings because either the classes are not accessible or because we want to avoid loading them if we can, but in this very case, I think it's better to use .class.getName().
There was a problem hiding this comment.
I understand your reasoning here, but doesn't this contradict, what you wrote above?
I think I would avoid transforming this to a class then a String again. You can make
runtimeInitializationServicesa set of strings and useliquibase.configuration.ConfigurationValueProvider.class.
I'm fine with restoring this to the class-based version, but I'd like to avoid any misunderstanding.
There was a problem hiding this comment.
I think you're confused because I made a mistake in my very first comment :).
Use liquibase.precondition.Precondition.class.getName() with .getName() so that you get the String name. That's what I wanted to write the first time but I somehow missed the .getName().
That way we will have compilation checked and we will avoid going back and forth with classes and strings.
Is it all clear now?
There was a problem hiding this comment.
OK, i see. I will change the code accordingly.
This comment has been minimized.
This comment has been minimized.
|
I enabled |
|
I realised, that my code for the runtime initialisation of the services is incorrect. It only registers the service type, and not the corresponding implementations. Correcting this leads unfortunately to another problem. The reason why I registered the When I register the unproblematic service implementations correctly, and Registering I think, the best course of action right now is, to not register the |
|
I agree we should ideally be in line with what was done before. |
This comment has been minimized.
This comment has been minimized.
|
If everything is in order, I will squash the commits. |
|
After digging a bit deeper into the codebase, I found the culprit for the strange GraalVM build time exception. The runtime package of the liquibase extension uses a GraalVM substitution ( This substitution deletes a field, that is later referenced by the default constructor, hence the Substituting the constructor leads only to a temporary relief. The build is now successful, but at runtime a We could limit the application of the substitution to the Windows platform, but on my Windows machine (Windows 10 22H2 19045 4651, GraalVM 21.02+13.1), the error Maybe the upgrade to liquibase 4.27.0 helped, or newer GraalVM versions remediated problem #22449, but it seems, the patch isn't necessary anymore. I'm not entirely sure if the patch was necessary at all in the latest Quarkus versions, because the targeted class wasn't even included in the native image. Ideally we could remove the Would the CI-Pipeline pick it up, if we introduce a regression by removing the patch? |
…EnvironmentValueProvider
There was a problem hiding this comment.
I think it's worth trying to drop the substitution and see how it goes.
One thing we need to do is to check if the quickstarts work: https://github.com/quarkusio/quarkus-quickstarts . You need to get the develop branch so that the quickstart points to 999-SNAPSHOT and run the specific liquibase quickstart with -Dnative. It has been failing for a while and I wonder if your patch would fix it: https://github.com/quarkusio/quarkus-quickstarts/actions/runs/10087017755/job/27890486199 .
If not, it's probably worth investigating. It won't block this PR though as I really want it in soon.
As for testing your patch on Windows, you could add the liquibase IT there: https://github.com/quarkusio/quarkus/blob/main/.github/native-tests.json#L135-L140 and it would then be run on Windows too (we don't run all the native tests on Windows).
| @@ -105,10 +113,11 @@ IndexDependencyBuildItem indexLiquibase() { | |||
| @BuildStep(onlyIf = NativeOrNativeSourcesBuild.class) | |||
| @Record(STATIC_INIT) | |||
| void nativeImageConfiguration( | |||
| LiquibaseRecorder recorder, | |||
| LiquibaseRecorder ignored, | |||
There was a problem hiding this comment.
I think you can drop this parameter and the @Record annotation if we don't need them.
|
FWIW, someone just complained about the issue we see in the quickstarts: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Native.20Image.20Issue.20with.20quarkus-liquibase . |
This comment has been minimized.
This comment has been minimized.
Sorry, that I wasn't really clear. I already built the I will remove the Would you prefer the commits being squashed, or separate for now? |
|
I think you can add two more semantic commits for these two changes as they are not entirely related to your changes. |
quarkus-bot
bot
added
the
area/infra-automation
|
This thread might also be of interest to you: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Native.20Image.20Issue.20with.20quarkus-liquibase |
|
I wonder, if in a next step it would be a good idea, to extract the functionality, to automatically find the resource paths in a dependency, to a utility class. At least the liquibase-mongodb extension seems to suffer from a similar problem as the liquibase extension and could benefit from it. Maybe there are other extensions as well? |
|
Yeah so I can see how the Liquibase MongoDB extension would suffer from the same issue and probably needs the same fix. I wouldn't generalize things too much though and use this approach when it's not risky at all (typically, versioned XSD resources makes perfect sense, blinding adding all services might not be what we want to do). Let's get this in first though. |
|
I agree with you, that we should concentrate on this fix first. My proposal wasn't intended to be included in this PR, but further down the road I also don't want to automate registering resources for a native build, that should remain the prerogative of the extension. This would simply extract some common functionality to avoid code duplication. |
|
Yes, it makes perfect sense. |
gsmet
added
the
triage/waiting-for-ci
Status for workflow
|
quarkus-bot
bot
added
kind/bugfix
and removed
triage/waiting-for-ci
|
And merged, thanks a lot for this fix. |
Fixes #40574
Fixes #40575
Fixes #42143
It seems, that after upgrading the
liquibase-coreversion, the additional resource files that were added, weren't incorporated in the build.To avoid this problem for future updates, this patch loads the following
iquibase-coreresources automatically:.propertyfiles in the root folder.xsdXML Schema Definitions from thewww.liquibase.org/xml/ns/dbchangelogfolderMETA-INF/servicesThe i18n resources were already automatically loaded before.
If in a future liquibase version a service is added, that needs initialisation at runtime, this information needs to be manually added to the code.
Manual intervention is also necessary, should any other resource types or locations be added to liquibase.