Making sure the right RocksDB native binary is added for container build #2794
Conversation
|
@cescoffier, could you add me to the right team so I can "request a review" by you? |
gunnarmorling
force-pushed
the
2663-follow-up
branch
from
5b9ee4e
to
72689e6
Compare
|
@gunnarmorling You need to ask @gsmet. I don't have enough karma for this. |
| @@ -56,7 +56,19 @@ void build(RecorderContext recorder, | |||
| reflectiveClasses.produce(new ReflectiveClassBuildItem(true, false, false, ByteArraySerde.class)); | |||
| reflectiveClasses.produce(new ReflectiveClassBuildItem(true, false, false, FailOnInvalidTimestamp.class)); | |||
|
|
|||
| nativeLibs.produce(new SubstrateResourceBuildItem(Environment.getJniLibraryFileName("rocksdb"))); | |||
| // for RocksDB, either add linux64 native lib when targeting containers | |||
| String containerRuntime = System.getProperty("native-image.container-runtime"); | |||
There was a problem hiding this comment.
That's not the only flag. You need to check for -Dnative-image.docker-build too
There was a problem hiding this comment.
@gsmet @dmlloyd @stuartwdouglas Maybe we need a flag as we have for modes. WDYT?
There was a problem hiding this comment.
Yes, I didn't feel happy about this specific solution. I'll add the check for the other property, too, as you suggested (btw. why do they both exist, there seems to be some overlap?
Would be great to have this information in a more accessible way. Perhaps it could be exposed via something like RecorderContext#getTargetPlatform(), returning an enum of possible platforms?
There was a problem hiding this comment.
This doesn't make sense for RecorderContext, which relates to bytecode generation.
I think maybe we should have a NativeImageSetupBuildItem which contains information about the native image configuration. Then you could have a separate build step which consumes that and produces the substrate resource build item. If the step produces nothing else, it won't even run if there's no native image being produced.
There was a problem hiding this comment.
I've logged #2827 for adding an PI which makes that information accessible to extensions.
There was a problem hiding this comment.
Reworked this commit to also check -Dnative-image.docker-build.
...eams/deployment/src/main/java/io/quarkus/kafka/streams/deployment/KafkaStreamsProcessor.java
Show resolved
Hide resolved
...eams/deployment/src/main/java/io/quarkus/kafka/streams/deployment/KafkaStreamsProcessor.java
Outdated
Show resolved
Hide resolved
cescoffier
changed the title
gunnarmorling
force-pushed
the
2663-follow-up
branch
from
72689e6
to
494fd78
Compare
|
@cescoffier Reworked to address your remark on the system properties. I think this was the only blocker; is there anything else in the way of merging this? Thanks! |
|
@gunnarmorling You need to apply the formatting rules, the CI is complaining. Running |
gunnarmorling
force-pushed
the
2663-follow-up
branch
from
494fd78
to
c405935
Compare
|
Gasp. Pushed an update. |
|
@cescoffier The test failure looks unrelated AFAICS. |
|
@gunnarmorling Yes, the test failure in a race condition in the rest client tests. It should have been fixed in #2838 (by skipping the test...). Can you rebase? |
…hen doing in-container build; Also ensuring KafkaProcessor runs when using kafka-streams extension.
gunnarmorling
force-pushed
the
2663-follow-up
branch
from
c405935
to
d738abf
Compare
Done. |
|
Thanks! |
Fix #2663.
@gsmet, @cescoffier, a small follow-up to the Kafka Streams work. The wrong native binary for RocksDB ended up being added to the application image when doing a build targeting containers but running on macOS.