Headercheck??

changed milestone to %New CMake Build-System

The idea is to check that header are 'valid' or 'consistent', i.e., can be for example included on their on to avoid problems caused by missing includes, leading to a dependency on the order in which headers have to be included. If I remember correctly the headercheck also checks if headers are correctly added to the install target but I'm not sure if this was only for the autotools version. There might have been some additional consistency checks that I missed.

I use it. It helps to keep headers tidy: include guards, includes, that all headers compile.

The idea is to have a simple cpp file that includes the header twice. This is done for every header, that is not explicitly excluded.
It worked like a charm with Autotools but takes some time to get the helper files set up with CMake. That is the reason why it is deactivated by default.

It was documented in the buildsystem.pdf. This is gone, maybe you can still find it in Dune 2.4 tarball.

I would like to keep it. It would be great if we could improve it with modern CMake.

In the dune-common-2.4 package buildsystem.pdf documentation, I've found the sentence: "headercheck: This target tries to make sure that your header files can be parsed and are self-contained.", but the corresponding paragraph just says: "The headercheck target: TODO...". So, up to the one-sentence summary that essential does not say anything specific, there is not documentation.

Isn't this check that "header files can be parsed and are self-contained" the task of an external tool, like clang-tidy or include-what-you-use or cpplint or any of the thousand helper tools existing out there? Those tool can be added to cmake with essentially just 1 line:

cmake -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=include-what-you-use

or

cmake -DCMAKE_CXX_CLANG_TIDY=clang-tidy

Nowadays you would indeed use such a tool. Historically, headercheck predates both clang and iwyu.

I support switching to an external tool. But we should still define one tool and provide infrastructure for the tool, i.e., we currently have a macro that excludes headers from headercheck. Without such macro, we get false positives and nobody would be willing to run headercheck anymore.

Since this is optional and off by default I would like to keep it. Really does not hurt anybody. If you want to use an external tool, feel free to do so. I'm not gonna stop you.

Headercheck has it flaws. It is slow and not well maintained as few people are using it on a regular basis. We want to prepare a contemporary headercheck replacement. Once we are done, we will ask everybody for comments and try to get it integrated; similar to the logging library that was not (yet?) approved.

Would this mean that the headercheck could only be run on a Dune build with clang or what sort of other external tool are you suggesting?

(Maybe this was an old answer due to the by-mail-answering issues)

I was just looking into the cmake code and tried to understand the different modules and what they are doing and why. Headercheck is one of the cmake modules without any documentation, deactivated by default, with a difficult to read implementation. So I was checking whether this is just a lost old code or actually used. And if used, is it used just for historic reasons or might there be a better alternative.

It is not just about lost code, I looked into this because headercheck is included in several other cmake functions and I wanted to understand the consequences - so if another function is modified, what could be the effect on the headercheck feature. Would it even be possible to move it out of these functions.

I made just a quick search for alternatives.

"include-what-you-use" is a clang-based tool for the parsing of the code. It does actually do something different than checking that the headers compile successfully. It tells you which headers are missing and which headers are unnecessary/redundant.
"Precompiled-headers" might also be an alternative. They serve also a different purpose. But you get the same effect - headers are somehow compiled. Additionally, you will get reduced compile times for sources using any of these headers.

As far as I can get from the discussion above, the headercheck feature is used, but it is an expert tool only for developers. So, I will not touch it, but just advertise here these alternatives that could be tried out.

mentioned in issue #199 (closed)

Just tested ENABLE_HEADERCHECK in dune-common and it fails in the dune/python headers directory.

What is your setting, e.g., python bindings off? make headercheck ends for me locally in dune-comnmon with

Scanning dependencies of target headercheck
Headerchecks finished! Rerun CMake if a new file has not been checked!
[100%] Built target headercheck

Just checked with Python bindings disabled Python.h is not found included in the pybind11 headers

pybind11 probably needs to be excluded from headercheck

I tried that but I can't get it to work. I'm not quite sure why it is adding headercheck targets for the python subfolders anyway since in dune/CMakeLists.txt we have

if(DUNE_ENABLE_PYTHONBINDINGS)
  add_subdirectory(python)
endif()

I tried to add to the CMakeLists.txt file below dune/python the following

if(NOT DUNE_ENABLE_PYTHONBINDINGS)
  exclude_dir_from_headercheck()
endif()

but it still generates the headercheck targets.

This does not work, because this call must be inside the common directory and any directory that potentially has a *.hh file

But, unfortunately, this does also not work. No idea why.

Ah ok, I see, headercheck is implemented by a recursive globing of all files. Since the subdirectory python is not added to cmake, the exclude function is not called. Thus, all files are added to the header check, and it cannot be excluded.

In order to fix the headercheck, you could either

