Skip to content

[CMake] Overhaul; fix several issues#217

Merged
karastojko merged 22 commits into
karastojko:masterfrom
Spixmaster:master
Mar 24, 2026
Merged

[CMake] Overhaul; fix several issues#217
karastojko merged 22 commits into
karastojko:masterfrom
Spixmaster:master

Conversation

@Spixmaster

@Spixmaster Spixmaster commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

Update my initial pull request, #193.
With learnings from lexbor/lexbor#337.
No build file generated with export() which is installed.

Also fixed this error, https://stackoverflow.com/questions/25676277/cmake-target-include-directories-prints-an-error-when-i-try-to-add-the-source.

Update my initial pull request,
#193.
With learnings from lexbor/lexbor#337.
No build file generated with `export()` which is installed.
File needs to be installed.
Possible fixes.
Fix.
CMake fixes.
Fix.
Fix.
This reverts commit 43b7040.
Update to CMake version 3.30 to get rid of [this warning](https://stackoverflow.com/questions/79146083/finding-boost-without-cmake-find-module-cmp0167).
 Properly quote variables according to [this](https://stackoverflow.com/questions/35847655/when-should-i-quote-cmake-variable-references).
There were incosistencies with either `else()` or `else(something)`. Same is true for `endif()`. I adjusted.
Use `_CURRENT_` versions of directories, see [here](https://cmake.org/cmake/help/latest/variable/CMAKE_CURRENT_SOURCE_DIR.html).
Use placeholders in `mailio-config.cmake.in`.
Apply [linter and formatter](https://github.com/cheshirekow/cmake_format).

`MAILIO_DYN_LINK_TESTS` makes no sense without `MAILIO_BUILD_TESTS`. Fixed.
`CMAKE_THREAD_PREFER_PTHREAD` does not exist. I know of the [StackOverflow question](https://stackoverflow.com/questions/5395309/how-do-i-force-cmake-to-include-pthread-option-during-compilation) but the [documentation](https://cmake.org/cmake/help/latest/module/FindThreads.html) does not contain it (anymore). Fixed.
The dependencies were not well defined but rather propagated from the base target via scope `PUBLIC` which is not good practice. I reorganised and only used what is needed.
In `set(DOXYGEN_OUTPUT_PATH ${DOCOUTDIR})`,  `DOCOUTDIR` does not exist. Does it even make sense when `set(DOXYGEN_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/docs/mailio)` is executed before? I removed it.
`configure_file()` has no `IMMEDIATE`. Removed.
Use existing varialables instead of defining them for input of `configure_file()`.
`version.hpp.in` uses `PROJECT_VERSION_...` instead of `mailio_VERSION_...` to make project renamings easier.
Tests and examples were installed even twice. Once in the `CMakeLists.txt` and the individual files `test/CMakeLists.txt` and `examples/CMakeLists.txt`. Removed.
Fixes.
Fix.
Fix.
@Spixmaster

Spixmaster commented Feb 12, 2026

Copy link
Copy Markdown
Contributor Author

The fix turned into an overhaul.

The reason was the following. There were errors like this and variables were not used and thus unnecessarily declared which contributed to the build system not being organised and making corrections really hard.

General corrections

  • Update to CMake version 3.30 to get rid of this warning.
  • Properly quote variables according to this.
  • There were incosistencies with either else() or else(something). Same is true for endif(). I adjusted.
  • Use _CURRENT_ versions of directories, see here.
  • Use placeholders in mailio-config.cmake.in.
  • Apply linter and formatter.

Specific corrections

  • MAILIO_DYN_LINK_TESTS makes no sense without MAILIO_BUILD_TESTS. Fixed.
  • CMAKE_THREAD_PREFER_PTHREAD does not exist. I know of the StackOverflow question but the documentation does not contain it (anymore). Fixed.
  • OpenSSL::Crypto was not needed. Fixed.
  • In set(DOXYGEN_OUTPUT_PATH ${DOCOUTDIR}), DOCOUTDIR does not exist. Does it even make sense when set(DOXYGEN_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/docs/mailio) is executed before? I removed it.
  • configure_file() has no IMMEDIATE. Removed.
  • Use existing variables instead of defining them for input of configure_file().
  • version.hpp.in uses PROJECT_VERSION_... instead of mailio_VERSION_... to make project renamings easier.
  • Tests and examples were installed even twice. Once in the CMakeLists.txt and the individual files test/CMakeLists.txt and examples/CMakeLists.txt. Removed.
  • Applied check_required_components("@PROJECT_NAME@"), reason.
  • cmake_dependent_option() is completely useless here and even gives false
    impression of compatibility checks. The function basically just defines default
    values and toggles visibility in the GUI,
    CMakeDependentOption. It does not prevent from configuring incompatible settings. I tested it.

To consider

I am aware of file doxygen.conf.in. It is not used currently so I did not set the needed variables in CMake. I would advise against that and use and independent Doxygen configuration file.

Tests

Everything is backwards compatible.

I tested to build which works.

I also imported the created target in on of my applications which worked too.

@Spixmaster Spixmaster marked this pull request as ready for review February 12, 2026 14:51
@Spixmaster Spixmaster changed the title Improve CMake target export installation [CMake] Overhaul; fix several issues Feb 12, 2026
Change section order.
@Spixmaster

Copy link
Copy Markdown
Contributor Author

Fixes or closes #155 and #126 (should not be closed yet).

`cmake_dependent_option()` is completely useless here and even gives false
impression of compatibility checks. The function basically just defines default
values and toggles visibility in the GUI,
https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html.

It does not prevent from configuring incompatible settings. I tested it.
@karastojko

Copy link
Copy Markdown
Owner

Thanks for the PR. It works on Gcc/Linux but fails with Msvc/Windows, with the same CMake 3.31.3. It complains that it cannot find the Boost path although nothing has changed compared to the previous setup. Let me dig what is going on.

@Spixmaster

Copy link
Copy Markdown
Contributor Author

Thanks for your general positive attitude towards this pull request. I would like to assist in this but I do not use Windows.

@karastojko karastojko merged commit 0c462c4 into karastojko:master Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants