[tegrarcm] Don't assume cryptopp is system-wide installed

On Tue, Apr 19, 2016 at 04:32:46PM -0600, Stephen Warren wrote:
> On 04/19/2016 02:25 PM, Thomas Petazzoni wrote:
> >The current build system adds "-isystem /usr/include/$(CRYPTOLIB)" to
> >AM_CPPFLAGS, but this is wrong because cryptopp might not be installed
> >in this location. Instead, the build system should simply include
> ><cryptopp/...> or <crypto++/...> and rely on the compiler include
> >path.
> >
> >The tricky part is that it can be <cryptopp/...> or <crypto++/...>. To
> >solve this, we use a solution similar to the one used in
> >https://github.com/bingmann/crypto-speedtest/blob/master/m4/cryptopp.m4
> >and
> >https://github.com/bingmann/crypto-speedtest/blob/master/src/speedtest_cryptopp.cpp:
> >the configure script fills in a variable called
> >CRYPTOLIB_HEADER_PREFIX, and we use that in the C++ code to include
> >the right header file.
> >
> >It is worth mentioning that doing #include
> ><CRYPTOLIB_HEADER_PREFIX/foobar.h> doesn't work, and we have to use an
> >intermediate #define'd constant to overcome this C preprocessor
> >limitation.
> 
> I think this looks conceptually OK. CC += Allen to double-check
> since he wrote the original cryptopp autoconf support.

What about the following as an alternative?

--

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

On Tue, 19 Apr 2016 16:57:45 -0700, Allen Martin wrote:

> What about the following as an alternative?
> 
> --
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3dad0e6d5e72..22410b3f81bf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1,5 +1,6 @@
>  AM_CFLAGS = -Wall -std=c99
> -AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
> +CRYPTO_PREFIX = $(shell pkg-config --variable=includedir libcrypto++)

Using pkg-config is indeed a much better option. However, there are
several gotchas:

 - First, calling pkg-config like this is not the correct way of using
   pkg-config in autoconf/automake based packages. Instead, you should
   use the PKG_CHECK_MODULES() autoconf macro, which will fill in for
   you the <foo>_CFLAGS and <foo>_LIBS variables. Look at your
   configure.ac, it is already used for the detection of libusb!

 - Second, your logic assumes that libcrypto is called libcrypto++
   while the crux of the matter is that on some platforms it is named
   libcryptopp, on others it's called libcrypto++. Of course, this can
   be solved in the configure.ac by calling PKG_CHECK_MODULES() for
   both.

 - Third, the upstream version of libcrypto++ does *not* install a
   pkg-config file. The Debian packagers have added one, so you
   probably have one on your Debian system, but if you build
   libcrypto++ from source and simply "make install" it, you won't have
   a .pc file that describes it for pkg-config.

The last point is the very reason why my patch didn't switch to simply
use pkg-config.

Best regards,

Thomas

On Wed, Apr 20, 2016 at 11:27:35AM +0200, Thomas Petazzoni wrote:
> On Tue, 19 Apr 2016 16:57:45 -0700, Allen Martin wrote:
> 
> > What about the following as an alternative?
> > 
> > --
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 3dad0e6d5e72..22410b3f81bf 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -1,5 +1,6 @@
> >  AM_CFLAGS = -Wall -std=c99
> > -AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
> > +CRYPTO_PREFIX = $(shell pkg-config --variable=includedir libcrypto++)
> 
> Using pkg-config is indeed a much better option. However, there are
> several gotchas:
> 
>  - First, calling pkg-config like this is not the correct way of using
>    pkg-config in autoconf/automake based packages. Instead, you should
>    use the PKG_CHECK_MODULES() autoconf macro, which will fill in for
>    you the <foo>_CFLAGS and <foo>_LIBS variables. Look at your
>    configure.ac, it is already used for the detection of libusb!

Unfortunately, libcrypto++ doesn't export any CFLAGS, so
PKG_CHECK_MODULES() doesn't work:

  arm@chrome~$ pkg-config --cflags libcrypto++
 
  arm@chrome~$ pkg-config --libs libcrypto++
  -lcrypto++  
  arm@chrome~$ pkg-config --print-variables libcrypto++
  exec_prefix
  prefix
  libdir
  includedir
  arm@chrome~$ pkg-config --variable=includedir libcrypto++
  /usr/include


> 
>  - Second, your logic assumes that libcrypto is called libcrypto++
>    while the crux of the matter is that on some platforms it is named
>    libcryptopp, on others it's called libcrypto++. Of course, this can
>    be solved in the configure.ac by calling PKG_CHECK_MODULES() for
>    both.

Looking at configure.ac I think there's another assumption baked in
too.  The compile test uses:

  LDFLAGS="$LDFLAGS -lcryptopp -lpthread"

> 
>  - Third, the upstream version of libcrypto++ does *not* install a
>    pkg-config file. The Debian packagers have added one, so you
>    probably have one on your Debian system, but if you build
>    libcrypto++ from source and simply "make install" it, you won't have
>    a .pc file that describes it for pkg-config.
> 
> The last point is the very reason why my patch didn't switch to simply
> use pkg-config.
> 

Well that sucks.  What about a command line option to configure to
specify the libcrypto++ include path?  My issue here is that if you
have the headers in a non standard location (like $HOME/include), the
compiler probably won't know how to find them either, and the compile
check will fail at configure time.

I'd like to change the -isystem to a plain -I too.  I think I added
that because the source code used #include "foo.h" instead of 
#include <foo.h> and I didn't want to make any source code changes
with the automake changes. 

-Allen
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

On Thu, 21 Apr 2016 11:13:59 -0700, Allen Martin wrote:

> Unfortunately, libcrypto++ doesn't export any CFLAGS, so
> PKG_CHECK_MODULES() doesn't work:
> 
>   arm@chrome~$ pkg-config --cflags libcrypto++
>  
>   arm@chrome~$ pkg-config --libs libcrypto++
>   -lcrypto++  
>   arm@chrome~$ pkg-config --print-variables libcrypto++
>   exec_prefix
>   prefix
>   libdir
>   includedir
>   arm@chrome~$ pkg-config --variable=includedir libcrypto++
>   /usr/include

Ah, crap. So obviously relying on pkg-config is not an option.

> >  - Second, your logic assumes that libcrypto is called libcrypto++
> >    while the crux of the matter is that on some platforms it is named
> >    libcryptopp, on others it's called libcrypto++. Of course, this can
> >    be solved in the configure.ac by calling PKG_CHECK_MODULES() for
> >    both.
> 
> Looking at configure.ac I think there's another assumption baked in
> too.  The compile test uses:
> 
>   LDFLAGS="$LDFLAGS -lcryptopp -lpthread"

Yes, but no, because:

configure.ac:   [CRYPTOLIB="cryptopp"],
configure.ac:           [CRYPTOLIB="crypto++"],
[...]
src/Makefile.am:tegrarcm_LDADD = $(LIBUSB_LIBS) -l$(CRYPTOLIB) -lpthread

So the proper -lcryptopp or -lcrypto++ is added.

> Well that sucks.  What about a command line option to configure to
> specify the libcrypto++ include path?  My issue here is that if you
> have the headers in a non standard location (like $HOME/include), the
> compiler probably won't know how to find them either, and the compile
> check will fail at configure time.

Yes, of course, and it's my use case. I'm building tegracrm in the
context of a build system, as a native tool (i.e built for the build
machine, not cross-compiled for the target). And for such tools, we
install all host libraries in some non-standard location (since we
don't run as root, obviously).

The solution I proposed keeps the "autodetection" mechanism that is
currently in place.

> I'd like to change the -isystem to a plain -I too.  I think I added
> that because the source code used #include "foo.h" instead of 
> #include <foo.h> and I didn't want to make any source code changes
> with the automake changes. 

Sounds like a good idea.

Thomas