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

Native image - Correct Windows paths in resources #31308

Closed
wants to merge 2 commits into from

Conversation

melloware
Copy link
Contributor

Fix #31307: Correct Windows paths in native image

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 20, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

@melloware melloware changed the title Fix #31307: Correct Windows paths in native image Native image - Correct Windows paths in resources Feb 20, 2023
@quarkus-bot

This comment has been minimized.

@@ -13,6 +13,7 @@
public class ResourceHelper {

public static void registerResources(String resource) {
resource = resource.replace('\\', '/')); // correct Windows paths
Copy link
Member

Choose a reason for hiding this comment

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

this probably fixes it but why is the resource using non portable paths to begin with? should be platform neutral before it gets here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this doesn't seem right. We should tackle this earlier. At this point the path has already been written by Quarkus to a txt file and parsed in the wrong format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You guys are the experts 😄

Copy link
Member

Choose a reason for hiding this comment

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

@zakkak will you have a look at this issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but the sooner I will probably be able to have a look at it will be late next week.

In the meantime, #31185 is now merged, @melloware could you please confirm the issue is still present in Quarkus main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am running on 2.16.3 not on SNAPSHOT so I have not confirmed it in main yet. Will try to get to it.

@melloware
Copy link
Contributor Author

Agreed! But it's outside my depth of knowledge of why it's wrong when it gets here.

@Karm
Copy link
Member

Karm commented Feb 23, 2023

@zakkak I will drive this issue to a conclusion in #31307
I have Docker Desktop and Podman for Windows up and some slashes hunt exps https://github.com/quarkusio/quarkus/pull/25867/files

@melloware
Copy link
Contributor Author

@Karm would you like me to close this PR?

@Karm
Copy link
Member

Karm commented Feb 26, 2023

This is not the root cause for Quarkus Native Primefaces Windows problems. Let me close the PR now.
We carry on in #31307

  • Podman for Windows based run needs Q patches (I have it locally, PR pending)
  • Docker Desktop based run goes all the way up to starting the test container
  • pending PR to Prime faces integration tests to make it happen (custom Dockerfile installs fontconfig, uses quarkus-container-image-docker flow)

Both Podman and Docker though get stuck on a locked logfile target/quarkus.log. So that's what I am looking into now.
I think if we explicitly open just for reading the Windows lock might let us.

@Karm Karm closed this Feb 26, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Feb 26, 2023
@melloware melloware deleted the patch-1 branch February 26, 2023 11:26
@melloware
Copy link
Contributor Author

Great work. I think there will also be a bunch of open Native Windows tickets you can close here too once done as I found about 3-4 issues that I am pretty sure are already fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native Build on Windows has incorrect resource slashes
5 participants