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

Remove ./node_modules/.bin from $PATH #903

Closed
wants to merge 6 commits into from
Closed

Conversation

BanzaiMan
Copy link
Contributor

Some npm modules define commands with widely used bash functions
(and probably others).
Putting this component in $PATH causes subtle issues, such as
travis-ci/travis-ci#5092.
We are removing it, so that this is no longer the case.

This will most likely break some builds, so we add a warning.

Some npm modules define commands with widely used bash functions
(and probably others).
Putting this component in `$PATH` causes subtle issues, such as
travis-ci/travis-ci#5092.
We are removing it, so that this is no longer the case.

This will most likely break some builds, so we add a warning.
@BanzaiMan BanzaiMan deployed to org-staging December 6, 2016 00:17 Active
@BanzaiMan BanzaiMan deployed to org-staging December 6, 2016 00:20 Active
@BanzaiMan BanzaiMan deployed to org-staging December 6, 2016 00:27 Active
@BanzaiMan BanzaiMan changed the title Remvoe ./node_modules/bin from $PATH Remvoe ./node_modules/.bin from $PATH Dec 6, 2016
@BanzaiMan BanzaiMan deployed to org-staging December 6, 2016 00:48 Active
@BanzaiMan BanzaiMan deployed to org-staging December 6, 2016 00:49 Active
@BanzaiMan
Copy link
Contributor Author

@BanzaiMan BanzaiMan deployed to org-staging December 6, 2016 00:59 Active
@nathanhammond
Copy link

Note that, to my knowledge, this change breaks every Ember addon out there because of our default blueprint: https://github.com/ember-cli/ember-cli/blob/74562a4/blueprints/addon/files/.travis.yml#L40

@backspace
Copy link
Contributor

This will break things for almost all Ember projects, as ember test is the default way to run tests and that assumes the Node-installed ember command is available.

@nathanhammond
Copy link

This doesn't appear to have landed yet we currently have reported failures from users in our Slack channel. Also, hi, @backspace! <3

@joshk
Copy link
Contributor

joshk commented Dec 6, 2016

I'm not sure I support this change. This will cause major issues for many people using Node.

@Turbo87
Copy link

Turbo87 commented Dec 6, 2016

@BanzaiMan instead of removing the path completely, would it maybe be possible to move it to the end of the list as a fallback? that way the native executables are preferred, but the Ember addon builds won't fail.

@BanzaiMan
Copy link
Contributor Author

The offending bit is travis-ci/travis-cookbooks#786, which renders this PR obsolete. I'll work on a fix for the issue shortly.

@meatballhat meatballhat changed the title Remvoe ./node_modules/.bin from $PATH Remove ./node_modules/.bin from $PATH Dec 6, 2016
@kevinji
Copy link
Contributor

kevinji commented Jan 11, 2017

Since this PR is now obsolete, it can be closed now right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants