Skip to content

check for nosetests in configure#4449

Closed
dalgaaf wants to merge 3 commits into
masterfrom
wip-nosetests
Closed

check for nosetests in configure#4449
dalgaaf wants to merge 3 commits into
masterfrom
wip-nosetests

Conversation

@dalgaaf
Copy link
Copy Markdown
Contributor

@dalgaaf dalgaaf commented Apr 23, 2015

'make check' needs nosetests (python-nose) for some of the checks. Added a check to configure to make sure the package is installed.

Fixed also some links in documentation related to CentOS ftp server links.

dalgaaf added 3 commits April 23, 2015 16:41
Check if /usr/bin/nosetests is installed since needed to
run 'make check'.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
Don't use '7.0.1406' version in ftp address, use the '7'
symlink instead to avoid issues with outdated releases.

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>
@tchaikov
Copy link
Copy Markdown
Contributor

tchaikov commented May 5, 2015

@dalgaaf , i just learnt from @alfredodeza that we don't want to add more dependencies to configure stage. because the stage where jenkins.ceph.com creates the dist tarball might not have the required tools, among other things nosetest or sphinx-build. so, probably, you want to reconsider the change to configure.ac?

i am also adding a "--with-man-pages" to configure in order to loose the requirement for sphinx-build. see #4544

still under discussion, see http://permalink.gmane.org/gmane.comp.file-systems.ceph.devel/24688 .

@dalgaaf
Copy link
Copy Markdown
Contributor Author

dalgaaf commented May 8, 2015

@tchaikov: to be honest this is strange plan. What is the purpose of a CI tool if it doesn't test the complete build/package. If the CI system doesn't have the tools to build the basic dist tarball, then add the missing packages.

The nosetest package is needed to run make check successfully. Otherwise you have failing tests and to disable tests based on the installed packages in the system makes also no sense since then the checks are incomplete and the dist tarball is not ready for testing.

@tchaikov
Copy link
Copy Markdown
Contributor

@dalgaaf agreed. and we should use install-deps.sh to ready the CI system for configure.

so the only thing i'd suggest for this pull request is not to add nosetest to the list of documenting-ceph.rst . what do you think?

@ghost
Copy link
Copy Markdown

ghost commented May 11, 2015

@dalgaaf the proper way of dealing with python dependencies is by adding them in a requirements.txt file along with a tox.ini file that will be run via make check. You can see how it is done in https://github.com/ceph/ceph/tree/master/src/ceph-detect-init for instance. https://github.com/ceph/ceph/blob/master/install-deps.sh will install the dependencies and install-deps.sh is also used by CI.
Of course it would be better to add proper packages to BuildRequires but ... finding the python packages that exactly match the content of the requirements.txt files, recursively, not only is a lot of redundant manual work. It also is a blocker because many such packages do not exist on the operating systems supported by Ceph and on which we want to run tests.

Although I prefer py.test, I don't have strong feelings against nose, as long as it is run from tox.

@ghost ghost assigned ghost and unassigned dalgaaf May 13, 2015
@ghost
Copy link
Copy Markdown

ghost commented May 13, 2015

@dalgaaf I'm with @tchaikov on not including in documenting-ceph.rst.

@ghost ghost removed their assignment May 18, 2015
@ghost
Copy link
Copy Markdown

ghost commented May 18, 2015

(removing myself because I don't have the bandwidth to followup in the next week, feel free to ping me in two weeks if you need)

@ghost
Copy link
Copy Markdown

ghost commented Sep 1, 2015

@dalgaaf feel free to re-open when you have time to address the comments

@ghost ghost closed this Sep 1, 2015
@liewegas liewegas deleted the wip-nosetests branch November 23, 2016 23:26
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants