added test of full command line use.#98
Conversation
PythonCHB
requested review from
Anthchirp,
marcelotrevisani and
ngam
as code owners
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
|
Hi @PythonCHB, thank you for this. If you could identify which pytype versions are "broken" with your test, then we will mark them as broken en masse. For now, I stopped releasing pytype until we figure out what to do about ninja, see #99 |
|
Hi! This is the friendly automated conda-forge-linting service. I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug. |
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
|
@PythonCHB @eli-schwartz @henryiii could you have a look if this hacky solution is good enough? I think I will want to mark everything before this as broken, but I am happy to hear your thoughts. Thanks! |
| 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")] |
There was a problem hiding this comment.
What is this actually supposed to do when os.environ.get() falls back on None?
There was a problem hiding this comment.
Well, it never falls back on None when inside a conda env, but good question! What should it fall on then?
There was a problem hiding this comment.
I will skip the logic if CONDA_PREFIX isn’t available. I think that’s the goal.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Feedback well taken. I obvz was only thinking about conda-forge (because does anything else exist?!?!?!)
will incorporate edits soon
There was a problem hiding this comment.
I think I addressed your feedback. Will merge in ~2–3 days unless there are more issues
I went back to a year ago -- and no luck, I think this has been broken for a long time :-( I"m kinda surprised there haven't been nore complaints -- maybe folks aren't using the pacakge, or they are either:
I"ll try to go back an find a working version tonight. |
|
Most people would use in the ci, that’s how I used it :) and in the ci, you would just get it from pypi |
| + if sys.executable is not None: | ||
| + return [sys.executable, '-m', module or binary] | ||
| + else: | ||
| + return [binary] |
There was a problem hiding this comment.
The diff hunk would be slightly smaller here if you preserved the indentation for the sys.executable check and just switched it to an elif, rather than nesting it inside of an else.
That's really just style, so it doesn't matter one way or another, just thought I'd mention it.
There was a problem hiding this comment.
Ugh, something isn't working...
TypeError: unsupported operand type(s) for +: 'NoneType' and 'list'
Something went wrong running pytype
There was a problem hiding this comment.
what in this is returning a None? I had a loooooong day... :(
There was a problem hiding this comment.
(Will incorporate your suggestion once confirmed to work btw)
| ]) | ||
| - if sys.executable is not None: | ||
| + importable = importlib.util.find_spec(module or binary) | ||
| + if sys.executable is not None and importable is not None: |
There was a problem hiding this comment.
@eli-schwartz I hope you're damn proud of this 😆
|
Thanks @eli-schwartz and @PythonCHB. (You're welcome to add yourselves as maintainers if you'd like.) Learned quite a bit from you, and had good fun doing this, albeit with a headache after a lengthy day with crazy numerical models... |
|
@ngam: Thanks for making the pytype PR -- that's great! |
3 participants
Fix #97
Due to the issue with pytype packages being broken for ages -- thanks to the ninja python wrapper, I thought it would be handy to have a test in the recipe that tests if you can actually run pytype.
So here it is -- It's failing now, which is the point. I *think( it will pass if the issue gets resolved upstream -- but we'll see, it's hard to test that.