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

Qute: ultimate fix for the problem with registering NativeImageResourceBuildItem correctly on Windows #40158

Merged
merged 3 commits into from Apr 22, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Apr 19, 2024

This pull request should contain the ultimate fix for the problem with registering NativeImageResourceBuildItem correctly on Windows.

It also fixes #40055.

There's a zulip topic about the contract of NativeImageResourceBuildItem: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/NativeImageResourceBuildItem.20-.20format.20of.20resource.20params.3F. However, this problem will be probably addressed in a separate PR.

We've also added the quarkus-qute-integration-test module into the native Windows CI category. Hopefully, this will improve our ability to detect this kind of problems.

@quarkus-bot quarkus-bot bot added area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/qute The template engine labels Apr 19, 2024
@mkouba mkouba requested review from gsmet and gastaldi April 19, 2024 14:22
@gastaldi
Copy link
Contributor

Looks like this needs to be backported too?

@mkouba
Copy link
Contributor Author

mkouba commented Apr 19, 2024

Looks like this needs to be backported too?

Yes, if possible...

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 19, 2024

Status for workflow Quarkus CI

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

✅ 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)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

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.

I think we can merge but I wouldn't qualify it as ultimate as I think the issue with NativeImageResourceBuildItem being brittle is not fixed :)

watchedPaths.produce(new HotDeploymentWatchedFileBuildItem(osAgnosticResourcePath, restartNeeded));
nativeImageResources.produce(new NativeImageResourceBuildItem(osSpecificResourcePath));
watchedPaths.produce(new HotDeploymentWatchedFileBuildItem(resourcePath, restartNeeded));
nativeImageResources.produce(new NativeImageResourceBuildItem(resourcePath));
Copy link
Member

Choose a reason for hiding this comment

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

It's good enough, probably but we still have the issue that this is gonna be interpreted like a regexp, right? So . will mean any char for instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that's another issue that should be addressed in a separate pull request.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 22, 2024

I think we can merge but I wouldn't qualify it as ultimate as I think the issue with NativeImageResourceBuildItem being brittle is not fixed :)

Yes, it's "ultimate" from Qute POV, not from the core POV ;-)

@gsmet gsmet merged commit 1ef8ebe into quarkusio:main Apr 22, 2024
51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 22, 2024
@gsmet gsmet modified the milestones: 3.11 - main, 3.10.0 Apr 23, 2024
@gsmet gsmet modified the milestones: 3.10.0, 3.9.5 Apr 26, 2024
@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

@mkouba@ @gastaldi @gsmet : Unfortunately, I have some problems with these additions. I am running on Linux and have some @CheckedTemplates in places like src/main/resources/templates/foo/bar.xml. When we arrive at io.quarkus.qute.deployment.QuteProcessor#scanTemplateRootSubtree, the default quarkus.qute.template-path-exclude ("^\\..*|.*\\/\\..*$") now matches templatePath, which is ../../classes/templates/foo/bar.xml at this point. The only Qute-specific configuration I have is quarkus.qute.suffixes=html,txt,xml.

Does #40067 resolve this already? Sadly, I don't really have time to contribute here for the next 2 months, sorry!

@mkouba
Copy link
Contributor Author

mkouba commented Apr 30, 2024

@mkouba@ @gastaldi @gsmet : Unfortunately, I have some problems with these additions. I am running on Linux and have some @CheckedTemplates in places like src/main/resources/templates/foo/bar.xml. When we arrive at io.quarkus.qute.deployment.QuteProcessor#scanTemplateRootSubtree, the default quarkus.qute.template-path-exclude ("^\\..*|.*\\/\\..*$") now matches templatePath, which is ../../classes/templates/foo/bar.xml at this point. The only Qute-specific configuration I have is quarkus.qute.suffixes=html,txt,xml.

Does #40067 resolve this already? Sadly, I don't really have time to contribute here for the next 2 months, sorry!

In which mode does it happen? Dev/prod? We do have a test for the default value of quarkus.qute.template-path-exclude. I've tried to add a specific test for a template located in templates/bar/_foo.txt but it seems to work.

CC @aloubyansky

@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

@mkouba : super interestingly, it does NOT occur in dev, but only during my tests.

By the way: the @CheckedTemplates are annotated like this:

@CheckedTemplate(basePath = "foo",
                 defaultName = CheckedTemplate.HYPHENATED_ELEMENT_NAME)
public class ManagerTemplates {
    public static native TemplateInstance bar(/*arguments omitted*/)
}

But I have also tried to set basePath to "/foo", "templates/foo", "/templates/foo", but all of them produce a No template matching the path foo/bar could be found for: org.acme.FooTemplates.bar

@mkouba
Copy link
Contributor Author

mkouba commented Apr 30, 2024

super interestingly, it does NOT occur in dev, but only during my tests.

@mzellho Do you run the tests from the command line or IDE?

By the way: the @CheckedTemplates are annotated like this:

CheckedTemplate.HYPHENATED_ELEMENT_NAME has no effect here - it's only used if the method name contains upper-case letters.

@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

@mkouba: Well, in reality my template / static method is not called bar, but contains camelCase, e.g. "confirmationReport" :-)

Both, I had it failing on our Build Server when running a mvn package, but also when running it in IntelliJ via right click on test/java and so on.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 30, 2024

Well, in reality my template / static method is not called bar, but contains camelCase, e.g. "confirmationReport" :-)

Sure ;-)

Both, I had it failing on our Build Server when running a mvn package, but also when running it in IntelliJ via right click on test/java and so on.

Ok, your build server is also Linux? Does it fail if you run the tests locally with mvn clean test?

@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

Yes, both are on Linux and it also fails locally with a mvn clean test

@mkouba
Copy link
Contributor Author

mkouba commented Apr 30, 2024

Yes, both are on Linux and it also fails locally with a mvn clean test

Uf, I wasn't able to reproduce the problem locally with the qute-quickstart which also contains @CheckedTemplate and the template is located in templates/ItemResource/items.html.

What version of Quarkus do you use?

@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

3.9.5, 3.9.4 worked nicely. I'll try to reproduce my setup with that repo see if I can reproduce the issue.

@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

So, reproduced, but I don't think it would affect too many people, and likely it also doesn't need to be fixed urgently.

The scenario is that there is a @CheckedTemplate at src/main/resources/templates/foo/barBaz.xml and another file at src/test/resources/templates/foo/barBaz-test.xml, that holds the expected value for the assertion inside the test. Until the file within the test resources exists, everything is working nicely, but as soon as the test resource is there, it seems that only files from src/test/resources/templates are being looked at and that cannot the actual template cannot be found anymore.

I will also share the reproducer in a bit / after lunch.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 30, 2024

I will also share the reproducer in a bit / after lunch.

Pls create a new issue and attach the reproducer there. Thanks a lot!

@mzellho
Copy link
Contributor

mzellho commented Apr 30, 2024

alright, thank you --> #40366

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/qute The template engine triage/backport-3.8 triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to compile qute codestart to native on windows
5 participants