Fix Banner check on filesystem containing special chars #24635

andreaTP · 2022-03-30T12:10:26Z

This is a fix for the issue discovered in Keycloak here: keycloak/keycloak#10954

I'm completely open for guidance in changing the approach for the test.

aloubyansky · 2022-03-30T13:23:41Z

core/deployment/src/main/java/io/quarkus/deployment/steps/BannerProcessor.java

@@ -114,9 +113,10 @@ private boolean isQuarkusCoreBanner(URL url) throws IOException {
        // We determine whether the banner is the default by checking to see if the jar that contains it also
        // contains this class. This way although somewhat complicated guarantees that any rename of artifacts
        // won't affect the check
-        try (JarFile jarFile = new JarFile(Paths.get(new URI(jarPath)).toFile())) {
+        try {
+            JarFile jarFile = new JarFile(new File(jarPath));


The jarFile needs to be closed. Was there a reason to move it inside the try?

no, you are right @aloubyansky reverted 👍 good catch!

aloubyansky

Approved. Thanks @andreaTP
I am not a fan of committing JARs into the repo but perhaps it's just me. An alternative could be to generate them preparing for the test run. @gsmet do you have any opinion on that?

andreaTP · 2022-03-30T15:48:44Z

@aloubyansky thanks! I'm not a fan of committing jar files either, in this case we are actually committing 2 plain zip files (renamed to .jar) and they both only contain one single (different) empty file each.

gsmet · 2022-03-30T16:04:14Z

I think it's OK in theory but my guess is that Windows might not like it. We will see what CI says.
If you can reproduce your problem with é or à, I would recommend to use these characters to be a bit more portable.

andreaTP · 2022-03-30T16:29:16Z

@gsmet you are right, the CI is failing on Windows 🙁
Unfortunately the é and à characters are not triggering the issue.

Can we skip this test on Windows? Any alternative approach?

gsmet · 2022-03-30T16:45:27Z

Let's wait for the full CI report and we will decide from there.

quarkus-bot · 2022-03-31T15:12:57Z

Failing Jobs - Building `b05e852`

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

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/smallrye-reactive-messaging-amqp/deployment 
! Skipped: integration-tests/reactive-messaging-amqp

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

✖ io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest.test line 30 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

gsmet · 2022-03-31T15:21:44Z

Thanks!

Postremus · 2022-04-01T07:49:49Z

FTR. This introduces a regression, see #24686.

gsmet · 2022-04-01T11:40:14Z

I had to revert this fix and the jar files added in this PR also breaks the Jakarta transformation:

jbang alias add --name transform org.eclipse.transformer:org.eclipse.transformer.cli:0.2.0
jbang transform -o core TEMP

leads to:

[main] ERROR Transformer - Unexpected failure:
java.lang.NullPointerException: name
	at java.base/java.util.Objects.requireNonNull(Objects.java:233)
	at java.base/java.util.zip.ZipEntry.<init>(ZipEntry.java:106)
	at org.eclipse.transformer.action.impl.ContainerActionImpl.apply(ContainerActionImpl.java:281)
	at org.eclipse.transformer.action.impl.ContainerActionImpl.apply(ContainerActionImpl.java:159)
	at org.eclipse.transformer.action.impl.ActionImpl.apply(ActionImpl.java:622)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:103)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.transform(DirectoryActionImpl.java:93)
	at org.eclipse.transformer.action.impl.DirectoryActionImpl.apply(DirectoryActionImpl.java:63)
	at org.eclipse.transformer.Transformer$TransformOptions.transform(Transformer.java:1781)
	at org.eclipse.transformer.Transformer.run(Transformer.java:1874)
	at org.eclipse.transformer.jakarta.JakartaTransformer.main(JakartaTransformer.java:30)

andreaTP · 2022-04-01T12:15:27Z

sorry for the trouble!

@gsmet would you like to provide guidance on how to test this?

gsmet · 2022-04-01T13:07:46Z

See the jbang lines above. If you just run them at the root of the Quarkus project, you will have the error.

I tried building a proper jar with the jar command but it didn't fix the issue and then I gave up :).

andreaTP · 2022-04-01T13:29:23Z

Thanks, I'll take a look next week probably.

gsmet · 2022-04-01T13:43:03Z

Yeah it's quite weird. I haven't digged too much as I had too many other things on my plate.

Postremus · 2022-04-01T17:59:10Z

Hi @andreaTP,

if you want, I can take this effort over.
I already dug a bit into the banner code in #24690. Solving the jakarta issues shouln't be that big of an additional burden (famous last words..).

Alternativly, if you wnat to continue working on this, I could also just verify that everything works on windows?
Up to you how we proceed with these changes, whatever fits better with your shedule.

andreaTP · 2022-04-01T18:20:47Z

@Postremus I will not look into this until next week, if you want to take over feel free to go ahead. I just needed to fix the special characters thing 🙂
Ping me if you open a PR otherwise I'll send a message here when I start to work on this 👍

quarkus-bot bot added the area/core label Mar 30, 2022

This comment has been minimized.

Sign in to view

andreaTP force-pushed the fix-banner-special-chars branch from 9932bac to 0c4ffb7 Compare March 30, 2022 12:18

gsmet requested a review from aloubyansky March 30, 2022 13:02

aloubyansky reviewed Mar 30, 2022

View reviewed changes

andreaTP force-pushed the fix-banner-special-chars branch from 0c4ffb7 to b7ab201 Compare March 30, 2022 13:40

aloubyansky approved these changes Mar 30, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Fix Banner check on filesystem containing special chars

b05e852

andreaTP force-pushed the fix-banner-special-chars branch from b7ab201 to b05e852 Compare March 31, 2022 10:04

gsmet merged commit 46fd4d8 into quarkusio:main Mar 31, 2022

quarkus-bot bot added this to the 2.9 - main milestone Mar 31, 2022

gsmet added triage/backport? labels Mar 31, 2022

Postremus mentioned this pull request Apr 1, 2022

main - Banner is not displayed #24686

Closed

gsmet removed triage/backport-2.7? labels Apr 1, 2022

gsmet removed this from the 2.9 - main milestone Apr 1, 2022

gsmet mentioned this pull request Apr 1, 2022

Fix Banner check on filesystem containing special chars #24690

Merged

famod mentioned this pull request Apr 5, 2022

Bump asm.version from 9.2 to 9.3 #24747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Banner check on filesystem containing special chars #24635

Fix Banner check on filesystem containing special chars #24635

andreaTP commented Mar 30, 2022 •

edited

Loading

This comment has been minimized.

aloubyansky Mar 30, 2022

andreaTP Mar 30, 2022

aloubyansky left a comment

andreaTP commented Mar 30, 2022

gsmet commented Mar 30, 2022

andreaTP commented Mar 30, 2022

gsmet commented Mar 30, 2022

This comment has been minimized.

quarkus-bot bot commented Mar 31, 2022

gsmet commented Mar 31, 2022

Postremus commented Apr 1, 2022

gsmet commented Apr 1, 2022

andreaTP commented Apr 1, 2022

gsmet commented Apr 1, 2022

andreaTP commented Apr 1, 2022

gsmet commented Apr 1, 2022

Postremus commented Apr 1, 2022

andreaTP commented Apr 1, 2022

Fix Banner check on filesystem containing special chars #24635

Fix Banner check on filesystem containing special chars #24635

Conversation

andreaTP commented Mar 30, 2022 • edited Loading

This comment has been minimized.

aloubyansky Mar 30, 2022

Choose a reason for hiding this comment

andreaTP Mar 30, 2022

Choose a reason for hiding this comment

aloubyansky left a comment

Choose a reason for hiding this comment

andreaTP commented Mar 30, 2022

gsmet commented Mar 30, 2022

andreaTP commented Mar 30, 2022

gsmet commented Mar 30, 2022

This comment has been minimized.

quarkus-bot bot commented Mar 31, 2022

Failing Jobs - Building b05e852

Failures

⚙️ JVM Tests - JDK 11 Windows #

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

gsmet commented Mar 31, 2022

Postremus commented Apr 1, 2022

gsmet commented Apr 1, 2022

andreaTP commented Apr 1, 2022

gsmet commented Apr 1, 2022

andreaTP commented Apr 1, 2022

gsmet commented Apr 1, 2022

Postremus commented Apr 1, 2022

andreaTP commented Apr 1, 2022

andreaTP commented Mar 30, 2022 •

edited

Loading

Failing Jobs - Building `b05e852`