Fix: Local Includes with "" in Lin. Solvers MLMG

[ax3l]

Summary

This fixes AMReX-local includes inside LinearSolvers/MLMG/ to use include "AMReX_<...>.H" instead of <...>. Using quotes is important here for isolation, since compilers will otherwise look on the system first, will then potentially find an (outdated or incompatible) installation of AMReX, will mix it in and cause sometimes really hard to debug compile (or even runtime) issues, since text inclusion is a copy-paste operation.

Additional background

Free after the LLVM project's include guidelines and the "Include-What-You-Use Project":

• Use "" for AMReX-local includes
• Order:
  • AMReX includes with ""
  • other third party includes with <>, e.g. MPI, OpenMP, etc.
  • last: std libraries with <> (so one catches missing includes early)

Checklist

The proposed changes:

• fix a bug or incorrect behavior in AMReX
• add new capabilities to AMReX
• changes answers in the test suite to more than roundoff level
• are likely to significantly affect the results of downstream AMReX users
• are described in the proposed changes to the AMReX documentation, if appropriate

[WeiqunZhang]

We probably should reorder the headers. But I don't think we should change from <> to "". We use -I explicitly. I don't think compilers will search for the system directories before the paths specified by -I. And we never compile inside any directories inside Src/. So it does not matter that "" searches the local directory first. In one case with Castro or Maestro, "" actually caused an issue. But I cannot remember what the issue was. Maybe @maxpkatz, @zingale or @harpolea remembers.

[maximumcats]

The issue was that we specifically did want versions of the headers in the local build directory to override the "true" Source/ copy, since that's a code pattern we use in Castro -- provide a header file with an empty definition of a function and then allow the user to copy the header and override it in their problem setup, to implement something like a problem-specific physics source term.

[ax3l]

Thanks!
@maxpkatz do you use this pattern generally, including for AMReX files, or is this specific to the Castro code base?

[maximumcats]

I don't use it on files from AMReX. I would be fine if AMReX didn't implement that code pattern, and I do not really see a use for it within AMReX.

[ax3l]

I don't think we should change from <> to "". We use -I explicitly. I don't think compilers will search for the system directories before the paths specified by -I.

The problem should hopefully only arise with sub-directories, but I cannot vouch for each all compilers and IIRC it's not uniformly defined. (GCC: docs; Clang: docs)

sudo -s
echo "#error ohoh" > /usr/include/header.H
echo "#pragma once" > header.H

cat <<EOF >> main.cpp
#include <header.H>

int main()
{
   return 0;
}
EOF

g++ main.cpp
# error
g++ -I$PWD main.cpp
# ok
g++ -isystem $PWD main.cpp
# ok

I just had recently such an issue again with AppleClang on macOS: openPMD/openPMD-api#640

Adding the local "" headers would definitely be more robust in new environments, I would say. Also with respect to alternative build systems and package management that might manipulate and symlink include paths around. For example, a downstream dependency might have -isystem and -I passed to various locations.

[WeiqunZhang]

One could argue it makes the code more robust if the user has headers named "AMReX_*.H" and it's put in the local build directory. But we consider AMReX_ is reserved and we do not use relative path like #include <a/b.h> in amrex.

[WeiqunZhang]

What I am trying to say is I cannot imagine a realistic case where #include "AMReX_MLMG.H" works but #include <AMReX_MLMG.H> does not. If the compiler searches system directory before the paths specified by -I, use "" will not help.

[ax3l]

Distinct prefix in all files and explicit flat structure makes it robust, agreed.
The "" would address a clean separation between a system-wide (or package-manager environment-wide) install of AMReX in conjunction with local directories. Most issues I saw in the past were collisions with an older version of oneself; I am not driven by a concrete problem with AMReX builds atm.

We can certainly wait if something manifests at some point, too - and only change the include order so long.

[WeiqunZhang]

We can certainly wait if something manifests at some point, too - and only change the include order so long.

Sounds good.

[ax3l]

No probs, includes in #1496.