apache / incubator-kie-issues

Apache License 2.0
12 stars 1 forks source link

`dmn-marshaller-backend-compatibility-tester` install script failing on Windows #1183

Open thiagoelg opened 6 months ago

thiagoelg commented 6 months ago

If Windows has WSL installed it will make the command bash execute WSL by default instead of git-bash. This is a problem because the jbang library makes the following comparison to know how to install the jbang binary:

if (shell.which("curl") && shell.which("bash")) {
  console.log("using curl + bash:", argLine);
  cmdResult = shell.exec("curl -Ls https://sh.jbang.dev | bash -s - " + argLine);
} else if (shell.which("powershell")) {
  console.log("using powershell:", argLine);
  shell.exec('echo iex "& { $(iwr -useb https://ps.jbang.dev) } $args" > %TEMP%/jbang.ps1');
  cmdResult = shell.exec('powershell -Command "%TEMP%/jbang.ps1 ' + argLine + '"');
}

Having bash pointing to WSL means that the jbang binary is being installed inside WSL and it's not available on Windows itself.

Looking at the library I propose 3 possible fixes:

  1. Open a PR on the jbang npm library with a fix (checking if bash points to git-bash or the WSL bash, falling back to the Powershell install method);
  2. Remove the library from our dependencies and write our own script to install the jbang binary during bootstrap;
  3. Remove the library and add jbang as a repository dependency via kieTools.requiredPreinstalledCliCommands on the package.json.
yesamer commented 6 months ago

Thank you @thiagoelg for catching this and thank you for the proposed solutions.

It seems that we have another package using jbang for testing (kogito-swf-common), and a script is used for that case. Do you think we can use the same approach and unify the code in a single place?

As an alternative, I would evaluate the option 3 you proposed. What's not clear: I see how is used in the package.json with Java, but in that case Java is already installed in the machine, right? How jbang installation should be managed in such a case? Thx :)

thiagoelg commented 6 months ago

Oh, yeah, kogito-swf-common has the same issue, I didn't catch it because we skipped its build/test scripts on WIndows. The script there still uses bash and doesn't check the current OS.

For option 3, whoever is developing on kie-tools is responsible for installing jbang on their systems (we need to add this to the README and the devbox.json as a convenience). The property on the package.json only purpose is to warn that there could be missing binaries during bootstrap (if the user bootstrap only a subset of packages, and this subset doesn't include a package that needs jbang, the warning is not shown).

yesamer commented 6 months ago

ok, what I like about option 3 is that doesn't force all the kie-tools users to install jbang if they are not interested in running the tests. The drawback is that users interested in running those tests (and our CI-CD) must manually install it. That is the same approach we're using with Java. Option 1 would be the ideal solution, but it doesn't depend on us - jbang-npm community doesn't seem so responsive. Option 2 is the opposite of the 3rd option.

thiagoelg commented 6 months ago

ok, what I like about option 3 is that doesn't force all the kie-tools users to install jbang if they are not interested in running the tests. The drawback is that users interested in running those tests (and our CI-CD) must manually install it. That is the same approach we're using with Java.

Somehow the dmn-marshaller-backend-compatibility-tester is an indirect dependency of online-editor and probably many other packages, so most kie-tools users will probably need jbang.

If we want to rewrite the jbang install script, it should be easy (the jbang library is really simple), but I personally don't like to install global binaries or call install scripts without the developer knowledge or permission, by just running pnpm boostrap. Maybe we could install the binary locally? kogito-serverless-operator installs required binaries inside a ./bin directory on the package itself and then executes them via their path (./bin/binaryName arg1 arg2), instead of adding them to the global PATH environment variable.

tiagobento commented 4 months ago

+1 for installing the binaries locally.