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

Switch to python 3 #1639

Closed
wants to merge 2 commits into from
Closed

Conversation

nickva
Copy link
Contributor

@nickva nickva commented Oct 3, 2018

Ran 2to3 and fixed a few deprecated warnings

@wohali
Copy link
Member

wohali commented Oct 3, 2018

@nickva I think this will work, but it will break Jenkins in other ways (not every Docker image has python3 installed right now). In fact, it'll turn everything in Jenkins red other than CentOS 6 I expect (and maybe Ubuntu 18.04? Not sure...) That's on me, I am happy to shift us entirely over to python3.

It may also break developers' environments if they don't have python3 installed. You'll want at the very least to update these other places:

  • README-DEV.rst
  • apache/couchdb-ci:bin/*-dependencies.sh (make sure Python 3 gets installed everywhere)

If you can do both of the above then it'll be a snap for me to rebuild the Docker images for Jenkins and re-push them to Docker Hub. (In fact, it's a single command.)

Please and thank you! :D

@nickva
Copy link
Contributor Author

nickva commented Oct 3, 2018

Will do, good idea.

Also we run mango with python2.7 currently. Let's see if that can run with python3 as well.

Will make it a separate commit

@nickva
Copy link
Contributor Author

nickva commented Oct 3, 2018

Updated with a commit to run mango test with Python 3

@nickva
Copy link
Contributor Author

nickva commented Oct 3, 2018

PR to couchdb-ci apache/couchdb-ci#8

I saw failures in the Travis CI with it as you predicted.

Wonder if it is better to just revert the change on master for now, as latest Fedora is less critical to support than existing platforms? And then return to it later, post 2.3? I'll follow your guidance since you'd know a lot more about the scope of issues involved.

Copy link
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

Hi Nick,

We need consistency around the Python environment for the mango tests. In some places we're expecting nose and hypothesis to be installed at the system level. In others, it's in the venv.

I don't really care which way we go, but I want consistency, especially because it affects how the CI images are prepared. Without this PR, here's where we are at:

  • Travis CI uses before_script to prepare a venv, changes directories to activate, then runs make check.
  • Jenkins CI installs the packages globally and runs make check with no preparation.
  • Individual developers must either install those packages globally, or have the awareness to create their own virtualenv and activate it prior to running make check.

A fourth option is to alter Makefile/Makefile.win to run the mango tests inside the venv without activation; this is done by explicitly running the python interpreter linked under path/to/ENV/bin. However this will not extend to subprocesses since PATH and VIRTUAL_ENV are not updated. I haven't studied the mango test process enough to know if this will work, especially with make in the loop as well; see https://virtualenv.pypa.io/en/stable/userguide/ for more details.

Finally, I just got the mango tests working under Windows; if we do intend to alter Makefile please don't ignore changes necessary for Makefile.win (meaning: doing cd src/mango && make test is problematic since there is no Makefile.win in src/mango.)

README-DEV.rst Outdated
@@ -60,7 +57,7 @@ Debian-based (inc. Ubuntu) Systems
::

sudo apt-get install help2man python-sphinx gnupg nodejs npm \
python-hypothesis python-requests python-nose
python3-pip python-virtualenv
Copy link
Member

Choose a reason for hiding this comment

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

You can't remove hypothesis and nose here since the instructions lower down to use pip to install system versions is only for OSX.

There are python3-hypothesis and python3-nose and python3-requests packages. Check https://packages.debian.org/ and https://packages.ubuntu.com/ to ensure these are available for our supported OSes:

  • Debian stretch
  • Debian jessie
  • Ubuntu trusty
  • Ubuntu xenial
  • Ubuntu bionic

README-DEV.rst Outdated

::

sudo yum install help2man python-sphinx python-docutils \
python-pygments gnupg nodejs npm python-nose python-requests \
python-hypothesis
python-pygments gnupg nodejs npm python34-pip python-virtualenv
Copy link
Member

Choose a reason for hiding this comment

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

Same problem as above. You're going to have to be careful though:

https://pkgs.org/download/python-nose

the package name changes depending on the version of python that ships with that release AND it requires a custom repo. Might be easier to use a global pip or pip3 here - you'll need to see what the distro calls it at the CLI. Sorry about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's why I think it would be good to move to venvs

@@ -39,7 +39,7 @@ help:
.PHONY: test
# target: test - Runs test suite
test:
nosetests
./venv/bin/nosetests
Copy link
Member

Choose a reason for hiding this comment

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

I thought we agreed we wouldn't use a virtualenv for this.

Besides, you haven't even pip installed the prerequisites necessary into that venv.

Finally, the top level Makefile is unchanged in this PR, meaning when you do a make check or make mango-test you execute:

.PHONY: mango-test
# target: mango-test - Run Mango tests
mango-test: devclean all
    @cd src/mango && ../../dev/run -n 1 --admin=testuser:testpass nosetests

Would like to see consistency here, one way or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to make it use a venv and remove this Makefile altogether. This was probably more useful when mango was a separate repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wohali I made mango tests use virtualenvs and delete the Makefile in src/mango

Ran 2to3 and fixed a few deprecated warnings

Issue apache#1632
@nickva nickva force-pushed the switch-scripts-to-python3 branch 2 times, most recently from 22c7b6a to 86fefb5 Compare November 26, 2018 20:10
Copy link
Member

@wohali wohali left a comment

Choose a reason for hiding this comment

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

Looks good. Need two fixes:

  1. See comment in review, shouldn't be modifying that one file
  2. Needs Windows fixes. I'd be happy to work on a branch with you but I can't on this one since you're PR'ing from cloudant/couchdb, not apache/couchdb. Can you port the branch over and I'll work with you there?

+1 once the above are done and Travis passes.

src/couch/test/fixtures/os_daemon_bad_perm.sh Outdated Show resolved Hide resolved
Before we were ignoring venv setup in the Makefile, so update the test runner
to use that instead of pestering developers to install those dependencies by
hand.

Issue apache#1632
@nickva
Copy link
Contributor Author

nickva commented Nov 26, 2018

Closing this one and re-opening as #1764 with an ASF branch

@nickva nickva closed this Nov 26, 2018
@nickva nickva deleted the switch-scripts-to-python3 branch August 7, 2019 05:25
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

2 participants