Fix header self-sufficiency and add compile-time check

[upsj]

This works by building a separate source file for each header that only includes this file. The PR also contains fixes for all found issues.

Fixes #412

[codecov]

Codecov Report

Merging #552 into develop will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #552      +/-   ##
===========================================
- Coverage    84.15%   84.14%   -0.01%     
===========================================
  Files          296      296              
  Lines        19838    19838              
===========================================
- Hits         16694    16693       -1     
- Misses        3144     3145       +1     

Impacted Files	Coverage Δ
omp/components/format_conversion.hpp	60.00% <ø> (ø)
reference/components/format_conversion.hpp	100.00% <ø> (ø)
include/ginkgo/core/base/range_accessors.hpp	31.57% <0.00%> (-5.27%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89f889c...4a507e8. Read the comment docs.

[tcojean]

Although GLOBs are supposed to be evil in CMake, I think this would really be a case where they can actually be useful. I think maintaining all these header lists manually can be a bother. What do you think?
I think that would also allow to have everything part of the macro, probably even without modifying our target sources to add header files.

[upsj]

The reason I didn't go for a GLOB is the following: I regularly develop on the build system where the files are being copied over from my editor via scp, but if I delete/rename a file, it's not automatically deleted -- GLOB would still find these files. But of course that's not a strong argument, since I rarely to never build with CIRCULAR_DEPS on. I am not sure how likely something like this would be in practice. Git usually takes care of this when changing branches, only maybe if somebody uses Ginkgo without git might we run into issues.

So basically the question is what's more cumbersome: Updating the header list when adding a new header or deleting files on remote when they are no longer used.

Also another advantage of listing the files explicitly: Some IDEs (Qt Creator and older versions of CLion come to mind) treat files that are listed as target sources differently or even don't show them at all.

I initially found some of these points in the discussion below this poll: https://twitter.com/lefticus/status/1245008742170554368

[tcojean]

I guess the main reason for including headers in the targets would be for VisualStudio if I understand this properly? Otherwise the GLOB approach can be put into the ginkgo_check_headers macro, in particular since this concerns only CIRCULAR_DEPS builds so is mostly CI related. I don't think the GLOB will have much (or any) side effect in that case.

Another issue with this approach is that this doesn't seem to take into account std headers and relies on indirect includes for some functionality. For example, omp/matrix/csr_kernels.hpp directly uses shared_ptr so it should have <memory>, and it also uses size_type so it should include #include <ginkgo/core/base/types.hpp>.

Maybe what we have with iwyu is sufficient, although it should be configured a bit if possible to remove some false positives. If so, I think we would have all we want/need for this. iwyu is able to find the previous issues I showed.

[upsj]

I don't have too much of a strong opinion on GLOB vs. explicit list. I also think that the missing header files in IDEs were mostly an issue with older versions of CMake and/or Visual Studio, so that shouldn't matter nowadays.

I would however argue that iwyu and this approach achieve different (albeit related) things:
The declared goal of iwyu is to "thin out" the include dependency DAG of a project. It should also notice when a file is missing an include, but I can't really see these warnings stand out in any way among the huge number of warnings we currently get. Also in my experience (which is somewhat dated though), iwyu tends to produce a large number of false-positives.
This compilation step on the other hand is supposed to explicitly fail when a file is not self-contained, i.e. including it without some missing dependencies causes a build to fail. This step should not produce any false-positives as far as I can tell.

Let me just quickly check whether we can get rid of some of the warnings...

[upsj]

So after a quick look over a few of the iwyu suggestions, it looks to me like some of them indeed have merit, especially related to standard library headers (missing that was included somewhere else, ...), but there were also a few false positives (like proposing to include internal standard library headers) and a few false negatives (like wanting to remove a header declaring kernels, probably due to the macro usage).

So to me it looks like iwyu would be able to detect some of these issues, but we can't really use it as a general-purpose tool due to the large number of false suggestions.

[tcojean]

I just found out that iwyu has quite a few pragmas which we could use to control its output significantly. I believe the issues we cannot fix with these are the false negatives. I admit though that getting the output to a usable state would take quite a bit of work since there is so much to fix, on top of having to tackle the bad suggestions it proposes.
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md

[upsj]

I moved from explicitly listing the headers to GLOBs now ;)
Regarding IWYU, while it might be possible, I believe this is too much work for too little pay-off in terms of build time.

[pratikvn]

LGTM!

[tcojean]

LGTM. At some point in the future I would like to move to iwyu for this, but this is fine for now.

[tcojean]

This is just a comment, but I think this target can be useful at some point since that would allow for example to use it to create the main ginkgo.hpp file, or to install our headers in a specific location. It's a good thing to have that available now.

[thoasm]

In general, it looks good to me and also works good on my system.
However, I have 3 questions / requests for it:

1. Can we make it a cached variable in CMake (easier to change it with ccmake and also an additional documentation for users)?
2. Can you add documentation for it?
3. Can we add this to the CI, so we catch some errors centralized?

[upsj]

This is already part of the no-circular-deps pipeline. I will add comments to the INSTALL.md and CONTRIBUTING.md

[thoasm]

LGTM!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

The version of Java (1.8.0_121) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here