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 on the bash/sh code #22

Open
daniel100097 opened this issue Jun 3, 2018 · 15 comments · May be fixed by #40
Open

Windows support on the bash/sh code #22

daniel100097 opened this issue Jun 3, 2018 · 15 comments · May be fixed by #40

Comments

@daniel100097
Copy link

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

Error: spawn bash ENOENT
`

@zephraph
Copy link
Contributor

zephraph commented Jun 3, 2018

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

@zephraph
Copy link
Contributor

zephraph commented Jun 4, 2018

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.

@AddictArts
Copy link

+1

@tunnckoCore
Copy link
Contributor

Such issues are not very helpful. 😕

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

@AddictArts
Copy link

AddictArts commented Jun 6, 2018

@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.

@zephraph
Copy link
Contributor

zephraph commented Jun 6, 2018

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.

@tunnckoCore
Copy link
Contributor

tunnckoCore commented Jun 6, 2018

@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)

@AddictArts
Copy link

AddictArts commented Jun 6, 2018

@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

@tunnckoCore
Copy link
Contributor

tunnckoCore commented Jun 6, 2018

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.

@daniel100097 daniel100097 changed the title Windows support ? Windows support on the bash/sh code Jun 6, 2018
@tunnckoCore
Copy link
Contributor

tunnckoCore commented Jun 6, 2018

@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?

@AddictArts
Copy link

@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.
nodejs/node-v0.x-archive#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.

@tunnckoCore
Copy link
Contributor

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.

@AddictArts
Copy link

@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.

@tunnckoCore
Copy link
Contributor

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

@kt3k
Copy link

kt3k commented Jun 28, 2018

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

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 a pull request may close this issue.

5 participants