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

Enable GraalVM tests on Windows #4272

Merged
merged 39 commits into from
Jan 11, 2025
Merged

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Jan 9, 2025

No description provided.

@lihaoyi lihaoyi added the run-all-tests Disables selective test execution on this pR and just runs all tests label Jan 9, 2025
@lihaoyi lihaoyi marked this pull request as ready for review January 10, 2025 04:11
Comment on lines 34 to 36
val executableName =
if (mill.main.client.Util.isWindows) "native-image.exe" else "native-image"

Copy link
Contributor

Choose a reason for hiding this comment

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

I currently just have:
val executableName = "native-image" instead of the if check, because the Graal native-image script will automatically append an .exe extension for Windows builds.

If you don't remove the trailing .exe, the built file will have the name native-image.exe.exe leading to a file not found error in the Mill tests.

val command = Seq.newBuilder[String]
.+=(nativeImageTool().path.toString)
.++=(nativeImageOptions())
.+=("-cp")
.+=(nativeImageClasspath().iterator.map(_.path).mkString(java.io.File.pathSeparator))
.+=(finalMainClass())
.+=(executableName)
.+=((dest / executableName).toString())
.result()
os.proc(command).call(cwd = dest, stdout = os.Inherit)
PathRef(dest / executableName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently using the if-isWindows check here instead:

PathRef(dest / (if (mill.main.client.Util.isWindows) s"${executableName}.exe" else executableName))


> ./out/foo/nativeImage.dest/native-image
> ./out/foo/nativeImage.dest/native-image.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

> ./out/foo/nativeImage.dest/native-image without the .exe worked in my testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it seems that native-image appends the .exe automatically, but at the same time the windows terminal lets you run the file without writing out the .exe and it still works (at least until it hits the classloading issue earlier)

Copy link
Contributor

Choose a reason for hiding this comment

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

I finally got it working here: https://github.com/ayewo/mill/actions/runs/12719302376/job/35459278855

I'll tidy up my changes and open a PR.

@lihaoyi lihaoyi merged commit 4b8205b into com-lihaoyi:main Jan 11, 2025
26 checks passed
@lefou lefou added this to the 0.12.6 milestone Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-all-tests Disables selective test execution on this pR and just runs all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants