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: rework options to enable curl and libcurl docs #12773

Conversation

vszakats
Copy link
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
Copy link
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

CMakeLists.txt Outdated Show resolved Hide resolved
lib/curl_config.h.cmake Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@levitte
Copy link
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
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
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
Member Author

Thanks, committed these updates.

@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
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
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.

None yet

2 participants