Restore computation of include directories

[alalazo]

This PR restores the computation of the include directories that was remove as part of the hotfix in #10604 It also adds the following modifications:

• Remove the separate addition of <prefix>/include to the list of include dirs

• Add unit tests to stress the behavior of HeaderList.directories

• Implement a way for packages to customize the include dirs injected into compiler wrappers (refers to blindly adding prefix.include in compiler wrappers can break Spack #8324 or libxml2 - thanks to @scheibelp for the reference)

was WIP

[davydden]

@alalazo FYI https://github.blog/2019-02-14-introducing-draft-pull-requests/ 😄

[alalazo]

@davydden Can you please double check this PR works for the cases reported in #10601 and #10603 also on MacOS?

[davydden]

@davydden Can you please double check this PR works for the cases reported in #10601 and #10603 also on MacOS?

I will try it today evening, on the conference the whole week.

[davydden]

@alalazo

Can you please double check this PR works for the cases reported in #10601 and #10603 also on MacOS?

I can confirm that both cmake and expat (as a dependency) builds fine with this PR on macOS. I think it's ready to be merged, @scheibelp ping.

[scheibelp]

I think libxml2 is a case where this may be problematic: it defines headers in <prefix>/include/libxml2/, and at least some dependents appear to import from this directly (rather than as #include <libxml2/foo.h>). My inclination is that there needs to be a separate object/function for handling each of the following behaviors:

• Finding all headers and their directories
• Reporting the include directories that dependents should use

That being said, if dependents are split on how to refer to headers in libxml2 (e.g. some of them refer to foo.h and some to libxml2/foo.h) then there wouldn't be a libxml2-"centric" solution: the dependents themselves would have to choose how to refer to it.

See:

• https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/libxml2/package.py
• pkgconfig: Disable pkgconf and pkg-config's path stripping #10644

[davydden]

@scheibelp

it defines headers in <prefix>/include/libxml2/, and at least some dependents appear to import from this directly (rather than as #include <libxml2/foo.h>).

that's too bad. But I don't think such issues should hold this PR. I would be inclined to consider them anomalies that one can discuss how to handle or maybe convince downstream package authors to do "proper" includes (i.e. #include <foo/utilities.h is safe way compared to #include <utilities.h> as there could be a number of utilities.h provided by various packages. Frankly I don't understand why would anyone want to import headers that way...)

[alalazo]

@scheibelp Despite not being a recommended best practice, are you aware of packages that are using libxml2 with include directives like:

#include "libxml2/foo.h"

? It seems that libxml2's web-page advises to use what xml2-config --cflags returns and as far as I can see this always include:

-I<prefix>/include/libxml2

If that's the case we can allow setting the HeaderList.directories property and add a custom headers to libxml2. This will solve the current problem and we can then discuss a more extensive refactoring that involves a splitting of responsibilities in another issue.

[scheibelp]

Despite not being a recommended best practice, are you aware of packages that are using libxml2 with include directives like:
#include "libxml2/foo.h"

No - in fact I think that's the issue (i.e. the fact that libxml2 wants users to build with -I<libxml2-prefix>/include/libxml2 is in conflict with this PR). Right now I think it would be the intent of this PR that for libxml2, .headers.directories would report <libxml2-prefix>/include (vs. <libxml2-prefix>/include/libxml2).

If that's true, .headers.directories would be insufficient for specifying -I entries for dependents of libxml2. My goals are:

• sensible defaults for .headers.directories (which I think this achieves)
• allow packages to provide some method to specify -I entries to dependents

Right now, I don't think this PR handles the second goal; that wouldn't be a deal breaker, but I also don't see a clear path forward for handling the second goal.

I wanted to check: do you think dependents should be responsible for calling pkgconfig to retrieve the appropriate -I entries, and that Spack should only be responsible for reporting the "root" include directory?

[alalazo]

do you think dependents should be responsible for calling pkgconfig to retrieve the appropriate -I entries, and that Spack should only be responsible for reporting the "root" include directory?

No, I don't think so. My idea for now would be that libxml2 and other similar packages should override headers and be able to do the following:

@property
def headers(self):
    # Compute the HeaderList in some way
    hl = ...
    hl.directories = [self.spec.prefix.include.libxml2]
    return hl

I was asking the question above because having a few dependencies that use -I<prefix>/include and others that do -I<prefix>/include/libxml2 would complicate things a bit and require a solution similar to what we do for libs (give comma separated query parameters after the spec name when getting items).

I also think that separating concerns might make sense, but that requires more extensive modifications - at least a refactoring of HeaderList and the addition of a new ForwardQueryToPackage descriptor in Spec - that's why I am proposing to save it for later in case.

[alalazo]

Updated description at the top

[scheibelp]

So to be clear (not that we have documented it anywhere) HeaderList.directories used to be a shorthand to refer to the directories where the header files are stored. That API has now changed to be: the directories that dependents should reference with -I. I think it would be worthwhile to document that in HeaderList

[alalazo]

@scheibelp With 7c2e5c9 and 74852c4 we now have:

$ spack-python
Spack version 0.12.1
Python 2.7.15rc1, Linux x86_64
>>> import spack.spec
>>> s = spack.spec.Spec('hwloc')
>>> s.concretize()
>>> s['libxml2'].headers.directories
['/home/mculpo/PycharmProjects/spack/opt/spack/linux-ubuntu18.04-x86_64/gcc-8.2.0/libxml2-2.9.8-imjssszfoiodyeesuf5xpm5da5cl25vs/include/libxml2']
>>> s['libxml2'].headers.cpp_flags
'-I/home/mculpo/PycharmProjects/spack/opt/spack/linux-ubuntu18.04-x86_64/gcc-8.2.0/libxml2-2.9.8-imjssszfoiodyeesuf5xpm5da5cl25vs/include/libxml2'

Would that be enough?

[alalazo]

@michaelkuhn I thought you might be interested too in the latest commits that involve libxml2

[scheibelp]

I have a couple requests regarding names, and a minor request about simplifying some logic.

Also I'd like to make it slightly easier for users to override Package.headers (the comment in libxml2).

[alalazo]

@scheibelp The comments should have been addressed. Let me know if there are other things to be fixed.

[scheibelp]

I have a couple updates to requests.

[alalazo]

@scheibelp Done

[scheibelp]

Thanks!