| Message ID | 1403702174-23850-3-git-send-email-pascal.huerst@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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, 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
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 >
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
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
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"
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