This is an archive of the discontinued LLVM Phabricator instance.

Add gtest include directories before system include directories on FreeBSD and DragonFlyBSD
ClosedPublic

Authored by ppenzin on Sep 2 2017, 5:35 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

ppenzin created this revision.Sep 2 2017, 5:35 PM
MatzeB added a subscriber: MatzeB.

I find it surprising that this is necessary/helps. Usually include_directories() should have priority over all system include directories, unless somehow the system directories end up getting passed to include_directories() somewhere in your build?

Main CMakeLists.txt invokes include_directories, which is necessary on FreeBSD:

if(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")
  # On FreeBSD, /usr/local/* is not used by default. In order to build LLVM
  # with libxml2, iconv.h, etc., we must add /usr/local paths.
  include_directories("/usr/local/include")
  link_directories("/usr/local/lib")
endif(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")

A side effect is that system libraries get picked instead of local libraries (unless BEFORE is specified).

I am not sure how to add a link here, it is line 796 on Github.

Main CMakeLists.txt invokes include_directories, which is necessary on FreeBSD:

if(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")
  # On FreeBSD, /usr/local/* is not used by default. In order to build LLVM
  # with libxml2, iconv.h, etc., we must add /usr/local paths.
  include_directories("/usr/local/include")
  link_directories("/usr/local/lib")
endif(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")

This feels like a hack to me. So those BSDs really install the libxml stuff in /usr/local/include/libxml/*.h and have no intermediate libxml2 folder that needs to be explicitely included to get the xml headers? That's what happens on every system I know of... Could you try building llvm without this odd hack (it seems to be from 2013 maybe things improved)?

It is a FreeBSD-ism: non-system libraries and their headers go into /usr/local, which is not on default search paths. But there might be a more elegant way to do it, I will take a look.

It is a FreeBSD-ism: non-system libraries and their headers go into /usr/local, which is not on default search paths. But there might be a more elegant way to do it, I will take a look.

That I understand. I'm just surprised the libxml stuff doesn't end up in /usr/local/include/libxml2/libxml/*.h and people need to explicitely state -I /usr/local/include/libxml2 (or rather pkg-config --cflags libxml2). Or is the comment not up-to-date and this the /usr/local/include is needed for something else than libxml2?

dim edited edge metadata.Sep 25 2017, 2:30 PM

It is a FreeBSD-ism: non-system libraries and their headers go into /usr/local, which is not on default search paths. But there might be a more elegant way to do it, I will take a look.

That I understand. I'm just surprised the libxml stuff doesn't end up in /usr/local/include/libxml2/libxml/*.h and people need to explicitely state -I /usr/local/include/libxml2 (or rather pkg-config --cflags libxml2). Or is the comment not up-to-date and this the /usr/local/include is needed for something else than libxml2?

It does end up in /usr/local/include/libxml2, and the actual headers are in a libxml subdirectory under this; on my FreeBSD boxes with the libxml2 package installed, pkg-config shows:

$ pkg-config --cflags libxml-2.0
-I/usr/local/include/libxml2

For most other ports, headers are simply installed into /usr/local/include, though. If somebody installs the googletest package, its headers end up in /usr/local/include/gtest, and they might be found before local copy in LLVM's tree.

In D37415#880673, @dim wrote:

It is a FreeBSD-ism: non-system libraries and their headers go into /usr/local, which is not on default search paths. But there might be a more elegant way to do it, I will take a look.

That I understand. I'm just surprised the libxml stuff doesn't end up in /usr/local/include/libxml2/libxml/*.h and people need to explicitely state -I /usr/local/include/libxml2 (or rather pkg-config --cflags libxml2). Or is the comment not up-to-date and this the /usr/local/include is needed for something else than libxml2?

It does end up in /usr/local/include/libxml2, and the actual headers are in a libxml subdirectory under this; on my FreeBSD boxes with the libxml2 package installed, pkg-config shows:

$ pkg-config --cflags libxml-2.0
-I/usr/local/include/libxml2

That's what I would have expected, but then this block in llvm/CMakeLists.txt that includes /usr/local/include should not be necessary (at least not for libxml2)...

For most other ports, headers are simply installed into /usr/local/include, though. If somebody installs the googletest package, its headers end up in /usr/local/include/gtest, and they might be found before local copy in LLVM's tree.

But that is because we add a -I /usr/local/include flag in llvm/CMakeLists.txt for FreeBSD/DragonFlyBSD. I was wondering whether that is really necessary/a good idea. With that flag I could easily see us running into similar issues with localcopies of LLVM/clang/etc. installed (a bunch of those are probably no problem because the directory is added late in the process but still).

I'm not necessarily opposed to this patch either if it turns out we need the explicit inclusion of /usr/local/include after all. If that is the case then I'd just ask to change this patch:

  • Add a comment on why the BEFORE flag is needed
  • I'd simply always use the BEOFRE flag instead of adding the if() block; it shouldn't hurt the other platforms.

Adding BEFORE unconditionally would work, I thought about that, but decided to be conservative. Turns out there is another way to fix include directories - by adding SYSTEM to the call to include_directories in main CMakeLists.txt. I think it can be a cleaner solution, I will look into that as well.

ppenzin updated this revision to Diff 116749.Sep 26 2017, 7:10 PM

To make sure that include directories in the tree override /usr/local/include on FreeBSD, we need to mark the latter as "system".

MatzeB accepted this revision.Sep 26 2017, 7:13 PM

That looks cleaner and good to me, thanks.

This revision is now accepted and ready to land.Sep 26 2017, 7:13 PM

I don't have commit privileges, I would need someone to commit this for me. Should I update the commit message with the reference to the review?

I must have missed this in the flood of E-Mail. In the future, feel free to ping the review thread when nothing happened for a week.

This revision was automatically updated to reflect the committed changes.

Note that I took the liberty and changed the commit message to match the new solution.

Thank you! Since I am new here, what should I do with the bug: https://bugs.llvm.org/show_bug.cgi?id=30877

Thank you! Since I am new here, what should I do with the bug: https://bugs.llvm.org/show_bug.cgi?id=30877

Set the status to "Fixed"; looks like there is also a new "fixed by commit" field you can fill. Any bugzilla user is allowed to do so I think.