[Buildroot] [PATCH V5 0/2] google-breakpad: new package

[Buildroot] [PATCH V5 0/2] google-breakpad: new package

[Pascal Huerst]

Changes v4 -> v5 (All suggested by Yann E. MORIN)
	Fixed some typo
	Reorganized patch structure
	  which parts belong to 'new package' and which parts belong to
	  'intergration into Makefile and Config.in'
	Changed and Rewrote gen-symy.sh - script
	  Passing $(EXTRA_ENV)
	  Adapted a proposed solution from Yann E. MORIN, which
	  uses ls instead of find commands
	Made better descriptions for users in:
	  Config.in
	  package/google-breakpad/Config.in

Changes v3 -> v4
	Fixed minor issue in google-breakpad_gen-syms.sh
	  Had to remove -print from find command

Changes v2 -> v3 (All suggested by thomas.petazzoni at free-electrons.com)
        Removed dependency of BR2_ENABLE_DEBUG, but added comment,
          that this flag might have to be set in order to use breakpad
          properly.
        Removed "find -name ..." for libs and binaries by generic patterns,
          such as "*.so" and so forth. This will never work properly. Instead
          I added a list to insert libs and binaries, that will be symbol-
          dumped and prepared for breakpad (see Config.in patch v3 2/2)
        Changed storage path for dumps from: $(TARGET_DIR).. to
          $(STAGING_DIR)/usr/share/google-breakpad-symbols/
        Removed most logic from Makefile. Added script instead, which is
          called by Makefile
        Added dependency for compatible targets: ARM, i386, MIPS...
        Set fixed version for checkout (instead of head)
        Fixed typos, and some minor mistakes

Changes v1 -> v2 (All suggested by maxime.hadjinlian at gmail.com)
        Added dependency from BR2_ENABLE_DEBUG
        Removed symbolstore.py -> Its all done in Makefile now
        Removed new Target -> It in target-finalize before stripping now
        Added LICENSE and LICENSE_FILE to *.mk

Pascal Huerst (2):
  google-breakpad: new package
  google-breakpad: integration into Makefile and Config.in

 Config.in                                  | 14 ++++++++++++++
 Makefile                                   |  5 +++++
 package/Config.in                          |  1 +
 package/google-breakpad/Config.in          | 22 ++++++++++++++++++++++
 package/google-breakpad/gen-syms.sh        | 25 +++++++++++++++++++++++++
 package/google-breakpad/google-breakpad.mk | 17 +++++++++++++++++
 6 files changed, 84 insertions(+)
 create mode 100644 package/google-breakpad/Config.in
 create mode 100755 package/google-breakpad/gen-syms.sh
 create mode 100644 package/google-breakpad/google-breakpad.mk

-- 
1.9.3

[Buildroot] [PATCH V5 1/2] google-breakpad: new package

[Pascal Huerst]

Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
---
 package/Config.in                          |  1 +
 package/google-breakpad/Config.in          | 19 +++++++++++++++++++
 package/google-breakpad/google-breakpad.mk | 17 +++++++++++++++++
 3 files changed, 37 insertions(+)
 create mode 100644 package/google-breakpad/Config.in
 create mode 100644 package/google-breakpad/google-breakpad.mk

diff --git a/package/Config.in b/package/Config.in
index c46c0ec..392ae40 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -54,6 +54,7 @@ menu "Debugging, profiling and benchmark"
 	source "package/duma/Config.in"
 	source "package/fio/Config.in"
 	source "package/gdb/Config.in"
+	source "package/google-breakpad/Config.in"
 	source "package/iozone/Config.in"
 	source "package/kexec/Config.in"
 	source "package/ktap/Config.in"
diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in
new file mode 100644
index 0000000..fa1e8d9
--- /dev/null
+++ b/package/google-breakpad/Config.in
@@ -0,0 +1,19 @@
+config BR2_PACKAGE_GOOGLE_BREAKPAD
+	bool "google-breakpad"
+	depends on BR2_INSTALL_LIBSTDCPP
+	depends on BR2_TOOLCHAIN_USES_GLIBC
+	depends on BR2_i386 || BR2_x86_64 || BR2_arm || BR2_aarch64 || BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el
+	help
+	  Google-Breakpad is a library and tool suite that allows you to distribute 
+	  an application to users with compiler-provided debugging information 
+	  removed, record crashes in compact "minidump" files, send them back to 
+	  your server, and produce C and C++ stack traces from these minidumps. 
+	  Breakpad can also write minidumps on request for programs that have not 
+	  crashed. 	 
+
+	  You may want to set BR2_ENABLE_DEBUG, in order to get useful results.
+
+	  http://code.google.com/p/google-breakpad/
+
+comment "google-breakpad requires an (e)glibc toolchain w/ C++ enabled"
+	depends on !(BR2_INSTALL_LIBSTDCPP && BR2_TOOLCHAIN_USES_GLIBC)
diff --git a/package/google-breakpad/google-breakpad.mk b/package/google-breakpad/google-breakpad.mk
new file mode 100644
index 0000000..e5b69c0
--- /dev/null
+++ b/package/google-breakpad/google-breakpad.mk
@@ -0,0 +1,17 @@
+################################################################################
+#
+# google-breakpad
+#
+################################################################################
+
+GOOGLE_BREAKPAD_VERSION = 1320
+GOOGLE_BREAKPAD_SITE = http://google-breakpad.googlecode.com/svn/trunk
+GOOGLE_BREAKPAD_SITE_METHOD = svn
+GOOGLE_BREAKPAD_CONF_OPT = --disable-processor --disable-tools
+GOOGLE_BREAKPAD_DEPENDENCIES = host-google-breakpad
+GOOGLE_BREAKPAD_INSTALL_STAGING = YES
+GOOGLE_BREAKPAD_LICENSE = BSD-3c
+GOOGLE_BREAKPAD_LICENSE_FILES = LICENSE
+
+$(eval $(host-autotools-package))
+$(eval $(autotools-package))
-- 
1.9.3

[Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in

[Pascal Huerst]

Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
---
 Config.in                           | 14 ++++++++++++++
 Makefile                            |  5 +++++
 package/google-breakpad/Config.in   |  5 ++++-
 package/google-breakpad/gen-syms.sh | 25 +++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100755 package/google-breakpad/gen-syms.sh

diff --git a/Config.in b/Config.in
index bf1ca86..4f2bd32 100644
--- a/Config.in
+++ b/Config.in
@@ -480,6 +480,20 @@ config BR2_OPTIMIZE_S
 
 endchoice
 
+config BR2_GOOGLE_BREAKPAD_INCLUDE_FILES
+	string "executables and libraries to be used by google-breakpad"
+	depends on BR2_PACKAGE_GOOGLE_BREAKPAD
+	default ""
+	help
+	  You may specify a space-separated list of binaries and libraries
+	  with full paths relative to $(TARGET_DIR) of which debug symbols 
+	  will be dumped for further use with google breakpad.
+
+	  A directory structure that can be used by minidump-stackwalk will
+	  be created at:
+
+	  $(STAGING_DIR)/usr/share/google-breakpad-symbols
+
 config BR2_ENABLE_SSP
 	bool "build code with Stack Smashing Protection"
 	depends on BR2_TOOLCHAIN_HAS_SSP
diff --git a/Makefile b/Makefile
index 4fe370a..300e86e 100644
--- a/Makefile
+++ b/Makefile
@@ -553,6 +553,11 @@ endif
 ifeq ($(BR2_PACKAGE_PYTHON_PYC_ONLY)$(BR2_PACKAGE_PYTHON3_PYC_ONLY),y)
 	find $(TARGET_DIR)/usr/lib/ -name '*.py' -print0 | xargs -0 rm -f
 endif
+ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y)
+	$(EXTRA_ENV) package/google-breakpad/gen-syms.sh $(STAGING_DIR) \
+		$(TARGET_DIR) $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES))
+endif
+
 	rm -rf $(TARGET_DIR)/usr/lib/luarocks
 	rm -rf $(TARGET_DIR)/usr/lib/perl5/$(PERL_VERSION)/$(PERL_ARCHNAME)/CORE
 	find $(TARGET_DIR)/usr/lib/perl5/ -name '*.bs' -print0 | xargs -0 rm -f
diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in
index fa1e8d9..99dc7a1 100644
--- a/package/google-breakpad/Config.in
+++ b/package/google-breakpad/Config.in
@@ -11,7 +11,10 @@ config BR2_PACKAGE_GOOGLE_BREAKPAD
 	  Breakpad can also write minidumps on request for programs that have not 
 	  crashed. 	 
 
-	  You may want to set BR2_ENABLE_DEBUG, in order to get useful results.
+	  Add all binaries and libraries you want to debug by google-breakpad to 
+	  BR2_GOOGLE_BREAKPAD_INCLUDE_FILES.
+
+	  You may also want to set BR2_ENABLE_DEBUG, in order to get useful results.
 
 	  http://code.google.com/p/google-breakpad/
 
diff --git a/package/google-breakpad/gen-syms.sh b/package/google-breakpad/gen-syms.sh
new file mode 100755
index 0000000..d890752
--- /dev/null
+++ b/package/google-breakpad/gen-syms.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+STAGING_DIR="${1}"
+TARGET_DIR="${2}"
+shift 2
+
+SYMBOLS_DIR="${STAGING_DIR}/usr/share/google-breakpad-symbols"
+rm -rf "${SYMBOLS_DIR}"
+mkdir -p "${SYMBOLS_DIR}/tmp"
+
+for FILE in $(eval ls "${TARGET_DIR}/${@}"); do
+	if [ -d "${FILE}" ]; then
+		printf "Error: '%s' is a directory\n" "${FILE}" >&2
+		exit 1
+	fi
+	if dump_syms "${FILE}" > "${SYMBOLS_DIR}/tmp/tmp.sym" 2>/dev/null; then
+		HASH=$(head -n1 "${SYMBOLS_DIR}/tmp/tmp.sym" | cut -d ' ' -f 4);
+		FILENAME=$(basename "$FILE");
+		mkdir -p "${SYMBOLS_DIR}/${FILENAME}/${HASH}"
+		mv "${SYMBOLS_DIR}/tmp/tmp.sym" "${SYMBOLS_DIR}/${FILENAME}/${HASH}/${FILENAME}.sym";
+	else
+		printf "Error dumping symbols for: '%s'\n" "${FILE}" >&2
+		exit 1
+	fi
+done
+rm -rf "${SYMBOLS_DIR}/tmp"
-- 
1.9.3

[Buildroot] [PATCH V5 1/2] google-breakpad: new package

[Arnout Vandecappelle]

On 25/06/14 15:16, Pascal Huerst wrote:
> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
> ---
>  package/Config.in                          |  1 +
>  package/google-breakpad/Config.in          | 19 +++++++++++++++++++
>  package/google-breakpad/google-breakpad.mk | 17 +++++++++++++++++
>  3 files changed, 37 insertions(+)
>  create mode 100644 package/google-breakpad/Config.in
>  create mode 100644 package/google-breakpad/google-breakpad.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index c46c0ec..392ae40 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -54,6 +54,7 @@ menu "Debugging, profiling and benchmark"
>  	source "package/duma/Config.in"
>  	source "package/fio/Config.in"
>  	source "package/gdb/Config.in"
> +	source "package/google-breakpad/Config.in"
>  	source "package/iozone/Config.in"
>  	source "package/kexec/Config.in"
>  	source "package/ktap/Config.in"
> diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in
> new file mode 100644
> index 0000000..fa1e8d9
> --- /dev/null
> +++ b/package/google-breakpad/Config.in
> @@ -0,0 +1,19 @@
> +config BR2_PACKAGE_GOOGLE_BREAKPAD
> +	bool "google-breakpad"
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on BR2_TOOLCHAIN_USES_GLIBC
> +	depends on BR2_i386 || BR2_x86_64 || BR2_arm || BR2_aarch64 || BR2_mips || BR2_mipsel || BR2_mips64 || BR2_mips64el

 Too long, the line should be split after mips.

> +	help
> +	  Google-Breakpad is a library and tool suite that allows you to distribute 
> +	  an application to users with compiler-provided debugging information 
> +	  removed, record crashes in compact "minidump" files, send them back to 
> +	  your server, and produce C and C++ stack traces from these minidumps. 
> +	  Breakpad can also write minidumps on request for programs that have not 
> +	  crashed. 	 

 I think these lines are also too long to display on an 80-char display (it
should be wrapped at 72 chars IIRC, where the tab counts for 8 chars).

 Also, there should be no whitespace at the end of the lines.

 I would also mention in the help text that this installs the host tools in
$(HOST_DIR)/usr/bin and list the host tools.

 If I understand correctly, for the target, we just build the (static) client
library, which should be linked and enabled explicitly by applications that want
to make use of breakpad. Is that correct? If yes, perhaps it's better to clarify
that in the help text as well.


> +
> +	  You may want to set BR2_ENABLE_DEBUG, in order to get useful results.
> +
> +	  http://code.google.com/p/google-breakpad/
> +
> +comment "google-breakpad requires an (e)glibc toolchain w/ C++ enabled"
> +	depends on !(BR2_INSTALL_LIBSTDCPP && BR2_TOOLCHAIN_USES_GLIBC)

 Here you should repeat the arch dependencies.

> diff --git a/package/google-breakpad/google-breakpad.mk b/package/google-breakpad/google-breakpad.mk
> new file mode 100644
> index 0000000..e5b69c0
> --- /dev/null
> +++ b/package/google-breakpad/google-breakpad.mk
> @@ -0,0 +1,17 @@
> +################################################################################
> +#
> +# google-breakpad
> +#
> +################################################################################
> +
> +GOOGLE_BREAKPAD_VERSION = 1320

 Any particular reason to take a 2-month old commit?

> +GOOGLE_BREAKPAD_SITE = http://google-breakpad.googlecode.com/svn/trunk
> +GOOGLE_BREAKPAD_SITE_METHOD = svn
> +GOOGLE_BREAKPAD_CONF_OPT = --disable-processor --disable-tools
> +GOOGLE_BREAKPAD_DEPENDENCIES = host-google-breakpad
> +GOOGLE_BREAKPAD_INSTALL_STAGING = YES

 There is nothing to install on the target, right? So I'd expect

GOOGLE_BREAKPAD_INSTALL_TARGET = NO

> +GOOGLE_BREAKPAD_LICENSE = BSD-3c
> +GOOGLE_BREAKPAD_LICENSE_FILES = LICENSE
> +
> +$(eval $(host-autotools-package))
> +$(eval $(autotools-package))

 We normally put the host-package after the target-package.


 Regards,
 Arnout

> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in

[Arnout Vandecappelle]

On 25/06/14 15:16, Pascal Huerst wrote:
> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
> ---
>  Config.in                           | 14 ++++++++++++++
>  Makefile                            |  5 +++++
>  package/google-breakpad/Config.in   |  5 ++++-
>  package/google-breakpad/gen-syms.sh | 25 +++++++++++++++++++++++++
>  4 files changed, 48 insertions(+), 1 deletion(-)
>  create mode 100755 package/google-breakpad/gen-syms.sh
> 
> diff --git a/Config.in b/Config.in
> index bf1ca86..4f2bd32 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -480,6 +480,20 @@ config BR2_OPTIMIZE_S
>  
>  endchoice
>  
> +config BR2_GOOGLE_BREAKPAD_INCLUDE_FILES

 I don't really like the name of this option. What do you think of
BR2_GOOGLE_BREAKPAD_GENSYMS_PATTERNS ?

> +	string "executables and libraries to be used by google-breakpad"
> +	depends on BR2_PACKAGE_GOOGLE_BREAKPAD

 Hm, this is counter-intuitive, though honestly I don't know how to improve it.
The user would first have to go to the target packages and select google
breakpad, and then return to the Build options menu at the top to set the patterns.

 Actually, this symbol could be defined inside the google-breakpad/Config.in,
no? Though you probably got a comment before that that is not the right place :-)

> +	default ""
> +	help
> +	  You may specify a space-separated list of binaries and libraries
> +	  with full paths relative to $(TARGET_DIR) of which debug symbols 

 Again trailing whitespace and line-wrapping.

 I think this text is slightly better:

	  You may specify a space-separated list of glob patterns of binaries
	  and libraries of which debug symbols will be dumped for further use
	  with google breakpad. Th patterns are evaluated relative to
	  $(TARGET_DIR).

> +	  will be dumped for further use with google breakpad.
> +
> +	  A directory structure that can be used by minidump-stackwalk will
> +	  be created at:
> +
> +	  $(STAGING_DIR)/usr/share/google-breakpad-symbols
> +
>  config BR2_ENABLE_SSP
>  	bool "build code with Stack Smashing Protection"
>  	depends on BR2_TOOLCHAIN_HAS_SSP
> diff --git a/Makefile b/Makefile
> index 4fe370a..300e86e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -553,6 +553,11 @@ endif
>  ifeq ($(BR2_PACKAGE_PYTHON_PYC_ONLY)$(BR2_PACKAGE_PYTHON3_PYC_ONLY),y)
>  	find $(TARGET_DIR)/usr/lib/ -name '*.py' -print0 | xargs -0 rm -f
>  endif
> +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y)
> +	$(EXTRA_ENV) package/google-breakpad/gen-syms.sh $(STAGING_DIR) \
> +		$(TARGET_DIR) $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES))

 To be more compatible with Fabio's series that reworks this part of the
infrastructure, it's better to define this as a new variable which is called
here. That will make it easier to resolve the conflict between these two series.
Fabio's series will also make it possible to define this completely inside the
google-breakpad.mk.

 If the config variable contains a glob pattern, then this will be evaluated
here (relative to the buildroot directory). Usually that will fail so it will be
passed unexpanded, but perhaps it's safer to explicitly turn off globbing.

 Together with my other suggestions, this would become:

ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y)
GOOGLE_BREAKPAD_INCLUDE_FILES = $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES))
ifneq ($(GOOGLE_BREAKPAD_INCLUDE_FILES),)
# BR2_GOOGLE_BREAKPAD_INCLUDE_FILES may include glob files, so turn off globbing
define GOOGLE_BREAKPAD_GEN_SYMS
	$(Q)set -o noglob; \
	$(EXTRA_ENV) package/google-breakpad/gen-syms.sh \
		$(STAGING_DIR) $(TARGET_DIR) $(GOOGLE_BREAKPAD_INCLUDE_FILES)
endef
endif
endif

> +endif
> +
>  	rm -rf $(TARGET_DIR)/usr/lib/luarocks
>  	rm -rf $(TARGET_DIR)/usr/lib/perl5/$(PERL_VERSION)/$(PERL_ARCHNAME)/CORE
>  	find $(TARGET_DIR)/usr/lib/perl5/ -name '*.bs' -print0 | xargs -0 rm -f
> diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in
> index fa1e8d9..99dc7a1 100644
> --- a/package/google-breakpad/Config.in
> +++ b/package/google-breakpad/Config.in
> @@ -11,7 +11,10 @@ config BR2_PACKAGE_GOOGLE_BREAKPAD
>  	  Breakpad can also write minidumps on request for programs that have not 
>  	  crashed. 	 
>  
> -	  You may want to set BR2_ENABLE_DEBUG, in order to get useful results.
> +	  Add all binaries and libraries you want to debug by google-breakpad to 

 Trailing whitespace again. Also, I'd write

	  Add all binaries and libraries that link with the google-breakpad
	  client library and that you want to debug with minidump-stackwalk
	  to BR2_GOOGLE_BREAKPAD_INCLUDE_FILES.

> +	  BR2_GOOGLE_BREAKPAD_INCLUDE_FILES.
> +
> +	  You may also want to set BR2_ENABLE_DEBUG, in order to get useful results.
>  
>  	  http://code.google.com/p/google-breakpad/
>  
> diff --git a/package/google-breakpad/gen-syms.sh b/package/google-breakpad/gen-syms.sh
> new file mode 100755
> index 0000000..d890752
> --- /dev/null
> +++ b/package/google-breakpad/gen-syms.sh
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +STAGING_DIR="${1}"
> +TARGET_DIR="${2}"
> +shift 2
> +
> +SYMBOLS_DIR="${STAGING_DIR}/usr/share/google-breakpad-symbols"
> +rm -rf "${SYMBOLS_DIR}"
> +mkdir -p "${SYMBOLS_DIR}/tmp"
> +
> +for FILE in $(eval ls "${TARGET_DIR}/${@}"); do

 This does not really work: if one of the patterns matches a directory, the
directory will still be listed. So you have to use ls -d.

 However, I think the following is easier to understand, even if it's more wordy:

for PATTERN in "$@"; do
	for FILE in ${TARGET_DIR}/${PATTERN}; do
		...
	done
done

> +	if [ -d "${FILE}" ]; then
> +		printf "Error: '%s' is a directory\n" "${FILE}" >&2
> +		exit 1
> +	fi
> +	if dump_syms "${FILE}" > "${SYMBOLS_DIR}/tmp/tmp.sym" 2>/dev/null; then

 Do we really want to redirect stderr here?

 Why do you put tmp.sym in a subdirectory, and not straignt into ${SYMBOLS_DIR}?

