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

[build] Make building documentation configurable #672

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

Artturin
Copy link

@Artturin Artturin force-pushed the optional-docs branch 2 times, most recently from e25bd8f to d9e92e9 Compare July 12, 2024 15:48
@Artturin
Copy link
Author

There seems to be a random segfault issue going on in the tests
https://github.com/ngs-lang/ngs/actions/runs/9910596236/job/27381341997?pr=672#step:4:270 (darwin)
Previous CI run https://github.com/ngs-lang/ngs/actions/runs/9910403102/job/27380707730?pr=672 (arch)
I got it once locally(NixOS) but after rebuilding it worked.

Copy link
Contributor

@ilyash-b ilyash-b left a comment

Choose a reason for hiding this comment

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

  • See inline comment
  • I'm thinking of 3 modes switch: build, don't build, build if pandoc is available (default). What do you think?

CMakeLists.txt Outdated
@@ -125,7 +126,14 @@ add_custom_command(

target_link_libraries(ngs m Threads::Threads ${CMAKE_DL_LIBS} ${LIBGC_LIBRARIES} ${LIBFFI_LIBRARIES} ${JSONC_LIBRARIES} ${PCRE_LIBRARIES} ${Backtrace_LIBRARY})

add_custom_target(man ALL WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/doc COMMAND make man DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/doc/*.1.md)
if(BUILD_DOCS)
Copy link
Contributor

Choose a reason for hiding this comment

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

More appropriate would be BUILD_MAN I think.

@Artturin
Copy link
Author

* I'm thinking of 3 modes switch: build, don't build, build if `pandoc` is available (default). What do you think?

Build if available is not easy to do in cmake.

find_program(PANDOC pandoc)

Outside if (BUILD_MAN)

Stops the build with CMake Error at CMakeLists.txt:134 (message): Could not find pandoc if pandoc isn't found

In meson it would be as simple.

pandoc = find_program('pandoc', required : get_option('man'))
if pandoc.found()
   ...
endif

And having the man option as auto

@ilyash-b
Copy link
Contributor

ilyash-b commented Jul 14, 2024

* I'm thinking of 3 modes switch: build, don't build, build if `pandoc` is available (default). What do you think?

Build if available is not easy to do in cmake.

find_program(PANDOC pandoc)

Outside if (BUILD_MAN)

This part I did not understand.

Stops the build with CMake Error at CMakeLists.txt:134 (message): Could not find pandoc if pandoc isn't found

Docs:

If the program is found the result is stored in the variable and the search will not be repeated unless the variable is cleared. If nothing is found, the result will be <VAR>-NOTFOUND.

Added find_program(PANDOC pandoc) in the middle of the file. No error. Please elaborate.

Edit: added find_program(PANDOC pandoc-not-found), not the above

@Artturin
Copy link
Author

* I'm thinking of 3 modes switch: build, don't build, build if `pandoc` is available (default). What do you think?

Build if available is not easy to do in cmake.

find_program(PANDOC pandoc)

Outside if (BUILD_MAN)

This part I did not understand.

Stops the build with CMake Error at CMakeLists.txt:134 (message): Could not find pandoc if pandoc isn't found

Docs:

If the program is found the result is stored in the variable and the search will not be repeated unless the variable is cleared. If nothing is found, the result will be -NOTFOUND.

Added find_program(PANDOC pandoc) in the middle of the file. No error. Please elaborate.

Edit: added find_program(PANDOC pandoc-not-found), not the above

My bad, I forgot to remove the

if(NOT PANDOC)
	  message(FATAL_ERROR "Could not find pandoc")
endif()

The error message had the line of the error message in it put I overlooked it

PR Ready

@ilyash-b
Copy link
Contributor

I'm thinking of 3 modes switch: build, don't build, build if pandoc is available (default).

I'm still for this design unless convinced otherwise. It should be possible to have control as follows (not solvable with single boolean flag):

  • "I need the man pages to be built". Fails if there is no pandoc
  • "I don't need the man pages to be built". Skips completely. Doesn't even check for pandoc presence.
  • "I don't mind. Build the man pages if possible". If pandoc is not available, gives a message and moves on, doesn't fail. Builds the man pages if pandoc is there. This should be the default.

@Artturin
Copy link
Author

I'm thinking of 3 modes switch: build, don't build, build if pandoc is available (default).

I'm still for this design unless convinced otherwise. It should be possible to have control as follows (not solvable with single boolean flag):

* "I need the man pages to be built". Fails if there is no `pandoc`

* "I don't need the man pages to be built". Skips completely. Doesn't even check for `pandoc` presence.

* "I don't mind. Build the man pages if possible". If `pandoc` is not available, gives a message and moves on, doesn't fail. Builds the man pages if `pandoc` is there. This should be the default.

Found the way to do it in https://cmake.org/pipermail/cmake/2016-October/064342.html

@ilyash-b ilyash-b merged commit 2556955 into ngs-lang:dev Jul 31, 2024
22 checks passed
@ilyash-b
Copy link
Contributor

Thanks!

@Artturin Artturin deleted the optional-docs branch August 5, 2024 20:35
@Artturin
Copy link
Author

Artturin commented Aug 5, 2024

🚀

@jirutka The changes in your patch are now merged (in this PR and a few others) https://git.alpinelinux.org/aports/tree/testing/ngs/cmakelists.patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants