pytest-dev / py

Python development support library (note: maintenance only)
MIT License
68 stars 106 forks source link

also accept link files when looking for executable on Windows #142

Closed gaborbernat closed 7 years ago

gaborbernat commented 7 years ago

these could be used to solve name clashes; e.g. installing multiple Pythons via conda - each version in installed as a separate env into its own folder; than you can expose these globally by creating symlinks from a top level

gaborbernat commented 7 years ago

@RonnyPfannschmidt the main benefit of this would be be the ability for tox to discover more installed interpreters on Windows - e.g. the ones symlink-ed aka python26.exe.lnk (not just python26.exe).

RonnyPfannschmidt commented 7 years ago

@gaborbernat im not familiar with windows details, @nicoddemus please take a look

gaborbernat commented 7 years ago

actually it seems the Python sub process module does not support running link files (which is a shortcut); it throws some not a valid Windows 32 executable - which is just wrong as you can run shortcuts from within a command line 馃槓

nicoddemus commented 7 years ago

@gaborbernat so the PR does not work in the end then?

gaborbernat commented 7 years ago

@nicoddemus it works at this level, but breaks at another place; I did not investigate how to fix it there. That would be a separate PR I think.

nicoddemus commented 7 years ago

Which another place, could you be more specific please? I would hate to merge this and break something else.

RonnyPfannschmidt commented 7 years ago

as far as things look, i dont think this pr is acceptable without a working functional test

nicoddemus commented 7 years ago

as far as things look, i dont think this pr is acceptable without a working functional test

Oh yeah definitely. I will mark it as "Changes Requested" meanwhile.

gaborbernat commented 7 years ago

@nicoddemus @RonnyPfannschmidt the resulting problem is not this libraries bug, but the Pythons subprocess modules bug, see ( python27.exe.lnk is a shortcut to python.exe):

C:\Python27
位 ls python*
python.exe*  python27.exe.lnk*  pythonw.exe*

位 python.exe -c "import sys; print(sys.version)"
2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:42:59) [MSC v.1500 32 bit (Intel)]

C:\Python27
位 python.exe -c "import subprocess; print(subprocess.call(['./python.exe', '-c', '\"import sys; print(sys.version)\"']))"
0

C:\Python27
位 python27.exe.lnk -c "import subprocess; print(subprocess.call(['./python27.exe.lnk', '-c', '\"import sys; print(sys.version)\"']))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\Python27\lib\subprocess.py", line 168, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\Python27\lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "C:\Python27\lib\subprocess.py", line 640, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 is not a valid Win32 application

Although python27.exe.lnk is callable from any command line tool, with any of its arguments the subprocess module thinks it's not a valid Win32 application.

RonnyPfannschmidt commented 7 years ago

then as of now we cant merge this pr on the basis that its broken on python due to a python bug

gaborbernat commented 7 years ago

yeah, from point of view of tox adding a fix here would not be enough :+1: (would fail with the same handled message though as it does up to now); will follow up with the Python bug, so let's keep this open for now

nicoddemus commented 7 years ago

Thanks for the explanation and following up @gaborbernat!

gaborbernat commented 7 years ago

