Move brotli native library handling to a feature #43828
Conversation
|
🎊 PR Preview fa93db7 has been successfully built and deployed to https://quarkus-pr-main-43828-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -100,6 +100,14 @@ | |||
| <artifactId>mockito-junit-jupiter</artifactId> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.graalvm.sdk</groupId> | |||
| <artifactId>nativeimage</artifactId> | |||
There was a problem hiding this comment.
We usually mark this one as provided as I'm pretty sure we don't want the artifact around in JVM mode? Except if we ended up doing it in the parent or the BOM?
There was a problem hiding this comment.
You are right. I added it through the IDE and didn't look back.
|
|
||
| @Override | ||
| public void beforeAnalysis(BeforeAnalysisAccess access) { | ||
| // TODO reuse Brotli4jLoader's code instead of reinventing the wheel? |
There was a problem hiding this comment.
I think this makes a lot of sense. In most cases we were reinventing the wheel, we were doing it because we couldn't actually resolve the OS at build time for in container build.
By using this pattern, we can probably do it now.
Now I can understand if you want to do it in two steps.
| </dependency> | ||
| <dependency> | ||
| <groupId>io.quarkus</groupId> | ||
| <artifactId>quarkus-devtools-utilities</artifactId> |
There was a problem hiding this comment.
(This comment was added at the end just so you have the chronology of my thoughts)
I'm not exactly excited about this one. It's supposed to be about devtools utilities and while it only contains two classes for now, I'm not sure it's good idea to use it - it could contain dependencies we don't want in the future.
So, maybe we should actually tackle the issue of using the Brotli4jLoader instead of our own code from the start?
Also, @dmlloyd , I wonder if we should have something about the architecture in SmallRye Common OS - in the vein of what we have here: https://github.com/quarkusio/quarkus/blob/main/independent-projects/tools/utilities/src/main/java/io/quarkus/utilities/OS.java#L35
Finally, please make sure you add the actual dependencies before the test ones. That's a convention we try to use.
zakkak
force-pushed
the
2024-10-11-fix-43770
branch
from
f3b95db to
9955ab9
Compare
Always registers the correct native library as the logic runs on the same architecture we are targeting (even when using containers). Related to quarkusio#43782
zakkak
force-pushed
the
2024-10-11-fix-43770
branch
2 times, most recently
from
fbaf8d8 to
c6ce08a
Compare
👍 I resolved all of them. Please take another look. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zakkak
force-pushed
the
2024-10-11-fix-43770
branch
from
c6ce08a to
33ccb87
Compare
|
I had included an extra slash in the beggining of the path. Fixed. |
Status for workflow
|
zakkak
added
the
triage/waiting-for-ci
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
quarkus-bot
bot
removed
the
triage/waiting-for-ci
Always registers the correct native library as the logic runs on the
same architecture we are targeting (even when using containers).
Related to #43782