maid icon indicating copy to clipboard operation
maid copied to clipboard

Windows support on the bash/sh code

Open daniel100097 opened this issue 6 years ago • 15 comments

`events.js:182 throw er; // Unhandled 'error' event ^

Error: spawn bash ENOENT `

daniel100097 avatar Jun 03 '18 19:06 daniel100097

Could you provide a little more context? What maid file were you trying to run?

zephraph avatar Jun 03 '18 21:06 zephraph

I think this will involve a few things. First, is support for powershell and cmd. That's probably the easiest thing. The next consideration: Should there be a fallback. i.e. a bash script for unix systems and a ps or cmd script for windows. Maybe we can just add a platform support section to checkTypes and have it automatically skip things it doesn't support.

zephraph avatar Jun 04 '18 00:06 zephraph

+1

AddictArts avatar Jun 05 '18 19:06 AddictArts

Such issues are not very helpful. :confused:

Please provide example maidfile and what you run, @AddictArts & @daniel100097 ...

tunnckoCore avatar Jun 06 '18 01:06 tunnckoCore

@olstenlarck

Here you go

## out 

```sh
yarn outdated —depth=0

Pretty much the simplest task will fail. It seems like there is a lack of Windows machines used to test on. I think the error clearly shows “sh” or “bash” does not exist to execute or spawn.

AddictArts avatar Jun 06 '18 02:06 AddictArts

Yep, exactly that. We need to check to ensure the script execution language exists before running it... question is, how do we fail? We probably need a way to denote that a block only runs on a certain platform.

zephraph avatar Jun 06 '18 02:06 zephraph

@AddictArts then just not use sh/bash, use powershell or whatever there is in windows world :D I don't know why anyone will expect to work.

Probably good thing would be @daniel100097 or @egoist to change the title of this issue. We are talking here for windows support on the bash/sh code fences (so called "languages"), which is totally normal to fail, imho. Btw using execa/execa-pro may be better for OS compatibility.

@AddictArts does other "languages" work? e.g. js code fence? Try powershell codefence or how Bash on Windows is started? bash.exe? Sorry but i'm not Windows user for a long long loooong time.

(offtopic: how strage is user with debian-like logo avatar to ask for Windows ;d)

tunnckoCore avatar Jun 06 '18 03:06 tunnckoCore

@olstenlarck it would be great to use command or powershell, but maid doesn't support either, look at the code, checkTypes ... ['sh', 'bash'] ... ['py', 'python'] ... ['js', 'javascript']. That is it.

I would suggest that "sh" work for the "CMD" prompt, but not "bash", unless support for something like mingw32/64 bash terminal was desired. The reason is so that the maid task can run on multi os. Yes I understand that not all commands in an sh task will work, but things like simple echo's would and maybe some others.

@olstenlarck Yes 'js' works

AddictArts avatar Jun 06 '18 18:06 AddictArts

Oh yea, really.. the checkTypes thing.

Okey how we can fix that? With another fence name? Or it is just a problem how the bash is executed on windows?

edit: ahhhh yeah... definitely. If we switch to execa it will be fixed.

tunnckoCore avatar Jun 06 '18 18:06 tunnckoCore

@AddictArts can you please try with the following runCLICommand.js, so we can confirm it works.

const path = require('path')
const execa = require('execa')
const MaidError = require('./MaidError')

module.exports = ({ task, type = task.type, resolve, reject }) => {
  const modulesBin = path.resolve(path.join('node_modules', '.bin'))

  const promise = execa(type, [task.script, ...process.argv.slice(2)], {
    stdio: 'inherit',
    env: Object.assign({}, process.env, {
      PATH: `${modulesBin}:${process.env.PATH}`
    })
  })

  promise.on('close', code => {
    if (code === 0) {
      resolve()
    } else {
      reject(new MaidError(`task "${task.name}" exited with code ${code}`))
    }
  })

  // we don't need to return
  // 1. because this function is executed in new Promise(() => {})
  // 2. we have its resolve and reject, so it's better to use them 
  promise.then(resolve).catch(reject)
}

I definitely believe that it would work.

edit: @egoist probably add AppVeyor?

tunnckoCore avatar Jun 06 '18 18:06 tunnckoCore

@olstenlarck thanks, but I didn't have time to fork and try the patch. Exca is an additional dependency if that matters. Also exca just uses cross-spawn, so maybe just that? https://github.com/moxystudio/node-cross-spawn or https://github.com/MarcDiethelm/superspawn

This did help me learn the issues around spawn and Windows, here are some relevant bits, including cross-spawn above. https://github.com/nodejs/node-v0.x-archive/issues/25895 Gist for ref, see comments: https://gist.github.com/antivanov/20ee20b7a40995b1836c

This issue can be closed, I've abandoned maid for now, really needed it to work out of the box to see how it could be useful. Thanks for the help, suggestions, and comments.

AddictArts avatar Jun 14 '18 18:06 AddictArts

Also exca just uses cross-spawn, so maybe just that?

If it was "just that" it would not exist.

This did help me learn the issues around spawn and Windows

You don't need to waste time.


And yea, even i still can't figure out how to fix it, that's why i paused the work on #40 and that's why it fails. We also need AppVeyor, otherwise it would be just hard to us to continue work on such support.

tunnckoCore avatar Jun 14 '18 18:06 tunnckoCore

@olstenlarck just to be clear, I was inputting that only cross-spawn could be considered versus using exca. Use whatever you want though and the exca project is certainly more than cross-spawn.

I'm not understanding the "waste time" comment; However, for me it was not a waist of time and I hope this helps the project and someone else like it helped me.

AddictArts avatar Jun 14 '18 18:06 AddictArts

Yea, totally. Sorry, don't take me hard, i forgot the emojis :D

tunnckoCore avatar Jun 14 '18 18:06 tunnckoCore

I created a very similar tool, called saku, which only supports shell for task langauge, but works in windows as well.

kt3k avatar Jun 28 '18 09:06 kt3k