-
Notifications
You must be signed in to change notification settings - Fork 3k
Patch for issue 7357, courtesy of @mathieucarbou #12366
Conversation
As I said when I closed #7357:
This is still true. npm explicitly doesn't offer support for Cygwin. For more context, this chunk from this week's npm CLI team meeting (link goes to correct spot in video) is relevant, where I discuss the fact that npm doesn't (yet) support being used in the Ubuntu compatibility environment on Windows. Because npm supports both Windows and Unix, and does so in different ways, it's tricky for it to support environments that try to be both. Cygwin makes it especially hard. If you want to apply that patch to your own copy of npm, that's fine, but in the form it's in here, it's unlikely to be merged into npm proper. Either way, thanks for your time and your understanding! |
Like your quoted post says (which I read beforehand), this PR keeps
|
@othiym23 : sad to see that such little modification helping a lot of users is not welcomed. I didn't see it as a request to support cygwin. I much more see it as making life better for a lot of people that will be happy to see this patch included and will be able to continue working with their environment they like, EVEN IF you do not officially support it. In my opinion, it's not because something is not officially supported and documented that it shouldn't be there... It's up to the users to know that if they use an unsupported env, then it is up to them. But at least, it will make life easier for these people ;-) I agree Cygwin is difficult to entirely support, and I fear it won't be possible. But there is a lot of small things like this one that would help solving 80% of the issues people have in Cygwin. So it's not a big demand ;-) |
If it's in code that's published and distributed by the project, it's de facto supported. We're responsible for dealing with the consequences of shipping changes, and although this does handle the case of Your patch is available for people who want to make it work for themselves, and I believe that the act of applying that patch signals a willingness to accept the complications that result far more effectively than the project landing it and then not documenting it. |
Since this PR has a very explicit check for Cygwin, it's pretty safe to say there is not going to be any impact to non-Cygwin users. Win32 is even being detected the same way as the original npm code, via I am with @mathieucarbou in stating that this will help make lives easier. I really enjoy trying to make open source contributions, even if unsuccessful. And as a Windows dev, I can fully sympathize with folks that have a bad time, because I live that every day. Every step forward helps, even if it's a small one 😄 |
cc'ing the folks that also encountered the issue and were savvy enough to find the patch on GitHub |
I went back and fixed the errors reported by travis-ci.org (didn't see those until now, definitely my fault). Are there other repo owners / contributors that can consider re-opening this? Having this contribution rejected feels like an arbitrary company mandate by npm, Inc employees (@isaacs, @othiym23). Non-employees should have a say also, IMO. |
I'll discuss this PR during the npm CLI team meeting on 2016-04-19, but to speak to this specific point, when @isaacs wrote that commit back in 2011, npm, Inc. wasn't even an idea yet. Back then, Isaac was more or less the sole developer and maintainer of npm, which in practice meant he was solely responsible for supporting its users. He passed responsibility for maintaining the CLI on to me after hiring me into the newly established npm, Inc., and similarly, I and my team have inherited the responsibility for support. Supportability is a primary design criterion for the CLI, because npm is a very powerful, flexible, and complicated tool. (Supporting it is hard.) Most of the time this means putting thought into designing the user experience and effectively communicating to users what's happened (often, what's gone wrong). Occasionally, it means choosing between tradeoffs rooted in the conflicting needs of different segments of the user base. The team's decision to not support Cygwin is an example of the latter. I understand that it seems frustrating and unresponsive to you. It's a decision made with the best interests of the community as a whole in mind, though, and wasn't, and isn't, an arbitrary one. |
I definitely do appreciate the work you all do; being able to revisit this topic (this thread and then the meeting tomorrow) is a great thing, regardless of the outcome, IMO 😄 My proposal for tomorrow's discussion would be: changing from a stance of "No." to "Cygwin is not supported, use at your own risk." meaning...
|
HIHIHIHIH I sort of feel like I accidentally fell into this issue by nature of me stumbling across the Cygwin issue - as such, I have no real desire to see this patch added or not. Maybe, if you'll permit me, this allows me to comment on both sides of this discussion. Normally I wouldn't bother, however like @othiym23 I have spent years supporting and leading teams that support software. It's a tough job, with very little reward, and I have seen first-hand how fixing an issue in an unsupported area of the software can lead to the floodgates opening & every tom, dick & harry wanting a "free fix". @othiym23, given @bsclifton's last post, and the commitment to a non-support of Cygwin, can you comment on what your concern is about allowing the community-driven fix? Is it the floodgates scenario I mentioned? If so, is there anything that can be said or done to reassure the core dev team that this change would not be a precursor to official Cygwin support? Just my tuppence; feel free to dismiss me as an irrelevant mad-man... |
I don't think it would ever make sense to have Cygwin officially supported. By that, I mean that contributors for this repo should close Cygwin-specific issues instead of actively looking for fixes to resolve them. Like @othiym23 has shown, there are already a large number of outstanding issues and this would blow up the scope. If people found a resolution for a Cygwin-specific issue themselves and submitted it in the form of a PR though... I'd be more than happy to review it 😀 |
@othiym23 Wouldn't it better to accept the patch but warn about the non supported situation? It's much better to have a giant message in red letters "CYGWIN NOT SUPPORTED, MAY BREAK AT ANY MOMENT, USE AT YOUR OWN RISK" than having random error messages sometimes? It did work for me without the patch for a while and if I was warned beforehand it would have made things easier. @bsclifton @Doormouse2House What does imply supporting cygwin officially (or at least a working "run at your own risk" stance)? Running many tests locally for each alpha/beta release and fix it when it breaks? Does anyone else want to join efforts? @mathieucarbou ? |
I don't think cygwin can be completely supported. There are some npm dependencies requiring some native compiling for example at npm install. |
@mathieucarbou that and many other issues may be worked around by calling CMD with the user's env variables instead of cygwin's. I.e. only using git from cygwin/msys/git bash, but using the rest of the supported tools (VS, python for windows...) |
Cygwin is a very popular package on Windows and having npm not supporting it is a big deal. We've moved away from Git bash to Cygwin due to numerous issues. If major patches like that could be incorporated it would be a huge plus. |
I don't care if npm continues to advertise that Cygwin is not supported, so long as they merge the patch. Given that the community widely reports that this patch is good and useful, one would hope that this would be evidence enough that it is a worthwhile addition to the codebase. |
@bsclifton : is it possible that you update the patch to fix the build syntax check ? https://travis-ci.org/npm/npm/jobs/123364024
Should be ok with:
|
@mathieucarbou I already did actually- but since PR was marked as closed, it never picked it up 😢 |
At today's CLI team meeting, I spent about 10 minutes addressing this issue. The summary is this: npm is not going to accept patches to support Cygwin, quietly or otherwise. This thread contains several examples of the hazards that can arise when doing so – it's impossible (and illogical) to completely disavow support for a set of features while still having code that conditionally takes advantages of those features. I do think there are two solid avenues for those of you who want a usable version of npm in Cygwin, though:
Either way, that (again) makes clear to consumers and potential contributors who to look to for help and support, and lets you make whatever changes you think desirable to make npm work better with Cygwin. To be as explicit as possible: the CLI team isn't going to merge this patch, and won't merge any patch that's intended to solely change the behavior of npm for Cygwin. The video above explains our reasoning. I hope you find these explanations useful, and I thank you all for your understanding. |
Ultimately, I think a lot of these issues boil down to
A wrapper script around Feel free to join the discussion on the Cygwin mailing list: |
I don't really have any more comments regarding this PR or the issue (definitely appreciate other folks taking interest though 😃)- I'm taking part in the Cygwin mailing list to work on a resolution independent of npm. But I am curious if npm's CLI collects platform/shell information when packages are requested. I'd be super interested to see an analytics breakdown of Windows users by shell. It could make a great blog post if that information is available. I looked at the mingw project (which is a fork of Cygwin, BTW), but it hasn't been updated since 2013 and the docs are all from mid or early 2000's. I'm having a hard time believing that more people use mingw than Cygwin, which is why seeing data would be cool. |
Submitted a patch for Windows-style path compatibilty to Cygwin's git maintainer: |
Cygwin is massive (see all the download mirrors) as Windows without a proper shell is much less usable for developers. MS tried to fix it with PowerShell and they failed, now they are going with full Linux shell support. At the same time, having read all the threads,
Apart from installing from repo URLs, NPM works perfectly for me on Cygwin, and while installation of NPM under Cygwin is not supported (and doesn't need to be), the operation of NPM under Cygwin already works 99% so why not push it to full 100%, bend a rule here and there. |
Indeed!
Not surer if this will work at all. Will install ubuntu-bash later today when I get back home,. and check that for my self. |
Thanks for the quick feedback +1:
[edit: ] Did it my self, indeed, you may run only the WSL binaries, not from the Windows releases; yet, reusing the same binaries from linux, is joyful & creepy at the same time :-) |
Jesus! I can't believe all these discussions over merging a trivial patch that would have helped all Cygwin users, using npm. Instead you will have to continue discussing these issues for the rest of the lifetime of npm, as Cygwin users will not disappear anytime soon, not even with the emerging native Ubuntu bash (which has it's own npm issues.) It's really sad to see the npm folks blatantly denying the wide use of Cygwin. After all, Cygwin is the only reason developers still bother to use Windows. ;) |
@emigenix I agree that this patch could have helped. I championed it for a while and after attending their weekly meeting, it's safe to say Cygwin will never be supported by them. In the meantime, besides applying the patch, you can try redefining git in your bashrc: function git {
declare -a ARGS
for n in "$@" ; do ARGS+=("$(cygpath -u -- "${n}")") ; done
command git "${ARGS[@]}"
}
} I didn't get a chance to finish it... it works as long as the paths are quoted. Shout-outs to Eliot Moss on the Cygwin mailing list for suggesting this and also giving me a rev1 to work with. |
@bsclifton Being in Cygwin world and switching to Git Bash just to run sh: /home/dmoore/.bashrc: line 7: syntax error near unexpected token `('
sh: /home/dmoore/.bashrc: line 7: ` for n in "$@" ; do ARGS +=("$(cygpath -u -- "${n}")") ; done' My current $ cat ~/.bashrc
---
source ~/.git-prompt.sh
export PS1='\e[0;32m\u@\h:\e[m\W\e[1;33m`__git_ps1 " (%s)"`\e[m\n$ '
function git {
declare -a ARGS
for n in "$@" ; do ARGS +=("$(cygpath -u -- "${n}")") ; done
command git "${ARGS[@]}"
}
} Thanks. |
@demisx yeah, it does suck having to switch back and forth. There are other times where you have no choice (for project to build using Visual Studio vs GCC) Apologies for not finishing the method above. All these months later, I've switched to Mac as my primary OS. But you should be able test/tweak it using the |
I don't know the details of the "npm problems under windows ubuntu native emulator" but I think all npm-on-windows efforts should now be focused there. |
WSL bash is a lightweight virtualized environment to execute ELF binaries (ie Ubuntu). |
Well that sucks royally. Count me back into the cygwin-for-the-masses camp. |
This works for me in zsh, I assume it should work in bash too: function git {
declare -a ARGS
for n in "$@"; do ARGS+=("$(cygpath -u -- "${n}")"); done
command git "${ARGS[@]}"
} You can test it with a local clone: $ git clone c:\\some\\repo somedir |
@bsclifton Yeah, I am on the same bandwagon. My go-to OS for everything is Mac OSX. Unfortunately, we still have some client projects that require us to run Visual Studio, .NET and the whole myriad of other Microsoft goodies. @silverwind Thanks, that made the error go away, but
|
Yeah that's because Enter this hack:
#!/bin/bash
declare -a ARGS
for arg in "$@"; do
if [[ $arg == *"\\"* ]]; then
ARGS+=("$(cygpath "$arg")");
else
ARGS+=("$arg");
fi
done
command /bin/gitbin "${ARGS[@]}" |
Hi Guys! (@silverwind, @bsclifton, @ankostis ) Thanks for looking into this. I've tried the WSL Ubuntu and not only are the node and npm packages way outdated, but it also introduce a lot of other weirdness that is gonna cause a whole bunch of issues to users. Among those are the default directories you get when using CMD from desktop link, vs cmd from Start button and also depending on if you're running as Admin or not... In addition the entire CMD often crashes without any visible reason. There is no future on WSL as it will never be able to run any x-windows or open additional command windows from within, like opening windows explorer windows from within bash etc... $ node -v && npm -v
v0.10.25
1.3.10 To install and update do this: #----------------------------------------------------------
# Using node and npm in W10 WSL (Ubuntu Bash)
#----------------------------------------------------------
# Updating Ubuntu on W10:
sudo apt dist-upgrade
sudo apt upgrade
# Installing node and npm on W10-WSL
sudo apt-get install nodejs
sudo ln -s /usr/bin/nodejs /usr/bin/node
sudo apt-get install npm
# Updating npm on W10-WSL
npm install npm@latest -g
# Updating node on W10-WSL
sudo npm cache clean -f
sudo npm install -g n
sudo n stable Now re-start bash shell to reload new environment $ node -v && npm -v
v6.4.0
3.10.6 |
Also, don't forget the $ shopt
autocd off
cdable_vars off
cdspell off
checkhash off
checkjobs off
checkwinsize off
cmdhist on
compat31 off
compat32 off
compat40 off
compat41 off
compat42 off
complete_fullquote on
direxpand off
dirspell off
dotglob off
execfail off
expand_aliases on
extdebug off
extglob off
extquote on
failglob off
force_fignore on
globstar off
globasciiranges off
gnu_errfmt off
histappend on
histreedit off
histverify off
hostcomplete on
huponexit off
interactive_comments on
lastpipe off
lithist off
login_shell on
mailwarn off
no_empty_cmd_completion off
nocaseglob off
nocasematch off
nullglob off
progcomp on
promptvars on
restricted_shell off
shift_verbose off
sourcepath on
xpg_echo off
$ set -o
allexport off
braceexpand on
emacs on
errexit off
errtrace off
functrace off
hashall on
histexpand on
history on
igncr off
ignoreeof off
interactive-comments on
keyword off
monitor on
noclobber off
noexec off
noglob off
nolog off
notify off
nounset off
onecmd off
physical off
pipefail off
posix off
privileged off
verbose off
vi off
xtrace off |
@emigenix why not just use nodesource repos? https://github.com/nodesource/distributions#debian-and-ubuntu-based-distributions |
@silverwind I need the simple Cygwin out-of-the-box experience, for multiple Windows 8/10 installations. |
For all those of you who want to run npm on Cygwin, I've created a repo with clear working instructions in: https://github.com/emigenix/npm_on_cygwin. Works like a charm! |
See #7357 (comment) for the patch by @mathieucarbou. This PR is me manually applying the patch, which has slight changes.
I confirmed it works great by testing locally (before and after). Before
npm install
would fail, after it works great (see other comments in the issue for reports of +1s)