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

tools: do not rewrite npm shebang in install.py #6098

Closed
wants to merge 1 commit into from

Conversation

@evanlucas
Copy link
Member

@evanlucas evanlucas commented Apr 7, 2016

Checklist
  • the commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of change

It violates user expectations among other things.

The shebang in npm.js is kept as #!/usr/bin/env node.

See #6095 for more details

It violates user expectations among other things.

The shebang in npm.js is kept as #!/usr/bin/env node.

Fixes: nodejs#6095
@evanlucas evanlucas added this to the 6.0.0 milestone Apr 7, 2016
@mscdex mscdex added the npm label Apr 7, 2016
@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 7, 2016

LGTM

Loading

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Apr 7, 2016

Just making sure I understand what is going on 100%. In the past we would load the file and rewrite it to change the shebang. The change removes the bits that did that leaving the shebang exactly the way it was.

If my assumptions are correct LGTM

Loading

@evanlucas
Copy link
Member Author

@evanlucas evanlucas commented Apr 7, 2016

@thealphanerd correct

Loading

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 7, 2016

LGTM

Loading

@evanlucas
Copy link
Member Author

@evanlucas evanlucas commented Apr 7, 2016

Loading

@evanlucas
Copy link
Member Author

@evanlucas evanlucas commented Apr 8, 2016

Loading

@jbergstroem
Copy link
Member

@jbergstroem jbergstroem commented Apr 8, 2016

LGTM

Loading

jasnell added a commit that referenced this issue Apr 9, 2016
Rewriting npm shebang in install.py violates user expectations
among other things.

The shebang in npm.js is kept as #!/usr/bin/env node.

Fixes: #6095
PR-URL: #6098
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 9, 2016

Landed in 8ffa20c

Loading

@jasnell jasnell closed this Apr 9, 2016
@evanlucas evanlucas deleted the 6095 branch Apr 10, 2016
jasnell added a commit that referenced this issue Apr 26, 2016
Rewriting npm shebang in install.py violates user expectations
among other things.

The shebang in npm.js is kept as #!/usr/bin/env node.

Fixes: #6095
PR-URL: #6098
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@gibfahn gibfahn mentioned this pull request Apr 29, 2016
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants