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

run nosetests with the Python interpreter (2 vs. 3) that was requeste… #984

Merged
merged 3 commits into from Jul 18, 2018

Conversation

@clinssen
Copy link
Contributor

@clinssen clinssen commented Jul 12, 2018

run nosetests with the Python interpreter (2 vs. 3) that was requested when invoking cmake

I ran into the same problem as #892, that is, nosetests fails because it is run using the system default Python version (in my case, /usr/bin/python -> python2.7), instead of the one that is requested during the installation process (in my case, cmake [...] -Dwith-python=3).

The proposed code tries to extract the requested version from CMakeCache.txt and selects the binary accordingly by invoking "which".

Tested on Linux, but it should work on any system where the "which" utility is available.

…d when invoking cmake
@jougs jougs requested a review from hakonsbm Jul 17, 2018
nosetests -v --with-xunit --xunit-file=${TEST_OUTDIR}/pynest_tests.xml ${NEST_PYTHON_PREFIX:-@CMAKE_INSTALL_PREFIX@/@PYEXECDIR@}/nest/tests ${NEST_PYTHON_PREFIX:-@CMAKE_INSTALL_PREFIX@/@PYEXECDIR@}/nest/topology/tests 2>&1 \
# if possible, pick the python version that was specified during the installation

WITH_PYTHON=`cat CMakeCache.txt | grep with-python | cut -d "=" -f2`

This comment has been minimized.

@jougs

jougs Jul 17, 2018
Contributor

Isn't the Python executable used for configuration available in CMake's variable PYTHON and couln't that be just replaced into this file using @PYTHON@?

@clinssen
Copy link
Contributor Author

@clinssen clinssen commented Jul 17, 2018

Indeed, if we use cmake, the code can be simplified and we no longer need which!

Copy link
Contributor

@jougs jougs left a comment

Thanks for considering my comments. Here's some more!

@@ -103,6 +103,7 @@ endif ()
################################################################################

# needed for target doc and fulldoc
find_program( NOSETESTS NAMES nosetests )

This comment has been minimized.

@jougs

jougs Jul 18, 2018
Contributor

Very nice!
Can you please remove the comment above as that seems outdated and not very helpful anyway.

@@ -724,13 +724,13 @@ if test "x${TEST_PYNEST}" = xtrue ; then
# of support for nosetests. We use the TEST_OUTDIR as Python-free
# dummy directory to search for tests.

if command -v nosetests >/dev/null 2>&1 && nosetests --with-xunit --xunit-file=/dev/null --where=${TEST_OUTDIR} >/dev/null 2>&1; then
if command -v @NOSETESTS@ >/dev/null 2>&1 && @NOSETESTS@ --with-xunit --xunit-file=/dev/null --where=${TEST_OUTDIR} >/dev/null 2>&1; then

This comment has been minimized.

@jougs

jougs Jul 18, 2018
Contributor

This line checks

  1. if the nosetests executable is available (command -v yields the same as which, but is more portable) and
  2. if it can be run successfully using the options we are using below in line 733
    I think that 2. should also use the correct Python to be meaningful
@clinssen
Copy link
Contributor Author

@clinssen clinssen commented Jul 18, 2018

Fixed the comments and the check for nosetests availability.

N.B. the with-python check is verbatim from https://github.com/nest/nest-simulator/blob/master/cmake/ProcessOptions.cmake#L341

@jougs
jougs approved these changes Jul 18, 2018
Copy link
Contributor

@jougs jougs left a comment

Thanks for the changes and your contribution!

Copy link
Contributor

@hakonsbm hakonsbm left a comment

Looks good to me!

@jougs jougs merged commit 484f3e1 into nest:master Jul 18, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.