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

No longer force uber-jar when building native images on Windows #26289

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

Sanne
Copy link
Member

@Sanne Sanne commented Jun 22, 2022

This reverts #8860 as it seems it's no longer necessary, and also using uber-jar is slower and an additional maintenance burden: by getting rid of it, we ensure all native images are built in the same way.

More importantly, using uber-jars breaks our ability to declare targeted exclusions for graalvm metadata: the --exclude-config parameter requires a regex to match the jar it's targeting.

We issue --exclude-config instructions for each ExcludeConfigBuildItem - so currently this build item would fail the build when running on Windows.

untested as I don't have a Windows machine - it seems simple enough to let CI validate it.

See also:

Example of issues caused by Windows builds being "different":

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't see the CI actually testing it, i.e. it didn't perform a native-image test on Windows.

I scheduled a run on Mandrel's CI.
@Karm is there any chance you could test this as well?

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

@Sanne Sanne force-pushed the NoMoreUberJarForcedForWindowsNative branch from df994da to dd03d60 Compare June 22, 2022 15:28
@Karm
Copy link
Member

Karm commented Jun 22, 2022

I will take a look on our Windows CI and on a workstation. Let's not trust Github windows runners on this as they have extended paths and possible other things tweaked in registers.

@Sanne Sanne requested a review from Karm June 22, 2022 19:48
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 22, 2022

Failing Jobs - Building dd03d60

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

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: devtools/cli 

📦 devtools/cli

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M7:test (default-test) on project quarkus-cli: There was a timeout in the fork

Copy link
Member

@Karm Karm left a comment

Choose a reason for hiding this comment

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

@Sanne @zakkak

This PR brings a regression in comparison to today's main (assuming it's reasonably close to main).

It all takes so much time, but I did 3 test runs of hibernate-validator as that was something where I arbitrarily spotted a regression for the first time. So this is by no means necessarily limited to this one module.

Patched, Windows workstation, Podman running Linux builder image ❌

Builder image: quay.io/quarkus/ubi-quarkus-native-image:22.1-java11
It fails with: %1 is not a valid Win32 application
Full log: https://karms.biz/pastebin/gh-26289-NoMoreUberJarForcedForWindowsNative-podman.txt

Patched, Windows workstation, Mandrel locally ❌

Mandrel version: Mandrel 22.1.0.0-Final Java 17
It fails with: %1 is not a valid Win32 application
Full log: https://karms.biz/pastebin/gh-26289-NoMoreUberJarForcedForWindowsNative.txt

Main, Windows workstation, Mandrel locally ✔️

Mandrel version: Mandrel 22.1.0.0-Final Java 17
Runs just fine.
Full log: https://karms.biz/pastebin/gh-26289-main.txt

@Karm
Copy link
Member

Karm commented Jun 23, 2022

I'll give #25954 a shot and try to suggest a better handling of params here, while I have it all up and running. Unlikely today though :) (the virt Windows disk I/O is killing me, it all takes so much time)

@zakkak
Copy link
Contributor

zakkak commented Jun 23, 2022

@Karm checking the logs I see that the last file separator is a slash instead of a backslash. Where does this come from?

C:\tmp\quarkus\integration-tests\hibernate-validator\target/quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner

@Karm
Copy link
Member

Karm commented Jun 23, 2022

@zakkak Don't know from the top of my head, but there are places in Quarkus where such normalization happens to avoid that, e.g. replace('\\', '/'). GitHub runners are extra benevolent, don't recall the particular registry settings. It might be the pertinent difference. I will take a closer look...

@zakkak
Copy link
Contributor

zakkak commented Jun 24, 2022

After having a better look, in the first case, "Patched, Windows workstation, Podman running Linux builder image". it builds a linux binary using the builder container and then it appears like it tries to run it on the host (windows). I fail to see how this PR could result in this behavior though.

In the second case ,"Patched, Windows workstation, Mandrel locally", it builds a windows binary with the .exe suffix but then tries to run C:\tmp\quarkus\integration-tests\hibernate-validator\target/quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner which shouldn't exist, but even if it exists (from a previous run) it's not what it should run. I again fail to see how this PR could result in this behavior though.

What's interesting though is that in the third case, "Main, Windows workstation, Mandrel locally", it again builds a windows binary with the .exe suffix but then tries (and succeeds) to run C:\tmp\quarkus\integration-tests\hibernate-validator\target/quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.

@Karm
Copy link
Member

Karm commented Jun 24, 2022

The error is indeed the mixing of the slashes. This works fine:

KARM 0: C:\tmp\quarkus\integration-tests\hibernate-validator\target/quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner
KARM 1: C:\tmp\quarkus\integration-tests\hibernate-validator\target/quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner
KARM 2: C:\tmp\quarkus\integration-tests\hibernate-validator\target\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner
Executing "C:\tmp\quarkus\integration-tests\hibernate-validator\target\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner -Dquarkus.http.port=8081 -Dquarkus.http.ssl-port=8444 -Dtest.url=http://localhost:8081 -Dquarkus.log.file.path=C:\tmp\quarkus\integration-tests\hibernate-validator\target\quarkus.log -Dquarkus.log.file.enable=true"

I will append the patch here when ready...

It's easy to just replace the symbol if IS_WINDOWS, but it's a head scratcher as to where it comes from:

System.setProperty("native.image.path", finalExecutablePath.toAbsolutePath().toString());

in NativeImageBuildStep.java is
C:\tmp\quarkus\integration-tests\hibernate-validator\target\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.exe

And then

System.getProperty("native.image.path")

in DefaultNativeImageLauncher.java is
C:\tmp\quarkus\integration-tests\hibernate-validator\target/quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner

The missing .exe is not the problem, it's like with .cmd, the filename without suffix is enough. The / is the problem and replace('/', '\\') fixes that, but I would like to know who and where appended that in the first place :-D

I feel really dumb right now :)

Copy link
Member

@Karm Karm left a comment

Choose a reason for hiding this comment

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

Hello @zakkak and @Sanne,

The PR is fine. I am sorry for the noise.

I wanted to prepare a fresh VM, this time not a W10 workstation (tainted with all Podman and Docker Desktop and WSL2 stuff), but a clean room w2k19.

It works, with locally installed Mandrel, regardless of not/having:

# Enable long paths
$regkey = "HKLM:\SYSTEM\CurrentControlSet\Control\FileSystem"
set-itemproperty -path $regkey -name LongPathsEnabled -value 1

I read @zakkak's comments again, ran it again, and...the problematic thing is the behavior of the podman run I did before the local run. Plus failed clean.

First, the fact that podman run fails: Isn't it supposed to run the binary in an UBI runtime container? Not pertinent to this PR, it's actually the same on main.

Second, something (podman?) having still a lock on the filesystem resulted in a silent mvn clean failure and having two binaries produced, a Linux one (no extension), and a Windows one (exe extension).


       398 quarkus-artifact.properties
           quarkus-integration-test-hibernate-validator-999-SNAPSHOT-native-image-source-jar
76,221,312 quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner
77,078,528 quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.exe

If you call the file like this:

λ C:\tmp\quarkus\integration-tests\hibernate-validator\target\quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner

It works. Because Windows sees that quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner is not a Windows executable and it seamlessly tries quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.cmd and quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.exe. So it starts and works fine.

And that confused the subsequent run, where it appeared quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner was executed while in fact it was quarkus-integration-test-hibernate-validator-999-SNAPSHOT-runner.exe.

Who, where and why mingles the contents of System.setProperty("native.image.path", finalExecutablePath.toAbsolutePath().toString());, I don't know. I will address that in a separate issue/pr. It is not caused by this change.

@Sanne
Copy link
Member Author

Sanne commented Jun 28, 2022

glad to hear :) thank you both for the in-depth testing!

@Sanne
Copy link
Member Author

Sanne commented Jun 28, 2022

To merge it I need an approval though

@Karm
Copy link
Member

Karm commented Jun 28, 2022

@Sanne We are but lesser daemons. We shall wait for one of the great ones equipped with the locket of write access for the approval.

@Sanne Sanne requested a review from geoand June 28, 2022 15:54
@Sanne
Copy link
Member Author

Sanne commented Jun 28, 2022

thanks @geoand :)

@Sanne Sanne merged commit 617d484 into quarkusio:main Jun 28, 2022
@Sanne Sanne deleted the NoMoreUberJarForcedForWindowsNative branch June 28, 2022 20:02
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 28, 2022
@geoand
Copy link
Contributor

geoand commented Jun 29, 2022

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants