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

Introduce CurateOutcomeBuildItem.getApplicationModuleDirectory() #44104

Closed
wants to merge 1 commit into from

Conversation

aloubyansky
Copy link
Member

Closes #44013

FYI @ia3andy @melloware @mcruzdev

I'm curious what this method will be used for.
There is a case when this project is actually not available, for example during re-augmentation of a mutable-jar (remote-dev being one of the use-cases), in which case I'm returning the current directory. Although that may not match the expectation. OTOH, if I return null instead, every caller of this method will have to check for null and do something about it. So it depends on what this will be used for.

@quarkus-bot quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Oct 25, 2024
@aloubyansky
Copy link
Member Author

@iocanel fyi, iirc you were looking for something like this

@aloubyansky aloubyansky changed the title Introduce ApplicationModel.getApplicationModuleDirectory() Introduce CurateOutcomeBuildItem.getApplicationModuleDirectory() Oct 25, 2024
Copy link
Contributor

@melloware melloware left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcruzdev
Copy link
Contributor

Thank you!

This comment has been minimized.

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

LGTM

@ia3andy
Copy link
Contributor

ia3andy commented Oct 30, 2024

@melloware would you give it a try with Quinoa? With it we can remove the check for null as it will always be returned.

@melloware
Copy link
Contributor

Yes let me test and report back.

@melloware
Copy link
Contributor

@aloubyansky
Copy link
Member Author

2024-10-30 13:32:57,045 WARN  [io.qua.boo.BootstrapAppModelFactory] (main) Expected project directory C:\Users\RUNNER~1\AppData\Local\Temp\quarkus-dev-mode-test4152983395253692498 does not match current project directory D:\a\quarkus-quinoa\quarkus-quinoa\deployment
2024-10-30 13:32:58,901 WARN  [io.qua.qui.dep.QuinoaProcessor] (build-9) Quinoa directory not found 'quarkus.quinoa.ui-dir=src/main/webui' resolved to 'D:\a\quarkus-quinoa\quarkus-quinoa\deployment\src\main\webui'. It is recommended to remove the quarkus-quinoa extension if not used.

It locating the module running the test, not the put together "test project".

@aloubyansky
Copy link
Member Author

Since that's what is exposed as the ApplicationModel.getAppArtifact()

@ia3andy
Copy link
Contributor

ia3andy commented Oct 30, 2024

@aloubyansky then we need to fix this right? because the project is the put together test project right?

@aloubyansky
Copy link
Member Author

That could be tricky, I'll have a look.

@ia3andy
Copy link
Contributor

ia3andy commented Oct 30, 2024

In the other case, if we can't, we should change the test to adapt

@iocanel
Copy link
Contributor

iocanel commented Nov 11, 2024

this seems approved and ci is passing, should we merge?

@aloubyansky
Copy link
Member Author

Unfortunately, no. There is nothing in the core CI that uses it.
I'll get back to this a bit later.
The issue is with Quarkus extension tests that setup a test project "on a side". In that case the project module dir will still point to the extension deployment dir.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Adding a Request changes so that we don't merge by mistake.

See @aloubyansky 's comment above.

@aloubyansky
Copy link
Member Author

With now merged #44825 this one should be sorted now

Copy link

quarkus-bot bot commented Nov 30, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a3f6746.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/maven

io.quarkus.maven.it.TestMojoIT.testThatTheTestsAreReRunMultiModule - History

  • Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils.waitForNextCompletion(TestModeContinuousTestingMavenTestUtils.java:50)
	at io.quarkus.maven.it.LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule(LaunchMojoTestBase.java:56)

io.quarkus.maven.it.TestMojoIT.testThatTheTestsAreReRunMultiModule - History

  • Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils was not fulfilled within 3 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.maven.it.continuoustesting.TestModeContinuousTestingMavenTestUtils.waitForNextCompletion(TestModeContinuousTestingMavenTestUtils.java:50)
	at io.quarkus.maven.it.LaunchMojoTestBase.testThatTheTestsAreReRunMultiModule(LaunchMojoTestBase.java:56)

@aloubyansky
Copy link
Member Author

Actually, it won't work in all configs. This will require a different approach.

@aloubyansky aloubyansky closed this Dec 1, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Dec 1, 2024
@ia3andy
Copy link
Contributor

ia3andy commented Dec 2, 2024

@aloubyansky will you still find something for this?

@aloubyansky
Copy link
Member Author

Yes, it may be end up being more involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins triage/flaky-test triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a UserProjectRootBuildItem
6 participants