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 manual pages (curl.1 and libcurl (3) manpages) #1288

Closed
wants to merge 3 commits into from

Conversation

Lekensteyn
Copy link
Contributor

@Lekensteyn Lekensteyn commented Feb 25, 2017

Also make Perl mandatory to allow building the docs.

While CMakeLists.txt could probably read the list of manual pages from
Makefile.am, actually putting those in CMakeLists.txt is cleaner so that
is what is done here.

Fixes #1230


cmake: add support for building HTML and PDF docs

Note that for some reason there is this warning (that also exists with
autotools, added since curl-7_15_1-94-ga718cb05f):

docs/libcurl/curl_multi_socket_all.3:1: can't open `man3/curl_multi_socket.3': No such file or directory

Additionally, adjust the roffit --mandir option to support creating
links when doing out-of-tree builds.


(NOTE: this does not install manpages yet)

@mention-bot
Copy link

@Lekensteyn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @billhoffman, @snikulov and @Sukender to be potential reviewers.

jay
jay previously requested changes Feb 25, 2017
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

Please dynamically generate those manpage filenames. I suggest doing the same thing we already do in lib and src and use transform_makefile_inc to extract the data from the appropriate Makefile.inc. This way you don't have to rely on gawk which not everyone may have. For example in lib/CMakeLists.txt there's this:

transform_makefile_inc("Makefile.inc" "${CMAKE_CURRENT_BINARY_DIR}/Makefile.inc.cmake")
include(${CMAKE_CURRENT_BINARY_DIR}/Makefile.inc.cmake)

To do something similar without any friction would likely require moving the filename lists such as DPAGES for example from Makefile.am into Makefile.inc, then in the Makefile.am put

# Makefile.inc provides DPAGES
include Makefile.inc

and in the Makefile.inc put
# These variables are also used by cmake to auto generate output
or something
then convert into Makefile.inc.cmake in your script

@Lekensteyn
Copy link
Contributor Author

I created a new change to separate the variables and rebased the other changes on top of them. Tested with cmake only.

(If another iteration is needed, I'll probably add try to move the MANPAGE variable to cmdline-opts/Makefile.inc, but that is a small issue.)

@Lekensteyn Lekensteyn changed the title cmake: build manual pages (including curl.1) cmake: Build manual pages (curl.1 and libcurl (3) manpages) Feb 25, 2017
@bagder
Copy link
Member

bagder commented Mar 14, 2017

I'd be happy to see this change land, anything still pending @Lekensteyn ? (apart from a needed rebase that is)

@Lekensteyn
Copy link
Contributor Author

Rebased and resolved conflicts in Makefile.am; git diff be426dbe1 4343de2b8 -- ":docs/*/Makefile.inc":

diff --git a/docs/cmdline-opts/Makefile.inc b/docs/cmdline-opts/Makefile.inc
index 79db71119..97c0547ee 100644
--- a/docs/cmdline-opts/Makefile.inc
+++ b/docs/cmdline-opts/Makefile.inc
@@ -35,8 +35,10 @@ DPAGES = abstract-unix-socket.d anyauth.d append.d basic.d cacert.d capath.d cer
   service-name.d show-error.d silent.d socks4a.d socks4.d socks5.d      \
   socks5-gssapi-nec.d socks5-gssapi-service.d socks5-hostname.d         \
   speed-limit.d speed-time.d ssl-allow-beast.d ssl.d ssl-no-revoke.d    \
-  ssl-reqd.d sslv2.d sslv3.d stderr.d tcp-fastopen.d tcp-nodelay.d      \
+  ssl-reqd.d sslv2.d sslv3.d stderr.d suppress-connect-headers.d        \
+  tcp-fastopen.d tcp-nodelay.d                                          \
   telnet-option.d tftp-blksize.d tftp-no-options.d time-cond.d          \
+  tls-max.d                                                             \
   tlsauthtype.d tlspassword.d tlsuser.d tlsv1.0.d tlsv1.1.d tlsv1.2.d   \
   tlsv1.3.d tlsv1.d trace-ascii.d trace.d trace-time.d tr-encoding.d    \
   unix-socket.d upload-file.d url.d use-ascii.d user-agent.d user.d     \
diff --git a/docs/libcurl/opts/Makefile.inc b/docs/libcurl/opts/Makefile.inc
index c8edade6a..5a201fe3f 100644
--- a/docs/libcurl/opts/Makefile.inc
+++ b/docs/libcurl/opts/Makefile.inc
@@ -278,6 +278,7 @@ man_MANS =                                      \
   CURLOPT_STREAM_DEPENDS.3                      \
   CURLOPT_STREAM_DEPENDS_E.3                    \
   CURLOPT_STREAM_WEIGHT.3                       \
+  CURLOPT_SUPPRESS_CONNECT_HEADERS.3            \
   CURLOPT_TCP_FASTOPEN.3                        \
   CURLOPT_TCP_KEEPALIVE.3                       \
   CURLOPT_TCP_KEEPIDLE.3                        \

Tested by building with cmake. TODO for later: add install targets for manpages

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Not a cmake person but it looks fine to me and as you tested it, I think it is fine to proceed!

@jay jay dismissed their stale review March 15, 2017 16:28

fixed

@bagder
Copy link
Member

bagder commented Mar 15, 2017

Is that travis failure just totally bogus? I can't understand why this PR would trigger that.

@jay
Copy link
Member

jay commented Mar 15, 2017

Is that travis failure just totally bogus? I can't understand why this PR would trigger that.

I see it also. Here is the bad section:
capture

So probably a typo

diff --git a/docs/libcurl/opts/Makefile.am b/docs/libcurl/opts/Makefile.am
index 02623b6..cd9eada 100644
--- a/docs/libcurl/opts/Makefile.am
+++ b/docs/libcurl/opts/Makefile.am
@@ -23,7 +23,6 @@
 AUTOMAKE_OPTIONS = foreign no-dependencies
 
 include Makefile.inc
- CURLOPT_SUPPRESS_CONNECT_HEADERS.3             \
 
 man_DISTMANS = $(man_MANS:.3=.3.dist)

@Lekensteyn
Copy link
Contributor Author

Oops, that was a merge fail. Removed that line, the other files seem OK. Attempt 2, counting on travis to do the autotools build.

@jay
Copy link
Member

jay commented Mar 16, 2017

CI has passed. Before you push these upstream can you please add a line to the commit messages that reference this PR. For example Closes or Ref.

Ref: https://github.com/curl/curl/pull/1288

For easier sharing with CMake. The contents were reformatted to use
two-space indent and expanded tabs (matching lib/Makefile.common).

Ref: curl#1288
Also make Perl mandatory to allow building the docs.

While CMakeLists.txt could probably read the list of manual pages from
Makefile.am, actually putting those in CMakeLists.txt is cleaner so that
is what is done here.

Fixes curl#1230
Ref: curl#1288
Note that for some reason there is this warning (that also exists with
autotools, added since curl-7_15_1-94-ga718cb05f):

    docs/libcurl/curl_multi_socket_all.3:1: can't open `man3/curl_multi_socket.3': No such file or directory

Additionally, adjust the roffit --mandir option to support creating
links when doing out-of-tree builds.

Ref: curl#1288
@Lekensteyn
Copy link
Contributor Author

Rebased with tags added, will push when CI passed (when I am awake tomorrow).

Lekensteyn added a commit that referenced this pull request Mar 21, 2017
For easier sharing with CMake. The contents were reformatted to use
two-space indent and expanded tabs (matching lib/Makefile.common).

Ref: #1288
Lekensteyn added a commit that referenced this pull request Mar 21, 2017
Also make Perl mandatory to allow building the docs.

While CMakeLists.txt could probably read the list of manual pages from
Makefile.am, actually putting those in CMakeLists.txt is cleaner so that
is what is done here.

Fixes #1230
Ref: #1288
Lekensteyn added a commit that referenced this pull request Mar 21, 2017
Note that for some reason there is this warning (that also exists with
autotools, added since curl-7_15_1-94-ga718cb05f):

    docs/libcurl/curl_multi_socket_all.3:1: can't open `man3/curl_multi_socket.3': No such file or directory

Additionally, adjust the roffit --mandir option to support creating
links when doing out-of-tree builds.

Ref: #1288
@Lekensteyn
Copy link
Contributor Author

Fixed in:
898b012 cmake: add support for building HTML and PDF docs
84a226a cmake: build manual pages (including curl.1)
6f6e919 docs: split file lists into Makefile.inc

@Lekensteyn Lekensteyn closed this Mar 21, 2017
@Lekensteyn Lekensteyn deleted the cmake-docs branch March 21, 2017 13:59
@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

4 participants