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

Locate the Python interpreter via the py.exe launcher #53

Merged
merged 6 commits into from Sep 20, 2017

Conversation

Projects
None yet
3 participants
@pfmoore
Copy link
Contributor

commented Sep 19, 2017

On Windows, if the default strategy for finding a versioned interpreter fails, ask the py.exe launcher to locate it for us.

@pfmoore

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2017

Some outstanding questions:

  • Does this need tests? It probably should, but the existing tests say "Not testing multiple interpreters on Windows" and I can see why - it's going to be quite hard to test this without mocking a lot of stuff (and possibly reqriting some of the code to make it more testable)
  • Should this check replace the existing hard-coded list of interpreters? In theory, it's more reliable and the py.exe launcher should always be present.
  • If we do keep both approaches, should the default list or the py.exe check be first?
@theacodes

This comment has been minimized.

Copy link
Owner

commented Sep 19, 2017

Thank you so much for doing this, @pfmoore!

Does this need tests? It probably should, but the existing tests say "Not testing multiple interpreters on Windows" and I can see why - it's going to be quite hard to test this without mocking a lot of stuff (and possibly reqriting some of the code to make it more testable)

I can handle this - it's likely we'll need to mock some stuff for coverage purposes but have a real test for when on an actual windows platform.

Should this check replace the existing hard-coded list of interpreters? In theory, it's more reliable and the py.exe launcher should always be present.

I think so, yes.

If we do keep both approaches, should the default list or the py.exe check be first?

Let's just drop the old method.

"""
ver = "-%s.%s" % (v_maj, v_min)
script = "import sys; print(sys.executable)"
py_exe = distutils.spawn.find_executable('py')

This comment has been minimized.

Copy link
@theacodes

theacodes Sep 19, 2017

Owner

TIL about distutils.spawn.find_executable! Should we use that over py.path.local.sysfind which we use in command?

This comment has been minimized.

Copy link
@pfmoore

pfmoore Sep 19, 2017

Author Contributor

Honestly, I doubt it matters (I lifted that code from tox unchanged). I'd be inclined to go for consistency and follow current practice by using py.path.local.sysfind in both places. I'll change this.

(BTW, there's also shutil.which in Python 3.3+. Who said "one obvious way"? 😄)

This comment has been minimized.

Copy link
@theacodes

theacodes Sep 19, 2017

Owner

(BTW, there's also shutil.which in Python 3.3+. Who said "one obvious way"? 😄)

I'd love to make nox 3+ only. Sigh.

This comment has been minimized.

Copy link
@pfmoore

pfmoore Sep 19, 2017

Author Contributor

I'll also make the change to drop the old approach, as noted in your other comment. I can do the tests too, if you want, but if you have a preferred style/approach for the tests and would rather cover that yourself, that's fine too.

This comment has been minimized.

Copy link
@theacodes

theacodes Sep 19, 2017

Owner

We use pytest so as long as it's not terribly different from what's there go for it. :)

@pfmoore

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

Looking into the coverage failures...

@pfmoore pfmoore force-pushed the pfmoore:win_interpreter_via_launcher branch from 43724a7 to 6d26c83 Sep 20, 2017

@pfmoore

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

There's a remaining coverage failure - line 153 in nox/virtualenv.py:

        if self.interpreter:
            cmd.extend(['-p', self._resolved_interpreter])

I'm not sure why the changes I've made would have caused this line to no longer be executed. @jonparrott do you have any suggestions?

@theacodes

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2017

I'm not sure why the changes I've made would have caused this line to no longer be executed. @jonparrott do you have any suggestions?

It's my fault, you can disregard that coverage issue.

@pfmoore

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

I like that sort of answer ;-) In which case I think this is good to go. Could you check it over for me?

@theacodes

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2017

LGTM- I updated the docstring to mention pep 397. I'm gonna wait for CI to merge (I know it'll fail due to coverage, just want to make sure that's the only failure).

@theacodes

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2017

Thank you so much for doing this @pfmoore! 🍰

@theacodes

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2017

All good. Merging. :)

@theacodes theacodes merged commit 95863f7 into theacodes:master Sep 20, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pfmoore

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

Glad to help 😄

@dhermes

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2017

🎉 Yay Windows!

"""
script = "import sys; print(sys.executable)"
py_exe = py.path.local.sysfind('py')
if py_exe:

This comment has been minimized.

Copy link
@dhermes

dhermes Sep 20, 2017

Collaborator

@jonparrott Should this be a None-check?

This comment has been minimized.

Copy link
@theacodes

theacodes Sep 20, 2017

Owner

Possibly. If you want you can send a follow-up PR.

This comment has been minimized.

Copy link
@dhermes

dhermes Sep 20, 2017

Collaborator

Done #59

dhermes added a commit to dhermes/nox that referenced this pull request Sep 20, 2017

Adding None-check to `locate_via_py`.
Follow-up to theacodes#53. Also

- Using single quotes instead of double quotes
- Adding Args/Returns section to docstring

theacodes added a commit that referenced this pull request Sep 20, 2017

Adding None-check to `locate_via_py`. (#59)
* Adding None-check to `locate_via_py`.

Follow-up to #53. Also

- Using single quotes instead of double quotes
- Adding Args/Returns section to docstring

* Updating unit test mocks: faithfully mock `sysfind`.
@dhermes

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

BTW I am living life happily in Windows land and I can confirm this PR works great (and nox was broken-ish before I pip install git+https://github.com/jonparrott/nox/)

@theacodes

This comment has been minimized.

Copy link
Owner

commented Oct 9, 2017

@dhermes dhermes referenced this pull request Oct 12, 2017

Merged

Cutting release 0.18.2. #63

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.