Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Add Windows support #127

Merged
merged 8 commits into from
Aug 23, 2019
Merged

Add Windows support #127

merged 8 commits into from
Aug 23, 2019

Conversation

frozenice
Copy link
Contributor

@frozenice frozenice commented Jul 10, 2019

Hi there,

This PR adds Windows support to the only Gradle GraalVM plugin (this one!) - a much needed feature, I imagine (using Windows myself and experimenting with GraalVM). With this PR gradle-graal supports building native images and shared libraries on Windows, if you install the prerequisites GraalVM needs.

I haven't opened an issue, because discussing this code directly instead of theoretically seems better.

How to test

Install the required dependencies for GraalVM to work on windows (see substratevm/README.md) and make sure GraalVM can successfully compile something standalone (remember to use the Windows SDK Command Prompt when running native-image directly).

  • Clone my windows branch
  • Run the Gradle task publishToMavenLocal
  • Run the Gradle task printVersion, this will print something like 0.3.0-38-g2b56d74
  • In your project, add mavenLocal() and the locally published plugin to the buildscript dependencies, e.g.:
    buildscript {
      repositories {
        mavenLocal()
        mavenCentral()
      }
    
      dependencies {
        classpath 'com.palantir.graal:gradle-graal:0.3.0-38-g2b56d74'
      }
    }
    (must be the first block in your build.gradle)
  • Make sure you don't include gradle-graal via a plugins block or otherwise
  • Configure the graal extension, e.g.:
    graal {
      graalVersion "19.1.0"
      mainClass mainClassName
      outputName "graal-test"
    }
    (replace mainClassName with a string containing your entry class name if you haven't applied the application plugin)
  • Run the Gradle task nativeImage, it will print the output location (the sharedLibrary task also works)

Changes

DownloadGraalTask

Enabling the download of the windows archive required adding a WINDOWS case to getOperatingSystem and a new getArchiveExtension method (it's .zip on Windows).

There is a new [ext] placeholder, which is used in ARTIFACT_PATTERN_RELEASE_VERSION (RC versions don't have a Windows variant) and FILENAME_PATTERN. It's replaced in render.

ExtractGraalTask

The recent version 19 support added getExecutable and getArchitectureSpecifiedBinaryPath. I added Windows support to those (a bit of complexity arose, because some executables in the GraalVM Windows distribution have .cmd as an extension, others have .exe).

BaseGraalCompileTask

I added an abstract method getArchitectureSpecifiedOutputExtension so subclasses (NativeImageTask and SharedLibraryTask) can specify the platform-dependant file extension for their output.

There is also a new getArchitectureSpecifiedPathSeparator method, because path separators differ (; in Windows, : on Unix).

A WINDOWS case was added to getArchitectureSpecifiedBinaryPath, so it finds the native-image command.

To overcome the need for running inside the Windows SDK Command Prompt (SetEnv.cmd) I replicated the environment variables configuration, stripped to a minimum (it sets the following environment variables when launching native-image: CL, PATH, LIB, LIBPATH, INCLUDE, APPVER).

To do this, it needs to read some values from the Windows registry. This is done in readWindowsRegistryString, which is a very simple (but sufficient) wrapper around reg.exe to query a value from the registry.

fileSizeMegabytes was lifted from NativeImageTask so it can be used in all subclasses.

NativeImageTask

The overriden method getArchitectureSpecifiedOutputExtension returns .exe on Windows and an empty string on Linux and Mac.

configurePlatformSpecifics is called in the @TaskAction.

SharedLibraryTask

Basically the same as NativeImageTask, the extensions in getArchitectureSpecifiedOutputExtension are different, though.

Oh, and I also added the LogAction, like in NativeImageTask.

Tests

I upgraded some GraalVM version that's used in tests to the current 19.1.0 (it must be 19+ so the tests can run on Windows).

Some cross-platform enhancements include:

  • Specifying .exe and dll for the output file extensions on Windows (could use getArchitectureSpecifiedOutputExtension too)
  • Replacing hardcoded \ns with System.lineSeparator()
  • Escape backslashes in the generated gradle.properties file
  • Allowing both localhost and 127.0.0.1 in requestUrl returned by the mock server
  • Just compare the path of mock server requests (because the hostname is not always 127.0.0.1or even localhost).

The tests allows specifying different RC graal version, test 1.0.0-rc5 nativeImage and can build shared libraries on 1.0.0-rc5 are now ignored on Windows, because there is no GraalVM RC version for Windows.

The test allows specifying different GA graal version was split into a version for Windows and another version for Linux / Mac. There are some things that are different on Windows. This looks a bit like duplicate code (which it is), but parameterizing all the places looked a bit much.

Notes

There are some baseline warnings. SwitchStatementDefaultCase was already present, DefaultCharset is new, but I'm not sure what to do about it. Of course it needs to use the default platform charset to read the command output! Using Charset.defaultCharset() produced another warning.

I haven't run tests on Linux or Mac yet.

Cheers 🍺
David

@palantirtech
Copy link
Member

Thanks for your interest in palantir/gradle-graal, @frozenice! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@frozenice
Copy link
Contributor Author

The tests work on Linux, if you install native-image manually (./gu install native-image). So #119 has to be merged first (preferably with a newer GraalVM version).

@frozenice
Copy link
Contributor Author

I just noticed that the extractGraalTooling task works on my Windows because my it has a native tar command (since some update). So ExtractGraalTask should be split up into a tgz and a zip variant or something along those lines (maybe use a Copy task for zip?). 🤔

getExecutable: add extension on Windows
getArchitectureSpecifiedBinaryPath: add Windows (same as Linux)
ignore RC tests on Windows
use System.lineSeparator() instead of \n
syntax cleanup
compare only the path of mock requests
@frozenice
Copy link
Contributor Author

Seeing that #119 got merged, I rebased and added some more changes (I updated the description). Building works on my Windows (also tried with Graal 19.1.1). CircleCI seems happy, too!

@frozenice
Copy link
Contributor Author

Any chance this can be looked at? Maybe @markelliot?

@koppor
Copy link

koppor commented Aug 19, 2019

I would add following to README.md:

Preconditions when using on Windows

GraalVM needs the Microsoft Windows SDK for Windows 7 and .NET Framework 4 as well as the C compilers from KB2519277.

You can install it using chocolatey:

choco install windows-sdk-7.1 kb2519277

@koppor
Copy link

koppor commented Aug 19, 2019

@frozenice I wonder how "extractGraal()" works on your machine. Here, I get following tar output when trying locally:

$ tar -xzf graalvm-ce-19.0.2-amd64.zip
tar: This does not look like a tar archive

I included unzip in my code: https://github.com/palantir/gradle-graal/pull/139/files#diff-11e63293e0814b10b475bc2f9f2da60bR100

@frozenice
Copy link
Contributor Author

@koppor I wondered about that, too. :)

I'll take a look at your unzip logic and README changes later!

Copy link
Contributor

@markelliot markelliot left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

Would be interested in your thoughts on my comments -- am generally happy to adopt as-is, if you were interested in some refactoring first that'd be helpful, but good either way.

@markelliot
Copy link
Contributor

Using Charset.defaultCharset() produced another warning.

Yep, the standard fix here is to use StandardCharsets.<something>, where nominally we'd mostly expect UTF_8.

@frozenice
Copy link
Contributor Author

Using Charset.defaultCharset() produced another warning.

Yep, the standard fix here is to use StandardCharsets.<something>, where nominally we'd mostly expect UTF_8.

The thing is, I do want to use the default charset for the current platform, because I'm executing a native command which, I assume, also uses the platform's default charset.

I'll leave this as-is for now, as it's only a warning.

@koppor
Copy link

koppor commented Aug 20, 2019

I get following output when running test default version nativeImage

Caused by: java.io.IOException: Cannot run program "tar" (in directory "C:\Users\olive\.gradle\caches\com.palantir.graal\19.0.2"): CreateProcess error=2, Das System kann die angegebene Datei nicht finden
at net.rubygrapefruit.platform.internal.DefaultProcessLauncher.start(DefaultProcessLauncher.java:25)

@frozenice
Copy link
Contributor Author

Good news everyone!

I re(-)searched how to unzip an archive via Gradle APIs (without creating a new CopyTask). Turns out, it's rather simple (if not, I would have copied your unzip method @koppor). :) This commit handles .zip files, which should remove your error @koppor.

As per @markelliot's suggestion, .cmd binary names are now in a Set.

I'm not sure what to put into the readme, yet. If we list explicit preconditions for Windows, we should also list them for the other platforms, but they might change (link). If we don't want to list the explicit items, we should include the link to the Graal docs. @koppor's readme changes include some links, though.

Also check the review comments btw.

@markelliot
Copy link
Contributor

Cool, happy to take this and a README update later. Good to merge?

@frozenice
Copy link
Contributor Author

Waiting for @koppor's reply to this. Still making up my mind about which method to use (registry access or .cmd wrapper script).

Also, GraalVM 19.2.0 got released yesterday, need to run a test with this.

koppor and others added 2 commits August 22, 2019 00:46
- hide SetEnv.cmd ouput if Gradle is not running verbosely
- split long lines, fix style errors
- remove unused variable
@frozenice
Copy link
Contributor Author

We replaced my custom registry stuff with @koppor's temporary script file stuff. This is definitely more maintainable.

CircleCI tests look good, I also tested with Graal 19.2.0 locally.

I created two tracking issues (#141, #142) for the two unresolved items we discussed.

@markelliot Ready to merge!

@markelliot markelliot merged commit 58db239 into palantir:develop Aug 23, 2019
@koppor koppor mentioned this pull request Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants