Skip to content

use ninja importability to determine python-ninja usability#1378

Closed
ngam wants to merge 2 commits intogoogle:mainfrom
ngam:fixninja
Closed

use ninja importability to determine python-ninja usability#1378
ngam wants to merge 2 commits intogoogle:mainfrom
ngam:fixninja

Conversation

@ngam
Copy link
Copy Markdown
Contributor

@ngam ngam commented Feb 22, 2023

Fixes #1376

There doesn't seem to be a need to force people to use python-ninja when the ninja binary is available. The logic is better now such that if python-ninja isn't available (importable), the command can fall back to ninja the binary.

xref:

thanks to:

@ngam ngam changed the title fix ninja importability when python-ninja isn't available use ninja importability when python-ninja isn't available Feb 22, 2023
@eli-schwartz
Copy link
Copy Markdown

Also xref: #1376 (should be marked as closing that ticket).

])
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:
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.

this can be just ... and importable since find_spec does not return anything else that evaluates to False

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.

Do you prefer a one-liner or a two-liner?

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 prefer the two-liner, the variable name importable does add a nice bit of readability.

@martindemello
Copy link
Copy Markdown
Contributor

thanks, that looks like a great fix! sorry for all the conda issues.

@ngam ngam changed the title use ninja importability when python-ninja isn't available use ninja importability to determine python-ninja usability Feb 22, 2023
@martindemello
Copy link
Copy Markdown
Contributor

looks good, i'll merge it internally (retaining you as author) and push it out as part of the next pytype release

@ngam
Copy link
Copy Markdown
Contributor Author

ngam commented Feb 22, 2023

Perfect, thanks!

@rchen152 rchen152 closed this in 9c623fc Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pytype not using system ninja, even if it's there

3 participants