make "make install" also install headers and pkgconfig files

make "make install" also install headers and pkgconfig files

See https://code.google.com/p/google-breakpad/issues/detail?id=219

[vapier]

https://breakpad.appspot.com/1114002/diff/1/configure.ac
File configure.ac (right):

https://breakpad.appspot.com/1114002/diff/1/configure.ac#newcode94
configure.ac:94: AC_ARG_WITH(pkgconfigdir,
i don't think we need a flag for this.  no one makes it configurable.  in
Makefile.am, just do:

 pkgconfdir = $(libdir)/pkgconfig

and then use pkgconf_DATA

https://breakpad.appspot.com/1114002/diff/1/configure.ac#newcode160
configure.ac:160: AC_CONFIG_FILES([Makefile breakpad.pc breakpad-client.pc])
we can line wrap this before it gets out of control

 AC_CONFIG_FILES(m4_flatten([
   breakpad.pc
   breakpad-client.pc
   Makefile
 ]))

[vapier]

https://breakpad.appspot.com/1114002/diff/1/Makefile.am
File Makefile.am (right):

https://breakpad.appspot.com/1114002/diff/1/Makefile.am#newcode80
Makefile.am:80: includeclhdir =
$(prefix)/include/$(PACKAGE)/client/linux/handler
this should be using $(includedir) instead of constructing your own random path
based on $(prefix)

https://breakpad.appspot.com/1114002/diff/1/Makefile.am#newcode81
Makefile.am:81: includeclh_HEADERS = $(top_srcdir)/src/client/linux/handler/*.h
i didn't think you needed $(top_srcdir) to list the input headers ?  i never
have ...

https://breakpad.appspot.com/1114002/diff/1/Makefile.am#newcode91
Makefile.am:91: endif
i think you're missing install of common/linux/*.h too

[jon.turney]

Thanks for the review.  I have amended the patch as requested.

On 2014/07/30 17:28:56, vapier wrote:
> https://breakpad.appspot.com/1114002/diff/1/Makefile.am#newcode81
> Makefile.am:81: includeclh_HEADERS =
$(top_srcdir)/src/client/linux/handler/*.h
> i didn't think you needed $(top_srcdir) to list the input headers ?  i never
> have ...

This seems to be needed for installation to work correctly with non-srcdir
builds.
 
> https://breakpad.appspot.com/1114002/diff/1/Makefile.am#newcode91
> Makefile.am:91: endif
> i think you're missing install of common/linux/*.h too

I'm not sure these should be installed.  They don't seem to be part of the
interface for the installed libraries.

[vapier]

seems to me that libbreakpad_client.a includes at least elfutils.o and file_id.o
and guid_creator.o and linux_libc_support.o and memory_mapped_file.o and
safe_readlink.o which come from common/linux/ ...

https://breakpad.appspot.com/1114002/diff/90001/breakpad.pc.in
File breakpad.pc.in (right):

https://breakpad.appspot.com/1114002/diff/90001/breakpad.pc.in#newcode4
breakpad.pc.in:4: includedir=@includedir@/@PACKAGE_NAME@
hmm, i'm not sure we want to automatically add google-breakpad/ to the include
namespace.  wouldn't it be reasonable to expect clients to do #include
<google-breakpad/xxx> ?  otherwise the header files are not well namespaced.

[jon.turney]

On 2014/08/05 11:15:59, vapier wrote:
> seems to me that libbreakpad_client.a includes at least elfutils.o and
file_id.o
> and guid_creator.o and linux_libc_support.o and memory_mapped_file.o and
> safe_readlink.o which come from common/linux/ ...

...

I have added install of common/linux/*.h

> https://breakpad.appspot.com/1114002/diff/90001/breakpad.pc.in
> File breakpad.pc.in (right):
> 
> https://breakpad.appspot.com/1114002/diff/90001/breakpad.pc.in#newcode4
> breakpad.pc.in:4: includedir=@includedir@/@PACKAGE_NAME@
> hmm, i'm not sure we want to automatically add google-breakpad/ to the include
> namespace.  wouldn't it be reasonable to expect clients to do #include
> <google-breakpad/xxx> ?  otherwise the header files are not well namespaced.

Yes.  Although breakpad header files do not include other breakpad header files
in this form currently, so a follow on patch making some adjustments there will
be needed.

[vapier]

ok, good point.  the .pc file as is makes sense then.

[vapier]

https://breakpad.appspot.com/1114002/diff/150001/breakpad-client.pc.in
File breakpad-client.pc.in (right):

https://breakpad.appspot.com/1114002/diff/150001/breakpad-client.pc.in#newcode4
breakpad-client.pc.in:4: includedir=@includedir@
i guess you want what you had before ? :)

[jon.turney]

Please ignore patch set #4, it includes changes from autoreconf

[jon.turney]

On 2014/08/07 04:48:07, vapier wrote:
> https://breakpad.appspot.com/1114002/diff/150001/breakpad-client.pc.in
> File breakpad-client.pc.in (right):
> 
>
https://breakpad.appspot.com/1114002/diff/150001/breakpad-client.pc.in#newcode4
> breakpad-client.pc.in:4: includedir=@includedir@
> i guess you want what you had before ? :)

Reverted in patch set #5

[vapier]

lgtm, but i want to see if anyone else has feedback before merging

[jon.turney]

On 11/08/2014 02:23, vapier@chromium.org wrote:
> lgtm, but i want to see if anyone else has feedback before merging
>
> https://breakpad.appspot.com/1114002/

ping?

[vapier]

i've gone ahead and committed it, but my gcl commit wasn't able to close this
... guess i need to get perms

Error accessing url /1114002/close