cmake: Fix the option to disable man pages #2161
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes the handling of the `FREECIV_ENABLE_MANPAGES` option, leveraging the fact, that an `option` call is a no-op if the value is already set. While here, allow users on non-Unix systems to build manpages. Closes longturn#2155
|
Well I started as non-fluent in Cmake too and I've got a lot of changes made over time. It grows on ya. Thanks for the PR. I'll have a look soon. |
lmoureaux
reviewed
Jan 20, 2024
There was a problem hiding this comment.
I think we need to consider the following cases (assuming UNIX to simplify):
- The user doesn't configure anything special and has sphinx. In this case we want to build man pages. This is working.
- The user doesn't configure anything special and doesn't have sphinx. In this case we don't want to build man pages. This is working. (I think the option should still be defined in this case though, just default to
OFF.) - The user sets
FREECIV_ENABLE_MANPAGEStoOFF. In this case we don't want to build the pages. This is working. - The user sets
FREECIV_ENABLE_MANPAGEStoONand has sphinx. In this case we want to build man pages. This is working. - The user sets
FREECIV_ENABLE_MANPAGEStoOFFand doesn't have sphinx. In this case we want to build man pages but we can't and should emit an error at configure time. Currently we silently ignore the option.
To support 5 one would simply call find_package(sphinx REQUIRED) whenever FREECIV_ENABLE_MANPAGES is ON.
|
@blabber see @lmoureaux's comments. Let's support option 5 in the PR to make it complete. |
If the user requests to build manpages by setting `FREECIV_ENABLE_MANPAGES` to `ON`, `sphinx` is now treated as a hard dependency. While here simplify the code and re-add the default option set to `OFF`, as requested per review.
lmoureaux
approved these changes
Jan 21, 2024
|
Thanks for the fix! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fxes the handling of the
FREECIV_ENABLE_MANPAGESoption, leveraging the fact, that anoptioncall is a no-op if the value is already set.We also allow users on non-Unix systems to build manpages (for whatever reason one may want to do that).
Closes #2155
This needs a thorough review as I am non-fluent in cmake and don't have non-Unix systems available to test.