so this is a known limitation (see details at http://bugs.python.org/issue30979); in the meantime I'll fallback to open shortcuts either via shell mode for subprocess or os.startfile 馃憤 I'll update the PR once done and added test for it (although naturally it will only run on Windows)

gaborbernat commented 7 years ago

@nicoddemus @RonnyPfannschmidt

Actually, I think it's maybe better to do this explicitly at application level, meaning:

What do you guys think?

PS. If you guys think that allowing shortcuts all the sudden could break peoples code, maybe we can just add them via a flag, which by default is False.

RonnyPfannschmidt commented 7 years ago

I think We should add a flag, and ensure we do resolve links

gaborbernat commented 7 years ago

@RonnyPfannschmidt ensure to resolve it within the sysfind or by using another function? (by the way it seems on Windows there are two types of links - shortcuts and reparse points; while on Linux the hard and soft symlink), for a general solution all those four should be handled recursively.

RonnyPfannschmidt commented 7 years ago

posix transparently handles links, win32 is the mess, so please don't make a well working system worse to cater the needs of that windows shipwreck

gaborbernat commented 7 years ago

@RonnyPfannschmidt that may well be so, however I'm a bit worried about creating something that is not consistent across platforms

RonnyPfannschmidt commented 7 years ago

In stric

RonnyPfannschmidt commented 7 years ago

I'm strictly opposed to degraded and complicate the POSIX side over a windows platform issue,

Windows is the inconsistent platform so fix the wart for windows

nicoddemus commented 7 years ago

I'm with @RonnyPfannschmidt here. @gaborbernat what is the problem you are trying to fix exactly? I ask because windows shortcuts are not usually handled anyway by libraries in general AFAIK.

gaborbernat commented 7 years ago

So here's what I want to solve:

Run a tox test suite on Windows for all Python interpreters. So this includes Python 2.6, 2.7, 3.3, 3.4, 3.5, 3.6. And do all this without virtualisation so native-ly.

This involves first installing all this Pythons and then allowing to expose them as python2.7, python3.4 and so on. The part of installing it is the easiest achieved by using conda, and their environments. After installing conda with a single command line any additional Python can be installed. The way the installation happens is that all Pythons are installed in their own folder, and all of them have their python.exe.

Now I cannot add all these to path, cause that would be a nightmare to pick up the correct one, so I just created a shortcut to each of these pythons in another folder and add that to env. Afterwards I can do from the command line python2.7, python3.5, etc. (requires to extend the PATH_EXT with an EXE.LNK) but that's fine. The problem afterwards is that while py at the moment does find these shortcuts, running them fails because they are shortcuts.

So what do you guys think, do you see a better way, if someone wants to use Windows (for any random masochistic reason - things are so much easier on Arch Linux)?

The-Compiler commented 7 years ago

You can write a batch file instead which starts the real Python and passes arguments to it. I've done the same thing before, but unfortunately I can't check the syntax right now.

gaborbernat commented 7 years ago

I would strongly prefer not starting to write ugly batch files, and use tox for what it was built for - having a cross platform build configuration and runner. I want things to work out of box, as it does on Unix. I know Windows is a second class citizen here, and a mess but it's what most non-developers use.

gaborbernat commented 7 years ago

@RonnyPfannschmidt @nicoddemus do we have a consensus on this?

nicoddemus commented 7 years ago

So what do you guys think, do you see a better way, if someone wants to use Windows

I'm glad I asked for more details then. 馃榿

Windows does have soft and hard links, take a look at mklink:

位 mklink
Creates a symbolic link.

MKLINK [[/D] | [/H] | [/J]] Link Target

        /D      Creates a directory symbolic link.  Default is a file
                symbolic link.
        /H      Creates a hard link instead of a symbolic link.
        /J      Creates a Directory Junction.
        Link    Specifies the new symbolic link name.
        Target  Specifies the path (relative or absolute) that the new link
                refers to.

In fact conda uses this to create hard links instead of copying files when creating environments for speed and save disk space.

Perhaps we can extend tox to always resolve the full path of symlinks before executing, at least on Windows?

A quick test shows that pathlib correctly resolves a soft link:

>>> from pathlib import Path
>>> Path('e:\\python3.5.exe')
WindowsPath('e:/python3.5.exe')
>>> x=_
>>> x.resolve()
WindowsPath('E:/Miniconda3/python.exe')

I would strongly prefer not starting to write ugly batch files, and use tox for what it was built for - having a cross platform build configuration and runner. I want things to work out of box, as it does on Unix.

Well batch files are really simple to write, plus even your solution won't be "out of the box" given that you still have to create the Windows shortcuts anyway. It would be a matter of, instead of creating shortcuts, you create one line batch files like this:

@C:\Python27\python.exe %*

And name that python2.7.bat and put it somewhere on PATH. In fact seems easier IMO than creating shortcut files manually.

@RonnyPfannschmidt @nicoddemus do we have a consensus on this?

On trying to improve the situation? I think so. But we want to obtain the cleanest solution possible without polluting the code base if possible, I think that's @RonnyPfannschmidt's main concern.

gaborbernat commented 7 years ago

@nicoddemus fair enough with the place holder batch file. The pathlib stuff is Python 3.5+ specific so not good enough from tox point of view as it plans to support Python 2 too. The mklink is Vista + feature, but is also worth considering, although if WinAPI CreateProcess can execute that batch file I'm content enough to accept it, and withdraw this PR.

nicoddemus commented 7 years ago

The pathlib stuff is Python 3.5+ specific so not good enough from tox point of view as it plans to support Python 2 too.

It is not unfeasible IMO to depend on the pathlib backport for Python 2 if we want to support soft/hard links on Windows... it is a small dependency (a single .py file if I'm not mistaken).

The mklink is Vista + feature, but is also worth considering, although if WinAPI CreateProcess can execute that batch file I'm content enough to accept it, and withdraw this PR.

It seems subprocess supports it directly:

echo Hello world
Python 2.7.9 |Continuum Analytics, Inc.| (default, Dec 18 2014, 16:57:52) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
Anaconda is brought to you by Continuum Analytics.
Please check out: http://continuum.io/thanks and https://binstar.org
>>> import subprocess
>>> subprocess.call('hello.bat')
Hello there
0
gaborbernat commented 7 years ago

we should document this, but otherwise it's a fine solution

nicoddemus commented 7 years ago

@gaborbernat agreed. Could you come back here to share if ultimately the solution worked? We would also appreciate a PR for the docs documenting this. 馃槈

gaborbernat commented 7 years ago

@nicoddemus worked flawlessly, as far as documenting it it probably should go under tox documentation 馃憤 so I'll make the PR there

nicoddemus commented 7 years ago

Awesome, thanks! 馃憤