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

Jump to bottom

coc-nvim hardcode nodejs path #91447

Closed
wants to merge 1 commit into from
Closed

coc-nvim hardcode nodejs path #91447

wants to merge 1 commit into from

Conversation

sheeldotme
Copy link

@sheeldotme sheeldotme commented Jun 24, 2020 •
edited

Motivation for this change

coc-nvim complains that it cannot find the nodejs executable. This PR fixes that by patching the path.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

thufschmitt
thufschmitt reviewed Jun 26, 2020
View reviewed changes
pkgs/misc/vim-plugins/patches/coc-nvim/nodejs-path.patch Outdated
let node = expand(g:coc_node_path)
else
- let node = $COC_NODE_PATH == '' ? 'node' : $COC_NODE_PATH
+ let node = '@nodejs@' == '' ? 'node' : '@nodejs@'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ let node = '@nodejs@' == '' ? 'node' : '@nodejs@'
+ let node = '$COC_NODE_PATH' == '' ? '@nodejs@' : $COC_NODE_PATH

Don't we rather want to override the default nodejs location, and still allow overriding it with $COC_NODE_PATH?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could export COC_NODE_PATH or ask coc to default to g:node_host_prog that is set in neovim wrapper ?

Copy link
Author

@sheeldotme sheeldotme Jul 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we rather want to override the default nodejs location, and still allow overriding it with $COC_NODE_PATH?

@regnat I'm happy to defer to you here. I wasn't sure what the idiomatic way to do this was, my thinking was that if someone configured a path in nix they wouldn't then want to override it.

Copy link
Author

@sheeldotme sheeldotme Jul 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could export COC_NODE_PATH or ask coc to default to g:node_host_prog that is set in neovim wrapper ?

@teto:

  1. Setting COC_NODE_PATH would be ideal if executed directly but since it's a vim plugin I'm not sure whether it would be desirable to export COC_NODE_PATH to neovim. Is there a way to do this cleanly?

  2. Defaulting to g:node_host_prog may make sense but since that's not nix specific functionality so maybe that should be handled upstream?

@sheeldotme
Copy link
Author

@jonringer any thoughts on this, would love to get it merged.

@stale
Copy link

stale bot commented Apr 18, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 18, 2021
jonringer
jonringer reviewed Apr 20, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably change the patch to be just

  postPatch  = ''
    substituteInPlace autoload/coc/util.vim \
      --replace "'' ? 'node'" "'' ? '${nodejs}/bin/node'"
  '''

just to avoid the inflexibility that patches have to minor changes.

Otherwise LGTM

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 20, 2021
@spagy
Copy link

spagy commented Jun 24, 2021

@sheeldotme are you still interested in this PR?

if not i'd be happy to make the change's suggested. would the right thing to do be to open another PR with the suggested changes?

@sheeldotme
Copy link
Author

@sheeldotme are you still interested in this PR?

if not i'd be happy to make the change's suggested. would the right thing to do be to open another PR with the suggested changes?

I’ll look at this tomorrow. Thanks for the reminder!

@ncfavier
Copy link
Member

Ping

@sheeldotme
Copy link
Author

I'm no longer the best person to commit this as I no longer use coc / nvim and it seems the way coc plugins are imported has changed.

@sheeldotme sheeldotme closed this Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teto teto teto left review comments

@thufschmitt thufschmitt thufschmitt left review comments

@jonringer jonringer jonringer left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: vim 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sheeldotme @spagy @ncfavier @teto @thufschmitt @jonringer @ryantm