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

python-language-server: init at 2020-04-24 #85930

Merged
merged 2 commits into from May 9, 2020

Conversation

thomasjm
Copy link
Contributor

Motivation for this change

This is the Microsoft implementation of the Language Server Protocol server for Python, used to support IDE features in compatible development environments.

Nixpkgs already includes this, sort of--it's already bundled as part of vscode-extensions. However, the way it is bundled isn't ideal--it's simply downloaded from an API under https://pvsc.azureedge.net. I read that this is not recommended, since this URL is more of an internal API for VSCode. Downloading binaries also doesn't let us use the most recent code on the master branch of Github.

Instead, this PR a) builds the language server from source and b) exposes it as a top-level application, so it can be installed/used independently of VSCode.

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
    • Ubuntu
  • [N/A] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • [N/A] 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.

@Mic92
Copy link
Member

Mic92 commented Apr 24, 2020

similar (stalled) pull requests: #70058

@thomasjm
Copy link
Contributor Author

@Mic92 that PR follows the same binary-downloading approach I mentioned in the description. This PR should be better.

@Mic92
Copy link
Member

Mic92 commented Apr 24, 2020

Yes. This one seems better.

@mjlbach
Copy link
Contributor

mjlbach commented Apr 24, 2020

This looks great! I'm just wondering if there should be a microsoft somewhere in the package name to distinguish it from the palantir python module.

@pedrovelho
Copy link

So I am trying to test it, I have got your branch and could run successfully nix-env -iA python-language-server. I have the error thomasjm, which just confirms the issue detected by the CI.

Let me know if I can help with anything else.

@thomasjm
Copy link
Contributor Author

@mjlbach thanks! Yeah I wondered that too, I looked over Nixpkgs and didn't see too many organization names so I left it out for now, although there's definitely possibility for confusion here. Maybe someone with more Nix experience can make a suggestion about how to disambiguate from Palantir's version.

@thomasjm
Copy link
Contributor Author

@pedrovelho thanks for testing it -- could you clarify what the issue you saw was? If it was related to the maintainers issue that seems to be fixed in the CI now.

@thomasjm
Copy link
Contributor Author

Update: I finally encountered #73810 myself so I was able to push a fix. Would appreciate if you tried it again @pedrovelho.

Latest commit has some other changes also.

@pedrovelho
Copy link

I confirm the maintainers issue is gone. It does not work on my machine but I am not sure this is a problem only I have. It is strange but I have the same behavior as vscode-with-extensions, it is like the binary is not executable on my architecture.

$ ls /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer
/nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer
$ /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer 
zsh: no such file or directory: /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer
$ /usr/bin/bash /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer
zsh: no such file or directory: /usr/bin/bash
$ /usr/bin/env bash /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer
/nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer: /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer: cannot execute binary file
 $ ldd /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/lib/Microsoft.Python.LanguageServer
	linux-vdso.so.1 (0x00007fffaed7e000)
	libpthread.so.0 => /nix/store/6m2k8kx8h216jlx9dg3lp4m90bz05yck-glibc-2.30/lib/libpthread.so.0 (0x00007fc7153d0000)
	libdl.so.2 => /nix/store/6m2k8kx8h216jlx9dg3lp4m90bz05yck-glibc-2.30/lib/libdl.so.2 (0x00007fc7153cb000)
	libstdc++.so.6 => not found
	libm.so.6 => /nix/store/6m2k8kx8h216jlx9dg3lp4m90bz05yck-glibc-2.30/lib/libm.so.6 (0x00007fc71528b000)
	libgcc_s.so.1 => /nix/store/6m2k8kx8h216jlx9dg3lp4m90bz05yck-glibc-2.30/lib/libgcc_s.so.1 (0x00007fc715271000)
	libc.so.6 => /nix/store/6m2k8kx8h216jlx9dg3lp4m90bz05yck-glibc-2.30/lib/libc.so.6 (0x00007fc7150b2000)
	/lib64/ld-linux-x86-64.so.2 => /nix/store/6m2k8kx8h216jlx9dg3lp4m90bz05yck-glibc-2.30/lib64/ld-linux-x86-64.so.2 (0x00007fc7153f3000)

I installed it by cloning your fork, checking out your branch, and using the export NIXPKGS=~/tmpdev/nixpkgs as described on the wiki. I finally installed it using nix-env -f $NIXPKGS -iA python-language-server. Please let me know if I am not installing it right.

My architecture if it helps.

> uname -a 
Linux fox 5.4.33 #1-NixOS SMP Fri Apr 17 08:50:26 UTC 2020 x86_64 GNU/Linux

@thomasjm
Copy link
Contributor Author

You want the version under bin, something like /nix/store/nmrdpcsr4hp2inqm0691ndgw9b6cql9i-python-language-server-2020-04-17/bin/python-language-server. I suspect your lack of libstdc++.so.6 is some other problem...

@cole-h
Copy link
Member

cole-h commented Apr 26, 2020

@ofborg build python-language-server

@worldofpeace
Copy link
Contributor

Why does this contribution target release-20.03? All new contributions should target master.

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg build python-language-server

@worldofpeace
Copy link
Contributor

The darwin issue seems to be in dotnet

Cannot nix-instantiate `python-language-server' because:
warning: ignoring the user-specified setting 'restrict-eval', because it is a restricted setting and you are not a trusted user
error: while evaluating the attribute 'buildInputs' of the derivation 'python-language-server-2020-04-17' at /private/var/lib/ofborg/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/lnl7-mac/pkgs/stdenv/generic/make-derivation.nix:191:11:
while evaluating the attribute 'handled' at /private/var/lib/ofborg/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/lnl7-mac/pkgs/stdenv/generic/check-meta.nix:251:7:
while evaluating 'handleEvalIssue' at /private/var/lib/ofborg/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/lnl7-mac/pkgs/stdenv/generic/check-meta.nix:147:38, called from /private/var/lib/ofborg/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/lnl7-mac/pkgs/stdenv/generic/check-meta.nix:252:14:
Package ‘dotnet-sdk-3.1.101’ in /private/var/lib/ofborg/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/lnl7-mac/pkgs/development/compilers/dotnet/buildDotnet.nix:63 is not supported on ‘x86_64-darwin’, refusing to evaluate.
a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.
b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

Builds locally. Diff LGTM, minus commits needing squashed.

nixpkgs-review pr 85930

https://github.com/NixOS/nixpkgs/pull/85930
1 package built:
python-language-server

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Same conclusion as @drewrisinger: squash commits into something sensible that follows CONTRIBUTING.md, and this should be good to go!

And just to make sure: please make sure your "add to maintainers" commit is separate from the actual packaging commit :)

[8 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/85930
1 package built:
python-language-server

@drewrisinger
Copy link
Contributor

@GrahamcOfBorg build python-language-server

@thomasjm thomasjm force-pushed the microsoft-python-language-server branch from ab3f178 to 5ba390d Compare April 29, 2020 16:48
@thomasjm
Copy link
Contributor Author

Whoops missed a few, hang on

@thomasjm thomasjm force-pushed the microsoft-python-language-server branch from 5ba390d to 862b228 Compare April 29, 2020 16:51
@drewrisinger
Copy link
Contributor

@GrahamcOfBorg build python-language-server

@thomasjm
Copy link
Contributor Author

Sweet. After we land this a separate PR can replace the server used in vscode-extensions with this one.

Sadly nixpkgs-review pr 85930 doesn't work for me locally because I use a different Nix root from /nix and it doesn't like that, but glad you two were able to build.

@drewrisinger
Copy link
Contributor

Issues I still see blocking this:

@thomasjm thomasjm changed the title Package Microsoft's Python Language Server python-language-server: init at 2020-04-24 Apr 29, 2020
@thomasjm thomasjm force-pushed the microsoft-python-language-server branch from 862b228 to 8c36488 Compare April 29, 2020 18:24
@thomasjm
Copy link
Contributor Author

Just addressed both

@drewrisinger
Copy link
Contributor

@GrahamcOfBorg build python-language-server

@thomasjm
Copy link
Contributor Author

thomasjm commented May 2, 2020

Thank you both for the review!

Please hold off on merging for now, I've been trying to use it a bit more and identified another linker problem that can happen at runtime (involving openssl this time, there's some discussion about the same problem on #73810).

@thomasjm
Copy link
Contributor Author

thomasjm commented May 2, 2020

@GrahamcOfBorg build python-language-server

@mjlbach
Copy link
Contributor

mjlbach commented May 7, 2020

I've been using this for the past week and have thus far not run into any issues!

@thomasjm thomasjm force-pushed the microsoft-python-language-server branch from 1a5286f to 83e340b Compare May 8, 2020 00:04
@thomasjm thomasjm force-pushed the microsoft-python-language-server branch from 83e340b to 5db5f2f Compare May 8, 2020 00:07
@thomasjm
Copy link
Contributor Author

thomasjm commented May 8, 2020

Oh that reminds me, I've been using it as well and it seems fine after the last couple tweaks. I just re-squashed and re-rebased on master. I think this is ready to merge anytime.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

  • reviewed the diff and commit messages
  • made sure ofBorg build succeeded for all applicable platforms
  • run nix-review for a reasonable amount of time without any failures (or marked preexisting failures as broken)

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/151

@FRidh FRidh merged commit d963bf3 into NixOS:master May 9, 2020
@FRidh
Copy link
Member

FRidh commented May 9, 2020

Maybe the vscode extension package should be linking to this one instead?

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

Successfully merging this pull request may close these issues.

None yet

9 participants