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 broken Feature#runtimeInitializedPackages method #30027

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 22, 2022

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM @geoand, thanks !

Good catch by the way, this should have been caught by the CI though. Do you think you could add a test for it as well?

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

Do you think you could add a test for it as well?

The only way I can think of to do this would be to have some custom extension that generates the build item. I think it's a worthy addition :)

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 22, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 22, 2022

Failing Jobs - Building 6ac08e3

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: extensions/micrometer/deployment 
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/quartz/deployment extensions/scheduler/deployment and 21 more

📦 extensions/micrometer/deployment

io.quarkus.micrometer.deployment.binder.RedisClientMetricsTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:689)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

@zakkak
Copy link
Contributor

zakkak commented Dec 22, 2022

Do you think you could add a test for it as well?

The only way I can think of to do this would be to have some custom extension that generates the build item. I think it's a worthy addition :)

Perhaps you can reuse/extend this

@BuildStep
RuntimeInitializedPackageBuildItem runtimeInitializedPackage() {
return new RuntimeInitializedPackageBuildItem(RuntimeInitializedClass.class.getPackage().getName());
}
?

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

That integration test should actually be run when in the Misc2 category. There is some problem in the filter-native-tests-json.sh script

@geoand geoand merged commit 4397da6 into quarkusio:main Dec 22, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 22, 2022
@geoand geoand deleted the runtimeInitializedPackages-return branch December 22, 2022 11:23
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 22, 2022
@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

@famod any chance you can take a look at filter-native-tests-json.sh?
What seems to happen here is that because integration-tests/test-extension is a multi-module directory, the script gets mixed up and does not include it in the output.

@zakkak
Copy link
Contributor

zakkak commented Dec 22, 2022

There seems to be another issue with the test. In the mandrel nightly CI runs (that are not "incremental") the test is included in Misc2, yet it looks like there are no native tests in there.

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

That's weird because when I ran the test manually mvn verify -Dnative in integration-tests/test-extension the native tests ran

@zakkak
Copy link
Contributor

zakkak commented Dec 22, 2022

That's weird because when I ran the test manually mvn verify -Dnative in integration-tests/test-extension the native tests ran

Are these native tests though, do you see a native-image being generated? Can you try with mvn verify -Dnative.surefire.skip? I will give it a go locally as well, but I have some trouble being quarkus at the moment...

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

Yes, I saw the native binary being built with my fix while seeing the error mentioned in quarkiverse/quarkus-neo4j#115 without it

@zakkak
Copy link
Contributor

zakkak commented Dec 22, 2022

I tried locally on 4397da6 with ./mvnw -Dnative -pl integration-tests/test-extension/ clean verify and can't get it to build and run :/

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

mvn verify -Dnative -f integration-tests/test-extension/ does the trick for me

Note the use of -f. -pl is for specifiying the groupId and artifact Id of a module

@zakkak
Copy link
Contributor

zakkak commented Dec 22, 2022

OK, that seems to be an issue in the CI as well (both in the mandrel workflows and the quarkus), which run the tests using:

./mvnw -Dnative -f integration-tests -pl test-extension ...

run: ./mvnw $COMMON_MAVEN_ARGS -f integration-tests -pl "$TEST_MODULES" $NATIVE_TEST_MAVEN_ARGS -Dquarkus.native.container-build=$CONTAINER_BUILD

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

Yeah, that's certainly part of the problem. The other part is that $TEST_MODULES is not populated correctly :)

@zakkak
Copy link
Contributor

zakkak commented Dec 22, 2022

Yeah, that's certainly part of the problem.

I opened #30035 and graalvm/mandrel#462 to fix this.

The other part is that $TEST_MODULES is not populated correctly :)

For this part I can't help much but it looks like the test is correctly added when running on the main branch:

https://github.com/quarkusio/quarkus/actions/runs/3755597705/jobs/6381059498#step:12:15

@geoand
Copy link
Contributor Author

geoand commented Dec 22, 2022

Ah, that's nice!

@gsmet gsmet modified the milestones: 2.16 - main, 2.15.2.Final Jan 3, 2023
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.

4 participants