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

bug(python): python REPL detection is broken #344

Closed
pysan3 opened this issue Jun 19, 2023 · 5 comments · Fixed by #345
Closed

bug(python): python REPL detection is broken #344

pysan3 opened this issue Jun 19, 2023 · 5 comments · Fixed by #345

Comments

@pysan3
Copy link
Contributor

pysan3 commented Jun 19, 2023

Hi, thanks for the nice plugin!

I have noticed an issue when detecting the python REPL.
The current method mainly uses vim.fn.executable(command[1]) to detect whether the REPL is available and defines which REPL to use defined in table of each iron/fts/<filetype>.lua file.

The detecting mechanism is implemented in the following two places (which are doing basically the same thing) and is called mainly in iron.lowlevel.get_repl_def.

  • Default value in config.repl_definition
    repl_definition = setmetatable({}, {
    __index = function(tbl, key)
    local repl_definitions = require("iron.fts")[key]
    local repl_def
    for _, v in pairs(repl_definitions) do
    if vim.fn.executable(v.command[1]) == 1 then
    repl_def = v
    break
    end
    end
    if repl_def == nil then
    error("Failed to locate REPL executable, aborting")
    else
    rawset(tbl, key, repl_def)
    return repl_def
    end
    end
    }),
  • providers.first_matching_binary
    providers.first_matching_binary = function(ft)
    local repl_definitions = fts[ft]
    if not repl_definitions then
    error("No repl definition configured for " .. ft)
    end
    local repl_def
    for _, v in pairs(repl_definitions) do
    if vim.fn.executable(v.command[1]) == 1 then
    repl_def = v
    break
    end
    end
    if not repl_def then
    error("Couldn't find a binary available for " .. ft)
    end
    return repl_def
    end

However, the commands for all python REPLs defined in iron/fts/python.lua take the form of python -m <module-name> which makes the result of vim.fn.executable(command[1]) => vim.fn.executable("python") always true.

@Palpatineli I noticed you recently submitted a PR #337 which is the direct cause of this issue. Are you aware of this? Did you test your code against environment where there's only python raw REPL and no ipython or other rich REPLs? (CC @hkupty as you merged the PR)

It is sad to say, but I'm quite confused by the aim of that PR in the first place since ipython and other commands should be found in $PATH if the setup of python environment is done correctly (with poetry shell, source bin/activate, conda activate <env-name> etc) (please correct me if I'm wrong tho).

Thanks,
pysan3

@hkupty
Copy link
Collaborator

hkupty commented Jun 19, 2023

Hi @pysan3, I'm happy that you like the plugin and at the same time sad to hear that there's a bug disrupting your workflow.

I might have said that in other issues so sorry if I'm repeating myself too much, but it is becoming increasingly hard for me to keep up with iron as unfortunately I myself stopped using it and am lacking time to properly manage it, so I reckon it is my fault for overseeing this.

I'll partially revert that PR as it does some good by dealing with the virtualenv issue. At the same time, the defaults should be a bit more sane.

It has been ages since I did python professionally (which entails projects with proper development env setup, with virtualenv and requirements.txt), so forgive me if I'm missing something, but from my understanding, inside a VIRTUAL_ENV set up, ipython would be reachable as a module, not as the binary in PATH, as PATH might be unaltered, right?

I guess this means that the list of available binaries modules should change according to VIRTUAL_ENV, so when it is set we get the values currently set and when it is not set we should get the values prior to this PR.. How does that sound?

@pysan3
Copy link
Contributor Author

pysan3 commented Jun 19, 2023

Thanks for your quick response! I understand your pain and I want to thank you again for your hard work!

I'm not quite sure if this was added recently or was a feature from long ago but here's the content of mytest/bin/activate after running virtualenv -p python3 mytest.
If you look at line 54 below, or where I added a comment, it seems to prepend $VIRTUAL_ENV/bin to PATH if you source the script.

# This file must be used with "source bin/activate" *from bash*
# you cannot run it directly


if [ "${BASH_SOURCE-}" = "$0" ]; then
    echo "You must source this script: \$ source $0" >&2
    exit 33
fi

deactivate () {
    unset -f pydoc >/dev/null 2>&1 || true

    # reset old environment variables
    # ! [ -z ${VAR+_} ] returns true if VAR is declared at all
    if ! [ -z "${_OLD_VIRTUAL_PATH:+_}" ] ; then
        PATH="$_OLD_VIRTUAL_PATH"
        export PATH
        unset _OLD_VIRTUAL_PATH
    fi
    if ! [ -z "${_OLD_VIRTUAL_PYTHONHOME+_}" ] ; then
        PYTHONHOME="$_OLD_VIRTUAL_PYTHONHOME"
        export PYTHONHOME
        unset _OLD_VIRTUAL_PYTHONHOME
    fi

    # The hash command must be called to get it to forget past
    # commands. Without forgetting past commands the $PATH changes
    # we made may not be respected
    hash -r 2>/dev/null

    if ! [ -z "${_OLD_VIRTUAL_PS1+_}" ] ; then
        PS1="$_OLD_VIRTUAL_PS1"
        export PS1
        unset _OLD_VIRTUAL_PS1
    fi

    unset VIRTUAL_ENV
    if [ ! "${1-}" = "nondestructive" ] ; then
    # Self destruct!
        unset -f deactivate
    fi
}

