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

[Bug] Native dependencies fail to build if the project's path contains spaces #716

Closed
1 task done
ngryman opened this issue Jan 21, 2020 · 12 comments · Fixed by #824
Closed
1 task done

[Bug] Native dependencies fail to build if the project's path contains spaces #716

ngryman opened this issue Jan 21, 2020 · 12 comments · Fixed by #824
Labels
bug Something isn't working reproducible This issue can be successfully reproduced

Comments

@ngryman
Copy link
Contributor

ngryman commented Jan 21, 2020

  • I'd be willing to implement a fix

Describe the bug

Native dependencies fail to build if there is a space in the project's path. It's happening during the compilation phase of the dependency.

I've tracked down the error and it comes the fact we add a --require /path/to/.pnp.js in NODE_OPTIONS. The path gets truncated if it contains a space (cf. screenshot).

NODE_OPTIONS documentation mentions that options containing spaces should be escaped with a \. However when I tried to monkey patch to test if it works, it was still truncating the path. I'm not exactly sure why though...

Do you know where to properly patch the code for this, in order to test if adding a \ really works?

To Reproduce

await expect(packageJsonAndInstall({
  dependencies: {
    [`node-sass`]: `latest`
  }
})).resolves.toBeTruthy();

Screenshots

In the following path: /Users/ngryman/Projects/OSS/Code/berry spaces, here is the log of the build when running yarn add node-sass:

Environment if relevant (please complete the following information):

  • OS: Mac OS Catalina
  • Node version 10.17.0
  • Yarn version master branch
@ngryman ngryman added the bug Something isn't working label Jan 21, 2020
@ngryman ngryman changed the title [Bug] [Bug] Native dependencies fail to build if the project's path contains spaces Jan 21, 2020
@yarnbot yarnbot added the broken-repro The reproduction in this issue is broken label Jan 21, 2020
@yarnbot

This comment has been minimized.

@yarnbot

This comment has been minimized.

@yarnbot

This comment has been minimized.

@yarnbot yarnbot added unreproducible This issue cannot be reproduced on master and removed broken-repro The reproduction in this issue is broken labels Jan 21, 2020
@yarnbot

This comment has been minimized.

1 similar comment
@yarnbot

This comment has been minimized.

@yarnbot yarnbot added reproducible This issue can be successfully reproduced and removed unreproducible This issue cannot be reproduced on master labels Jan 21, 2020
@yarnbot

This comment has been minimized.

1 similar comment
@yarnbot

This comment has been minimized.

@yarnbot
Copy link
Collaborator

yarnbot commented Jan 21, 2020

This issue reproduces on master:

Error: expect(received).resolves.toBeTruthy()

Received: undefined
    at Object.toBeTruthy (/github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-1.zip/node_modules/expect/build/index.js:202:20)
    at module.exports (evalmachine.<anonymous>:6:14)
    at /github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.37-bb4398f5f1-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:19
    at executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.37-bb4398f5f1-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:22)
    at Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.37-bb4398f5f1-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:18)
    at ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.37-bb4398f5f1-1.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:59)
    at async ExecCommand.validateAndExecute (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-1.zip/node_modules/clipanion/lib/advanced/Command.js:161:26)
    at async Cli.run (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-1.zip/node_modules/clipanion/lib/advanced/Cli.js:74:24)
    at async Cli.runExit (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-1.zip/node_modules/clipanion/lib/advanced/Cli.js:83:28)

@ngryman
Copy link
Contributor Author

ngryman commented Jan 21, 2020

Ahah it seems that I finally managed to make the repo fail correctly 🎉 Sorry for the noise 😅

@arcanis
Copy link
Member

arcanis commented Jan 21, 2020

Can you try with Node 12? Up until then Node had a longstanding issue with that (cf my PR at nodejs/node#24065). We could add a more helpful error message, however, by checking whether semver.lt(process.versions.node, '12.0.0').

@ngryman
Copy link
Contributor Author

ngryman commented Jan 22, 2020

@arcanis I can confirm that it works with the active LTS 👍
Yea an error message could do it. Do you think that's something I could help with?

@arcanis
Copy link
Member

arcanis commented Jan 22, 2020

Yep, for sure!

ngryman added a commit to ngryman/berry that referenced this issue Jan 28, 2020
arcanis added a commit that referenced this issue Jan 29, 2020
…#824)

* improv: throw error if using node <12 and the path to .pnp.js includes spaces (#716)

* fix: bump plugin-pnp version

* Adds at-types and bumps the CLI

Co-authored-by: Maël Nison <nison.mael@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reproducible This issue can be successfully reproduced
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants