Skip to content

cmake: rework options to enable curl and libcurl docs#12773

Closed
vszakats wants to merge 9 commits intocurl:masterfrom
vszakats:cmake-separate-curl-and-libcurl-manual-options
Closed

cmake: rework options to enable curl and libcurl docs#12773
vszakats wants to merge 9 commits intocurl:masterfrom
vszakats:cmake-separate-curl-and-libcurl-manual-options

Conversation

@vszakats
Copy link
Copy Markdown
Member

@vszakats vszakats commented Jan 24, 2024

Rework CMake options for building/using curl tool and libcurl manuals.

  • rename ENABLE_MANUAL to ENABLE_CURL_MANUAL, meaning:
    to build man page and built-in manual for curl tool.

  • rename BUILD_DOCS to BUILD_LIBCURL_DOCS, meaning:
    to build man pages for libcurl.

  • BUILD_LIBCURL_DOCS now works without having to enable
    ENABLE_CURL_MANUAL too.

  • drop support for existing CMake-level USE_MANUAL option to avoid
    confusion. (It used to work with the effect of current
    ENABLE_CURL_MANUAL, but only by accident.)

Ref: #12771
Closes #12773

- `ENABLE_MANUAL`: build man page and built-in manual for curl tool
- `BUILD_DOCS`: build man pages for libcurl

`USE_MANUAL` in CMake means we have the necessary tool to build these,
which also propagates down to C to enable the built-in manual for curl
tool.

Ref: curl#12771
Closes #xxxxx
Comment thread CMakeLists.txt
Copy link
Copy Markdown
Contributor

@levitte levitte left a comment

Choose a reason for hiding this comment

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

If I may, I think that it's still useful to have USE_MANUAL up front, including in curl_config.h. I believe these changes acheive that

Comment thread CMakeLists.txt Outdated
Comment thread lib/curl_config.h.cmake
Comment thread src/CMakeLists.txt
@levitte
Copy link
Copy Markdown
Contributor

levitte commented Jan 24, 2024

I gotta admit, I still find it a bit weird that curl.1 wouldn't be built as part of building the docs, regardless of if ENABLE_MANUAL is set or not.

However, there is also BUILD_CURL_EXE, and I can fully understand that if this is disabled, building the corresponding manual would be a bit weird. Could it be that this and ENABLE_MANUAL are a bit confounded?

vszakats and others added 2 commits January 24, 2024 15:27
Co-authored-by: Richard Levitte <levitte@openssl.org>
@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Jan 24, 2024

curl.1 comes with the official source release tarball, so anyone using that
doesn't need ENABLE_MANUAL=ON to have curl.1. Same for tool_hugehelp.c.

ENABLE_MANUAL=ON has the effect of unconditionally rebuilding (I haven't
retested that after recent changes) curl.1, which breaks reproducibiliy in some
build envs. Not enabling it by default is a workaround for that (and not doing this
extra step for release builds, which is presumably most builds out there.).

We can rename BUILD_DOCS to BUILD_LIBCURL_DOCS to better reflect
its function. (possibly even ENABLE_MANUAL to ENABLE_CURL_MANUAL.)

As for BUILD_CURL_EXE, what about doing this?:

--- a/docs/CMakeLists.txt
+++ b/docs/CMakeLists.txt
@@ -25,6 +25,6 @@
 if(BUILD_DOCS)
   add_subdirectory(libcurl)
 endif()
-if(ENABLE_MANUAL)
+if(ENABLE_MANUAL AND BUILD_CURL_EXE)
   add_subdirectory(cmdline-opts)
 endif()

@levitte
Copy link
Copy Markdown
Contributor

levitte commented Jan 24, 2024

We can rename BUILD_DOCS to BUILD_LIBCURL_DOCS to better reflect its function. (possibly even ENABLE_MANUAL to ENABLE_CURL_MANUAL.)

That would be much less confusing, so yes please

As for BUILD_CURL_EXE, what about doing this?:

--- a/docs/CMakeLists.txt
+++ b/docs/CMakeLists.txt
@@ -25,6 +25,6 @@
 if(BUILD_DOCS)
   add_subdirectory(libcurl)
 endif()
-if(ENABLE_MANUAL)
+if(ENABLE_MANUAL AND BUILD_CURL_EXE)
   add_subdirectory(cmdline-opts)
 endif()

Looks good to me

@vszakats
Copy link
Copy Markdown
Member Author

Thanks, committed these updates.

Comment thread src/CMakeLists.txt
@vszakats vszakats changed the title cmake: separate BUILD_DOCS from ENABLE_MANUAL cmake: rework options to enable curl and libcurl docs Jan 24, 2024
@vszakats
Copy link
Copy Markdown
Member Author

vszakats commented Jan 24, 2024

curl.1 comes with the official source release tarball, so anyone using that doesn't need ENABLE_MANUAL=ON to have curl.1. Same for tool_hugehelp.c.

ENABLE_MANUAL=ON has the effect of unconditionally rebuilding (I haven't retested that after recent changes) curl.1, which breaks reproducibiliy in some build envs. Not enabling it by default is a workaround for that (and not doing this extra step for release builds, which is presumably most builds out there.).

Reviewing curl-for-win script again, this remains having another
downside: To avoid rebuilding existing curl.1 / tool_hugehelp.c
yet still embedding it into curl, the CPPFLAGS -DUSE_MANUAL
must be passed manually. This was so before this week's changes
and still true.

Doing the rebuild only when curl.1 (and/or tool_hugehelp.c)
are missing or outdated would be one solution; making the output
deterministic would be another (this would require dropping nroff,
which is on bagder's plans). Ideally both.
C USE_MANUAL also has the requirement that tool_hugehelp.c
does actually offer a hugehelp symbol in it, otherwise the build
breaks. Using src/tool_hugehelp.c.cvs satisfies this, but not the
empty stub generated by CMake (or autotools for that matter).

Perhaps an approach where we have a static tool_hugehelp.c in
the source tree, while expecting the manual content inside a
generated tool_hugehelp.h (depending on USE_MANUAL)
could help to avoid some failure modes. But touching any of that looks
like a deep rabbit-hole.

In any case this is left for a separate PR.

@levitte
Copy link
Copy Markdown
Contributor

levitte commented Jan 24, 2024

Re this discussion, I clearly don't have a deeper understanding of all the ramifications, so I defer to your knowledge.

@vszakats vszakats closed this in a808aab Jan 24, 2024
@vszakats vszakats deleted the cmake-separate-curl-and-libcurl-manual-options branch January 24, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants