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

Windows support to run integration tests with Gradle #14051

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Oct 9, 2015

Implementation note:

The Windows elasticsearch.bat start script does not provide a daemon mode (nor is it easy to add one). As Gradle's Exec task does not support spawning a process in the background, I had to fall back to Ant's exec task. To make things uniform for Windows and Unix, I removed the daemon flag from the Unix shell script invocation. As consequence, the error output is a bit reduced.

Relates to #13930.

@ywelsch ywelsch added the :Delivery/Build Build or test infrastructure label Oct 9, 2015
@ywelsch
Copy link
Contributor Author

ywelsch commented Oct 9, 2015

@rjernst can you have a look?

@@ -44,14 +45,18 @@ class ClusterConfiguration {

@Input
void plugin(String name, FileCollection file) {
setupCommands.put(name, ['bin/plugin', 'install', new LazyFileUri(file: file)])
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
setupCommands.put(name, ['call', 'bin/plugin', 'install', new LazyFileUri(file: file)])
Copy link
Member

Choose a reason for hiding this comment

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

In the ant integ tests, we do this logic within the exec code. I think we should do the same (ie in ClusterFormationTasks, where we set the exec args, we can prepend call)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but I would like to go one step further: The method void plugin(String name, FileCollection file) should just store the <String, FileCollection> pairs. The method void setupCommand(String name, Object... args) can be removed (as it is too broad and OS-dependent). This leaves it up to ClusterFormationTasks.groovy to implement how plugins are actually installed.

@rjernst
Copy link
Member

rjernst commented Oct 10, 2015

As consequence, the error output is a bit reduced.

This is my main concern. We need to find a way to have the output of the current command (ie starting ES since this is the only one that needs to spawn) output when there is a failure.

There is a longstanding gradle issue for this:
https://issues.gradle.org/browse/GRADLE-1254

And from that, it seems there is a plugin to do exactly what we want?
https://github.com/marcovermeulen/gradle-spawn-plugin

@rjernst
Copy link
Member

rjernst commented Oct 10, 2015

Nevermind about that plugin, it has some inherent flaws in the api (eg it expects the process to emit something on stdout when it is "ready", and it is meant to just support unix).

Maybe it is worthwhile to use the ant impl as a template? It is not much code and self contained.
https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/taskdefs/Exec.java

Even better would be to try and fix this in gradle...the code already has an "isDaemon" flag, but it is completely internal (looks like it is used for the gradle daemon).
https://github.com/gradle/gradle/blob/master/subprojects/core/src/main/groovy/org/gradle/process/internal/ExecHandleRunner.java

@ywelsch
Copy link
Contributor Author

ywelsch commented Oct 13, 2015

My understanding of the original implementation is as follows (please correct me if I'm wrong). The daemon flag is added as a parameter to the Gradle Exec task. This means that the Unix shell script spawns the Elasticsearch Java process with exec "$JAVA" ... org.elasticsearch.bootstrap.Elasticsearch start "$@" <&- & and does not wait for the forked process to end. Grade's errorOutput only captures errors in the shell script but not any errors related to the actual Elasticsearch Java process.

The question now is: Why can't we just do the same thing on Windows? First of all, the Windows .bat file does not have a daemon option (yet). This could be implemented, however, using start "Elasticsearch" cmd /c "%JAVA_HOME%\bin\java" .... Seems easy at first but does not work unfortunately. The reason is that child processes spawned using start inherit filehandles stdin, stdout and stderr. This means that Gradle's exec process cannot exit as filehandles of the Elasticsearch Java child process are still open. Unfortunately, there is no easy way to spawn a proper daemon process from a .bat file. The hacky solution for this is to use another bootstrapping executable that starts the actual command using CreateProcess with bInheritHandles set to false. Possible solutions for this are to write a custom executable in C (1), or to use the windows scripting host cscript.exe (2).

  1. http://stackoverflow.com/questions/1536205/running-another-program-in-windows-bat-file-and-not-create-child-process
  2. https://wiki.jenkins-ci.org/display/JENKINS/Spawning+processes+from+build and
    https://wiki.jenkins-ci.org/download/attachments/1835010/antRunAsync.js

I see two solutions here:

  • Support the same error handling behavior for Windows as for Linux. This means implementing a daemon flag in elasticsearch.bat, for example using cscript.exe. Still feels like a hack on Windows.
  • Not support the same error handling behavior. The implementation for Linux is kept as is. The Windows batch file is started in ClusterFormationTasks.groovy by using Ant's exec task that supports spawn (Future work would be to add the spawn functionality to Gradle's Exec task). No error output is captured for Windows.

http(url: "http://localhost:${config.httpPort}")
}
}
if (ant.properties["failed${task.name}#start"]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The condition was not properly working before with
if (ant.properties.containsKey("failed${task.name}#start")) { because containsKey is called with a GString which does not equal to the represented String.

Copy link
Member

Choose a reason for hiding this comment

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

Can we do an actual check like .isEmpty() == false? I don't like relying on groovyness here..

@rjernst
Copy link
Member

rjernst commented Oct 14, 2015

LGTM, thank you for this!

@rjernst
Copy link
Member

rjernst commented Oct 14, 2015

Maybe add a longer comment in the windows exec block explaining why it is different (and maybe create an issue to follow up on this)?

@ywelsch ywelsch force-pushed the fix/windows-support-for-gradle-integrationtests branch from 627fd95 to 1816af5 Compare October 14, 2015 17:30
ywelsch pushed a commit that referenced this pull request Oct 14, 2015
…-integrationtests

Windows support to run integration tests with Gradle
@ywelsch ywelsch merged commit bd43c74 into elastic:burn_maven_with_fire Oct 14, 2015
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants