(setq elpy-rpc-virtualenv-path "current") doesn't allow use of system-wide packages

[sten0]

Hi @galaunay,

Unfortunately the new changes blew up in my face.

Summary

22 unexpected results:
   FAILED  elpy-company-backend-should-add-shell-candidates
   FAILED  elpy-config--get-config-should-be-evaluated-in-the-rpc-virtualenv
   FAILED  elpy-doc-should-be-updated-automatically
   FAILED  elpy-doc-should-not-be-updated-if-deactivated
   FAILED  elpy-doc-should-not-be-updated-if-doc-not-visible
   FAILED  elpy-rpc--open-should-open-in-a-dedicated-virtualenv
   FAILED  elpy-rpc--open-should-set-local-variables
   FAILED  elpy-rpc-get-completions-should-return-completion
   FAILED  elpy-rpc-get-completions-should-return-completion-for-variable-with-numbers
   FAILED  elpy-rpc-get-virtualenv-path-should-return-current-venv-path
   FAILED  elpy-rpc-get-virtualenv-path-should-return-default-path
   FAILED  elpy-rpc-get-virtualenv-path-should-return-global-venv-path
   FAILED  elpy-rpc-get-virtualenv-should-NOT-update-the-virtualenv-when-it-is-not-the-default-venv
   FAILED  elpy-rpc-get-virtualenv-should-create-the-virtualenv-if-necessary
   FAILED  elpy-rpc-get-virtualenv-should-return-virtualenv
   FAILED  elpy-rpc-get-virtualenv-should-update-the-virtualenv-when-rpc-command-change
   FAILED  elpy-with-rpc-virtualenv-activated-should-temporarily-activate-the-rpc-virtualenv
   FAILED  elpy-xref--apropos-should-give-an-xref-buffer
   FAILED  elpy-xref--definitions-should-return-definitions
   FAILED  elpy-xref--get-completion-table-should-return-completion-table
   FAILED  elpy-xref--references-test
   FAILED  elpy-yapf-fix-code-in-region-should-retain-line-and-column

Steps to reproduce

I think using a1002f3, and the following configuration, running self-tests will reproduce it.

My configuration

(setq elpy-rpc-pythonpath nil
      python-shell-interpreter "/usr/bin/python3"
      python-shell-interpreter-args "-i"
      elpy-rpc-python-command "/usr/bin/python3"
      elpy-rpc-virtualenv-path "current")

As ever, this configuration is necessary (Debian Policy) for reproducible builds without network access, and also so that the user's out-of-the-box experience matches what was tested, even if they've run random scripts of the internet to add local venvs to PATH.

OS

Debian sid/unstable clean chroot.

elpy_1.31.0+27.ga1002f3-1_amd64-2019-09-26T22:57:15Z.build.zip

[galaunay]

elpy-rpc-virtualenv-path works with symbols, not strings. So (setq elpy-rpc-virtualenv-path 'current) should do the trick. (I may need to add safeguards here to avoid the confusion in the future).

Some tests were still failling when using the current environment by default, I made f6de769 to address this.

By the way, Elpy is now asking before installing anything from PyPI, would it be enough to go back to the default elpy-rpc-virtualenv-path in the Debian package ?

[sten0]

elpy-rpc-virtualenv-path works with symbols, not strings. So (setq elpy-rpc-virtualenv-path 'current) should do the trick. (I may need to add safeguards here to avoid the confusion in the future).

I confess I used your suggestion verbatim, without consulting the source, and with the assumption elpy-rpc-virtualenv-path used strings, because path-type vars usually take strings. BTW, doesn't using symbols for this value fail to defend against the (imho bad style) use-case of a venv that has a space in its path?

Some tests were still failling when using the current environment by default, I made f6de769 to address this.

Thanks :-) I'll test HEAD again sometime next week. As another data-point, I tried elpy-rpc-virtualenv-path 'disabled with a1002f3 with the following failures:

20 unexpected results:
   FAILED  elpy-company-backend-should-add-shell-candidates
   FAILED  elpy-config--get-config-should-be-evaluated-in-the-rpc-virtualenv
   FAILED  elpy-doc-should-be-updated-automatically
   FAILED  elpy-doc-should-not-be-updated-if-deactivated
   FAILED  elpy-doc-should-not-be-updated-if-doc-not-visible
   FAILED  elpy-rpc--open-should-open-in-a-dedicated-virtualenv
   FAILED  elpy-rpc-get-completions-should-return-completion
   FAILED  elpy-rpc-get-completions-should-return-completion-for-variable-with-numbers
   FAILED  elpy-rpc-get-virtualenv-path-should-return-current-venv-path
   FAILED  elpy-rpc-get-virtualenv-path-should-return-default-path
   FAILED  elpy-rpc-get-virtualenv-path-should-return-global-venv-path
   FAILED  elpy-rpc-get-virtualenv-should-NOT-update-the-virtualenv-when-it-is-not-the-default-venv
   FAILED  elpy-rpc-get-virtualenv-should-create-the-virtualenv-if-necessary
   FAILED  elpy-rpc-get-virtualenv-should-return-virtualenv
   FAILED  elpy-rpc-get-virtualenv-should-update-the-virtualenv-when-rpc-command-change
   FAILED  elpy-with-rpc-virtualenv-activated-should-temporarily-activate-the-rpc-virtualenv
   FAILED  elpy-xref--apropos-should-give-an-xref-buffer
   FAILED  elpy-xref--definitions-should-return-definitions
   FAILED  elpy-xref--get-completion-table-should-return-completion-table
   FAILED  elpy-xref--references-test

By the way, Elpy is now asking before installing anything from PyPI, would it be enough to go back to the default elpy-rpc-virtualenv-path in the Debian package ?

Reading the docstring of elpy-rpc-virtualenv-path, it reads like maybe 'global is now the way to get the old behaviour. It's a bit of a semantics thing, but it seems like elpy-rpc-virtualenv-path 'disabled would be ideal, because if it's possible to defend against user [mis]configurations of venvs caused by the (terrifyingly) increasingly popular "curl http://hack.me/script.sh | bash" then we should do that. eg: A config key that skips any existing or activated venvs would be ideal, because I want to ship a package that guaranties a good out-of-the-box experience, and encourages new devs to use Emacs and not PyCharm, Spyder, or KDevelop--P.S. and I'm a KDE user and advocate!

And of course I've documented how to reenable your default config, and it's likely that more advanced users will switch to using the MELPA package directly ;-)

BTW, I appreciate your work and agree that using a venv for the elpy-rpc is the correct upstream approach! Thanks again for continuing to accommodate Debian packaging. I truly believe it's a symbiotic relationship that is good for Elpy's popularity and success.

Cheers, Nicholas

[galaunay]

BTW, doesn't using symbols for this value fail to defend against the (imho bad style) use-case of a venv that has a space in its path?

My idea was to use strings to set specific paths (so you can use (setq elpy-rpc-virtualenv-path "/path/to/venv")), or symbols for other options (like (setq elpy-rpc-virtualenv-path 'current)).

A config key that skips any existing or activated venvs would be ideal.

I guess it makes perfect sense for the Debian package, as you install the python dependencies from Debian repositories (if I got it right ?). So that is indeed (setq elpy-rpc-virtualenv-path 'global) you want: it will use the global python binaries, whatever virtualenv may be activated.

Thanks as well for the work you are putting in this, I understand the necessity of making Elpy more accessible to user and I appreciate that we can work together on this.

[sten0]

BTW, doesn't using symbols for this value fail to defend against the (imho bad style) use-case of a venv that has a space in its path?

My idea was to use strings to set specific paths (so you can use (setq elpy-rpc-virtualenv-path "/path/to/venv")), or symbols for other options (like (setq elpy-rpc-virtualenv-path 'current)).

Oh, now I see :-)

A config key that skips any existing or activated venvs would be ideal.

I guess it makes perfect sense for the Debian package, as you install the python dependencies from Debian repositories (if I got it right ?).

Yup. Essential deps are auto-installed from Debian packages (and repositories). The instructions in README.Debian include a snippet on how to show optional deps (in case the user doesn't know how to access this, or if they're using one of the new and misfeatured GUI package managers that don't support this). Finally, one can install everything and the kitchen sink with sudo apt-get install --install-suggests, but this isn't generally recommended, nor what people use (I think it's unlikely a user will use both ipython and jupyter-notebook).

So that is indeed (setq elpy-rpc-virtualenv-path 'global) you want: it will use the global python binaries, whatever virtualenv may be activated.

I'm unclear about the distinction between current and global. What I'm requesting is a method to specifically ignore any activated venvs, because I'm worried that some beginner users will have activated one or more venvs using copy and pasted instructions off the internet (or one of those insane curl | bash installation instructions) for an unrelated piece of software, and I want to defend against possible malinteraction between these and Elpy in the default configuration of Debian's Elpy package.

Thanks as well for the work you are putting in this, I understand the necessity of making Elpy more accessible to user and I appreciate that we can work together on this.

:-D

[galaunay]

I'm unclear about the distinction between current and global.

'current is using the currently active virtualenv (by checking which python executable comes first in PATH) and 'global is using the system python binaries (by checking which python executable comes last in PATH).

Do you find the names confusing ? I guess we could use 'system instead of 'global.

What I'm requesting is a method to specifically ignore any activated venvs,

(setq elpy-rpc-virtualenv-path 'global) should do just that: ignoring every virtualenvs and using the system binaries.

[sten0]

Thank you for taking the time to explain this!

I'm unclear about the distinction between current and global.

'current is using the currently active virtualenv (by checking which python executable comes first in PATH) and 'global is using the system python binaries (by checking which python executable comes last in PATH).

Current makes sense :-)

Do you find the names confusing ? I guess we could use 'system instead of 'global.

Yes please! It's also more congruent with virtualenv's documentation, which uses --system-site-packages and not --global-packages. Also, pip's use of the global keys are as follows:

       -b,--build <dir>
              Directory  to unpack packages into and build in.  The default in
              a virtualenv is "<venv path>/build".  The default for global in‐
              stalls is "<OS temp dir>/pip_build_<username>".
…
       --src <dir>
              Directory to check out editable projects into.  The default in a
              virtualenv  is  "<venv  path>/src".   The default for global in‐
              stalls is "<current dir>/src".
…
       --global-option <options>
              Extra  global options to be supplied to the setup.py call before
              the install command.

Effectively, system is an unambiguous term where global seems to (ironically) always mean something different depending on context.

What I'm requesting is a method to specifically ignore any activated venvs,

(setq elpy-rpc-virtualenv-path 'global) should do just that: ignoring every virtualenvs and using the system binaries.

Unfortunately it doesn't seem to be working correctly yet :-( Here are the test results for 90d7a5a with (setq elpy-rpc-virtualenv-path 'global):

22 unexpected results:
   FAILED  elpy--sort-and-strip-duplicates-should-sort-and-strip-duplicates
   FAILED  elpy-company-backend-should-add-shell-candidates
   FAILED  elpy-config--get-config-should-be-evaluated-in-the-rpc-virtualenv
   FAILED  elpy-doc-should-be-updated-automatically
   FAILED  elpy-doc-should-not-be-updated-if-deactivated
   FAILED  elpy-doc-should-not-be-updated-if-doc-not-visible
   FAILED  elpy-module-django-get-test-runner-cache-size-limit
   FAILED  elpy-rpc--default-error-callback-code-500-should-pop-up-window
   FAILED  elpy-rpc--open-should-include-virtualenv-name
   FAILED  elpy-rpc--open-should-open-in-a-dedicated-virtualenv
   FAILED  elpy-rpc-get-completions-should-return-completion
   FAILED  elpy-rpc-get-completions-should-return-completion-for-variable-with-numbers
   FAILED  elpy-rpc-get-virtualenv-path-should-return-current-venv-path
   FAILED  elpy-rpc-get-virtualenv-should-NOT-update-the-virtualenv-when-it-is-not-the-default-venv
   FAILED  elpy-rpc-get-virtualenv-should-create-the-virtualenv-if-necessary
   FAILED  elpy-rpc-get-virtualenv-should-return-virtualenv
   FAILED  elpy-rpc-get-virtualenv-should-update-the-virtualenv-when-rpc-command-change
   FAILED  elpy-with-rpc-virtualenv-activated-should-temporarily-activate-the-rpc-virtualenv
   FAILED  elpy-xref--apropos-should-give-an-xref-buffer
   FAILED  elpy-xref--definitions-should-return-definitions
   FAILED  elpy-xref--get-completion-table-should-return-completion-table
   FAILED  elpy-xref--references-test

elpy_1.31.0+38.g90d7a5a-1_amd64-2019-10-02T00:46:52Z.build.zip

[galaunay]

Unfortunately it doesn't seem to be working correctly yet :-( Here are the test results for 90d7a5a with (setq elpy-rpc-virtualenv-path 'global):

So it works with 'default but not with 'global ?

Elpy error popup ignored, see ‘elpy-rpc-error-timeout’: /sbuild-nonexistent/.virtualenvs/elpy-test-venv/bin/python seems to be missing.

This seems to indicate that the elpy-test-venv virtualenv has not been created as it is supposed to be done in the setup script. Are you doing it before running the tests ?

[sten0]

Hi @galaunay,

Sorry this reply is a bit long. I cut and edited it as well as I could, and took tried to cut anything that has been discussed before.

galaunay notifications@github.com writes:

Unfortunately it doesn't seem to be working correctly yet :-( Here are the test results for 90d7a5a with (setq elpy-rpc-virtualenv-path 'global):

So it works with 'default but not with 'global ?

'global results in 22 failures, 'current in 55, and 'default also has 55 failures.

Elpy error popup ignored, see ‘elpy-rpc-error-timeout’: /sbuild-nonexistent/.virtualenvs/elpy-test-venv/bin/python seems to be missing.

This seems to indicate that the elpy-test-venv virtualenv has not been created as it is supposed to be done in the setup script. Are you doing it before running the tests ?

Unfortunately, this isn't an option available to me at this time, and I have to use modules from /usr/lib/python3/dist-packages (no venvs), and additionally testing Elpy as-installed, with its Python parts in dist-packages and its Lisp parts in /usr/share/emacs/site-lisp.

A categorical requirement to use a venv for Elpy bypasses most of the system-level QA (particularly upgrades) that goes into Debian, introduces problems with reproducible builds...effectively I'd need to start more or less from scratch and write (and maintain) a custom testing framework that works without pip, pypi, or network access, burden the user with tracking elpy-venv state and upgrades, and I probably won't be able to find time for such a large project until June 2020...and the Debian Elpy package would have to stay at 1.31 until then :-(

Please consider restoring the run-using_dist-packages ability, which was present in Elpy ≤ 1.31 🙏

We have about 100 confirmed Elpy users in Debian now, which means there are at least 400 among all derivatives. Also, since Ubuntu LTS releases (and derivatives) are so far behind Debian, with poor access to backports, I believe that those users are switching to MELPA-provided packages. Assuming bugs exist, evenly distributed across all operating systems, an increasing proportion of issues coming from Ubuntu users using an up-to-date version of Elpy with venvs activated will confirm this hypothesis. I consider this a success, because it means my package has succeeded at inspiring users to go further, and because it's good for the Elpy project. eg: the work is worthwhile even if 75% of users on my targeted platforms switch to MELPA and stop using my package.

Thinking about the semantics some more, it seems like something like the following is now needed to restore the ability to exclusively use system-packages as-installed to dist-packages:

(setq elpy-rpc-virtualenv-path 'disabled)

And yes, I've prominently documented such Debian-specific customisations so that intermediate users can easily and persistently reenable Elpy's full power :-D

Sincerely, Nicholas

[galaunay]

effectively I'd need to start more or less from scratch and write (and maintain) a custom testing framework

Doesn't seem like a very reasonable solution indeed.

Please consider restoring the run-using_dist-packages ability, which was present in Elpy ≤ 1.31

I expect 'global to do just that. The problem may just be the additional tests I added to cover for the different options of elpy-rpc-virtualenv-path (which obviously fail if virtualenvs are prohibited). I could update the tests to behave differently on Debian. Is there any environment variable I could use to write test code specifically for Debian ?

Ps: some of the errors are very weird, and doesn't even involve the RPC at all (the first one elpy--sort-and-strip-duplicates-should-sort-and-strip-duplicates for example). The benchmark script may still be causing trouble as well, but we will see after fixing the obvious failing tests already.

[sten0]

effectively I'd need to start more or less from scratch and write (and maintain) a custom testing framework

Doesn't seem like a very reasonable solution indeed.

Whew, that's a relief ^^

Please consider restoring the run-using_dist-packages ability, which was present in Elpy ≤ 1.31

I expect 'global to do just that. The problem may just be the additional tests I added to cover for the different options of elpy-rpc-virtualenv-path (which obviously fail if virtualenvs are prohibited).

I think you're right about this ;-) It seems like the tests should take a branch that doesn't use venvs whenelpy-rpc-virtualenv-path has been set to 'global/'system/'disabled.

I could update the tests to behave differently on Debian. Is there any environment variable I could use to write test code specifically for Debian ?

Yes, there are two or three environmental variables that could be used to detect when tests are being run on 2/3 of the automated infrastructure, but these variables aren't a stable interface. Also, this approach won't solve the case of a user/dev building the package on a real system (vs a container or chroot that that activates the buildd or autopkgtest profiles).

Here's a link someone following this thread might appreciate: "By default, a virtual environment is entirely isolated from the system-level site-packages directories" PEP-405.

It sounds like 'global` is intended to address this case by cutting activated venvs from the PYTHONPATH and sys.path.

Ps: some of the errors are very weird, and doesn't even involve the RPC at all (the first one elpy--sort-and-strip-duplicates-should-sort-and-strip-duplicates for example). The benchmark script may still be causing trouble as well, but we will see after fixing the obvious failing tests already.

Here's a build log of 4a4f421 with the benchmark script disabled: elpy_1.31.0+40.g4a4f421-1_amd64-2019-10-04T04:36:03Z.build.zip

Sorry I don't have enough free time to provide a patch these days!

[galaunay]

I think you're right about this ;-) It seems like the tests should take a branch that doesn't use venvs whenelpy-rpc-virtualenv-path has been set to 'global/'system/'disabled.

I guess I could do that, running different tests depending if elpy-rpc-virtualenv-path has been changed from its default value or not. I will have a go at it.

It sounds like 'global` is intended to address this case by cutting activated venvs from the PYTHONPATH and sys.path.

It should indeed :). But there is still something I am not very happy with: It is difficult to find the systemwide python binaries when Emacs has been started from a virtualenv. At the moment, I am taking the last python binaries found in PATH, but it is hardly a proper way of getting the systemwide binaries... Would you have any idea on how to properly do that ?

[galaunay]

I think you're right about this ;-) It seems like the tests should take a branch that doesn't use venvs whenelpy-rpc-virtualenv-path has been set to 'global/'system/'disabled.

I guess I could do that, running different tests depending if elpy-rpc-virtualenv-path has been changed from its default value or not. I will have a go at it.

I made some modifications (PR #1689) to allow skipping tests using virtualenvs. You just have to set the environment variable ELPY_TEST_DONT_USE_VIRTUALENV (a bit long, but at least explicit name). elpy-rpc-virtualenv-path will automatically be set to system.

It will need PR #1688 to be merged first in order to work though, but I am working on it.

[sten0]

Hi @galaunay, Sorry for the long delay, I had a busy Octobre. With

(setq elpy-rpc-pythonpath nil
      python-shell-interpreter "/usr/bin/python3"
      python-shell-interpreter-args "-i"
      elpy-rpc-python-command "/usr/bin/python3"
      elpy-rpc-virtualenv-path 'global)

and ELPY_TEST_DONT_USE_VIRTUALENV set two tests fail on 864ad20; they are elpy-rpc-get-virtualenv-should-return-virtualenv and elpy-with-rpc-virtualenv-should-activate-venv-when-using-different-venv. The envvar is correctly detected with the message ELPY_TEST_DONT_USE_VIRTUALENV is set, skipping tests using virtualenvs. I'm guessing the issue is something simple like these tests aren't yet checking for if that envvar is set.

Full build log is attached. elpy_1.31.0+62.g864ad20-1_amd64-2019-11-05T00:27:26Z.build.zip

[sten0]

I think you're right about this ;-) It seems like the tests should take a branch that doesn't use venvs whenelpy-rpc-virtualenv-path has been set to 'global/'system/'disabled.

I guess I could do that, running different tests depending if elpy-rpc-virtualenv-path has been changed from its default value or not. I will have a go at it.

It sounds like 'global` is intended to address this case by cutting activated venvs from the PYTHONPATH and sys.path.

It should indeed :). But there is still something I am not very happy with: It is difficult to find the systemwide python binaries when Emacs has been started from a virtualenv. At the moment, I am taking the last python binaries found in PATH, but it is hardly a proper way of getting the systemwide binaries... Would you have any idea on how to properly do that ?

The best I could find it from the Python manpage:

     -E     Ignore environment variables like PYTHONPATH and PYTHONHOME that
              modify the behavior of the interpreter.`

So maybe the best approach is to add "-E" to python-shell-interpreter-args and possibly also elpy-rpc-python-command? This might not work if the user is not using an absolute path, but I already am, so we're half-way there ;-)

[galaunay]

I'm guessing the issue is something simple like these tests aren't yet checking for if that envvar is set.

That was it, just some tests I forgot. cea996c should have fixed that.

This might not work if the user is not using an absolute path.

That's gonna be a problem :). I will ask around, see if I can find an elegant solution to this.

[sten0]

Hi @galaunay,

Any news on if there's a better way to provide an out-of-the-box configuration that uses ignores venvs that is better than setting an absolute path for the interpreter, plus using "-E" as an argument?

[galaunay]

Unfortunately no success so far... It is still on my todo list, but as it will just be a problem in some very specific cases, I don't consider it a priority at the moment.

I will get back to it as soon as the list shrinks a bit :).