protontricks icon indicating copy to clipboard operation
protontricks copied to clipboard

Look for winetricks in /usr/local/bin/winetricks

Open aljelly opened this issue 7 years ago • 7 comments

If you use the manual installation script to install winetricks it gets installed to /usr/local/bin/winetricks, so this PR makes the script look in there too.

aljelly avatar Oct 21 '18 05:10 aljelly

Why do you even use hardcoded paths? Just look in $PATH for winetricks. Something like this:

def find_executable(executable, path=None):
    """Find if 'executable' can be run. Looks for it in 'path'
    (string that lists directories separated by 'os.pathsep';
    defaults to os.environ['PATH']). Checks for all executable
    extensions. Returns full path or None if no command is found.
    """
    if path is None:
        path = os.environ['PATH']
    paths = path.split(os.pathsep)
    extlist = ['']
    for ext in extlist:
        execname = executable + ext
        if os.path.isfile(execname):
            return execname
        else:
            for p in paths:
                f = os.path.join(p, execname)
                if os.path.isfile(f):
                    return f
    else:
        return None

Based on https://gist.github.com/techtonik/4368898

NoXPhasma avatar Nov 07 '18 08:11 NoXPhasma

I am wondering, why couldn't you just use $PATH instead of hardcoding it? I think paths shouldn't necessarily be hardcoded but rather picked by environment variables only if necessary.

Sirmentio avatar Nov 07 '18 17:11 Sirmentio

Why do you even use hardcoded paths? Just look in $PATH for winetricks. Something like this:

def find_executable(executable, path=None):
    """Find if 'executable' can be run. Looks for it in 'path'
    (string that lists directories separated by 'os.pathsep';
    defaults to os.environ['PATH']). Checks for all executable
    extensions. Returns full path or None if no command is found.
    """
    if path is None:
        path = os.environ['PATH']
    paths = path.split(os.pathsep)
    extlist = ['']
    for ext in extlist:
        execname = executable + ext
        if os.path.isfile(execname):
            return execname
        else:
            for p in paths:
                f = os.path.join(p, execname)
                if os.path.isfile(f):
                    return f
    else:
        return None

Based on https://gist.github.com/techtonik/4368898

Python 3.3+ already seems to have this as a built-in:

https://docs.python.org/3.7/library/shutil.html#shutil.which

or in other words

>>> import shutil
>>> shutil.which("winetricks")
'/bin/winetricks'

Seems like a no-brainer to me since we're on Python 3.

Matoking avatar Nov 07 '18 17:11 Matoking

Yes I know. But by using the code above, it would also work on Python < 3.3. However, if backwards compatibility is not wanted/needed, then I would go with shutil.which as well.

NoXPhasma avatar Nov 07 '18 17:11 NoXPhasma

Python 3.2 reached end-of-life on 2014-10-12 and I'm pretty sure most Proton users are running fairly modern software, so I don't think this will be an issue.

I've created a pull request for this under #34 which uses shutil.which.

Matoking avatar Nov 07 '18 18:11 Matoking

My stance with this is that I don't think the effort for supporting end of life python versions should particularly be needed, since they're end of life and all. Though that would probably give some problems to those who use really old LTS versions of certain Linux distros at some point. I suppose it's a balancing act of being in with supporting the latest technology but not keeping those who prefer stable distribution versions in the dust.

Sirmentio avatar Nov 12 '18 17:11 Sirmentio

Why do you even use hardcoded paths?

It was because it was a quick edit to the original behavior so that it could find where winetricks was on my system, which is where winetricks would likely be for other people too who also did a manual install of winetricks (that's why I made this PR). Yes, #34 is a preferable solution and would also work in, say ~/.local/bin if winetricks was there.

I'd imagine you could probably just fallback to what is already done + what this PR does if you wanted to support old Python? I'm not really familiar with Python, is that something you can do in it?

aljelly avatar Nov 13 '18 21:11 aljelly