Fix executing scripts with spaces #809

Merged
merged 1 commit into from Oct 12, 2016

Projects

None yet
@ticky
Contributor
ticky commented Oct 12, 2016 edited

Summary
The script quoting behaviour introduced in #663 breaks package.json script entries with spaces in them on Unix.

Instead, this quotes node_modules/.bin executable paths, which makes sure both package.json script entries, and executables can be called.

Test plan

For this package.json;

{
  "name": "yarntest_pr809",
  "version": "1.0.0",
  "devDependencies": {
    "jest": "^16.0.1"
  },
  "scripts": {
    "test": "jest",
    "test-help": "jest --help"
  }
}

with dependencies installed,

  1. In a path with no spaces (e.g. ~/yarntest_pr809);
    1. yarn run test -- --help should print jest’s help message on the current release, and with this patch.
    2. yarn run test-help will fail on the current release, and print jest’s help message with this patch.
    3. yarn run jest -- --help will print jest’s help message on both the current release and with this patch.
  2. In a path with spaces (e.g. ~/yarn test pr809);
    1. yarn run test --help should print jest’s help message on the current release, and with this patch.
    2. yarn run test-help will fail on the current release, and work fine with this patch.
    3. yarn run jest -- --help will print jest’s help message on both the current release and with this patch.

Note that the use of a help flag within a script is a contrived example, but triggers the incorrect behaviour in this case.

fixes #727, #733, #735 😄

@Daniel15
Member
Daniel15 commented Oct 12, 2016 edited

Hi @ticky! Nice to see another Melburnian contributing to Yarn. I'm from Melbourne too 😄

For your test plan, I think it'd be good to include some examples of scripts that you tested with yarn run, and show the package.json you used. Yarn should handle both some sort of "global" command (eg. node foo.js, which runs node directly) as well as some local command (eg. jest foo-test.js when jest is listed as a devDependency in project.json but not installed globally, which runs node_modules\.bin\jest).

Maybe something like this:

{
  "name": "yarntest_issue733",
  "version": "1.0.0",
  "devDependencies": {
    "jest": "^16.0.1"
  },
  "scripts": {
    "first": "node foo.js",
    "second": "jest __tests__/hello-test.js"
  }
}

Both yarn run first and yarn run second should work (assuming foo.js and __tests__/hello-test.js exist)

This is mainly so we can keep track of the test plan, and other people can re-test the exact same scenarios in the future if they're going to change the same code.

I suspect this will fix #735 too.

@ticky
Contributor
ticky commented Oct 12, 2016

Just don’t tell anyone I actually started out in Perth 😜

I didn’t see anywhere in the existing test suite where this kind of CLI interop was tested, so I figured I’d leave adding such a thing to the repo proper for another intrepid soul.

Yarn does not currently run any executables on the system path; only executables within node_modules/.bin and package.json scripts, so the scenario you describe here in yarn run first isn’t applicable.

To that end, and given the current behaviour, I’ve updated the test scenarios in the PR description.

@AlicanC
Contributor
AlicanC commented Oct 12, 2016

I'm adding tests for this in #821. I can't complete the tests right now because master is broken. If we can merge this I can continue with the tests.

@ticky
Contributor
ticky commented Oct 12, 2016

@AlicanC if you take a look at the newly updated PR comment, it should lead you to six good test scenarios for this! 😄 I can’t really comment on how the test hooks in, as I’m not that familiar with this application of async stuff!

@ticky ticky Fix executing scripts with spaces
a5f1898
@ticky
Contributor
ticky commented Oct 12, 2016

The current build failure seems to be a flaky test 😉

@Bartuz
Bartuz commented Oct 12, 2016

Is running scripts with npm run still valid? Does it cooperate well with what yarn does?

@ticky
Contributor
ticky commented Oct 12, 2016

Yarn puts executables in the same place npm is expecting them, so it works fine

@acateland

i hope this will be merged soon as it effectively resolve this issue. Good job

@kittens kittens merged commit 558aa38 into yarnpkg:master Oct 12, 2016

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci Your tests passed on CircleCI!
Details
@kittens
Member
kittens commented Oct 12, 2016

Thank you so much!

@lfittl
lfittl commented Oct 12, 2016 edited

@kittens Any ETA on a patch release that has this fix, or a way to workaround until then? (looks like there is a bunch of folks depending on this fix)

Thanks for the amazing work on yarn btw :)

@icd2k3
icd2k3 commented Oct 12, 2016

@lfittl I've just been using npm run in the meantime and everything still works fine

@Daniel15
Member
Daniel15 commented Oct 12, 2016 edited

or a way to workaround until then?

If you like, you can clone the Yarn repository and build it to get all the latest bugfixes. Something like this should be sufficient on Linux (or Mac OS):

cd /usr/local/src
git clone git://github.com/yarnpkg/yarn.git
cd yarn
yarn install
npm run build

Then add /usr/local/src/yarn/bin to your PATH, or use /usr/local/src/yarn/bin/yarn directly.

@fczuardi fczuardi added a commit to fczuardi/controltower that referenced this pull request Oct 13, 2016
@fczuardi fczuardi yarn run has troubles
probably bc of yarnpkg/yarn#809 so we'll run the npm scripts with npm for now
ad8ec5a
@Kovensky
Kovensky commented Oct 13, 2016 edited

@Daniel15 npm link in yarn's source folder seems to DWIM and make yarn's git version available on npm's bin path.

@chiedo
chiedo commented Oct 14, 2016

When does this version get pushed as the latest release so I can get it with brew upgrade or with Ubuntu?

@joyfulelement

Currently having the same issue attempting to execute run script with yarn on Mac OS and Unix on CI. Do we know if this fix would be released as part of v0.15.2? (Currently using v0.15.1 at the time of this writing)

@Petesta
Petesta commented Oct 14, 2016 edited

Having the same problem and posted a question about it. Looks like we just have to wait.

http://stackoverflow.com/questions/40049508/trouble-running-yard-run-on-scripts-object-in-package-json

@icd2k3
icd2k3 commented Oct 14, 2016 edited

Just wanted to note that I don't see this issue when I install yarn via curl -o- -L https://yarnpkg.com/install.sh | bash so that might be another option while waiting for v0.15.2.

@gekorob
gekorob commented Oct 15, 2016

Thx @icd2k3, uninstalled yarn 0.15.1 (previously npm installed) and used the bash installer...
Everything is working now.

@seehou
seehou commented Oct 16, 2016 edited

Still hitting this issue on my osx.

  "scripts": {
    "start": "node server.js"
  },
yarn run start
yarn run v0.15.1
$ "node server.js" 
sh: node server.js: command not found
error Command failed with exit code 127.

Thanks @icd2k3 , it works!

@dting dting referenced this pull request in NYTimes/kyt Oct 17, 2016
Open

Support Yarn #254

0 of 2 tasks complete
@chiedo
chiedo commented Oct 17, 2016

Anyone have a rough ETA on when this will be released?

@guitarmanvt

Anyone interested, please see #1156 for a work-around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment