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

Initial support for virtualenv #3216

Merged
merged 2 commits into from Nov 13, 2018
Merged

Initial support for virtualenv #3216

merged 2 commits into from Nov 13, 2018

Conversation

ptomulik
Copy link
Contributor

@ptomulik ptomulik commented Oct 12, 2018

Currently, if one runs SCons in a virtualenv, SCons tools will invoke only system executables, instead of execs provided by virtualenv. If, for example, some tool invokes python interpreter, SCons will use the /usr/bin/python, not the one from virtualen's directory. Despite SCons is running under virtualenv's python interpreter, the tool switches to /usr/bin/python.

With this patch SCons automatically picks'up execs from the current virtualenv. This is achieved by prepending to env['ENV']['PATH'] sub-directories of virtualenv's home found in os.environ['PATH']. This is done during platform initialization (SCons.Platform.Platform), so all Environments, including the DefaultEnvironment see this change.

This patch, I hope, shall also simplify usage of pipenv in SCons based projects, SCons modules and tools could be distributed via pip and installed automatically with the use of Pipfile. Pipenv resolves most of the current problems with tool inter-dependencies and tool development requirements. At the moment I was successful in converting two or three of my tools to the form of pipenv-ready pip modules, but without this patch, end-to-end tests (under virtualenv) lead to inconsistent use of out-of-virtualenv executables.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

@ptomulik ptomulik force-pushed the virtualenv branch 5 times, most recently from 94605bf to 1e99958 Compare October 12, 2018 17:51
@ptomulik ptomulik changed the title Initial support for virtualenv WIP Initial support for virtualenv Oct 12, 2018
@bdbaddog
Copy link
Contributor

Looks like there's no way to not use the virtualenv's tools if you are in a virtualenv?
No IGNORE_VENV, or similar?

Also, since this would grossly change the default path behavior, perhaps best to opt in first, then swap the defaults?

Some tests failing under py3+ in travis. Please take a look.

@ptomulik ptomulik force-pushed the virtualenv branch 4 times, most recently from e8e23e1 to 57714c0 Compare October 12, 2018 21:49
@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 12, 2018

I've added --ignore-virtualenv option and handled OS environment variable SCONS_IGNORE_VIRTUALENV ("yes", "y", "true" or non-zero integer enables the "ignorance").

My feeling is, that escaping from virtualenv is strange and unexpected. Usually considered this behavior as a bug. If someone installs, configures and activates virtualenv to run SCons within it, then, I believe, he is doing this for purpose. So speaking, I'd like to make scons to stay within virtualenv by default.

By the way, SCons itself is quite inconsistent in searching external executables. For example env.WhereIs("foo") uses env['ENV']['PATH'] (the hardcoded one), while the global WhereIs("foo") uses os.environ['PATH']. In spite of this, it's not quite possible to ignore virtualenv by the global WhereIs.

@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 12, 2018

Travis seems to run tests under virtualenv. Regarding some of the failing tests, e.g. test/long-lines/signature.py, I guess before this patch, these tests were simply escaping from python 3.x (virtualenv) to python 2 (OS). Note that this test invokes an executable python script from SConstruct, the script has sheabang #!/usr/bin/env python. So, you were unintentionally testing some code against python 2 instead of python 3 and regressions were masked.

In Debian and Ubuntu, the command python (/usr/bin/python) defaults to python 2, but under virtualenv python defaults to the version chosen at the configuration time (python 3 in this case). Without virtualenv, or without the virtualenv support in scons the mentioned test simply escapes to python 2.

@bdbaddog
Copy link
Contributor

Regardless of your belief in the correctness of this patch (as compared to current behavior), changes which would change the current behavior of SCons should be phased in and not dropped into the next release.

The default behavior of SCons for all tools is to pick them up from the default system paths, and if users want to use other paths they need to explicitly request them.

It's probably better to bring this discussion the users mailing list and get other feedback.

@ptomulik
Copy link
Contributor Author

Posted to user mailing list.

@mwichmann
Copy link
Collaborator

Regardless of your belief in the correctness of this patch (as compared to current behavior), changes which would change the current behavior of SCons should be phased in and not dropped into the next release.

The default behavior of SCons for all tools is to pick them up from the default system paths, and if users want to use other paths they need to explicitly request them.

That position makes zero sense for Python. Python is so fundamental to running scons - given that it's the interpreter for the entire codebase - that you have to keep running the Python you started with or you risk inconsistencies. If you want to say "you get the default gcc unless you take other steps", fine, but Python can't be considered an external tool.

@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 13, 2018

It's how I initially thought about this. Actually, virtualenv (usually) contains only python and pip-installed packages, which are in most cases installed specifically to fulfill dev-requirements of the project being compiled with scons.

@bdbaddog
Copy link
Contributor

I've seen usage where the user had a virtualenv for SCons install only, and had used system python or other python location for the rest.
Likewise I've seen usage as you suggest.

To be clear, I'm not saying that your suggested usage is wrong, but that a) users are used to the way SCons functions with relation to PATH, b) it's not the only way people may want to pick up python and/or installed pip packages, c) if we migrate to a more standard install and require other pypi packages, we could end up with versions that users don't want to use in such virtualenv.

We don't want to force the users PATH'ing in a way which is different than previous.
At least not without some warning which is usually done through deprecation cycle as documented in the wiki.

This is a heavy handed assumption.

@mwichmann
Copy link
Collaborator

Hmm, odd, but okay. In that case maybe there should be a defined python tool that a user can redefine if needs be? "Note: this is the Python used to launch external scripts, which can be different than the Python used to run SCons itself"

@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 13, 2018

I've seen usage where the user had a virtualenv for SCons install only, and had used system python or other python location for the rest.

Things, like the above, are achievable in three ways:

  1. python /path/to/virtualenv/bin/scons - runs SCons under OS python, without activating virtualenv, the SCons PATH is not altered, so it prefers OS executables over virtualenv's ones.
  2. /path/to/virtualenv/bin/python /path/to/virtualenv/bin/scons - runs SCons under virtualenv's python, without activating virtualenv; the SCons PATH is still unaltered, so it prefers OS executables over virtualenv ones.
  3. . /path/to/virtualenv/bin/activate && scons --ignore-virtualenv && deactivate - runs SCons under virtualenv's python, in activated virtualenv, but --ignore-virtualenv makes the virtualenv to be ignored by scons (so still... OS executables over the virtualenv ones).

of course default behavior changes for the case 3. without --ignore-virtualenv, which may be regarded as broken backward compatibility.

Likewise I've seen usage as you suggest.

To be clear, I'm not saying that your suggested usage is wrong, but that a) users are used to the way SCons functions with relation to PATH, b) it's not the only way people may want to pick up python and/or installed pip packages, c) if we migrate to a more standard install and require other pypi packages, we could end up with versions that users don't want to use in such virtualenv.

Well, this is interesting. I thought for a while about issues similar to b), and concluded that actually users will not be left without choice. Regarding c) - a standard solution is to specify dev-requirements (requirements-dev.txt or Pipfile) in the user's project including the required packages (and version constraints) along with scons. In worst case, these requirements may contain conflicts, which is immediately indicated by pip. If an out-of-virtualenv packages are really need to be used, while some of them are installed in virtualenv, we still have three abovementioned techniques, plus the one of specifying this somehow in the SConscript code.

We don't want to force the users PATH'ing in a way which is different than previous.
At least not without some warning which is usually done through deprecation cycle as documented in the wiki.

This is a heavy handed assumption.

I'm not gonna to blindly defend this work, so ok, let's make the virtualenv "support" an optional feature, enabled on demand. It's not so hard to be done.

@ptomulik ptomulik force-pushed the virtualenv branch 3 times, most recently from c1d7afe to 9fd004c Compare October 14, 2018 01:50
@ptomulik ptomulik changed the title WIP Initial support for virtualenv Initial support for virtualenv Oct 14, 2018
@ptomulik
Copy link
Contributor Author

Well, I've just finished polishing the code. The virtualenv stuff is optional, i.e. it can be activated with an environment variable or CLI flag. I think, the PR is ready for review.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 80.207% when pulling 9fd004c on ptomulik:virtualenv into 0c20ae2 on SCons:master.

@coveralls
Copy link

coveralls commented Oct 14, 2018

Coverage Status

Coverage decreased (-1.8%) to 79.993% when pulling 0414380 on ptomulik:virtualenv into bccc8cf on SCons:master.

@bdbaddog
Copy link
Contributor

bdbaddog commented Oct 14, 2018 via email

@bdbaddog
Copy link
Contributor

Can you rebase this off current trunk?

@bdbaddog
Copy link
Contributor

And/or merge from current trunk

@ptomulik
Copy link
Contributor Author

rebased

@bdbaddog
Copy link
Contributor

Lots of new failures on windows:
https://ci.appveyor.com/project/SCons/scons/builds/19910814/job/fb2injqc1f5c24yy

Please take a look

@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 30, 2018

I've fixed, the two tests

  • test/virtualenv/option/enable-virtualenv.py,
  • test/virtualenv/option/ignore-virtualenv.py.

My mistake was assuming that WheIs('python') will always return string. On windows it returns None, because typically python (C:\Python37\python.exe, for example) is not in SCons PATH. Anyway, most (all) virtualenv end-to-end tests will yield NO RESULT on appveyor, because tests run not in virtualenv.

@bdbaddog
Copy link
Contributor

Do you need to create a virtualenv to run the tests in to ensure they're working propelry? we could add that to appveyor and travis setup files..?

@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 30, 2018

Do you need to create a virtualenv to run the tests in to ensure they're working propelry? we could add that to appveyor and travis setup files..?

It's good idea, I think. Maybe a separate appveyor job for running these few virtualenv-related tests within a virtualenv. Travis actually runs within virtualenv, as far as I see.

To yield a result, all tests within test/virtualenv should be executed within fully activated virtualenv, except these two:

  • test/virtualenv/virtualenv_detect_regularenv.py - this yields a result only in regular environment, and skipped in virtualenv,
  • test/virtualenv/virtualenv_unactivated_python.py - this yields a result when run in a not-activated virtualenv, via /my/virtualenv/bin/python runtest.py ..., so it should be executed this way to yield any result,

perhaps I should somehow reorganize them to put these exceptional tests to separate subdirectories?

@ptomulik
Copy link
Contributor Author

@bdbaddog
Copy link
Contributor

You can also do:
python runtest.py -P <virtualenv'd python path>

@bdbaddog
Copy link
Contributor

cancelled..

@ptomulik
Copy link
Contributor Author

ptomulik commented Oct 31, 2018

Ok, I reorganized directory structure under test/virtualenv so it should be easier to organize the run. The idea is to run these tests like:

  • . /my/virtualenv/bin/activate && python runtest.py test/virtualenv/{activated,always}; deactivate,
  • /my/virtualenv/bin/python runtest.py test/virtualenv/{unactivated,always},
  • python runtest.py test/virtualenv/{regularenv,always}

You can also cancel https://ci.appveyor.com/project/SCons/scons/builds/19930088.

</para>

<para>
This issue may be overcame by using <literal>--enable-virtualenv</literal>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overcame should be overcome

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import SCons.Util


def _get_bool_envvar(name, default=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to SCons.Util ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,136 @@
"""SCons.Platform.VE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.virtualenv instead of .VE please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@bdbaddog
Copy link
Contributor

bdbaddog commented Nov 7, 2018

Do any of the tests run under a virtualenv?

@ptomulik
Copy link
Contributor Author

ptomulik commented Nov 7, 2018

Do any of the tests run under a virtualenv?

Yes. Depending of what environment the tests are started in (virtualenv activated, virtualenv "unactivated", regular environment), some tests get skipped but some still run. This comment tries to explain some details.

Generally, tests under test/virtualenv are grouped in four groups (subdirectories): activated, unactivated, regularenv and always.

  • Tests in activated/ are skipped, unless we run in a fully activated virtualenv.
  • Tests in unactivated/ are skipped, unless we use a python which is a part of some virtualenv (but the virtualenv is not activated, we just use a virtualenv-provided interpreter, like /my/virtualenv/bin/python runtest.py ...).
  • Tests in regularenv/ are skipped, unless we run them in a regular environment (/usr/bin/python or such).
  • Tests in always/ are never skipped and should be examined in all three circumstances.

Because travis-ci provides activated virtualenv by default, tests under regularenv/ and unactivated/ are skipped. Only the always/ and activated/ are examined.

@bdbaddog
Copy link
Contributor

Can you rebase this from main?

@ptomulik
Copy link
Contributor Author

Can you rebase this from main?

Done

@bdbaddog
Copy link
Contributor

Merging.

@bdbaddog bdbaddog merged commit 648cf42 into SCons:master Nov 13, 2018
@ptomulik ptomulik deleted the virtualenv branch November 13, 2018 04:39
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 this pull request may close these issues.

None yet

4 participants