Mlpack: bfd.h: config.h must be included before this header

[u8sand]

I had the following error while compiling:

In file included from src/mlpack/core/util/backtrace.cpp:22:0:
/usr/include/bfd.h:35:2: error: #error config.h must be included before this header
 #error config.h must be included before this header
  ^
src/mlpack/CMakeFiles/mlpack.dir/build.make:398: recipe for target 'src/mlpack/CMakeFiles/mlpack.dir/core/util/backtrace.cpp.o' failed
make[2]: *** [src/mlpack/CMakeFiles/mlpack.dir/core/util/backtrace.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CMakeFiles/Makefile2:206: recipe for target 'src/mlpack/CMakeFiles/mlpack.dir/all' failed
make[1]: *** [src/mlpack/CMakeFiles/mlpack.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

I fixed it by adding to CMakeLists:

add_definitions(-DPACKAGE)
add_definitions(-DPACKAGE_VERSION)

This will probably happen to everyone with a new enough binutils; fix from https://bbs.archlinux.org/viewtopic.php?pid=1430683#p1430683

My versions:

$ uname -a
Linux 4.4.5-1-ARCH #1 SMP PREEMPT Thu Mar 10 07:38:19 CET 2016 x86_64 GNU/Linux
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-unknown-linux-gnu/5.3.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: /build/gcc-multilib/src/gcc-5-20160209/configure --prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ --enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++ --enable-shared --enable-threads=posix --enable-libmpx --with-system-zlib --with-isl --enable-__cxa_atexit --disable-libunwind-exceptions --enable-clocale=gnu --disable-libstdcxx-pch --disable-libssp --enable-gnu-unique-object --enable-linker-build-id --enable-lto --enable-plugin --enable-install-libiberty --with-linker-hash-style=gnu --enable-gnu-indirect-function --enable-multilib --disable-werror --enable-checking=release
Thread model: posix
gcc version 5.3.0 (GCC) 

This could probably be fixed harmlessly by populating those defines with mlpack's info.

[rcurtin]

I also hit this on an Arch box yesterday. The libbfd maintainers are obstinate:

https://sourceware.org/bugzilla/show_bug.cgi?id=14243
https://sourceware.org/bugzilla/show_bug.cgi?id=15920

They claim that bfd is an "internal only" library and should not be used by other projects, and use this to justify their decision to force users to define PACKAGE and PACKAGE_VERSION themselves (thereby making the check meaningless to begin with!).

I don't like it, but probably the only solution here is to define those two variables in backtrace.hpp right before bfd.h is included, and then undefine them right after bfd.h is included.

[rcurtin]

(I renamed the title for clarity; hope that is okay)

[u8sand]

@rcurtin No problem at all. It may be ugly but here is one way...

diff --git a/src/mlpack/core/util/backtrace.cpp b/src/mlpack/core/util/backtrace.cpp
index 494242c..ed5d469 100644
--- a/src/mlpack/core/util/backtrace.cpp
+++ b/src/mlpack/core/util/backtrace.cpp
@@ -19,7 +19,19 @@
   #include <signal.h>
   #include <unistd.h>
   #include <cxxabi.h>
-  #include <bfd.h>
+  #ifndef PACKAGE
+    #define PACKAGE
+    #ifndef PACKAGE_VERSION
+      #define PACKAGE_VERSION
+      #include <bfd.h>
+      #undef PACKAGE_VERSION
+    #else
+      #include <bfd.h>
+    #endif
+    #undef PACKAGE
+  #else
+    #include <bfd.h>
+  #endif
   #include <dlfcn.h>
 #endif

If you need bfd.h anywhere else you'd want to have a local bfd.h that wraps it, but it successfully compiled for me so I'm guessing for now it's only needed there.

[rcurtin]

Thanks for the code! But I don't think this handles the case where PACKAGE is defined but PACKAGE_VERSION is not. If you want to update your code to handle that and then submit a PR, I'd be happy to merge it in. I think backtrace.cpp is the only place bfd.h is included, so there's no need to handle this anywhere else.

[u8sand]

@rcurtin Good catch. I'll fix it and submit a pull request shortly.

[rcurtin]

Fixed with the merge of #575. Thanks for pointing this issue out!