| Message ID | 1399631550-4058-1-git-send-email-pascal.huerst@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Pascal, all, Thanks a lot for this patch ! Here are a few comments inlined: On Fri, May 9, 2014 at 12:32 PM, Pascal Huerst <pascal.huerst@gmail.com> wrote: > This adds all necessary host and target tools to use google-breakpad > within buildroot. If the package is selected, the symbols needed by > google-breakpad's stackwalk are deployed to: > > output/images/google_breakpad_symbols > > Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com> > --- > Makefile | 11 ++++++++++- > package/Config.in | 1 + > package/google-breakpad/Config.in | 12 ++++++++++++ > package/google-breakpad/google-breakpad.mk | 21 +++++++++++++++++++++ > 4 files changed, 44 insertions(+), 1 deletion(-) > create mode 100644 package/google-breakpad/Config.in > create mode 100644 package/google-breakpad/google-breakpad.mk > > diff --git a/Makefile b/Makefile > index 2f18aab..613f451 100644 > --- a/Makefile > +++ b/Makefile > @@ -530,9 +530,18 @@ define TARGET_PURGE_LOCALES > endef > endif > > +# Dump symbols and create directorytree for google breakpad > +BREAKPAD_SEARCH_CMD = $(shell find $(STAGING_DIR) -type f \( -perm /111 -a ! -name "*.py" -a ! -name "*.sh" -a ! -type l -a ! -name "*~" -a ! -name "*.la" \) -print) I think, it would be better if this could be splitted as a string (without the call to shell) like STRIP_FIND_CMD. > + > +extract-breakpad-symbols: $(TARGETS) > +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) > + mkdir -p $(BINARIES_DIR)/google_breakpad_symbols > + $(PYTHON) $(HOST_DIR)/usr/bin/symbolstore.py $(HOST_DIR)/usr/bin/dump_syms $(BINARIES_DIR)/google_breakpad_symbols $(BREAKPAD_SEARCH_CMD) > +endif And here instead of $(BREAKPAD_SEARCH_CMD) I would see: $(shell $(BREAKPAD_SEARCH_CMD)) Also, I had an issues with symbolstore.py, it relies on /bin/env whereas on my Debian, it's /usr/bin/env. > + > $(TARGETS_ROOTFS): target-finalize > > -target-finalize: $(TARGETS) > +target-finalize: extract-breakpad-symbols > $(TARGET_PURGE_LOCALES) > rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ > $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ Instead of defining a new targets, I would really do as split is done. I would put this: +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) + mkdir -p $(BINARIES_DIR)/google_breakpad_symbols + $(PYTHON) $(HOST_DIR)/usr/bin/symbolstore.py $(HOST_DIR)/usr/bin/dump_syms $(BINARIES_DIR)/google_breakpad_symbols $(BREAKPAD_SEARCH_CMD) +endif Right before calling STRIP_FIND_CMD. > diff --git a/package/Config.in b/package/Config.in > index 2da27ce..e2f04c6 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -82,6 +82,7 @@ source "package/tinymembench/Config.in" > source "package/trace-cmd/Config.in" > source "package/valgrind/Config.in" > source "package/whetstone/Config.in" > +source "package/google-breakpad/Config.in" > endmenu > > menu "Development tools" > diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in > new file mode 100644 > index 0000000..fc68e6a > --- /dev/null > +++ b/package/google-breakpad/Config.in > @@ -0,0 +1,12 @@ > +config BR2_PACKAGE_GOOGLE_BREAKPAD > + bool "google-breakpad" > + depends on BR2_INSTALL_LIBSTDCPP > + depends on BR2_TOOLCHAIN_USES_GLIBC > + help > + An open-source multi-platform crash reporting system > + > + http://code.google.com/p/google-breakpad/ > + > +comment "google-breakpad requires an (e)glibc toolchain with C++" > + 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..a1c4224 > --- /dev/null > +++ b/package/google-breakpad/google-breakpad.mk > @@ -0,0 +1,21 @@ > +############################################################# > +# > +# 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 > +HOST_GOOGLE_BREAKPAD_DEPENDENCIES = host-python > +GOOGLE_BREAKPAD_INSTALL_STAGING = YES You need to specify a LICENSE and the LICENSE_FILES, have a look at http://buildroot.org/downloads/manual/manual.html#generic-package-reference for more information about theses variables. > + > +define HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE > + wget -O $(HOST_DIR)/usr/bin/symbolstore.py "http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py?raw=1" && chmod +x $(HOST_DIR)/usr/bin/symbolstore.py > +endef Here you are downloading a specific python scripts for a URL, why ? It is not included in the main sources ? Could it be a patch that goes along the package instead of downloading this like that ? Also, if you need to download it, doing it step by step, is more lisible than a really long line. Also the URL could maybe be a variable. > + > +HOST_GOOGLE_BREAKPAD_POST_INSTALL_HOOKS += HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE > + > +$(eval $(host-autotools-package)) > +$(eval $(autotools-package)) > -- > 1.9.0 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Maxime, all, thanks for your replay! See above for my thoughts on your comments. On Die, 2014-05-13 at 19:58 +0200, Maxime Hadjinlian wrote: > Hi Pascal, all, > > Thanks a lot for this patch ! > Here are a few comments inlined: > > On Fri, May 9, 2014 at 12:32 PM, Pascal Huerst <pascal.huerst@gmail.com> wrote: > > This adds all necessary host and target tools to use google-breakpad > > within buildroot. If the package is selected, the symbols needed by > > google-breakpad's stackwalk are deployed to: > > > > output/images/google_breakpad_symbols > > > > Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com> > > --- > > Makefile | 11 ++++++++++- > > package/Config.in | 1 + > > package/google-breakpad/Config.in | 12 ++++++++++++ > > package/google-breakpad/google-breakpad.mk | 21 +++++++++++++++++++++ > > 4 files changed, 44 insertions(+), 1 deletion(-) > > create mode 100644 package/google-breakpad/Config.in > > create mode 100644 package/google-breakpad/google-breakpad.mk > > > > diff --git a/Makefile b/Makefile > > index 2f18aab..613f451 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -530,9 +530,18 @@ define TARGET_PURGE_LOCALES > > endef > > endif > > > > +# Dump symbols and create directorytree for google breakpad > > +BREAKPAD_SEARCH_CMD = $(shell find $(STAGING_DIR) -type f \( -perm /111 -a ! -name "*.py" -a ! -name "*.sh" -a ! -type l -a ! -name "*~" -a ! -name "*.la" \) -print) > I think, it would be better if this could be splitted as a string > (without the call to shell) like STRIP_FIND_CMD. OK, i fixed that. > > + > > +extract-breakpad-symbols: $(TARGETS) > > +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) > > + mkdir -p $(BINARIES_DIR)/google_breakpad_symbols > > + $(PYTHON) $(HOST_DIR)/usr/bin/symbolstore.py $(HOST_DIR)/usr/bin/dump_syms $(BINARIES_DIR)/google_breakpad_symbols $(BREAKPAD_SEARCH_CMD) > > +endif > And here instead of $(BREAKPAD_SEARCH_CMD) I would see: > $(shell $(BREAKPAD_SEARCH_CMD)) > > Also, I had an issues with symbolstore.py, it relies on /bin/env > whereas on my Debian, it's /usr/bin/env. I see. Is this really an issue, if the script is called by "$(PYTHON) symbolstore.py"? Otherwise, I'm not sure how to fix that, but maybe we don't need the script at all. -> see above. > > + > > $(TARGETS_ROOTFS): target-finalize > > > > -target-finalize: $(TARGETS) > > +target-finalize: extract-breakpad-symbols > > $(TARGET_PURGE_LOCALES) > > rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ > > $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ > > Instead of defining a new targets, I would really do as split is done. > I would put this: > +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) > + mkdir -p $(BINARIES_DIR)/google_breakpad_symbols > + $(PYTHON) $(HOST_DIR)/usr/bin/symbolstore.py > $(HOST_DIR)/usr/bin/dump_syms $(BINARIES_DIR)/google_breakpad_symbols > $(BREAKPAD_SEARCH_CMD) > +endif > Right before calling STRIP_FIND_CMD. Ok, I fixed that. > > diff --git a/package/Config.in b/package/Config.in > > index 2da27ce..e2f04c6 100644 > > --- a/package/Config.in > > +++ b/package/Config.in > > @@ -82,6 +82,7 @@ source "package/tinymembench/Config.in" > > source "package/trace-cmd/Config.in" > > source "package/valgrind/Config.in" > > source "package/whetstone/Config.in" > > +source "package/google-breakpad/Config.in" > > endmenu > > > > menu "Development tools" > > diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in > > new file mode 100644 > > index 0000000..fc68e6a > > --- /dev/null > > +++ b/package/google-breakpad/Config.in > > @@ -0,0 +1,12 @@ > > +config BR2_PACKAGE_GOOGLE_BREAKPAD > > + bool "google-breakpad" > > + depends on BR2_INSTALL_LIBSTDCPP > > + depends on BR2_TOOLCHAIN_USES_GLIBC > > + help > > + An open-source multi-platform crash reporting system > > + > > + http://code.google.com/p/google-breakpad/ > > + > > +comment "google-breakpad requires an (e)glibc toolchain with C++" > > + 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..a1c4224 > > --- /dev/null > > +++ b/package/google-breakpad/google-breakpad.mk > > @@ -0,0 +1,21 @@ > > +############################################################# > > +# > > +# 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 > > +HOST_GOOGLE_BREAKPAD_DEPENDENCIES = host-python > > +GOOGLE_BREAKPAD_INSTALL_STAGING = YES > You need to specify a LICENSE and the LICENSE_FILES, have a look at > http://buildroot.org/downloads/manual/manual.html#generic-package-reference > for more information about theses variables. > > + > > +define HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE > > + wget -O $(HOST_DIR)/usr/bin/symbolstore.py "http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py?raw=1" && chmod +x $(HOST_DIR)/usr/bin/symbolstore.py > > +endef > Here you are downloading a specific python scripts for a URL, why ? It > is not included in the main sources ? Could it be a patch that goes > along the package instead of downloading this like that ? It is *not* included in the main sources. It could be a patch, but if the folks at mozilla fix something in the script, we would have to provide a new patch, right? What the script does is explained here: http://code.google.com/p/google-breakpad/wiki/LinuxStarterGuide#Producing_symbols_for_your_application I think this could be done within the makefile itself (not using the script at all), but I was not able to do it. What do you think? > Also, if you need to download it, doing it step by step, is more > lisible than a really long line. Also the URL could maybe be a > variable. Ok, I fixed that. > > + > > +HOST_GOOGLE_BREAKPAD_POST_INSTALL_HOOKS += HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE > > + > > +$(eval $(host-autotools-package)) > > +$(eval $(autotools-package)) > > -- > > 1.9.0 > > > > _______________________________________________ > > buildroot mailing list > > buildroot@busybox.net > > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Pascal, all [...] >> > diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in >> > new file mode 100644 >> > index 0000000..fc68e6a >> > --- /dev/null >> > +++ b/package/google-breakpad/Config.in >> > @@ -0,0 +1,12 @@ >> > +config BR2_PACKAGE_GOOGLE_BREAKPAD >> > + bool "google-breakpad" >> > + depends on BR2_INSTALL_LIBSTDCPP >> > + depends on BR2_TOOLCHAIN_USES_GLIBC >> > + help >> > + An open-source multi-platform crash reporting system >> > + >> > + http://code.google.com/p/google-breakpad/ >> > + >> > +comment "google-breakpad requires an (e)glibc toolchain with C++" >> > + depends on !(BR2_INSTALL_LIBSTDCPP && BR2_TOOLCHAIN_USES_GLIBC) I missed on my first review, but maybe breakpad should depends on BR2_ENABLE_DEBUG, if the package are built without debug symbols, it doesn't make a lot of sense to create symbols from them ? >> > + >> > diff --git a/package/google-breakpad/google-breakpad.mk b/package/google-breakpad/google-breakpad.mk >> > new file mode 100644 >> > index 0000000..a1c4224 >> > --- /dev/null >> > +++ b/package/google-breakpad/google-breakpad.mk >> > @@ -0,0 +1,21 @@ >> > +############################################################# >> > +# >> > +# 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 >> > +HOST_GOOGLE_BREAKPAD_DEPENDENCIES = host-python >> > +GOOGLE_BREAKPAD_INSTALL_STAGING = YES >> You need to specify a LICENSE and the LICENSE_FILES, have a look at >> http://buildroot.org/downloads/manual/manual.html#generic-package-reference >> for more information about theses variables. >> > + >> > +define HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE >> > + wget -O $(HOST_DIR)/usr/bin/symbolstore.py "http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py?raw=1" && chmod +x $(HOST_DIR)/usr/bin/symbolstore.py >> > +endef >> Here you are downloading a specific python scripts for a URL, why ? It >> is not included in the main sources ? Could it be a patch that goes >> along the package instead of downloading this like that ? > > It is *not* included in the main sources. It could be a patch, but if > the folks at mozilla fix something in the script, we would have to > provide a new patch, right? What the script does is explained here: > http://code.google.com/p/google-breakpad/wiki/LinuxStarterGuide#Producing_symbols_for_your_application > I think this could be done within the makefile itself (not using the > script at all), but I was not able to do it. What do you think? Yes, it can't be a patch, it was a bad idea. I am looking at it, and from what I see the Mozilla folks did this mostly to have a portable tool, which we don't need here since we only care about Linux. Really roughly, you would need to do something like: mkdir -p $(TMP_PATH) for bin in $(shell $(BREAKPAD_SEARCH_CMD)); do $(HOST_DIR)/usr/bin/dump_syms $(bin) > $(TMP_PATH)/$(bin) bin_id=$(shell head -n1 $(TMP_PATH)/$(bin)) mkdir -p $(BINARIES_DIR)/google_breakpad_symbols/$(bin_id) mv $(bin) $(BINARIES_DIR)/google_breakpad_symbols/$(bin_id) done Note that I haven't tested this code, I don't even know if it's up to Buildroot standards, I'm not a really good Makefile hacker :). I am pretty sure you could also do that with find and xargs, I don't know what would be preferred, I find a loop to be more readable here. That should give you an idea though and should be able to replace the python scripts.
diff --git a/Makefile b/Makefile index 2f18aab..613f451 100644 --- a/Makefile +++ b/Makefile @@ -530,9 +530,18 @@ define TARGET_PURGE_LOCALES endef endif +# Dump symbols and create directorytree for google breakpad +BREAKPAD_SEARCH_CMD = $(shell find $(STAGING_DIR) -type f \( -perm /111 -a ! -name "*.py" -a ! -name "*.sh" -a ! -type l -a ! -name "*~" -a ! -name "*.la" \) -print) + +extract-breakpad-symbols: $(TARGETS) +ifeq ($(BR2_PACKAGE_GOOGLE_BREAKPAD),y) + mkdir -p $(BINARIES_DIR)/google_breakpad_symbols + $(PYTHON) $(HOST_DIR)/usr/bin/symbolstore.py $(HOST_DIR)/usr/bin/dump_syms $(BINARIES_DIR)/google_breakpad_symbols $(BREAKPAD_SEARCH_CMD) +endif + $(TARGETS_ROOTFS): target-finalize -target-finalize: $(TARGETS) +target-finalize: extract-breakpad-symbols $(TARGET_PURGE_LOCALES) rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \ $(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \ diff --git a/package/Config.in b/package/Config.in index 2da27ce..e2f04c6 100644 --- a/package/Config.in +++ b/package/Config.in @@ -82,6 +82,7 @@ source "package/tinymembench/Config.in" source "package/trace-cmd/Config.in" source "package/valgrind/Config.in" source "package/whetstone/Config.in" +source "package/google-breakpad/Config.in" endmenu menu "Development tools" diff --git a/package/google-breakpad/Config.in b/package/google-breakpad/Config.in new file mode 100644 index 0000000..fc68e6a --- /dev/null +++ b/package/google-breakpad/Config.in @@ -0,0 +1,12 @@ +config BR2_PACKAGE_GOOGLE_BREAKPAD + bool "google-breakpad" + depends on BR2_INSTALL_LIBSTDCPP + depends on BR2_TOOLCHAIN_USES_GLIBC + help + An open-source multi-platform crash reporting system + + http://code.google.com/p/google-breakpad/ + +comment "google-breakpad requires an (e)glibc toolchain with C++" + 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..a1c4224 --- /dev/null +++ b/package/google-breakpad/google-breakpad.mk @@ -0,0 +1,21 @@ +############################################################# +# +# 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 +HOST_GOOGLE_BREAKPAD_DEPENDENCIES = host-python +GOOGLE_BREAKPAD_INSTALL_STAGING = YES + +define HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE + wget -O $(HOST_DIR)/usr/bin/symbolstore.py "http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py?raw=1" && chmod +x $(HOST_DIR)/usr/bin/symbolstore.py +endef + +HOST_GOOGLE_BREAKPAD_POST_INSTALL_HOOKS += HOST_GOOGLE_INSTALL_MOZILLA_SYMBOLSTORE + +$(eval $(host-autotools-package)) +$(eval $(autotools-package))
This adds all necessary host and target tools to use google-breakpad within buildroot. If the package is selected, the symbols needed by google-breakpad's stackwalk are deployed to: output/images/google_breakpad_symbols Signed-off-by: Pascal Huerst <pascal.huerst@gmail.com> --- Makefile | 11 ++++++++++- package/Config.in | 1 + package/google-breakpad/Config.in | 12 ++++++++++++ package/google-breakpad/google-breakpad.mk | 21 +++++++++++++++++++++ 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 package/google-breakpad/Config.in create mode 100644 package/google-breakpad/google-breakpad.mk