[READY] Remove nodejs workaround #1075

micbou · 2018-08-05T13:58:00Z

Opening a file without setting the encoding may fail on Python 2. See issue ycm-core/YouCompleteMe#3101. We could use the ReadFile utility function to fix the issue but I think we are better off dropping the nodejs workaround altogether (looking for the nodejs executable before node because nodejs is the name of the Node binary on Debian-based distributions). Users should install the nodejs-legacy package on such distributions.

Fixes ycm-core/YouCompleteMe#3101.

This change is

Users should install the nodejs-legacy package on Debian-based distributions.

bstaletic

I'm not sure this is the right way to solve the problem. Python2 is nearing its EOL, but node and nodejs executables seem like they will stay. If we drop the node/nodejs workaround we'd be inconveniencing the users, but then when we drop python2 we would be able to reintroduce the support for nodejs.

Telling the users to install a legacy package seems too much like the mess we have with libclang depending on libtinfo. Only now we're not saying "use a legacy package" to the rolling release users, who kind of signed up for getting their hands dirty, but to users of LTS distros and, even more concerning, users who want something that "just works"(tm). Reminder that users of debian based distros are far greater in number - it makes me a little afraid of the number of "bug reports" along the lines of what we have seen about libtinfo.

Reviewable status: 0 of 2 LGTMs obtained

codecov · 2018-08-05T19:53:14Z

Codecov Report

Merging #1075 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1075      +/-   ##
==========================================
- Coverage   97.53%   97.52%   -0.01%     
==========================================
  Files          90       89       -1     
  Lines        6968     6957      -11     
==========================================
- Hits         6796     6785      -11     
  Misses        172      172

micbou

The reason why I think we shouldn't bother looking for nodejs is that users have to install the nodejs-legacy package anyway if they want to use TypeScript. Otherwise, they'll get the following error when compiling:

$ tsc
/usr/bin/env: ‘node’: No such file or directory

Reviewed 2 of 6 files at r1.
Reviewable status: 0 of 2 LGTMs obtained

bstaletic

That's a fair argument, considering that tern completer isn't necessary for javascript completion any more. If we do decide to drop nodejs, we should be ready for the bug reports.

Reviewable status: 0 of 2 LGTMs obtained

puremourning

with one comment

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)

ycmd/completers/javascript/tern_completer.py, line 51 at r1 (raw file):

    'tern' ) )

PATH_TO_NODE = utils.FindExecutable( 'node' )

The challenge with removing this is that there is a node executable on those systems (iirc, like ubuntu) but it isn't a NodeJS node. I think this leads to obscure errors that most users won't understand.

Perhaps we could consider (perhaps for a future change) running node and trying to get its version or something, then failing if 'node' is not a recognisable NodeJS installation?

micbou

Reviewed 4 of 6 files at r1.
Reviewable status: 1 of 2 LGTMs obtained

ycmd/completers/javascript/tern_completer.py, line 51 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

The challenge with removing this is that there is a node executable on those systems (iirc, like ubuntu) but it isn't a NodeJS node. I think this leads to obscure errors that most users won't understand.

Perhaps we could consider (perhaps for a future change) running node and trying to get its version or something, then failing if 'node' is not a recognisable NodeJS installation?

We could but I don't want to spend too much time on this since the Tern completer is in low maintenance mode. I think the easiest would be to mention the issue in the docs.

bstaletic

Reviewable status: 1 of 2 LGTMs obtained

ycmd/completers/javascript/tern_completer.py, line 51 at r1 (raw file):

Previously, micbou wrote…

We could but I don't want to spend too much time on this since the Tern completer is in low maintenance mode. I think the easiest would be to mention the issue in the docs.

I agree with @micbou. We could also consider, when these issues pop up, suggesting to users a switch to TSServer.

puremourning

Reviewed 6 of 6 files at r1.
Reviewable status: 1 of 2 LGTMs obtained

ycmd/completers/javascript/tern_completer.py, line 51 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I agree with @micbou. We could also consider, when these issues pop up, suggesting to users a switch to TSServer.

OK sure. I didn't notice this was tern. No problem.

bstaletic

Reviewed 6 of 6 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale)

bstaletic · 2018-08-19T16:26:41Z

@zzbot r=ppuremourning

zzbot · 2018-08-19T16:26:41Z

📌 Commit 8713009 has been approved by ppuremourning

zzbot · 2018-08-19T16:26:45Z

⌛ Testing commit 8713009 with merge ddf11dc...

[READY] Remove nodejs workaround Opening a file without setting the encoding may fail on Python 2. See issue ycm-core/YouCompleteMe#3101. We could use the `ReadFile` utility function to fix the issue but I think we are better off dropping the `nodejs` workaround altogether (looking for the `nodejs` executable before `node` because `nodejs` is the name of the Node binary on Debian-based distributions). Users should install the `nodejs-legacy` package on such distributions. Fixes ycm-core/YouCompleteMe#3101.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/1075)

zzbot · 2018-08-19T18:21:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: ppuremourning
Pushing ddf11dc to master...

[READY] Update ycmd Include the following changes: - PR ycm-core/ycmd#1075: remove Node.js workaround for Debian-based distributions; - PR ycm-core/ycmd#1077: update Boost to 1.68; - PR ycm-core/ycmd#1078: add semantic trigger for Julia; - PR ycm-core/ycmd#1081: improve type information on C++ member functions; - PR ycm-core/ycmd#1086: check if Python headers are installed before building; - PR ycm-core/ycmd#1088: raise proper exception for commands when file is still being parsed in C-family languages; - PR ycm-core/ycmd#1098: update Go completer; - PR ycm-core/ycmd#1099: update regex submodule; - PR ycm-core/ycmd#1100: add Python path to debugging information; - PR ycm-core/ycmd#1103: support framework headers completion and code navigation in C-family languages; - PR ycm-core/ycmd#1107: update Clang to 7.0.0; - PR ycm-core/ycmd#1109: update jdt.ls to 0.25.0; - PR ycm-core/ycmd#1110: handle null hover response from language servers; - PR ycm-core/ycmd#1111: update list of completion kinds defined by LSP; - PR ycm-core/ycmd#1113: send completion item kinds capability to language servers; - PR ycm-core/ycmd#1116: update Parso to 0.3.1 and Jedi to 0.13.1. Closes #3138. Closes #3145.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/3175)

micbou mentioned this pull request Aug 5, 2018

LookupError: unknown encoding: with typescript ycm-core/YouCompleteMe#3101

Closed

12 tasks

Remove nodejs workaround

8713009

Users should install the nodejs-legacy package on Debian-based distributions.

micbou force-pushed the remove-nodejs-workaround branch from f7096cb to 8713009 Compare August 5, 2018 14:20

bstaletic reviewed Aug 5, 2018

View reviewed changes

micbou commented Aug 5, 2018

View reviewed changes

bstaletic reviewed Aug 5, 2018

View reviewed changes

puremourning reviewed Aug 19, 2018

View reviewed changes

micbou commented Aug 19, 2018

View reviewed changes

bstaletic approved these changes Aug 19, 2018

View reviewed changes

puremourning approved these changes Aug 19, 2018

View reviewed changes

bstaletic approved these changes Aug 19, 2018

View reviewed changes

zzbot merged commit 8713009 into ycm-core:master Aug 19, 2018

micbou deleted the remove-nodejs-workaround branch August 19, 2018 19:37

micbou mentioned this pull request Oct 7, 2018

[READY] Update ycmd ycm-core/YouCompleteMe#3175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Remove nodejs workaround #1075

[READY] Remove nodejs workaround #1075

micbou commented Aug 5, 2018 •

edited

Loading

bstaletic left a comment

codecov bot commented Aug 5, 2018 •

edited

Loading

micbou left a comment

bstaletic left a comment

puremourning left a comment

micbou left a comment

bstaletic left a comment

puremourning left a comment

bstaletic left a comment

bstaletic commented Aug 19, 2018

zzbot commented Aug 19, 2018

zzbot commented Aug 19, 2018

zzbot commented Aug 19, 2018

[READY] Remove nodejs workaround #1075

[READY] Remove nodejs workaround #1075

Conversation

micbou commented Aug 5, 2018 • edited Loading

bstaletic left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2018 • edited Loading

Codecov Report

micbou left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

puremourning left a comment

Choose a reason for hiding this comment

micbou left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

puremourning left a comment

Choose a reason for hiding this comment

bstaletic left a comment

Choose a reason for hiding this comment

bstaletic commented Aug 19, 2018

zzbot commented Aug 19, 2018

zzbot commented Aug 19, 2018

zzbot commented Aug 19, 2018

micbou commented Aug 5, 2018 •

edited

Loading

codecov bot commented Aug 5, 2018 •

edited

Loading