CMake: improvements and formatting

[jtxa]

Discussion about possible improvements

• [ ] local configurations: move all doxygen things into 1 file, move config headers to library
• [ ] Make doyxgen optional (and depending if tools found)
• [ ] Make website optional (and depending if tools found)
• [ ] Make PDF optional (and depending if tools found)
• [ ] Make documentation optional (and depending on tools found)
• [ ] Make LIB_GCRYPT fully optional (library linkage, test selection)
• [ ] With optional documentation it should be possible to build srecord on any C++11 aware system (even native Visual Studio)
• [ ] With optional documentation less tools needs to be loaded in some Workflows
• [ ] project name: SRecord, use SRecord as prefix for global things (either SRecord_ or SRecord::)
• [ ] Naming of lib target: srecord (lib prefix is added by CMake)
• [ ] Introduce Aliases: SRecord::srecord, SRecord::srec_cat, SRecord::srec_cmp, SRecord::srec_info
• [ ] Build srecord library STATIC or DYNAMIC (introduce cache var SRecord_BUILD_SHARED_LIBS, default defined by BUILD_SHARED_LIBS)
• [ ] choose the right package format (DEP/RPM) by default
• [ ] generate official binaries to bin/ in build dir (there's a variable to control that)
• [ ] avoid include_directories, use target_include_directories
• [ ] explicit filelists
• [ ] Introduce toolchains used in Workflows and for developers
• [ ] Introduce profiles used in Workflows and usable for developers

Tooling

There is some tooling: cmake language tools interesting tools contained: lint, format

Haven't used it myself yet, don't know how widely used and mature it is. Don't know what the formatter can do.

Discussion about format

Indentation

# now (sometimes):
add_doc(abc
        def)
# suggested:
add_doc(abc
    def)

rationale: No manual extra formatting of single spaces. Less effort to write.

closing parentheses

# now (sometimes):
add_doc(abc
    def)
# suggested:
add_doc(abc
    def
)
# alternative:
add_doc(abc
    def
    )

rationale: Better visibility where it ends. Easily insert something at the end, without extra care for parentheses.

multi-line function calls

# now (breaking at max width):
add_library(tgt KEY1 KEY2 value KEY3 value
    KEY4 KEY5 value KEY6 value)
# suggested (new line for every argument):
add_library(tgt # just the positionals, or most important on first line
    KEY1
    KEY2 value
    KEY3 value
    KEY4
    KEY5 value
    KEY6 value
)

rationale: readability, avoids merge conflicts

file lists (for multi-line function calls)

# now
add_custom_target(tgt
    DEPENDS file1 file2 file3)
# suggested:
add_custom_target(tgt
    DEPENDS
        file1
        file2
        file3
)
# alternative:
add_custom_target(tgt
    DEPENDS
    file1
    file2
    file3
)

rationale: readability, avoids merge conflicts

This could also be applied to COMMAND and others. For file lists with more than 1 file I would always do this, for other things I can't say this in general. I do an extra indent for values, don't know if the formatter can do such things.

Ideas to consider when writing CMake files

Just apply if really help in readability, not in general.

strings quotation marks

# example:
set(var ${a} ${b}_${a} ${b}${a} ${b}.${a})
# may be more readable
set(var "${a}" "${b}_${a}" "${b}${a}" "${b}.${a}")

rationale: readability (only with syntax coloring), easier to identify a single value

strings bracket argument

# example:
set(CPACK_SOURCE_IGNORE_FILES
  /\\.git/
  \\.gitignore
  \\.swp ...
# no extra quoting needed:
set(CPACK_SOURCE_IGNORE_FILES
  [[/\.git/]]
  [[\.gitignore]]
  [[\.swp]] ...

rationale: avoid extra backslashes, e.g. in RegEx drawback: inside bracket argument variables are not expanded (not relevant here)

[sierrafoxtrot]

I like the suggested format for Indentation, Closing Parens, multi-line function calls and file lists. I especially like the extra indent for values. Clearly shows a hierarchy.

# suggested:
add_custom_target(tgt
    DEPENDS
        file1
        file2
        file3
)

Embarrassed to say that it had never occurred to me that a new line for each arg could help prevent merge conflicts.

Regarding the args, I don't really see the value in forcing folks to string quote them. Am I missing something?

[sierrafoxtrot]

Somewhat of a meta-question, would these discussions be helped by using the discussions forum? I'm new to github (coming from gitlab, sourceforge and others) but I like the idea of having a chat about an approach before creating issues.

I guess the alternative is that we peel each of these topics off into a separate issue before this one becomes a multi-threaded discussion.

[jtxa]

Embarrassed to say that it had never occurred to me that a new line for each arg could help prevent merge conflicts.

In fact its irrelevant for SRecord, given its maturity. I do this also for array etc. in all programming languages. At my work if happens quite often that the same list is changed, and this way the risk to touch the same line is lower. Results in less conflicts, which have to be solved manually.

Regarding the args, I don't really see the value in forcing folks to string quote them. Am I missing something?

It wasn't meant that way, to enforce it. Perhaps I shouldn't even mention it here in this context. I updated the text and the first example. I hope, with that one my intention is more understandable. I guess there no such example in SRecords code, so it might not be relevant at all.

Somewhat of a meta-question, would these discussions be helped by using the discussions forum? I'm new to github (coming from gitlab, sourceforge and others) but I like the idea of having a chat about an approach before creating issues.

Agree. Github discussions is fairly new (just about 1 year), so it wasn't on my mind. We can move to any forum you prefer.

[oceanofthelost]

Something to consider is what generators to support. CMake supports single-target (Ninja, Makefile) and multi-target (XCode, VSCode).

I believe SRecord should support only single-target, which will then simplify some CMake rules.

To terminate cmake during configuration, something like the following can be

if(GENERATOR_IS_MULTI_CONFIG)
  message(FATAL_ERROR "Multi target builds not supported" )
endif()

[sierrafoxtrot]

Keen to know more. Why would you suggest excluding multi-target builds?

[oceanofthelost]

Keen to know more. Why would you suggest excluding multi-target builds?

After doing some more digging I mixed up multi config and multi target builds. Two generators, XCode and VSCode, support selecting a configuration to build from the GUI itself, other generators such as Make and Ninja, support single config (well new versions of CMake this is not completely true with Ninja). In multi config builds, one CMake invocation will generate all build configurations. By default (excluding VSCode and XCode), only a single config is generated.

I would say for now do not support multi config builds, as in this case the user would be building SRecord in a IDE and not at the terminal. In this multi config build, the CMake variable CMAKE_BUILD_TYPE would be set to "", which currently the current CMake based build system.

I am going o dig into this a bit more to see ways we can support multi configs without having the user need to call CMake to regenerate the configuration.

[jtxa]

Not sure which problem you want to avoid exactly. The current cmake files do not depend on CMAKE_BUILD_TYPE anywhere. There is no extra effort for multi-config, and it works already for SRecord (I did a short test on Ninja Multi-Config). In my opinion people shall use whatever they want, that's one of the benefits of CMake.

Single config is not the default of CMake, each generator is either this or that. On Linux, make is the default generator, but on Windows it's Visual Studio (the commercial one, not Code). And that's multi-config. I did try out Visual Studio with VC++ once. It worked, but could not be compiled fully, because unistd.h is a POSIX header.

CMake has no generator for VSCode. The CMake tools in VSCode allows you to select a predefined Kit and then calls CMake with make or ninja or whatever (don't know the details). Some Ninja Multi-Config support seems to haven been implemented in 2022.

[oceanofthelost]

I prototyped an updated CMake which addresses some of the improvements. If your interested can take a look within my fork of the repository.

[jtxa]

That's great. But I have seen some thing I might dislike or needs to be discussed. I opened #40. In contrast to these issues (all replies at the end), on discussions it's possible to answer on separate threads instead. So we could do better discuss multiple topics there. Please give me some time to go through more changes. I just start with some simple ones.