google-breakpad: new package
v5 cover google-breakpad: new package
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
Signature · 1 line
1.9.3
PATCH v5 1/2 google-breakpad: new package
--- 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
package/Config.in +1 -0
@@ -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"
package/google-breakpad/Config.in new file +19 -0
@@ -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)
package/google-breakpad/google-breakpad.mk new file +17 -0
@@ -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))
Signature · 1 line
1.9.3
PATCH v5 1/2 google-breakpad: new package
Quoted 27 lines · Pascal Huerst
Signed-off-by Pascal Huerst <[email protected]>--- 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.mkpackage/Config.in +1
-0@@ -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"package/google-breakpad/Config.in new file +0
-0
package/google-breakpad/Config.in line 5
Quoted 5 lines
package/google-breakpad/Config.in new file +5
-0@@ -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.
package/google-breakpad/Config.in line 12
Quoted 7 lines
package/google-breakpad/Config.in new file +7
-0+ 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.
package/google-breakpad/Config.in line 19
Quoted 7 lines
package/google-breakpad/Config.in new file +7
-0+ + 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.
package/google-breakpad/google-breakpad.mk line 7
Quoted 13 lines
package/google-breakpad/google-breakpad.mk new file +7
-0@@ -0,0 +1,17 @@ +################################################################################ +# +# google-breakpad +# +################################################################################ + +GOOGLE_BREAKPAD_VERSION = 1320
Any particular reason to take a 2-month old commit?
package/google-breakpad/google-breakpad.mk line 12
Quoted 5 lines
package/google-breakpad/google-breakpad.mk new file +5
-0+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
package/google-breakpad/google-breakpad.mk line 17
Quoted 5 lines
package/google-breakpad/google-breakpad.mk new file +5
-0+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
Quoted 1 line
Signature · 6 lines
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
PATCH v5 1/2 google-breakpad: new package
In reply to Pascal HuerstDear Pascal Huerst,
Quoted 8 lines · Pascal Huerst
Signed-off-by Pascal Huerst <[email protected]>--- 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v5 1/2 google-breakpad: new package
Hey Thomas, all
Quoted 13 lines · Thomas Petazzoni
Quoted 8 lines · Pascal Huerst
Signed-off-by Pascal Huerst <[email protected]>--- 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.mkThanks, I've applied your patch, after making some changes according to Arnout comments. From the commit log:
Quoted 13 lines
[Thomas:
- Introduce aBR2_PACKAGE_GOOGLE_BREAKPAD_ARCH_SUPPORTSConfig.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!
Quoted 2 lines
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.
Quoted 3 lines
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
PATCH v5 2/2 google-breakpad: integration into Makefile and Config.in
In reply to Pascal Huerst--- 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
Config.in +14 -0
@@ -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
Makefile +5 -0
@@ -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
package/google-breakpad/Config.in +4 -1
@@ -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/
package/google-breakpad/gen-syms.sh new file +25 -0
@@ -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"
Signature · 1 line
1.9.3
PATCH v5 2/2 google-breakpad: integration into Makefile and Config.in
Quoted 14 lines · Pascal Huerst
Signed-off-by Pascal Huerst <[email protected]>--- 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.shConfig.in +0
-0
Config.in config BR2_OPTIMIZE_S · line 483
Quoted 4 lines
Config.in +1
-0@@ -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 ofBR2_GOOGLE_BREAKPAD_GENSYMS_PATTERNS ?
Config.in config BR2_OPTIMIZE_S · line 485
Quoted 2 lines
Config.in +2
-0+ 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 :-)
Config.in config BR2_OPTIMIZE_S · line 489
Quoted 4 lines
Config.in +4
-0+ 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).
Quoted 15 lines
Config.in +7
-0+ 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_SSPMakefile +0
-0
Makefile endif · line 558
Quoted 6 lines
Makefile +3
-0@@ -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
Quoted 10 lines
Makefile +2
-0+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 -fpackage/google-breakpad/Config.in +0
-0
package/google-breakpad/Config.in config BR2_PACKAGE_GOOGLE_BREAKPAD · line 14
Quoted 5 lines
package/google-breakpad/Config.in +1
-1@@ -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.
Quoted 12 lines
package/google-breakpad/Config.in +3
-0+ 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/package/google-breakpad/gen-syms.sh new file +0
-0
package/google-breakpad/gen-syms.sh line 10
Quoted 10 lines
package/google-breakpad/gen-syms.sh new file +10
-0@@ -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
...
donedone
package/google-breakpad/gen-syms.sh line 15
Quoted 5 lines
package/google-breakpad/gen-syms.sh new file +5
-0+ 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}?
package/google-breakpad/gen-syms.sh line 17
Quoted 2 lines
package/google-breakpad/gen-syms.sh new file +2
-0+ 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
Quoted 9 lines
package/google-breakpad/gen-syms.sh new file +8
-0+ 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"
Signature · 6 lines
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
PATCH v5 2/2 google-breakpad: integration into Makefile and Config.in
Arnout, Pascal,
Quoted 4 lines · Arnout Vandecappelle
Quoted 1 line
+config
BR2_GOOGLE_BREAKPAD_INCLUDE_FILESI 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.
Quoted 9 lines
Config.in config BR2_OPTIMIZE_S · line 485
Quoted 2 lines
Config.in +2
-0+ string "executables and libraries to be used by google-breakpad" + depends on BR2_PACKAGE_GOOGLE_BREAKPADHm, 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
Quoted 9 lines
Makefile endif · line 558
Quoted 3 lines
Makefile +3
-0+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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v5 2/2 google-breakpad: integration into Makefile and Config.in
Quoted 6 lines · Thomas Petazzoni
Arnout, Pascal,
Quoted 2 lines · Arnout Vandecappelle
Quoted 1 line
+config
BR2_GOOGLE_BREAKPAD_INCLUDE_FILES
Quoted 8 lines
Quoted 2 lines
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.
Quoted 18 lines
Quoted 10 lines
Config.in config BR2_OPTIMIZE_S · line 485
Quoted 3 lines
Config.in +2
-0+ string "executables and libraries to be used by google-breakpad" + depends on BR2_PACKAGE_GOOGLE_BREAKPADHm, 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?
Quoted 4 lines
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
Quoted 15 lines
Quoted 10 lines
Makefile endif · line 558
Quoted 4 lines
Makefile +3
-0+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
Signature · 6 lines
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
PATCH v5 2/2 google-breakpad: integration into Makefile and Config.in
Hey Arnout, Thomas, all
-- snip --
Quoted 22 lines · Arnout Vandecappelle
Quoted 12 lines · Thomas Petazzoni
Quoted 2 lines · Arnout Vandecappelle
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?
Quoted 4 lines
However, I don't really like the prompt. Maybe it should be:
[ ] Enable Google Breakpad support
() List of binaries to extract symbols fromIf it is done in that way, then it really should be in the google breakpad
package and not in the toolchain menu...
Quoted 8 lines
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
PATCH v5 2/2 google-breakpad: integration into Makefile and Config.in
In reply to Thomas PetazzoniHey Thomas, Arnout, all
-- snip --
Quoted 14 lines
However, I don't really like the prompt. Maybe it should be:
[ ] Enable Google Breakpad support
() List of binaries to extract symbols fromQuoted 9 lines
Makefile endif · line 558
Quoted 3 lines
Makefile +3
-0+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?
Quoted 4 lines
Yes, having it all in the google-breakpad.mk seems like a good idea.
Thomas
Pascal