# unset irrelevant variables
deactivate nondestructive

VIRTUAL_ENV='/home/user/tmp/mytest'
if ([ "$OSTYPE" = "cygwin" ] || [ "$OSTYPE" = "msys" ]) && $(command -v cygpath &> /dev/null) ; then
    VIRTUAL_ENV=$(cygpath -u "$VIRTUAL_ENV")
fi
export VIRTUAL_ENV

_OLD_VIRTUAL_PATH="$PATH"
PATH="$VIRTUAL_ENV/bin:$PATH"  # =============== HERE, PATH IS MODIFIED !!! ================
export PATH

# unset PYTHONHOME if set
if ! [ -z "${PYTHONHOME+_}" ] ; then
    _OLD_VIRTUAL_PYTHONHOME="$PYTHONHOME"
    unset PYTHONHOME
fi

if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT-}" ] ; then
    _OLD_VIRTUAL_PS1="${PS1-}"
    if [ "x" != x ] ; then
        PS1="() ${PS1-}"
    else
        PS1="(`basename \"$VIRTUAL_ENV\"`) ${PS1-}"
    fi
    export PS1
fi

# Make sure to unalias pydoc if it's already there
alias pydoc 2>/dev/null >/dev/null && unalias pydoc || true

pydoc () {
    python -m pydoc "$@"
}

# The hash command must be called to get it to forget past
# commands. Without forgetting past commands the $PATH changes
# we made may not be respected
hash -r 2>/dev/null

After running cd mytest; source ./bin/activate; pip install ipython, here's the content of ./bin where you can find ipython as an executable.

$ source ./bin/activate
(mytest) $ pip install ipython
Requirement already satisfied: ipython in ./lib/python3.10/site-packages (8.14.0)
Requirement already satisfied: backcall in ./lib/python3.10/site-packages (from ipython) (0.2.0)
... -- skipping output
(mytest) $ tree bin
bin
├── activate
├── activate.csh
├── activate.fish
├── activate.nu
├── activate.ps1
├── activate_this.py
├── ipython
├── ipython3
├── pip
├── pip3
├── pip-3.10
├── pip3.10
├── pygmentize
├── python -> /home/user/.local/share/pyenv/versions/3.10.8/bin/python
├── python3 -> python
├── python3.10 -> python
├── wheel
├── wheel3
├── wheel-3.10
└── wheel3.10

0 directories, 20 files
(mytest) $ cat bin/ipython
#!/home/user/tmp/mytest/bin/python
# -*- coding: utf-8 -*-
import re
import sys
from IPython import start_ipython
if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(start_ipython())

I haven't got a setup to test on Windows but I'm pretty sure that you'll have access to ipython as an executable as well.

Note that I don't use virtualenv regularly (but poetry instead). I would love to hear from @Palpatineli as well.

Hope this helps ;)

@Palpatineli
Copy link
Contributor

Hi @pysan3 sorry my PR is giving you trouble. For the first part, no it did not occur to me that a change to the start up repl script would interfere with the selection script due to my ignorance of how the selection script works, so I only tested my PR against configurations where the repl is hardcoded.
For the motivation of the PR, it was because in some setups, the ipython module exists for the virtualenv but the executable script ipython may not be installed in the venv/bin/ folder. This is usually caused by multiple python installations in the system (with conda or other tools).
To think about it, the description in the PR was indeed not accurate. It was less about selecting global vs virtualenv local ipython, and more about selecting installed ipython from different global installations.

@pysan3
Copy link
Contributor Author

pysan3 commented Jun 21, 2023

Thanks for your response @Palpatineli.

no it did not occur to me that a change to the start up repl script would interfere with the selection script due to my ignorance of how the selection script works

As this is clearly a bug, please submit a PR to either revert your change or fix the current issue.

in some setups, the ipython module exists for the virtualenv but the executable script ipython may not be installed in the venv/bin/ folder

Just out of curiosity, could you give me a specific example / commands of this situation?

When pip installing globally, when using pyenv, you should find ipython in ~/.local/share/pyenv/versions/3.11.1/bin (where 3.11.1 is pyenv global's version). And with conda, it's in ~/miniconda3/bin or somewhere similar.
In any case, you are supposed to add these paths in $PATH, so nothing comes to my mind where ipython is not installed as an executable.

@pysan3
Copy link
Contributor Author

pysan3 commented Jun 25, 2023

As this is clearly a bug, please submit a PR to either revert your change or fix the current issue.

Any updates? @Palpatineli

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 a pull request may close this issue.

3 participants