rewrite headercheck, i.e., call setup_headercheck and finalize_headercheck explicitly instead of implicitly by dune-project and thus only include directories that are chosen by the location of the setup_headercheck call.
Or, you could remove the conditional include

if(DUNE_ENABLE_PYTHONBINDINGS)
  add_subdirectory(python)
endif()

and always include this subdiretory + protect all cmake lines in this subdirectory and all its subdirectories explicitly. Additionally add the

if(NOT DUNE_ENABLE_PYTHONBINDINGS)
  exclude_dir_from_headercheck()
endif()

must be called from all subdirectories where you expect something to fail if python is not available.

Or, you can manually exclude all files from subdirectories of dune/python by calling a function from a parent directory that is included by cmake. Therefore, you have to add a new exclude-function to the cmake file, since the current ones only work in the current-directory.

In my opinion, only option 1 is maintainable. This is what I would suggest for all dune modules that want to use headercheck: make it explicit.

Instead of option 1, it could also be implemented by inverting the current behavior: Instead of adding all files to the headercheck target and excluding manually, one could add only those files explicitly selected. Actually, then we are close to the precompiled-header feature of recent cmake.

Inverting the current behavior is a bad idea. By default I expect that headercheck passes for new headers. If not, it should be fixed. Exclusion should be an active choice.

I agree that switching things around would defeat the purpose in parts.

In fact testing that new headers have been added to the 'install' target as we had it in autotools would be nice to get back again. A header missing in the installation often doesn't get found out and when it is, it will often be by new users which is annoying because the whole package has to be rebuild to fix it.

Missing headers are tested in the system test and wrong install headers are tested by activating install in the ci as well. The first one tests that the installed module can be used by other downstream modules. If a header file is not included in the install list and not included in any file/test in a downstream module then in fact it is not detected. Probably then a test is missing. A file that is not used in any test is not a good idea either. @santiago.ospina suggested even a way to use the tests itself as installation tests by configuring them like an external module. This does not yet work, though, but would make the system tests even more complete.

I like to be explicit about things. Implicit collection of files in subdirectories without really control about it is difficult. Things fail and one has no glue why. Being explicit does not need to be more work or less complete. This is just a way of organizing your code. But at least no surprise happens. For example, the headercheck tool creates a new target with the simple name "headercheck" every time I call setup_headercheck(), actually it is called automatically by dune_project. Thus, I cannot call this function again without crashing by cmake build. Even if I have not activated the headercheck feature.

@santiago.ospina suggested even a way to use the tests itself as installation tests by configuring them like an external module. This does not yet work, though, but would make the system tests even more complete.

In case you are interested, here is the line that does the magic:

# test/CmakeLists.txt
if(NOT TARGET dune-copasi::dune-copasi)
  find_package(dune-copasi REQUIRED IMPORTED COMPONENTS Libraries Development Runtime)
endif()

# Use target
target_link_libraries(dune-copasi-test PRIVATE dune-copasi::dune-copasi)

In this case, the target dune-copasi::dune-copasi could come from a parent directory or could be found using the find_package function. The idea is that you have a CI job that makes the following steps:

Performs full installation of your main project (e.g. dune-copasi)
Removes the build directory
Creates another build for a test project (e.g. dune-copasi-tests) which finds with the installed package (e.g. dune-copasi).
Runs the test.

If this passes, it means the following:

Your package is installable.
Your package is findable.
Your package is usable. (This is of course tied to the quality of the tests themselves)

This is meant to be done in the CI because creating another build locally is too much hassle; that's why the test folder may also work as a subdirectory.

mentioned in merge request !955 (merged)

I added a branch !955 (merged) that fixes this by providing a cmake command that removes all headers below a given folder. This at least fixes things for now.

Although I don't like the headercheck feature, I respect that others are using it. However, because it is always turned on, this induces some difficulties with improving the rest of the build system. With "always turned on" I mean that a corresponding target is always created when you use dune_project() and all directories in your project are searched recursively for files with .hh extension. But, at the end, this information is only used when ENABLE_HEADERCHECK flags is set to true. So, I suggest, to turn off these functions completely, i.e., make them a no-op, in case headercheck is not enabled.

Example:

macro(setup_headercheck)
  if(ENABLE_HEADERCHECK)
    # ...
  endif()
endmacro()

The consequence is: There will not be a target headercheck created and you cannot call make headercheck if you have not enabled the headercheck feature.

This even seems like an improvement to me - I didn't realize that calling make headercheck with disabled header-check didn't produce an error; I would probably have expected that. If I'm not missing something, there doesn't seem any downside to me.

mentioned in merge request !1512 (merged)

mentioned in issue #405

Headercheck??

Designs

Child items ...

Activity