Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

npm creates incorrect cmd shim for jezebel package on windows #3380

Closed
basbossink opened this issue Apr 27, 2013 · 6 comments
Closed

npm creates incorrect cmd shim for jezebel package on windows #3380

basbossink opened this issue Apr 27, 2013 · 6 comments

Comments

@basbossink
Copy link

When trying to run jezebel on windows after installing it with npm, it fails. C:\Users\AppData\Roaming\npm\jezebel.cmd looks like this:

:: Created by npm, please don't edit manually.
@IF EXIST "%~dp0\NODE_PATH=./lib:$NODE_PATH.exe" (
  "%~dp0\NODE_PATH=./lib:$NODE_PATH.exe"  node %~dp0\node_modules\jezebel\bin\jezebel" %*
) ELSE (
     NODE_PATH=./lib:$NODE_PATH  node "%~dp0\node_modules\jezebel\bin\jezebel"   %*
)

Which is syntactically incorrect.
Looking at the jezebel repository the bin\jezebel file has the following first line:

#!/usr/bin/env NODE_PATH=./lib:$NODE_PATH node     

Looking at the npm source code, specifically node_modules\cmd-shim:22 shows:

    , shebangExpr = /^#\!\s*(?:\/usr\/bin\/env)?\s*([^ \t]+)(.*)$/

This regular expression was not built with the possibillity of variable declarations in mind.

It should be adapted to extract the variable declarations and translate them to the proper cmd syntax:

set NODE_PATH=.lib:%NODE_PATH%

in this case.

@ForbesLindesay
Copy link
Contributor

The source code for cmd-shim is here and a pull request to fix this would be welcome. However, it should be easier in this case to just make jezebel not do that, since there's no real reason why it would need to do that.

@basbossink
Copy link
Author

I'll try to create a fix. I see your point but I think it's best if the shim creation either fails if it can not extract the command from the file successfully or handles the given case correctly. My proposal would be to go for the latter option.

@isaacs
Copy link
Contributor

isaacs commented Apr 29, 2013

It looks like Jezebel is trying to use NODE_PATH as a polyfill for the now-removed require.paths.

That is not supported, recommended, safe, or guaranteed to work reliably. You should report the bug to them.

@isaacs isaacs closed this as completed Apr 29, 2013
@isaacs
Copy link
Contributor

isaacs commented Apr 29, 2013

Note: I do acknowledge that this is a bug in cmd-shim, but like many bugs, it's not worth fixing until and unless there's a real need for it, because the fix adds complexity.

@basbossink
Copy link
Author

As you can see I reported the issue to jezebel, do you see a legitimate reason to add support for the use of variables in cmd-shim?

@ForbesLindesay
Copy link
Contributor

Yes, in this case it's definitely a higher priority to just fix jezebel.

One option would be to just fix cmd-shim so that it reports this as an error and states that it's not supported?

isaacs pushed a commit to npm/cmd-shim that referenced this issue Aug 12, 2019
- Adapt the regular expression used to match the first line.

- Create a simple module that can convert shell style variable
  declarations: NODE_PATH=./lib:$NODE_PATH to the equivalent batch
  syntax: @set=NODE_PATH=./lib:%NODE_PATH%. Furthermore the structure of
  the generated shim now looks like this:

        @SETLOCAL
        <variable declarations>
        <original content>
        @endlocal

    - Note that the new segments are only added to the file if there were
      any variable declarations.

- The generated shell script carries over the captured variable
  declaration as is.

- Add some extra tests to validate the behavior.

- Remove some unnecessary white-space in the generated shim.

PR-URL: #2
Fix: npm/npm#3380
Credit: @basbossink
Close: #2
Reviewed-by: @isaacs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants