Respect custom mypy path when checking for mypy availability

fralik commented 2 years ago

First time contributor checklist

[x] I have read how to contribute to this project
[x] I have read the code of conduct to this project

Contributor checklist

[x] I am targeting the master branch (and not the release branch)
[x] I am using the provided codeStyleConfig.xml
[x] I have tested my contribution on these versions of PyCharm/IDEA:
- PyCharm Community 2021.3.1
- IntelliJ IDEA 2021.3.2
[x] My contribution is fully baked and ready to be merged as is
[x] I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

Function checkMypyAvailable is called from various places in the code. If user specifies a custom mypy executable, which is not part of project's venv, then this function will return false when called with showNotifications == true. This happens because the function checks for mypy presence in the project's venv and it doesn't respect the custom path. This change makes the function check for mypy in the project's venv only if no custom path is present.
Additional note. It may be better to check for mypy presence in the current venv after isMypyPathValid call. Thus, a user would get two notifications: mypy is not available, click here to install it.

Type of Changes

	Type
✓	:bug: Bug fix
	:sparkles: New feature
	:hammer: Refactoring
	:scroll: Docs

Related Issue

Closes #98

intgr commented 2 years ago

Hi, thanks for the PR.

I had a bit of time and tried this out. I couldn't make it work though, for some reason the plugin gives me a notice "executable ... not found" for any path I try, even though the path obviously exists. Possibly PEBKAC, I will try again when I have more time.

fralik commented 2 years ago

Thanks for looking at it. I will double check that it works and will try to also do it under Linux (I mainly use Windows).

fralik commented 2 years ago

@intgr , I got some time to look into it. The problem is in Linux system. Kind of. ;) I was able to reproduce the problem from a terminal:

If I simply run /usr/bin/mypy -V, then it works.
If I do /something/something/venv/bin/python /usr/bin/mypy -V, then it doesn't work. python here is inside a virtual environment without mypy in it.

There is this code in the plugin: https://github.com/leinardi/mypy-pycharm/blob/26299d03651b45f8721ea36b8d538be7a020b501/src/main/java/com/leinardi/pycharm/mypy/mpapi/MypyRunner.java#L369 in function called getMypyCommandLine. It only checks if an executable is a windows executable. On Linux it calls mypy as in line 2 from above instead of line 1. It could be fixed with simply checking if file is executable regardless of OS. Although, there was probably a reason why it is checked only under Windows. And checking if something is executable under Linux may be ambiguous.

I think that the whole process of executing mypy should be simplified:

If user didn't provide a custom path, do what is done today: check current python environment, auto-detect system-wide mypy.
If user provided a custom path, try to use that path directly. If it doesn't work, try to use it as current_python custom_mypy_path.

Either way, add --python-executable argument to mypy, which would point to project's interpreter. This will ensure correct packages could be imported by mypy.

intgr commented 2 years ago

I agree, the isWindowsExecutable check is suspicious. It was apparently added in #16 to fix bug #15. Apparently in Windows, mypy's entry point mypy.exe is a binary file rather than a Python script.

The issue I had with 'executable "/usr/local/bin/mypy" not found' on macOS https://github.com/leinardi/mypy-pycharm/pull/99#issuecomment-1067303500 seems to stem from the same root cause as #15:

2022-03-26 14:49:53,294 [ 245192]   INFO - .pycharm.mypy.mpapi.MypyRunner - Detected Mypy path: /usr/local/bin/mypy 
2022-03-26 14:49:53,387 [ 245285]   INFO - .pycharm.mypy.mpapi.MypyRunner - Command Line string: /usr/local/bin/python3.9 /usr/local/bin/mypy -V 
2022-03-26 14:49:53,387 [ 245285]   WARN - .pycharm.mypy.mpapi.MypyRunner - Error while checking Mypy path: Traceback (most recent call last):
  File "/usr/local/bin/mypy", line 5, in <module>
    from mypy.__main__ import console_entry
ModuleNotFoundError: No module named 'mypy'

Peering into /usr/local/bin/mypy gives a clue: a globally installed mypy script itself is supposed to know the correct Python interpreter:

#!/usr/local/Cellar/mypy/0.940/libexec/bin/python3.10

But this plugin tries to invoke it with the project's interpreter, which does not have mypy installed locally.

intgr commented 2 years ago

I think that the whole process of executing mypy should be simplified

You're on the right track. The plugin should distinguish between two different ways how mypy can be installed:

If mypy is installed in a virtualenv, invoke mypy via the virtualenv's interpreter: $VIRTUAL_ENV/bin/python $VIRTUALENV/bin/mypy ...
If mypy is installed globally, invoke the mypy binary directly without specifying a Python interpreter. Similar to the isWindowsExecutable condition, but should behave the same on all OSes.

So the question is, how to distinguish between (1) and (2). Perhaps we can get away with assuming that when a custom path is specified, it's referring to a global mypy installation?

Or make some attempt to detect whether the specified path is a virtualenv path? Seems fragile to me. But it sounds like a legitimate use case that someone may want to have a special virtualenv that contains mypy, and use that in multiple projects.

Perhaps the settings UI should be redesigned to offer three options:

Specify globally installed mypy by path, executed as (2) (e.g. my use case with /usr/local/bin/mypy)
Dropdown to select between virtual envs configured in PyCharm, these would be executed as (1)
(Default) Auto-detect: if the project's virtualenv contains mypy run it as (1), otherwise try to detect a global mypy installation in PATH environment variable and run it as (2).

intgr commented 2 years ago

Either way, add --python-executable argument to mypy, which would point to project's interpreter

Agreed, this is a good idea. But it solves a different problem and should probably be a separate pull request.

leinardi commented 11 months ago

Closing this since it seems to be addressed by #111

leinardi / mypy-pycharm