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

Support modules with non-empty default classifier #24735

Merged
merged 1 commit into from Apr 5, 2022

Conversation

aloubyansky
Copy link
Member

Fix #24697

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven labels Apr 4, 2022
@aloubyansky aloubyansky requested a review from famod April 4, 2022 11:57
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 4, 2022

Failing Jobs - Building 1d226f7

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

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/grpc-hibernate integration-tests/kafka-ssl 

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd line 61 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in com.example.grpc.hibernate.BlockingRawTest that uses java.util.List was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

📦 integration-tests/kafka-ssl

io.quarkus.it.kafka.SslKafkaConsumerTest.testReception - More details - Source on GitHub

java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:632)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:703)

@famod
Copy link
Member

famod commented Apr 4, 2022

I'll try to find some time for this tomorrow.

@famod famod merged commit 0337377 into quarkusio:main Apr 5, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 5, 2022
@gsmet gsmet modified the milestones: 2.9 - main, 2.8.1.Final Apr 11, 2022
@famod
Copy link
Member

famod commented Apr 19, 2022

@aloubyansky this seems to have an unexpected (?) side effect:
In 2.8.1 (when running tests) the following is returing a "jar:file:" URL whereas in 2.8.0 it was returning a "file" URL:

final var resource = Thread.currentThread().getContextClassLoader().getResource(resourceName);

This is only the case if the resource is located in an upstream module (e.g. resource in module A, test is run in B and B depends on A).
If the test is run in the same module where the resource is located (e.g. all in module A), then it works as before.

WDYT?

@aloubyansky
Copy link
Member Author

So it is finding the resource in the JAR instead of the directory? Do you think you can adapt one of your reproducers to show the difference between two versions?

@famod
Copy link
Member

famod commented Apr 19, 2022

So it is finding the resource in the JAR instead of the directory?

Exactly.

Do you think you can adapt one of your reproducers to show the difference between two versions?

I've failed to do so for now. There must be more to it...

@famod
Copy link
Member

famod commented Apr 21, 2022

@aloubyansky I tried a few more things whithout any luck.

So I changed the code in question to create a new FileSystem and get the Path from that (instead of directly via Path.of(URI)) and that works as expected, but during implementation I found an even bigger issue:
Via Eclipse, in module A I adjusted the code mentioned above, in that I added a new method to a class called ResourceLoader and used it in MariaDBContainerQuarkusLifecycleManager, instead of the now faulty method.
Then I ran the test in module B and all of a sudden I was getting NoSuchMethodError.

Turns out that when no test was run before via mvn, running that test in IDE is just fine - without adjusting anything!
But as soon as I run that test via mvn (which fails), test execution fails in IDE as well.
That's when I remembered test-app-model.dat. When I delete it after mvn, IDE is back to normal.
Unsurprisingly, next mvn run (with clean test) fails again, also again breaking IDE.
Consequently, running mvn test without clean (after running the test in IDE) is fine as well. So the problems start when test-app-model.dat is created via mvn (not via IDE).

How does the above relate to NoSuchMethodError? Well, deleting test-app-model.dat (that was created by mvn) also fixes that issue.
I suppose test-app-model.dat told test execution in Eclipse to load the respective class from the jar, but as Eclipse is doing workspace resolution and is not doing any install, things got inconsistent.
(Of course, at some point I ran mvn install on module A, which also "fixed" that issue.)

@famod
Copy link
Member

famod commented Apr 21, 2022

FYI @stuartwdouglas ⬆️

@aloubyansky
Copy link
Member Author

Thanks @famod for all the info. Is the JAR the class is loaded from in the module's target dir or the local Maven repo?
When you run tests in module B from the command line, has the module A been built?
Do you use Maven profiles? Does dependency on module A come from a profile?

@famod
Copy link
Member

famod commented Apr 21, 2022

Is the JAR the class is loaded from in the module's target dir or the local Maven repo?

It's (simplified) A/target/A.jar via mvn and A/target/classes via Eclipse. (I have to say though that I checked it with the resource I mentioned initially, because class.getProtectionDomain().getCodeSource().getLocation() wasn't giving me anything useful).
So

but as Eclipse is doing workspace resolution and is not doing any install

is misleading here, I should have said that Eclipse is not assembling a jar.

When you run tests in module B from the command line, has the module A been built?

Usually yes, but at some point it wasn't with the latest changes. My main concern here is that IDE should not be influenced by the different/limited/absent workspace resolution of the command line execution.

Do you use Maven profiles?

In general yes, but nothing specific in this case.

Does dependency on module A come from a profile?

There are two dependencies on A from B: A "regular" compile one and a test-jar one. But both are not in a profile.
There is a third dependency on A ("mocks" classifier) in a profile, but that profile is not active in this case.
I tried to mimic that in a reproducer but it didn't lead to what I'm seeing in my real project.

@famod
Copy link
Member

famod commented Apr 21, 2022

Btw, removing test-app-model.dat between test runs yields a 1.5-2 seconds runtime penalty in my case. So simply dropping it (as we discussed before) might not be the best idea.

@famod
Copy link
Member

famod commented Apr 21, 2022

I was also trying to wrap my head around how this PR is causing that URI change and I was suspecting the following in my root pom.xml to be related (in pluginManagement):

                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-jar-plugin</artifactId>
                    <version>${jar-plugin.version}</version>
                    <configuration>
                        <excludes>
                            <exclude>application-dev.yaml</exclude>
                        </excludes>
                    </configuration>
                </plugin>

But again, adding that to the small sample project didn't reproduce it either.

@famod
Copy link
Member

famod commented Apr 21, 2022

Ok, so this is the root cause of the URI change for my setup:
https://github.com/quarkusio/quarkus/blob/2.8.1.Final/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/bootstrap/workspace/DefaultWorkspaceModule.java#L164-L165
This is kicking in for A (when running tests via mvn in B), because two source sets are present: test and mocks. Thing is, mocks is also a test-jar result and so the check is incorrect at this point.
It is therefore not entering the following block anymore:
https://github.com/quarkusio/quarkus/blob/2.8.1.Final/independent-projects/bootstrap/maven-resolver/src/main/java/io/quarkus/bootstrap/resolver/maven/workspace/LocalProject.java#L368-L371
so it's missing an "ArtifactSources".

I don't see how DefaultWorkspaceModule.this.sourcesSets can be used to differentiate that case, at least not without adding some info where that set is coming from (so to say).

@famod
Copy link
Member

famod commented Apr 21, 2022

PS: What I don't understand at this point (might just be too late), is why the URI is unchanged in Eclipse (compared to 2.8.0), given that it's also mising that MAIN "ArtifactSources".

@aloubyansky
Copy link
Member Author

Oh, ok, thanks @famod

@aloubyansky
Copy link
Member Author

#25086 should fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/maven kind/bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEV mode fails when using classified dependencies
3 participants