> +		HASH=$(head -n1 "${SYMBOLS_DIR}/tmp/tmp.sym" | cut -d ' ' -f 4);
> +		FILENAME=$(basename "$FILE");

 I don't know if it is the official buildroot policy, but recently I've seen
most scripts use "${FILE##*/}" instead of basename.


 Regards,
 Arnout

> +		mkdir -p "${SYMBOLS_DIR}/${FILENAME}/${HASH}"
> +		mv "${SYMBOLS_DIR}/tmp/tmp.sym" "${SYMBOLS_DIR}/${FILENAME}/${HASH}/${FILENAME}.sym";
> +	else
> +		printf "Error dumping symbols for: '%s'\n" "${FILE}" >&2
> +		exit 1
> +	fi
> +done
> +rm -rf "${SYMBOLS_DIR}/tmp"
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH V5 1/2] google-breakpad: new package

[Thomas Petazzoni]

Dear Pascal Huerst,

On Wed, 25 Jun 2014 15:16:13 +0200, Pascal Huerst wrote:
> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
> ---
>  package/Config.in                          |  1 +
>  package/google-breakpad/Config.in          | 19 +++++++++++++++++++
>  package/google-breakpad/google-breakpad.mk | 17 +++++++++++++++++
>  3 files changed, 37 insertions(+)
>  create mode 100644 package/google-breakpad/Config.in
>  create mode 100644 package/google-breakpad/google-breakpad.mk

Thanks, I've applied your patch, after making some changes according to
Arnout comments. From the commit log:

    [Thomas:
     - Introduce a BR2_PACKAGE_GOOGLE_BREAKPAD_ARCH_SUPPORTS Config.in
       symbol to be able to easily propagate the architecture
       dependencies.
     - Wrap the help text, add some more details as suggested by Arnout.
     - Propagate the architecture dependencies to the comment, as
       suggested by Arnout.
     - Remove the dependency of google-breakpad on host-google-breakpad,
       since it's not needed.
     - Add <pkg>_TARGET = NO, because google-breakpad only installs a
       static library, so installation to staging is sufficient.
     - Reorder autotools-package/host-autotools-package invocations, as
       suggested by Arnout.]

Also, could you explain why you're passing --disable-processor when
building the target package google-breakpad?

For the second patch adding the core support for google-breakpad, there
is still some on-going discussion, so I believe it's not ready to be
applied yet.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in

[Thomas Petazzoni]

Arnout, Pascal,

On Wed, 25 Jun 2014 22:31:20 +0200, Arnout Vandecappelle wrote:

> > +config BR2_GOOGLE_BREAKPAD_INCLUDE_FILES
> 
>  I don't really like the name of this option. What do you think of
> BR2_GOOGLE_BREAKPAD_GENSYMS_PATTERNS ?

I think this explains some of your comments below: Pascal's patch only
support listing complete file names in
BR2_GOOGLE_BREAKPAD_INCLUDE_FILES, while you apparently thought his
intention was to support file patterns. It indeed seems like a good
idea, but it's clearly a different thing.

> > +	string "executables and libraries to be used by google-breakpad"
> > +	depends on BR2_PACKAGE_GOOGLE_BREAKPAD
> 
>  Hm, this is counter-intuitive, though honestly I don't know how to improve it.
> The user would first have to go to the target packages and select google
> breakpad, and then return to the Build options menu at the top to set the patterns.
> 
>  Actually, this symbol could be defined inside the google-breakpad/Config.in,
> no? Though you probably got a comment before that that is not the right place :-)

Yeah, that's tricky. We could turn it into a "select", but then
everybody will always see this option related to google-breakpad in the
Build options. So I believe the solution of using a 'depends on' is
still the most appropriate solution. Maybe a little piece of
documentation should be added in the Buildroot manual to explain how
the Google Breakpad integration works?

However, I don't really like the prompt. Maybe it should be:

	[ ] Enable Google Breakpad support
	   () List of binaries to extract symbols from

> > +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y)
> > +	$(EXTRA_ENV) package/google-breakpad/gen-syms.sh $(STAGING_DIR) \
> > +		$(TARGET_DIR) $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES))
> 
>  To be more compatible with Fabio's series that reworks this part of the
> infrastructure, it's better to define this as a new variable which is called
> here. That will make it easier to resolve the conflict between these two series.
> Fabio's series will also make it possible to define this completely inside the
> google-breakpad.mk.

Yes, having it all in the google-breakpad.mk seems like a good idea.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in

[Arnout Vandecappelle]

On 29/06/14 12:36, Thomas Petazzoni wrote:
> Arnout, Pascal,
> 
> On Wed, 25 Jun 2014 22:31:20 +0200, Arnout Vandecappelle wrote:
> 
>>> +config BR2_GOOGLE_BREAKPAD_INCLUDE_FILES
>>
>>  I don't really like the name of this option. What do you think of
>> BR2_GOOGLE_BREAKPAD_GENSYMS_PATTERNS ?
> 
> I think this explains some of your comments below: Pascal's patch only
> support listing complete file names in
> BR2_GOOGLE_BREAKPAD_INCLUDE_FILES, while you apparently thought his
> intention was to support file patterns. It indeed seems like a good
> idea, but it's clearly a different thing.

 My bad, I was reviewing this patch side-by-side with the comments on v4 and the
script looked a bit like what was proposed by Yann in his review so I assumed
that it would include support for glob patterns. Personally I don't really see
this as a requirement.

> 
>>> +	string "executables and libraries to be used by google-breakpad"
>>> +	depends on BR2_PACKAGE_GOOGLE_BREAKPAD
>>
>>  Hm, this is counter-intuitive, though honestly I don't know how to improve it.
>> The user would first have to go to the target packages and select google
>> breakpad, and then return to the Build options menu at the top to set the patterns.
>>
>>  Actually, this symbol could be defined inside the google-breakpad/Config.in,
>> no? Though you probably got a comment before that that is not the right place :-)
> 
> Yeah, that's tricky. We could turn it into a "select", but then
> everybody will always see this option related to google-breakpad in the
> Build options. So I believe the solution of using a 'depends on' is
> still the most appropriate solution. Maybe a little piece of
> documentation should be added in the Buildroot manual to explain how
> the Google Breakpad integration works?
> 
> However, I don't really like the prompt. Maybe it should be:
> 
> 	[ ] Enable Google Breakpad support
> 	   () List of binaries to extract symbols from

 If it is done in that way, then it really should be in the google breakpad
package and not in the toolchain menu...


 However, thinking a bit more about this: the dependency on the target package
is not correct. There's a dependency on the host package because the host tools
are used, but it's the responsibility of whatever package is linking against the
static library to also depend on the target package.

 So in that case, it is indeed best to keep it in the toolchain menu, remove the
dependency on the target package, but add a boolean config option to enable
breakpad to begin with.

 Regards,
 Arnout


> 
>>> +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y)
>>> +	$(EXTRA_ENV) package/google-breakpad/gen-syms.sh $(STAGING_DIR) \
>>> +		$(TARGET_DIR) $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES))
>>
>>  To be more compatible with Fabio's series that reworks this part of the
>> infrastructure, it's better to define this as a new variable which is called
>> here. That will make it easier to resolve the conflict between these two series.
>> Fabio's series will also make it possible to define this completely inside the
>> google-breakpad.mk.
> 
> Yes, having it all in the google-breakpad.mk seems like a good idea.
> 
> Thomas
> 


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH V5 1/2] google-breakpad: new package

[Pascal Huerst]

Hey Thomas, all

On 29.06.2014 12:30, Thomas Petazzoni wrote:
> On Wed, 25 Jun 2014 15:16:13 +0200, Pascal Huerst wrote:
>> Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com>
>> ---
>>  package/Config.in                          |  1 +
>>  package/google-breakpad/Config.in          | 19 +++++++++++++++++++
>>  package/google-breakpad/google-breakpad.mk | 17 +++++++++++++++++
>>  3 files changed, 37 insertions(+)
>>  create mode 100644 package/google-breakpad/Config.in
>>  create mode 100644 package/google-breakpad/google-breakpad.mk
> 
> Thanks, I've applied your patch, after making some changes according to
> Arnout comments. From the commit log:
> 
>     [Thomas:
>      - Introduce a BR2_PACKAGE_GOOGLE_BREAKPAD_ARCH_SUPPORTS Config.in
>        symbol to be able to easily propagate the architecture
>        dependencies.
>      - Wrap the help text, add some more details as suggested by Arnout.
>      - Propagate the architecture dependencies to the comment, as
>        suggested by Arnout.
>      - Remove the dependency of google-breakpad on host-google-breakpad,
>        since it's not needed.
>      - Add <pkg>_TARGET = NO, because google-breakpad only installs a
>        static library, so installation to staging is sufficient.
>      - Reorder autotools-package/host-autotools-package invocations, as
>        suggested by Arnout.]

Glad to hear that! And thank you for fixing up some Issues!

> Also, could you explain why you're passing --disable-processor when
> building the target package google-breakpad?

I made the assumption, that one will always do the debugging on the host
and that processor-tools such as minidump-stackwalk are not needed on
the target.

> For the second patch adding the core support for google-breakpad, there
> is still some on-going discussion, so I believe it's not ready to be
> applied yet.

Yes. I will have a look into that afterwards.

Thanks,
Pascal

[Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in

[Pascal Huerst]

Hey Thomas, Arnout, all

-- snip --

> However, I don't really like the prompt. Maybe it should be:
> 
> 	[ ] Enable Google Breakpad support
> 	   () List of binaries to extract symbols from
> 
>>> +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y)
>>> +	$(EXTRA_ENV) package/google-breakpad/gen-syms.sh $(STAGING_DIR) \
>>> +		$(TARGET_DIR) $(call qstrip,$(BR2_GOOGLE_BREAKPAD_INCLUDE_FILES))
>>
>>  To be more compatible with Fabio's series that reworks this part of the
>> infrastructure, it's better to define this as a new variable which is called
>> here. That will make it easier to resolve the conflict between these two series.
>> Fabio's series will also make it possible to define this completely inside the
>> google-breakpad.mk.

Can you give me a hint, on how this should be done then?

> Yes, having it all in the google-breakpad.mk seems like a good idea.
> 
> Thomas
> 

Pascal

[Buildroot] [PATCH V5 2/2] google-breakpad: integration into Makefile and Config.in

[Pascal Huerst]

Hey Arnout, Thomas, all

-- snip --

On 01.07.2014 08:19, Arnout Vandecappelle wrote:
> On 29/06/14 12:36, Thomas Petazzoni wrote:
>> On Wed, 25 Jun 2014 22:31:20 +0200, Arnout Vandecappelle wrote:

>>>  Actually, this symbol could be defined inside the google-breakpad/Config.in,
>>> no? Though you probably got a comment before that that is not the right place :-)
>>
>> Yeah, that's tricky. We could turn it into a "select", but then
>> everybody will always see this option related to google-breakpad in the
>> Build options. So I believe the solution of using a 'depends on' is
>> still the most appropriate solution. Maybe a little piece of
>> documentation should be added in the Buildroot manual to explain how
>> the Google Breakpad integration works?
>>
>> However, I don't really like the prompt. Maybe it should be:
>>
>> 	[ ] Enable Google Breakpad support
>> 	   () List of binaries to extract symbols from
> 
>  If it is done in that way, then it really should be in the google breakpad
> package and not in the toolchain menu...
> 
> 
>  However, thinking a bit more about this: the dependency on the target package
> is not correct. There's a dependency on the host package because the host tools
> are used, but it's the responsibility of whatever package is linking against the
> static library to also depend on the target package.
> 
>  So in that case, it is indeed best to keep it in the toolchain menu, remove the
> dependency on the target package, but add a boolean config option to enable
> breakpad to begin with.

Does this look reasonable?

config BR2_GOOGLE_BREAKPAD_ENABLE
  bool "Enable google-breakpad support"
  select BR2_PACKAGE_GOOGLE_BREAKPAD
  help
    This option will enable the use of google breakpad, a library and tool
    suite that allows you to distribute an application to users with
    compiler-provided debugging information removed, record crashes in
    compact "minidump" files, send them back to your server and produce
    C and C++ stack traces from these minidumps.  Breakpad can also
write minidumps on request for programs that have not crashed.

if BR2_GOOGLE_BREAKPAD_ENABLE

config BR2_GOOGLE_BREAKPAD_INCLUDE_FILES
  string "List of executables and libraries to extract symbols from"
  default ""
  help
    You may specify a space-separated list of binaries and libraries
    with full paths relative to $(TARGET_DIR) of which debug symbols
    will be dumped for further use with google breakpad.

    A directory structure that can be used by minidump-stackwalk will
    be created at:

    $(STAGING_DIR)/usr/share/google-breakpad-symbols

endif


regards
pascal