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.

Sign up for GitHub

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

Jump to bottom

Dev mode: fix HotDeploymentWatchedFileBuildItem predicate #36520

Merged
merged 3 commits into from
Nov 6, 2023
Merged

Dev mode: fix HotDeploymentWatchedFileBuildItem predicate #36520

merged 3 commits into from
Nov 6, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Oct 17, 2023

@mkouba mkouba requested review from glefloch and ia3andy October 17, 2023 08:49
@quarkus-bot quarkus-bot bot added area/core area/qute The template engine labels Oct 17, 2023
@mkouba mkouba requested a review from stuartwdouglas October 17, 2023 09:00
@ia3andy
Copy link
Contributor

ia3andy commented Oct 17, 2023

I will have a look early next week thanks @mkouba, don't hesitate to remind me if I don't..

@quarkus-bot

This comment has been minimized.

Sign in to view
glefloch
glefloch approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member

@glefloch glefloch left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@gsmet
Copy link
Member

gsmet commented Oct 17, 2023

@cescoffier the VT tests are really getting in the way. Not sure why they are so unstable but it's concerning. Could you have a look or ask someone to have a look?

@mkouba
Copy link
Contributor Author

mkouba commented Oct 17, 2023

@cescoffier the VT tests are really getting in the way. Not sure why they are so unstable but it's concerning. Could you have a look or ask someone to have a look?

I can try to take a look. Hopefully, it's not caused by the #36466 🤦.

@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Oct 18, 2023

@cescoffier the VT tests are really getting in the way. Not sure why they are so unstable but it's concerning. Could you have a look or ask someone to have a look?

I can try to take a look. Hopefully, it's not caused by the #36466 🤦.

@gsmet It seems that the tests fail because: "You weren't able to create an executor that spawns virtual threads, the default blocking executor will be used, please check that your JDK is compatible with virtual threads".

@cescoffier Do we specify --enable-preview somehow for the Quarkus CI / JVM Tests - JDK 20 job? I only found it for the Native Tests - Virtual Thread job..

@cescoffier
Copy link
Member

@mkouba @gsmet We need to bump everything to Java 21. We have the Mandrel builder image ready. That should improve things.

@gsmet
Copy link
Member

gsmet commented Oct 18, 2023

@cescoffier this is in progress on my side but somehow we have a disk space issue when the runner is using Java 21. I'm slowly diagnosing it but it takes time, given each round takes ~4-6 hours...

@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Oct 18, 2023

This pull request is ready to merge but let's wait for Andy's approval...

@gsmet
Copy link
Member

gsmet commented Oct 18, 2023

Ah ah, I removed the enable-preview thing because it was problematic for switching to the Java 21 images for native testing... but we have the opposite problem with the non-native tests now.

I will try to get the Java 21 thing in ASAP.

@mkouba
Copy link
Contributor Author

mkouba commented Oct 23, 2023

Ping @ia3andy ;-)

@ia3andy
Copy link
Contributor

ia3andy commented Oct 23, 2023

Thanks @mkouba :), It's on my today's daily tasks.

I had a few urgent things to do as I am just back from a conf trip. I am having a look now !

@ia3andy
Copy link
Contributor

ia3andy commented Oct 23, 2023

Ok it's still not working, I am debugging...

ia3andy
ia3andy requested changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@ia3andy ia3andy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

That should help:
image

It seems the Map contains a relative path, while the check is on the full path in the contains, is that expected?.

@ia3andy
Copy link
Contributor

ia3andy commented Oct 23, 2023

Also here (I guess this is why it doesn't work):
image

@quarkus-bot

This comment has been minimized.

Sign in to view
@ia3andy
Copy link
Contributor

ia3andy commented Oct 24, 2023

Hey @mkouba,

I've tested it in the situation that caused the issue and it now works as expected 🎉

This part of the code seems to have grown a bit too tentaculous, my concern is more on the fact you are not confident with your commit (which is a good improvement already), maybe we should create an issue and expand on those concerns?

ia3andy
ia3andy approved these changes Oct 24, 2023
View reviewed changes
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.

Thanks @mkouba

@mkouba
Copy link
Contributor Author

mkouba commented Oct 24, 2023

Hey @mkouba,

I've tested it in the situation that caused the issue and it now works as expected 🎉

This part of the code seems to have grown a bit too tentaculous, my concern is more on the fact you are not confident with your commit (which is a good improvement already), maybe we should create an issue and expand on those concerns?

Yes, we should definitely create a tracking issue to refactor this area.

@mkouba
Copy link
Contributor Author

mkouba commented Oct 24, 2023

The test failures seem to be related. Investigating...

@mkouba mkouba force-pushed the issue-36472 branch 2 times, most recently from 74365b7 to cfb4f27 Compare October 25, 2023 16:01
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 25, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Oct 27, 2023

It seems that some of the failures are related. Investigating again...

@mkouba mkouba force-pushed the issue-36472 branch 2 times, most recently from f7b9a80 to 93480e9 Compare October 27, 2023 08:53
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Oct 31, 2023

@ia3andy Could you pls retest your app with this PR? I did some more refactoring and I believe that it's ready to merge.

@gsmet I wonder if it's even possible these days to pass all the CI jobs if you make a change in the core modules? I mean there are timeouts, flaky tests, etc.

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 2, 2023

Failing Jobs - Building fc1b83f

Status Name Step Failures Logs Raw logs Build scan
✖ JVM Tests - JDK 11 Build Failures Logs Raw logs
✖ JVM Tests - JDK 17 Build Failures Logs Raw logs
✖ JVM Tests - JDK 21 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/hibernate-orm-compatibility-5.6/mariadb integration-tests/hibernate-orm-compatibility-5.6/postgresql

📦 integration-tests/hibernate-orm-compatibility-5.6/mariadb

✖ io.quarkus.it.hibernate.compatibility.CompatibilityTest.offsetTime line 122 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 11:58:30+01:00

📦 integration-tests/hibernate-orm-compatibility-5.6/postgresql

✖ io.quarkus.it.hibernate.compatibility.CompatibilityTest.offsetTime line 122 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 11:58:30+01:00

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/hibernate-orm-compatibility-5.6/mariadb integration-tests/hibernate-orm-compatibility-5.6/postgresql

📦 integration-tests/hibernate-orm-compatibility-5.6/mariadb

✖ io.quarkus.it.hibernate.compatibility.CompatibilityTest.offsetTime line 122 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 11:58:30+01:00

📦 integration-tests/hibernate-orm-compatibility-5.6/postgresql

✖ io.quarkus.it.hibernate.compatibility.CompatibilityTest.offsetTime line 122 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 11:58:30+01:00

⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/hibernate-orm-compatibility-5.6/mariadb integration-tests/hibernate-orm-compatibility-5.6/postgresql

📦 integration-tests/hibernate-orm-compatibility-5.6/mariadb

✖ io.quarkus.it.hibernate.compatibility.CompatibilityTest.offsetTime line 122 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 11:58:30+01:00

📦 integration-tests/hibernate-orm-compatibility-5.6/postgresql

✖ io.quarkus.it.hibernate.compatibility.CompatibilityTest.offsetTime line 122 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 11:58:30+01:00

@ia3andy
Copy link
Contributor

ia3andy commented Nov 3, 2023

testing...

@ia3andy
Copy link
Contributor

ia3andy commented Nov 3, 2023

@mkouba it works... thanks!!

@mkouba
Copy link
Contributor Author

mkouba commented Nov 6, 2023

Test failures are unrelated - merging...

@mkouba mkouba merged commit 2776bde into quarkusio:main Nov 6, 2023
48 of 51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Nov 6, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 6, 2023
@ia3andy ia3andy mentioned this pull request Nov 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ia3andy ia3andy ia3andy approved these changes

@glefloch glefloch glefloch approved these changes

@stuartwdouglas stuartwdouglas Awaiting requested review from stuartwdouglas

Assignees
No one assigned
Labels
area/core area/devmode area/qute The template engine kind/bugfix
Projects
None yet
Milestone
3.6.0.CR1
Development

Successfully merging this pull request may close these issues.

HotDeploymentWatchedFileBuildItem predicate not working when auto-build is enabled (vscode)
5 participants
@mkouba @ia3andy @gsmet @cescoffier @glefloch