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

use python instead of envpython #181

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

richm
Copy link
Contributor

@richm richm commented Mar 31, 2020

Many of the tests and tasks invoked from tox would try to use
the path to the python executable in the tox virtualenv, and
in some cases, would try to follow the symlink if the python
command were a symlink, in some cases, following the symlink to
the system python, and trying to use the absolute path. This
would fail in some cases because when using the system python
with the absolute path, the python modules installed in the
tox venv could not be found. So instead, just use the python
command from the venv with no path.

also enable travis 3.8-dev testing, since this fix allows that
platform to work correctly.

@coveralls
Copy link

coveralls commented Mar 31, 2020

Pull Request Test Coverage Report for Build 595

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 590: 0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

@richm richm requested a review from tyll March 31, 2020 15:47
@richm
Copy link
Contributor Author

richm commented Mar 31, 2020

[citest bad]

@richm richm mentioned this pull request Mar 31, 2020
.travis/utils.sh Outdated Show resolved Hide resolved
@tyll
Copy link
Member

tyll commented Apr 1, 2020

If this change also fixes using python 3.8-dev from Travis, the change to enable it again could be included here as well.

@richm richm force-pushed the use-python-not-envpython branch 2 times, most recently from 56d0d76 to 4628a89 Compare April 1, 2020 16:28
.travis/utils.sh Outdated
if [[ -z "$syspython" ]]; then
syspython=$(command -pv python2)
fi
echo ${syspython:-/usr/bin/python3}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I had to think about this. It seems to be rather odd that /usr/bin/python3 would be a correct choice without it being in PATH. If it is in PATH, it would be set already by line 144. Technically, it should probably fail in this case instead of assuming /usr/bin/python3 to be correct. But maybe this is a wrong assumption. It does not seem to me that this is that important, just wanted to raise it. I am also fine with merging it as-is, since it will probably not hurt. What is your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to this:

  if [[ -z "$syspython" ]]; then
    lsr_error Could not determine system python path
  fi
  echo $syspython

Copy link
Member

@tyll tyll left a comment

Choose a reason for hiding this comment

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

Please see my note, not a blocker. LGTM overall. It just needs to be rebased to master AFAICS. Thank you for adding 3.8-dev.

@richm richm force-pushed the use-python-not-envpython branch from 4628a89 to 2f5193c Compare April 1, 2020 17:29
Many of the tests and tasks invoked from `tox` would try to use
the path to the `python` executable in the tox virtualenv, and
in some cases, would try to follow the symlink if the `python`
command were a symlink, in some cases, following the symlink to
the system python, and trying to use the absolute path.  This
would fail in some cases because when using the system python
with the absolute path, the python modules installed in the
tox venv could not be found.  So instead, just use the `python`
command from the venv with no path.

also enable travis 3.8-dev testing, since this fix allows that
platform to work correctly.
@richm richm force-pushed the use-python-not-envpython branch from 2f5193c to 4303e47 Compare April 1, 2020 17:30
@richm
Copy link
Contributor Author

richm commented Apr 1, 2020

Please see my note, not a blocker. LGTM overall. It just needs to be rebased to master AFAICS. Thank you for adding 3.8-dev.

Fixed and rebased

if [[ -z "$syspython" ]]; then
syspython=$(command -pv python2)
fi
if [[ -z "$syspython" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

awesome, thank you!

Copy link
Member

@tyll tyll left a comment

Choose a reason for hiding this comment

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

👍

@tyll tyll merged commit 23937b6 into linux-system-roles:master Apr 1, 2020
@richm richm deleted the use-python-not-envpython branch April 1, 2020 21:29
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.

3 participants