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

nosetests: use /usr/bin/env to find nosetests #12091

Merged
merged 1 commit into from Jan 10, 2017

Conversation

Projects
None yet
2 participants
@wjwithagen
Contributor

wjwithagen commented Nov 21, 2016

  • Options are not required, so do not pass them to the script.

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

[ ! -z $CEPH_ROOT ] || exit 1
PYBIND=$CEPH_ROOT/src/pybind /usr/local/bin/nosetests --stop $CEPH_ROOT/src/test/pybind/test_ceph_argparse.py

This comment has been minimized.

@tchaikov

tchaikov Nov 21, 2016

Contributor

test_ceph_argparse.py does not use ${PYBIND}, i think.

This comment has been minimized.

@wjwithagen

wjwithagen Nov 21, 2016

Contributor

@tchaikov
Eh, that could be that case.
I first did the daemon case, and then copied the wrapper.
Even considered make it a generic one in taking arg0 and rewrite it to basename + .py.
What is your preference: ONE generic one, or that I check PYBIND usage.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 4, 2017

Contributor

@tchaikov
Could you please indicate which solution you prefer here?
I'll fix, and submit, so this one can be merged..

This comment has been minimized.

@tchaikov

tchaikov Jan 6, 2017

Contributor

@wjwithagen i think the root cause might be that the shebang line in src/test/pybind/test_ceph_daemon.py assume that nosetests is located in /usr/bin. while on your FreeBSD installation, it's not the case.

as to PYBIND, it's not necessary. as the cmake function add_ceph_test() takes care of the pybinding path already by setting PYTHONPATH=${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/cython_modules/lib.${PYTHON${PYTHON_VERSION}_VERSION_MAJOR}:${CMAKE_SOURCE_DIR}/src/pybind.

instead of using #!/usr/bin/nosetest --nocapture, probably we can just use #!/usr/bin/env nosetests. because shebang line does not support more than two or more parameters. i guess it's safe to remove the --nocapture option. it does not hurt to read the captured message sent to stdout along with the error message. @dachary what do you think?

This comment has been minimized.

@ghost

ghost Jan 10, 2017

@tchaikov I agree that it is safe to remove the --nocapture option

This comment has been minimized.

@ghost

ghost Jan 10, 2017

@wjwithagen I agree with @tchaikov, it would be better to just use #!/usr/bin/env nosetest to resolve that issue. It is simpler and looks like it would work for FreeBSD as well.

@ghost ghost added the tests label Nov 23, 2016

@tchaikov tchaikov self-assigned this Jan 4, 2017

@tchaikov tchaikov requested a review from Jan 6, 2017

[ ! -z $CEPH_ROOT ] || exit 1
PYBIND=$CEPH_ROOT/src/pybind /usr/local/bin/nosetests --stop $CEPH_ROOT/src/test/pybind/test_ceph_argparse.py

This comment has been minimized.

@ghost

ghost Jan 10, 2017

@tchaikov I agree that it is safe to remove the --nocapture option

[ ! -z $CEPH_ROOT ] || exit 1
PYBIND=$CEPH_ROOT/src/pybind /usr/local/bin/nosetests --stop $CEPH_ROOT/src/test/pybind/test_ceph_argparse.py

This comment has been minimized.

@ghost

ghost Jan 10, 2017

@wjwithagen I agree with @tchaikov, it would be better to just use #!/usr/bin/env nosetest to resolve that issue. It is simpler and looks like it would work for FreeBSD as well.

@tchaikov tchaikov assigned wjwithagen and unassigned tchaikov Jan 10, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 10, 2017

@dachary @tchaikov
I do like KISS as well, so I'll start with /usr/bin/env and see if that works.

nosetests: start using /usr/bin/env to find noestests
 - Option nocapture is not really required so no problem
   with the fact that FreeBSD env does not work for params
   for nosetests

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

@wjwithagen wjwithagen changed the title from nosetests: FreeBSD /usr/bin/env does not set the options correct to nosetests: use /usr/bin/env to find noestests Jan 10, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 10, 2017

@dachary @tchaikov
Run nicely in my wip with env and without options..

@tchaikov tchaikov assigned tchaikov and unassigned wjwithagen Jan 10, 2017

@tchaikov tchaikov added needs-qa and removed needs-qa labels Jan 10, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 10, 2017

Reviewed-by: once jenkins is back and green.

the latest change addressed the comment.

@tchaikov tchaikov changed the title from nosetests: use /usr/bin/env to find noestests to nosetests: use /usr/bin/env to find nosetests Jan 10, 2017

@tchaikov tchaikov merged commit 7b8eca5 into ceph:master Jan 10, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@wjwithagen wjwithagen deleted the wjwithagen:wip-wjw-nosetests branch Jan 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment