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

[ppenzin]

Summary

On the systems that have newer gtest installed headers picked up from system include directory will break the build.

https://bugs.llvm.org/show_bug.cgi?id=30877

[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?

[ppenzin]

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.

[MatzeB]

In D37415#880543, @ppenzin wrote:

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)?

[ppenzin]

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.

[MatzeB]

In D37415#880589, @ppenzin 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?

[dim]

In D37415#880595, @MatzeB wrote:

In D37415#880589, @ppenzin 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

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.

[MatzeB]

In D37415#880673, @dim wrote:

In D37415#880595, @MatzeB wrote:

In D37415#880589, @ppenzin 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).

[MatzeB]

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.

[ppenzin]

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]

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

[MatzeB]

That looks cleaner and good to me, thanks.

[ppenzin]

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?

[MatzeB]

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.

[MatzeB]

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

[ppenzin]

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

[MatzeB]

In D37415#899265, @ppenzin wrote:

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.

utils/unittest/CMakeLists.txt

 ########################################################################
 # Experimental CMake build script for Google Test.
 #
 # Consider this a prototype.  It will change drastically.  For now,
 # this is only for people on the cutting edge.
 #
 # To run the tests for Google Test itself on Linux, use 'make test' or
 # ctest.  You can select which tests to run using 'ctest -R regex'.
 # For more options, run 'ctest --help'.
 ########################################################################
 #
 # Project-wide settings
 
+if(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")
+  set(INCLUDE_ORDER BEFORE)
+endif()
+
 # Where gtest's .h files can be found.
 include_directories(
+  ${INCLUDE_ORDER}
   googletest/include
   googletest
   googlemock/include
   googlemock
   )
 
 if(WIN32)
   add_definitions(-DGTEST_OS_WINDOWS=1)