CMake improvements discussion

[jtxa]

Let's discuss several CMake topics in detail.

Let's start with those prototyped by @oceanofthelost and those thing already there

[jtxa]

The filelists don't need a ${CMAKE_CURRENT_LIST_DIR}/ prefix.

rationale: readability, simplification
Since CMake v3.13 this also works for subdirectories, see CMP0076.
I don't think we will need to support older versions anytime soon.

[oceanofthelost]

Your right, was doing out of habit. Removed in a new commit.

[jtxa]

Include path shall be added to srecord library. Executable shall depend on it. No need to handle it per exe.
Maybe needs some GenEx like $<BUILD_INTERFACE:...>.

rationale: Modern CMake

[jtxa]

Same goes for test executables.

[oceanofthelost]

Latest commit to branch tests this.

by setting:
target_include_directories(srecord PUBLIC ${CMAKE_CURRENT_LIST_DIR})

The include directory path will propagate to targets including library.

[jtxa]

Use CMAKE_RUNTIME_OUTPUT_DIRECTORY instead of setting each property separately.

rationale: readability, simplification

P.S.
And use the srecord root dir like this (we want it in toplevel of srecord, this way we don't need to care what is the current dir):

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${${PROJECT_NAME}_BINARY_DIR}/bin")

[oceanofthelost]

Setting CMAKE_RUNTIME_OUTPUT_DIRECTORY has the side effect of all executable being placed under bin. I had thought it better to have just the primary executables (cat, cmp, and info) here.

[oceanofthelost]

I guess it would not mater, when creating a package would correctly pull the correct executables from ${${PROJECT_NAME}_BINARY_DIR}/bin

[jtxa]

It should not matter and it may be convenient if you want to call them manually. The files have a clear prefix, and installation is handled independently.

If preferred the test/CMakesLists.txt could define set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin").

[oceanofthelost]

The one setting, set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${${PROJECT_NAME}_BINARY_DIR}/bin") will be fine. I tested and like you said, when creating a release, the specific files we need will be added and where files are located in the build folder will not mater.

[jtxa]

Dependency to gcrypt:
Should be added only once to srecord library as PUBLIC. Not need to handle it per exe.

rationale: readability, simplification, Modern CMake

[oceanofthelost]

Original intent was to follow what was done with gcrypt inked for each.

What is the rationale behind gcrypt? On my system I do not have gcrypt and tests pasts. Are we missing tests? Is gcrypt needed?

[jtxa]

GCrypt is used for hashes like SHA1, and is optional.

The tests, like 178, checks the error message string in case of failure and accepts missing libgcrypt.

Probably the manual should state for the relevant command line switches, that it depends on usage of libgcrypt.

[oceanofthelost]

I would like to have some way to have a test suite for SRecord when compiled with gcrypt. If installed, then tests are run otherwise skip the tests.

When testing the last few days I use the output of ctest to ensure my changes do not effect the build, but in a case like the above changes may.

[jtxa]

What's the benefit of adding ccache, is it worth it?
On my 5 year old computer it takes 10s for a full build. Even on single threaded GitHub runner it's about 25s.
For development you don't need full builds (at least with Ninja).

rationale: complexity, maintainability

[jtxa]

Tried it out: 13s for an initial full build, then 0.5s for new full builds of cached versions. Impressive.
But for Linux (and maybe MSYS2) it's enough to call cmake with -D CMAKE_CXX_COMPILER_LAUNCHER:STRING=ccache

Settings for pch and time_macros are irrelevant today (the clang and Ninja/make settings). MSVC and Visual Studio are currently irrelevant, because the code does not even fully compile. So only the special XCode variables seems to be left.
Is it even OK, to copy'n'paste the code from the book? Is there some repo where a maintained version could be loaded from (e.g. via FetchContent)?

[oceanofthelost]

The following of from the first page of the "Professional CMake"

This book or any portion thereof may not be reproduced in any manner or form without the
express written permission of the author, with the following specific exceptions:

- The original purchaser may make personal copies exclusively for their own use on their
  electronic devices, provided that all reasonable steps are taken to ensure that only the original
  purchaser has access to such copies.

- Permission is given to use any of the code samples in this work without restriction. Attribution
  is not required.

The advice and strategies contained within this work may not be suitable for every situation. This
work is sold with the understanding that the author is not held responsible for the results accrued
from the advice in this book.

Under the second bullet we should be okay. The above should appear in the UseCompilerCache.cmake

As far as I know, no maintained version exists, just what is in Appendix B.

[oceanofthelost]

I enjoy CMake as on my machine it can take upto a minute to build without a cache and less than 10 S with a cache. It makes the build/rebuild cycles quicker.

Having the option guarded by ${PROJECT_NAME}_ENABLE_COMPILER_CACHE defaulted to OFF, the user can then choose if they want to enable to not.

[jtxa]

Given that it's a separate file, only enabled if needed and the copyright is valid, it should be OK.

[oceanofthelost]

I went ahead and added the above title block to UseCompilerCache.cmake.

Should I create a PR to add ccache support?

[jtxa]

In PR !32 I added a target_compile_features(lib_srecord PUBLIC cxx_std_11), otherwise it didn't compile on MacOS anymore. This sets the minimum C++ version to be used.
There should be no other compiler settings done in the CMake files.

We should test with different settings to detect warnings (not implemented at the moment, just using compiler default).
And setting variables like CMAKE_CXX_STANDARD would overwrite this.

I think providing a profile and/or toolchain files should be better. They could be used in CI and by users/developers. The profiles should even be supported by IDEs directly to choose what you want to compiler.

[oceanofthelost]

The settings:

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

Should set the compiler specific flags for C++ standard without interfering with other flags. I can test on this on some platforms and report back.

For warnings flags this is a bit more tricky as most compilers have different sets of flags. There are some CMake templates that try to address this. Maybe we pull these templates in as needed?

[jtxa]

If the compiler default is not Std C++17, CMake will add a command line switch.
If I add C++11 command line switch like here, I don't think it's deterministic, which one will win.
And as this is a variable, I cannot overwrite it without changing the file.

That's why I suggest to not make any such settings inside the normal files, but provide useful profiles/toolchain files. With those you can provide settings for many different and contra dictionary variants. Like coverage test, sanitizer, ValGrind config, optimized release, different C++ languages, different compilers. Whatever we need for development or CI.

[jtxa]

About warnings:
There is progress in CMake v3.24, they added CMAKE_COMPILE_WARNING_AS_ERROR.

I don't think we need anything complex, SRecord currently compiles probably only on GCC and CLang. And for those I made it warning free with -Wall -Wextra -Wpedantic (ATM only checked for CLang in QA-workflow, needs a GCC too).
When VC++ will be working someday, it will also be warning free on highest /W4.

I would simply provide a toolchain file for each of those compilers, with the flags suggested. Everything beyond those settings are even more compiler specific, and doesn't seem to be handed by that script (also no other compilers).

[oceanofthelost]

Having tool chain files would work ans make explicit what SRecord is built against.