Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions recipe/meta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ package:
source:
- url: https://github.com/google/pytype/archive/refs/tags/{{ version }}.tar.gz
sha256: 5ad6439e5631077310a3217442bc9f13e91910d66755356828dbf15a3aacd2db

patches:
- patches/patch-ninja-exe.patch
- patches/remove-ninja-req.patch

- url: https://github.com/google/googletest/archive/e82d320.tar.gz
sha256: 7acdbac36f80ca68126c9112cf1e60132372aff81db7a14f08cb49e6e00af536
folder: googletest
Expand All @@ -21,7 +24,7 @@ source:
folder: typeshed

build:
number: 0
number: 1
skip: true # [win]
# no windows builds here: https://pypi.org/project/pytype/#files
entry_points:
Expand Down Expand Up @@ -59,15 +62,16 @@ test:
imports:
- pytype
- pytype.overlays
files:
- run_pytype.py
commands:
- annotate-ast --help
- merge-pyi --help
- pytd --help
- pytype --help
- pytype-single --help
- pyxref --help
# enable after https://github.com/conda-forge/staged-recipes/pull/19098
# this fails; guarding against providing broken? pytype from now on
- python run_pytype.py # checks that pytype can actually run with a file input.
- pip check
requires:
- pip
Expand Down
22 changes: 22 additions & 0 deletions recipe/patches/patch-ninja-exe.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
diff --git a/pytype/tools/analyze_project/pytype_runner.py b/pytype/tools/analyze_project/pytype_runner.py
--- pytype/tools/analyze_project/pytype_runner.py
+++ pytype/tools/analyze_project/pytype_runner.py
@@ -1,6 +1,7 @@
"""Use pytype to analyze and infer types for an entire project."""

+import os
import collections
import itertools
import logging
import subprocess
@@ -36,8 +37,10 @@


def _get_executable(binary, module=None):
"""Get the path to the executable with the given name."""
+ if binary == "ninja":
+ return [os.path.join(os.environ.get("CONDA_PREFIX", None), "bin", "ninja")]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is this actually supposed to do when os.environ.get() falls back on None?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, it never falls back on None when inside a conda env, but good question! What should it fall on then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will skip the logic if CONDA_PREFIX isn’t available. I think that’s the goal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If this is a patch specifically for the conda package, it may be OK. But ideally, we'd have a patch that the pytype folks could apply for universal use, So relying on CONDA_PREFIX is not great.

How about something like:

try:
    subprocess.call('ninja')
    ninja_exe = 'ninja'
except FileNotFoundError:
    ninja_exe = 'python -m ninja'

or

try:
    import ninja
    ninja_exe = 'python -m ninja'
except ImportError:
    ninja_exe = 'ninja'

which would prioritize teh pip-installed ninja if it's there.

But ideally this would get fixed upstream -- if pytype actually relies on something specific about the python ninja package, or even is only tested with it, then maybe we should jsut follow theri wishes and provide the package.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Well, you can't join None, that's just going to print a completely bizarre:

TypeError: expected str, bytes or os.PathLike object, not NoneType

If the environment variable ever doesn't exist.

This is why I asked what it's supposed to do. Supposed to! I'm not asking what it does, I know what it does.

I think asking "what should it fall back on then" is approaching the question from the wrong direction. The question is not what default to use to avoid a KeyError for an unset environment variable. The question instead is...

... When and why can that environment variable be unset? Is there ever a valid reason for it to be unset?

If it can't ever be unset, then don't use .get(), use os.environ['CONDA_PREFIX']. And if that ever fails, then the user gets a much clearer error. Instead of "expected str, bytes or os.Pathlike", they get a KeyError: 'CONDA_PREFIX' which points the finger at conda's environment setup.

Falling back on None actually makes the error message worse.

...

Is there any specific reason to not just return 'ninja' without a full path? My assumption is that the conda-installed ninja is always in $PATH whenever the conda-installed pytype is.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If this is a patch specifically for the conda package, it may be OK. But ideally, we'd have a patch that the pytype folks could apply for universal use, So relying on CONDA_PREFIX is not great.

Indeed.

But ideally this would get fixed upstream -- if pytype actually relies on something specific about the python ninja package, or even is only tested with it, then maybe we should jsut follow theri wishes and provide the package.

The python ninja package is effectively identical to regular C++ ninja with the exception of a couple patches from an open PR to ninja, which add support for detecting a parent GNU make process and attach to GNU make's jobserver. This doesn't change how users interact with ninja, and doesn't change how build.ninja files are parsed -- it's totally invisible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's hope the pytype folks appreciate that and fix their code then.

But in any case, it would be better find a solution that is not conda-specific -- could it use "which" on *nix and "where" on Windows?

Better yet: shutil.which

-CHB

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feedback well taken. I obvz was only thinking about conda-forge (because does anything else exist?!?!?!)

will incorporate edits soon

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I addressed your feedback. Will merge in ~2–3 days unless there are more issues

if binary == 'pytype-single':
custom_bin = path_utils.join('out', 'bin', 'pytype')
if sys.argv[0] == custom_bin:
# The Travis type-check step uses custom binaries in pytype/out/bin/.
13 changes: 13 additions & 0 deletions recipe/patches/remove-ninja-req.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
diff --git a/setup.cfg b/setup.cfg
--- setup.cfg
+++ setup.cfg
@@ -35,9 +35,8 @@
importlab>=0.8
jinja2>=3.1.2
libcst>=0.4.9
networkx<2.8.4
- ninja>=1.10.0.post2
pydot>=1.4.2
tabulate>=0.8.10
toml>=0.10.2
typed_ast>=1.5.0; python_version < '3.8'
44 changes: 44 additions & 0 deletions recipe/run_pytype.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
"""
A short script to run pytype with a real file to check

This should catch whether you can actually run pytype

(in particular if pytypw works with the conda-installed ninja)

NOTE: I suppose it would be more robust to see if it actually
does the check correctly, but that's really pytype's problem
"""
import sys
import subprocess
from pathlib import Path

TEST_FILE = """

def fun(x: int):
return x * 2

# this should type check with no errors
y = fun(2)

# this should type check with an error
# z = fun(2.0)
"""

filepath = Path("python_test_file.py")

with open(filepath, 'w', encoding='utf-8') as pyfile:
pyfile.write(TEST_FILE)

try:
subprocess.check_call(("pytype", str(filepath)))
# it worked
sys.exit(0)
except subprocess.CalledProcessError:
# something went wrong
print("Something went wrong running pytype")
raise
finally:
filepath.unlink(missing_ok=False)