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 Git Bash fix #98

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikehenry-io
Copy link

Maybe something as simple as this might work satisfactorily for solving #96?

I.e. we assume Windows only if OS is Windows (of course) and there is no *n?x style shell.

Maybe something as simple as this might work satisfactorily?
I.e. we assume Windows only if OS is Windows (of course) and there is no *n?x style shell.
@scottkurz
Copy link
Member

Thanks Mike, I'm not an expert but trying them all quick, that code does seem to solve the problem of detecting whether we're in cygwin, Git Bash vs. Windows cmd or Powershell.

There's still the "migration" concern though. I'm not sure we could introduce this mid-stream without making you opt-in via some new configuration.

I will try to gather some more input on this question.

@mikehenry-io
Copy link
Author

Sounds great, Scott! How to opt-in when running in Maven? By introducing a new build parameter in the plugin?

@cherylking cherylking requested a review from chyt June 14, 2019 14:19
@scottkurz
Copy link
Member

I think we're OK with the idea of switching to this as a new default behavior. Not seeing a real down side still, and if we're wrong we can fix it later and tell users to avoid the version(s) in question if they need to.

Let me just look over the rest of the isWindows logic in AbstractTask.java that I mentioned in #96.

Thanks again for the PR.

@scottkurz
Copy link
Member

scottkurz commented Jun 27, 2019

Unfortunately this fix doesn't seem like it's going to be enough, since the /bin/server script doesn't seem to be executable to the ProcessBuilder API we're using to launch the server.

Trying this results in Git Bash & Cygwin gives:

[ERROR] Failed to execute goal net.wasdev.wlp.maven.plugins:liberty-maven-plugin:2.8-SNAPSHOT:run (default-cli) on project batch-bonuspayout-application: java.io.IOException: Cannot run program "C:\git\sample.batch.bonuspayout\target\liberty\wlp/bin/server" (in directory "C:\git\sample.batch.bonuspayout\target\liberty\wlp"): CreateProcess error=193, %1 is not a valid Win32 application -> [Help 1]

You can see the code we use to build the ProcessBuilder command in ServerTask.java here:

        if (isWindows) {
            binDirectory = installDir + "\\bin\\";
            embeddedServerJar = binDirectory + "tools\\ws-server.jar";
            wlp = "\"" + binDirectory + "server.bat" + "\"";
            processBuilder.environment().put("EXIT_ALL", "1");
        } else {
            binDirectory = installDir + "/bin/";
            embeddedServerJar = binDirectory + "tools/ws-server.jar";
            wlp = binDirectory + "server";
        }

which we later launch with:

        processBuilder.command(command);
        Process p = processBuilder.start()

Having noticed this, I didn't bother trying to confirm the isWindows path I mentioned in #96 (for handing I/O from the child process) still functions as expected.

@scottkurz
Copy link
Member

So having made that comment, what about the idea of saving the value of System.getenv("SHELL") and building a ProcessBuilder command using this shell to launch ".../bin/server, etc." ?

@mikehenry-io
Copy link
Author

I see now that stashing $SHELL might not be enough since ProcessBuilder needs an actual (Windows-) executable to run; e.g. "%EXEPATH%\git-bash.exe -c .../bin/server ...".
I don't have Cygwin installed, but it seems like it is started with "%EXEPATH%\bash.exe" and in that case it is worth trying to stat one or the other and see if it can be launched.

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

Successfully merging this pull request may close these issues.

2 participants