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

vscode-extensions.ms-python.python: 2018.12.1 -> 2019.6.22090 #55149

Merged

Conversation

eadwu
Copy link
Member

@eadwu eadwu commented Feb 4, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

Looks good, but isn't this broken as described in #53923?

@eadwu
Copy link
Member Author

eadwu commented Feb 4, 2019

Just tested on a random python file, it is broken and requires patching for the Jedi language server.

Starting Jedi Python language engine.
Error: Traceback (most recent call last):
  File "/nix/store/1xxi7wv4ih13qa49zf9wjr9hddhrs53h-vscode-insiders-extension-ms-python-python-2019.1.0/share/vscode-insiders/extensions/ms-python.python/pythonFiles/sortImports.py", line 11, in <module>
    import isort.main
  File "/nix/store/1xxi7wv4ih13qa49zf9wjr9hddhrs53h-vscode-insiders-extension-ms-python-python-2019.1.0/share/vscode-insiders/extensions/ms-python.python/pythonFiles/lib/python/isort/main.py", line 31, in <module>
    import setuptools
ModuleNotFoundError: No module named 'setuptools'

@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.1.0 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.2.5433 Mar 4, 2019
@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from e8a0da9 to a6a7f0b Compare March 4, 2019 01:45
@veprbl
Copy link
Member

veprbl commented Mar 25, 2019

@eadwu Is this ready now?

@eadwu
Copy link
Member Author

eadwu commented Mar 26, 2019

Doesn't look like it, though I'll test it again tomorrow.

Still broken.

@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from a6a7f0b to b598023 Compare April 1, 2019 01:21
@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.2.5433 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.3.6215 Apr 1, 2019
@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.3.6215 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.3.6352 Apr 7, 2019
@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from b598023 to 98a36b9 Compare April 8, 2019 01:02
@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.3.6352 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.4.11987 Apr 29, 2019
@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from 98a36b9 to aebaa9e Compare April 29, 2019 01:07
@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from aebaa9e to 928979f Compare May 6, 2019 01:46
@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.4.11987 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.4.12954 May 6, 2019
@worldofpeace
Copy link
Contributor

If anyone wants this to be working again, I'm pretty sure if they pick up #53923 and supply the requested changes it should work. (i think)

@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from 928979f to ca83625 Compare May 25, 2019 14:02
@eadwu
Copy link
Member Author

eadwu commented May 25, 2019

Should've implemented the changes from that PR, will test later.

@eadwu
Copy link
Member Author

eadwu commented May 28, 2019

Looks to be fine, at least the language server part, since tooltips are showing up. Though maybe because of my settings, I'm getting an error with isort which I'll get around to fixing someday.

Error: Traceback (most recent call last):
  File "/nix/store/a1qpngl6wcrhi6i8pc0ic9l3pnwc4bmd-vscode-insiders-extension-ms-python-python-2019.4.12954/share/vscode-insiders/extensions/ms-python.python/pythonFiles/sortImports.py", line 11, in <module>
    import isort.main
  File "/nix/store/a1qpngl6wcrhi6i8pc0ic9l3pnwc4bmd-vscode-insiders-extension-ms-python-python-2019.4.12954/share/vscode-insiders/extensions/ms-python.python/pythonFiles/lib/python/isort/main.py", line 29, in <module>
    import setuptools
ModuleNotFoundError: No module named 'setuptools'

Copy link
Member

@jraygauthier jraygauthier left a comment

Choose a reason for hiding this comment

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

To me this would be a perfectly fine first take.

It would be better were the language server part be moved out to its own file but even from the pt of view of its developers, this really has only been tested against vscode.

Tested the changes with a project of mine and it seems to work fine as first glance (test discovery, debugging, symbols, linting integration).

So Approved with Suggestions (Not yet).

UPDATE: It might be wise to wait and test this a little bit more before merging. With some more complex projects the language server seem to have trouble finding some modules when running pytest test discovery which are properly discovered when running pytest --collect-only from the same environment. Seems related to the isort issue above.

UPDATE2: Alright, it seems that the extensions changed quite a bit in how it works with environments. Activating the environment by selecting the interpreter from the status bar did the trick. For some reason, when there is a single environment, vscode does not automatically activate this environment even though your interpreter setting is explicitly set to "python.pythonPath": "python". This change seems to have been persisted somewhere (most likely in one of the cache directory under ~/.config/Code) as it did work out of the box after reloading vscode. I hope this cache won't stick with an old interpreter once I update my nix environment... Might be related to the following doc fragment:

  • environments-and-terminal-windows

    However, launching VS Code from a shell in which a certain Python
    environment is activated does not automatically activate that
    environment
    in the default integrated terminal.

Might be of interest for other nix users:

  • manually-specify-an-interpreter

    You can use an environment variable in the path setting using the
    syntax ${env:VARIABLE}. For example, if you've created a variable
    named PYTHON_INSTALL_LOC with a path to an interpreter, you can then
    use the following setting value:

    "python.pythonPath": "${env:PYTHON_INSTALL_LOC}",
    

    By using an environment variable, you can easily transfer a project
    between operating systems where the paths are different, just be
    sure to set the environment variable on the operating system first.

UPDATE 3: It would seem that this is not totally functional yet. When I attempt to use the "Microsoft Python Analysis Engine" instead of "Jedi" for intelliSense by setting "python.jediEnabled": false, I get a "The Python Tool Server crashed 5 times in the last 3 minutes" error notification. and nothing works anymore. @eadwu : Did you test with "python.jediEnabled": false ?

}."${arch}";

# version is languageServerVersion in the package.json
languageServer = extractNuGet rec {
Copy link
Member

Choose a reason for hiding this comment

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

I believe what was meant by @worldofpeace in his review of the preceding incarnation of this PR was that it would be preferable that languageServer be moved in its own file (as it can be used by other packages) and taken as input.

This would indeed remove most of the complexity as the added build inputs are to my understanding meant only for the download, extraction and automated patchelf-ing of the language server:

stdenv, fetchurl, extractNuGet, icu, curl, openssl, lttng-ust, autoPatchelfHook

The extractNuGet utility could then be moved out of of the extension sub folder as well to be potentially used by others.

This is however merely a recommendation as it could indeed be done later as the need arises.


meta = with lib; {
license = licenses.mit;
maintainers = [ maintainers.jraygauthier ];
Copy link
Member

Choose a reason for hiding this comment

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

You might want to add yourself as a maintainer ;)

name = "Python-Language-Server";
version = "0.2.82";

src = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

@worldofpeace : My understanding as to why fetchNuGet is not used is that there is no official standalone release yet as mentioned in Add installation instructions to README · Issue #953 · microsoft/python-language-server.

We do not provide official downloads for the installation yet (although download URLs are not secret but they can change). So it is recommended for clients to build their own. One of the reasons is that build we use is only tested with VS Code.

It is also not possible (yet) to build this be ourselves as xbuild is officially deprecated in favor of msbuild (see Mono 5.0.0 Release Notes | Mono) and does not meet the minimal requirement of this project (tried it myself).

Adding support for msbuild (even borrowed from debian) is an unfinished process:

So to me, what's has been done here is fine for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep completely agree here. Perhaps a todo should be added though for when that's resolved.

@worldofpeace worldofpeace self-requested a review June 1, 2019 00:47
@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.4.12954 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.5.17517 Jun 3, 2019
@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from ca83625 to d4289bf Compare June 3, 2019 01:02
@jraygauthier
Copy link
Member

@eadwu / @worldofpeace : I have been using the extension with python.jediEnabled": true (which is the default) for quite some time now and everything works fine. I personally think we should merge this in its actual state and attempt to fix the python.jediEnabled": false separately (a second pass).

@eadwu eadwu changed the title vscode-extensions.ms-python.python: 2018.12.1 -> 2019.5.17517 vscode-extensions.ms-python.python: 2018.12.1 -> 2019.6.22090 Jul 8, 2019
@eadwu eadwu force-pushed the vscode-extensions.ms-python.python/2019.1.0 branch from d4289bf to a8b31a1 Compare July 8, 2019 01:21
@worldofpeace worldofpeace merged commit 696767a into NixOS:master Jul 14, 2019
@worldofpeace
Copy link
Contributor

It would be a good idea to open issues in nixpkgs for the known regressions.

@eadwu eadwu deleted the vscode-extensions.ms-python.python/2019.1.0 branch November 17, 2020 23:34
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

6 participants