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

Yarn run no longer works on windows with executables not mentioned in package.json/scripts #3773

Closed
betalb opened this issue Jun 30, 2017 · 13 comments

Comments

@betalb
Copy link

betalb commented Jun 30, 2017

Do you want to request a feature or report a bug?
bug

What is the current behavior?
When running executable, that is not mentioned in scripts section of package.json using yarn run, yarn fails

yarn run v0.27.3
$ "C:\work\repos\repo\node_modules\.bin\grunt" "--version"
'C:\work\repos\repo\node_modules\.bin\grunt" "--version' is not recognized as an internal or external command,
operable program or batch file.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

If the current behavior is a bug, please provide the steps to reproduce.

  • yarn init
  • yarn add grunt
  • yarn run grunt -- --version

Adding grunt to scripts resolves an issue

"scripts": {
    "grunt": "grunt"
}

What is the expected behavior?
Worked fine in v0.24.6

yarn run v0.24.6
$ "C:\work\repos\repo\node_modules\.bin\grunt" --version
grunt-cli v1.2.0
grunt v1.0.1
Done in 0.51s.

Please mention your node.js, yarn and operating system version.
Yarn version: 0.27.3
Node version: 7.10.0
Platform: win32 x64 (Windows 10)

@arcanis
Copy link
Member

arcanis commented Jun 30, 2017

Hm, we do have a test to check this, and it seems to be passing, even on Windows :/ Does anybody reproduce this? I can't make this happen on OSX.

@bdukes
Copy link

bdukes commented Jun 30, 2017

This appears to only be an issue when there are additional arguments passed. yarn run gulp works for me, but yarn run gulp -- --verbose or yarn run gulp -- watch do not

@jods4
Copy link

jods4 commented Jun 30, 2017

Same here: yarn run webpack -- --config webpack.config.vendor.js used to work.
Not anymore.

@craig-jennings
Copy link

craig-jennings commented Jul 5, 2017

Ran into this when I upgraded from 0.26.1 to 0.27.0 (also seeing this on 0.27.5)

image

My hunch is that this release comment has something to do with it

Fix arguments passed to scripts so that they are correctly escaped

@Volune
Copy link
Contributor

Volune commented Jul 7, 2017

From what I understand, the issue appeared with #3627, but was somehow hidden in the code earlier.

It can be reproduced on Windows with a test on util/execute-lifecycle-script.js:

/* @flow */

import Config from '../../src/config.js';
import {executeLifecycleScript} from '../../src/util/execute-lifecycle-script.js';

test('executeLifecycleScript', async () => {
  const config = await Config.create();
  const cwd = __dirname;
  const cmd = '"node" "--version"';
  await executeLifecycleScript('test', config, cwd, cmd);
  // Will throw an exception on Windows
});

Looking at the code, I don't understand why we need to process and concatenate the arguments, because child_process.spawn can take an array of arguments.

@Spongman
Copy link

child_process.spawn(,,{shell:true}) already handles finding the shell, appending '/d /s /c' on windows and quoting the arguments, if necessary.

@arcanis
Copy link
Member

arcanis commented Jul 17, 2017

Looking at the code, I don't understand why we need to process and concatenate the arguments, because child_process.spawn can take an array of arguments.

Because the script can have multiple arguments, ie:

{
    "scripts": {
        "foobar": "some-utility --some-opt-a --some-opt-b"
    }
}

So if we were to use the version of spawn that takes an array of arguments, we would need to split those scripts, which might be a bit complex - more than quoting arguments.

We'll add a test and try to fix this one, thanks!

@mjschlot
Copy link

mjschlot commented Jul 23, 2017

I just upgraded to Yarn 0.27.5 on Windows, and am now experiencing the same issue. When I run:
yarn run gulp clean-dev

I get:

"clean-dev' is not recognized as an internal or external command

This worked previously to run gulp and pass the task for gulp to run.

@pbarbiero
Copy link

@arcanis any updates? This is a major issue for those of us on windows, this should be flagged as high priority and hopefully not triaged.

@arcanis
Copy link
Member

arcanis commented Jul 25, 2017

Since fixing this bug requires a working Windows machine I haven't been able to check it yet. We will eventually do it, but it would help us a lot and would be faster if a Windows user could submit a PR.

@Spongman
Copy link

Facebook doesn't have Windows Dev VMs?

@Volune
Copy link
Contributor

Volune commented Jul 27, 2017

I made a pull request with a fix at #4031.

I tested it on my Windows and a linux VM.
There are no good coverage in automated tests, so more tests from the community (at least one on mac) will be appreciated.

For testing, checkout the branch, then:

yarn install
yarn build
#Windows
.\bin\yarn run gulp -- --version
.\bin\yarn run lint
.\bin\yarn run eslint
#Mac/linux
./bin/yarn run gulp -- --version
./bin/yarn run lint
./bin/yarn run eslint

Thanks

@BYK
Copy link
Member

BYK commented Jul 27, 2017

Facebook doesn't have Windows Dev VMs?

We do, it just takes you out of your normal flow and Yarn is almost exclusively run on macOS and Linux. That said I'm hoping to change this soon, it just takes time.

BYK pushed a commit that referenced this issue Jul 27, 2017
**Summary**

Fix for #3773
> Yarn run no longer works on windows with executables not mentioned in package.json/scripts

**Test plan**

I think we don't have automated tests for this case, it's OS specific and the fix relies on the `shell` option of `child_process.spawn`, so we cannot use mock to reliably test this issue.
(Any idea welcomed)

I ran manual tests on Windows 10 and a Linux VM.
@BYK BYK closed this as completed Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests