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

cmake: build man pages #9074

Merged
merged 2 commits into from May 12, 2016
Merged

cmake: build man pages #9074

merged 2 commits into from May 12, 2016

Conversation

tchaikov
Copy link
Contributor

Signed-off-by: Kefu Chai kchai@redhat.com

@tchaikov
Copy link
Contributor Author

@cbodley would you mind taking a look?

endforeach()

install(FILES ${sphinx_output} ceph_selinux.8
DESTINATION ${CEPH_MAN_DIR}/man8)
Copy link
Contributor Author

@tchaikov tchaikov May 11, 2016

Choose a reason for hiding this comment

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

we could use more sophisticated way to determine the section instead hard-wiring it to man8. but since we only have *.8 man pages at this moment, let's install all of them into "man8" now.

ceph-rest-api.8
ceph-post-file.8
DESTINATION ${CEPH_MAN_DIR}/man8)
set(manpages_srcs
Copy link
Contributor Author

@tchaikov tchaikov May 11, 2016

Choose a reason for hiding this comment

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

we list sphinx-build as one of the build dependencies, and i am not sure we will have a "make dist" for cmake (will cpack do this job?), so i put the generated man pages into the build directory instead of in "/man".

also it might be more natural if we have a CMakeLists.txt in into "/doc/man/8", and collect the man pages there. but i am good either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we will have a "make dist" for cmake

that's my understanding as well. i don't think we want to involve cpack, either

have a CMakeLists.txt in into "/doc/man/8", and collect the man pages there

i would prefer it this way too. for one, it would mean that the entries in manpages_srcs would be relative to their CMAKE_CURRENT_SOURCE_DIR, rather than having to add the ${CMAKE_SOURCE_DIR}/doc/man/ prefix. it also solves the install to "man8" piece because each CMakeLists.txt could do its own install(). but i'm fine with putting this off until we need it

Copy link
Contributor

Choose a reason for hiding this comment

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

typo on this line: manpages_srcs -> manpage_srcs

@cbodley
Copy link
Contributor

cbodley commented May 11, 2016

Thanks for taking this on @tchaikov, you're my hero! The install() part looks correct to me, but in testing 'make install' I'm getting the strangest errors:

-- Installing: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Up-to-date: /home/cbodley/local/share/man/man8
-- Installing: /home/cbodley/local/share/man/man8/ceph_selinux.8
CMake Error at man/cmake_install.cmake:36 (file):
  file INSTALL cannot copy file "/home/cbodley/ceph/man/ceph_selinux.8" to
  "/home/cbodley/local/share/man/man8/ceph_selinux.8".
Call Stack (most recent call first):
  cmake_install.cmake:38 (include)

The resulting directory looks like filesystem corruption:

$ ll ~/local/share/man/man8
ls: cannot access /home/cbodley/local/share/man/man8/ceph-fuse.8: Permission denied
total 0
?????????? ? ? ? ?            ? ceph-fuse.8

..but I was able to reproduce when installing to different devices, and @alimaredia reproduced it on his machine as well.

Digging further, I added some debugging to print the $sphinx_output variable and saw this:
/home/cbodley/ceph/build/doc/man/ceph-fuse.8/;/home/cbodley/ceph/build/doc/man/rbd-fuse.8/;/home/cbodley/ceph/build/doc/man/ceph-rbdnamer.8/;/home/cbodley/ceph/build/doc/man/rbd-nbd.8/;/home/cbodley/ceph/build/doc/man/rbd-replay-prep.8/;/home/cbodley/ceph/build/doc/man/rbd-replay.8/;/home/cbodley/ceph/build/doc/man/rbdmap.8/;/home/cbodley/ceph/build/doc/man/rbd.8/.

So it looks like we need to trim that trailing / off of the section variable that results from string(REGEX MATCH "^[0-9]/" section ${manpage}). There's probably a better way to do that, but adding string(REPLACE "/" "" section ${section}) seemed to work for me and the man pages installed correctly.

@cbodley
Copy link
Contributor

cbodley commented May 11, 2016

@tchaikov I pushed fixes for the issues I mentioned, feel free to cherry-pick them from https://github.com/cbodley/ceph/commits/wip-cmake-manpages

Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

@cbodley and @alimaredia thanks for testing this PR!

i merged your changes and restructured the CMakeLists.txt so we don't need to hardcode the "/doc/man" path.

rbdmap.8
rbd.8
DESTINATION ${CEPH_MAN_DIR}/man8)
if(WITH_SELINUX)
Copy link
Contributor Author

@tchaikov tchaikov May 12, 2016

Choose a reason for hiding this comment

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

WITH_SELINUX is not supported yet in cmake, will leave it for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in #9133

@cbodley
Copy link
Contributor

cbodley commented May 12, 2016

the new stuff in doc/ looks good. retested and everything seemed to work 👍

@tchaikov tchaikov merged commit 9eee5eb into ceph:master May 12, 2016
@tchaikov tchaikov deleted the wip-cmake-manpages branch May 12, 2016 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants