| Message ID | 1509822714-17178-1-git-send-email-sergio.prado@e-labworks.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | schedtop: new package | expand |
Hello, On Sat, 4 Nov 2017 17:11:54 -0200, Sergio Prado wrote: > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com> Thanks for this contribution! See a few comments/suggestions below. > diff --git a/package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch b/package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch > new file mode 100644 > index 000000000000..d0dc278dfb70 > --- /dev/null > +++ b/package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch > @@ -0,0 +1,54 @@ > +From 845e74ffc0d280163b66d81df963b4c3738f9666 Mon Sep 17 00:00:00 2001 > +From: Sergio Prado <sergio.prado@e-labworks.com> > +Date: Sun, 29 Oct 2017 13:20:10 -0200 > +Subject: [PATCH] Fix build with Boost Filesystem library V3 > + > +In version 3 of the boost filesystem library, string() is a member of > +path(), not directory_entry. > + > +Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com> What about using https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765 which does pretty much the same thing, but with additional fixes ? > diff --git a/package/schedtop/schedtop.mk b/package/schedtop/schedtop.mk > new file mode 100644 > index 000000000000..656e3716f26b > --- /dev/null > +++ b/package/schedtop/schedtop.mk > @@ -0,0 +1,30 @@ > +################################################################################ > +# > +# schedtop > +# > +################################################################################ > + > +SCHEDTOP_VERSION = 68dee649f96b3bf6db883de67f68ccc0b45cbc6e > +SCHEDTOP_SITE = $(call github,ghaskins,schedtop,$(SCHEDTOP_VERSION)) > +SCHEDTOP_LICENSE = GPL-2.0 > +SCHEDTOP_LICENSE_FILES = COPYING > + > +SCHEDTOP_DEPENDENCIES = boost ncurses > + > +SCHEDTOP_MAKE_ENV = $(TARGET_MAKE_ENV) OBJDIR="$(@D)/obj" LIBRARIES="-lboost_system" Why do you need to customize OBJDIR ? The LIBRARIES change would be taken care of if a patch like https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765 is used. > +# uses __atomic_fetch_add_4 > +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y) > +SCHEDTOP_MAKE_ENV += LIBRARIES+=" -latomic" Are you sure += in shell as the expected result ? Thanks! Thomas
Hello Thomas, 2017-11-23 20:13 GMT-02:00 Thomas Petazzoni < thomas.petazzoni@free-electrons.com>: > > Hello, > > On Sat, 4 Nov 2017 17:11:54 -0200, Sergio Prado wrote: > > Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com> > > Thanks for this contribution! See a few comments/suggestions below. Thanks for reviewing the patch! > What about using > https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765 > which does pretty much the same thing, but with additional fixes ? I did not use it because it was not (yet) accepted upstream and it does more than just fixing build errors, like changing an application error message. Should we use patches not accepted upstream that changes application behavior? > > +SCHEDTOP_MAKE_ENV = $(TARGET_MAKE_ENV) OBJDIR="$(@D)/obj" LIBRARIES="-lboost_system" > > Why do you need to customize OBJDIR ? As you can see from the application's Makefile, OBJDIR is set to the host architecture name: ARCH=$(shell uname -m) OBJDIR ?= obj/$(ARCH) And then in the end of the build you can see some build commands using the "obj/x86_64/" directory, and I thought that would be confuse, since we are cross-compiling. But I can remove it if you do not see any problem with this behavior. > The LIBRARIES change would be taken care of if a patch like > https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765 > is used. So it is better to patch the application's Makefile instead of changing the variable in the package's makefile? > > > +# uses __atomic_fetch_add_4 > > +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y) > > +SCHEDTOP_MAKE_ENV += LIBRARIES+=" -latomic" > > Are you sure += in shell as the expected result ? Yes, it works for shell scripts: $ cat testvar.sh #!/bin/sh echo TESTVAR="$TESTVAR" $ TESTVAR=1 TESTVAR+=2 ./testvar.sh TESTVAR=12 And for Makefiles: $ cat Makefile TESTVAR?=1 all: @echo TESTVAR="$(TESTVAR)" $ TESTVAR=A TESTVAR+=B make TESTVAR=AB Do you see any problem doing that? I can create an extra variable and append to it before passing to SCHEDTOP_MAKE_ENV, but that would make the makefile a little bit more complex. What do you think? > > Thanks! Thanks! Best regards, Sergio Prado Embedded Labworks <div dir="ltr">Hello Thomas,<br><br>2017-11-23 20:13 GMT-02:00 Thomas Petazzoni <<a href="mailto:thomas.petazzoni@free-electrons.com">thomas.petazzoni@free-electrons.com</a>>:<br>><br>> Hello,<br>><br>> On Sat, 4 Nov 2017 17:11:54 -0200, Sergio Prado wrote:<br>> > Signed-off-by: Sergio Prado <<a href="mailto:sergio.prado@e-labworks.com">sergio.prado@e-labworks.com</a>><br>><br>> Thanks for this contribution! See a few comments/suggestions below.<div><br></div><div>Thanks for reviewing the patch!</div><div><br>> What about using<br>> <a href="https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765">https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765</a><br>> which does pretty much the same thing, but with additional fixes ?</div><div><br></div><div>I did not use it because it was not (yet) accepted upstream and it does more than just fixing build errors, like changing an application error message. Should we use patches not accepted upstream that changes application behavior?</div><div><br>> > +SCHEDTOP_MAKE_ENV = $(TARGET_MAKE_ENV) OBJDIR="$(@D)/obj" LIBRARIES="-lboost_system"<br>><br>> Why do you need to customize OBJDIR ?</div><div><br></div><div>As you can see from the application's Makefile, OBJDIR is set to the host architecture name:</div><div><br></div><div><div>ARCH=$(shell uname -m)</div><div>OBJDIR ?= obj/$(ARCH)</div></div><div><br></div><div>And then in the end of the build you can see some build commands using the "obj/x86_64/" directory, and I thought that would be confuse, since we are cross-compiling. But I can remove it if you do not see any problem with this behavior.</div><div><br></div><div>> The LIBRARIES change would be taken care of if a patch like<br>> <a href="https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765">https://github.com/ghaskins/schedtop/pull/1/commits/19d9d3d16e4731c97f1395ba2127fe48a46bb765</a><br>> is used.</div><div><br></div><div>So it is better to patch the application's Makefile instead of changing the variable in the package's makefile?</div><div><br></div><div>><br>> > +# uses __atomic_fetch_add_4<br>> > +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)<br>> > +SCHEDTOP_MAKE_ENV += LIBRARIES+=" -latomic"<br>><br>> Are you sure += in shell as the expected result ?</div><div><br></div><div>Yes, it works for shell scripts:</div><div><br></div><div><div>$ cat testvar.sh </div><div>#!/bin/sh</div><div>echo TESTVAR="$TESTVAR"</div></div><div><br></div><div><div>$ TESTVAR=1 TESTVAR+=2 ./testvar.sh </div><div>TESTVAR=12</div></div><div><br></div><div>And for Makefiles:</div><div><br></div><div><div>$ cat Makefile </div><div>TESTVAR?=1</div><div>all:</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span>@echo TESTVAR="$(TESTVAR)"</div></div><div><br></div><div><div>$ TESTVAR=A TESTVAR+=B make</div><div>TESTVAR=AB</div></div><div><br></div><div>Do you see any problem doing that? I can create an extra variable and append to it before passing to SCHEDTOP_MAKE_ENV, but that would make the makefile a little bit more complex. What do you think?</div><div><br>><br>> Thanks!</div><div><br></div><div>Thanks!</div><div><br></div><div>Best regards,</div><div><br></div><div>Sergio Prado</div><div>Embedded Labworks</div></div>
diff --git a/DEVELOPERS b/DEVELOPERS index f0f83b0f4291..8516a6c81e55 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -1412,6 +1412,7 @@ F: package/xr819-xradio/ N: Sergio Prado <sergio.prado@e-labworks.com> F: package/libgdiplus/ F: package/mongodb/ +F: package/schedtop/ F: package/stella/ F: package/tunctl/ F: package/ubus/ diff --git a/package/Config.in b/package/Config.in index 55fe80139eaa..bda63c0804fb 100644 --- a/package/Config.in +++ b/package/Config.in @@ -1936,6 +1936,7 @@ menu "System tools" source "package/s6-linux-utils/Config.in" source "package/s6-portable-utils/Config.in" source "package/s6-rc/Config.in" + source "package/schedtop/Config.in" source "package/scrub/Config.in" source "package/scrypt/Config.in" source "package/smack/Config.in" diff --git a/package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch b/package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch new file mode 100644 index 000000000000..d0dc278dfb70 --- /dev/null +++ b/package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch @@ -0,0 +1,54 @@ +From 845e74ffc0d280163b66d81df963b4c3738f9666 Mon Sep 17 00:00:00 2001 +From: Sergio Prado <sergio.prado@e-labworks.com> +Date: Sun, 29 Oct 2017 13:20:10 -0200 +Subject: [PATCH] Fix build with Boost Filesystem library V3 + +In version 3 of the boost filesystem library, string() is a member of +path(), not directory_entry. + +Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com> +--- + schedtop.cc | 8 ++++---- + 1 file changed, 4 insertions(+), 4 deletions(-) + +diff --git a/schedtop.cc b/schedtop.cc +index 08aebae7170c..887b5e8b2f8f 100644 +--- a/schedtop.cc ++++ b/schedtop.cc +@@ -267,21 +267,21 @@ void ProcSnapshot(StatMap &smap) + fs::directory_iterator end; + for (fs::directory_iterator iter("/proc"); iter != end; ++iter) { + +- std::string path(iter->string() + "/schedstat"); ++ std::string path(iter->path().string() + "/schedstat"); + if (fs::exists(path)) { + std::ifstream is(path.c_str()); + + if (!is.is_open()) + throw std::runtime_error("could not open " + path); + +- Importer importer(smap, is, iter->string() + "/"); ++ Importer importer(smap, is, iter->path().string() + "/"); + + importer += "sched_info.cpu_time"; + importer += "sched_info.run_delay"; + importer += "sched_info.pcount"; + } + +- path = iter->string() + "/sched"; ++ path = iter->path().string() + "/sched"; + if (fs::exists(path)) { + std::ifstream is(path.c_str()); + +@@ -312,7 +312,7 @@ void ProcSnapshot(StatMap &smap) + lis >> tmp; + + Importer importer(smap, lis, +- iter->string() + "/"); ++ iter->path().string() + "/"); + + importer += type; + } +-- +1.9.1 + diff --git a/package/schedtop/Config.in b/package/schedtop/Config.in new file mode 100644 index 000000000000..2c13e50bb8e2 --- /dev/null +++ b/package/schedtop/Config.in @@ -0,0 +1,25 @@ +config BR2_PACKAGE_SCHEDTOP + bool "schedtop" + depends on BR2_INSTALL_LIBSTDCPP # boost + depends on BR2_TOOLCHAIN_HAS_THREADS # boost + depends on BR2_USE_WCHAR # boost + select BR2_PACKAGE_BOOST + select BR2_PACKAGE_BOOST_REGEX + select BR2_PACKAGE_BOOST_SYSTEM + select BR2_PACKAGE_BOOST_FILESYSTEM + select BR2_PACKAGE_BOOST_PROGRAM_OPTIONS + select BR2_PACKAGE_NCURSES + help + This utility will process statistics from /proc/schedstat + such that the busiest stats will bubble up to the top. It + can alternately be sorted by the largest stat, or by name. + Stats can be included or excluded based on reg-ex pattern + matching. + + You should enable CONFIG_SCHEDSTATS in your Linux kernel + configuration to use this utility. + + https://github.com/ghaskins/schedtop + +comment "schedtop needs a toolchain w/ C++, threads, wchar" + depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_USE_WCHAR diff --git a/package/schedtop/schedtop.mk b/package/schedtop/schedtop.mk new file mode 100644 index 000000000000..656e3716f26b --- /dev/null +++ b/package/schedtop/schedtop.mk @@ -0,0 +1,30 @@ +################################################################################ +# +# schedtop +# +################################################################################ + +SCHEDTOP_VERSION = 68dee649f96b3bf6db883de67f68ccc0b45cbc6e +SCHEDTOP_SITE = $(call github,ghaskins,schedtop,$(SCHEDTOP_VERSION)) +SCHEDTOP_LICENSE = GPL-2.0 +SCHEDTOP_LICENSE_FILES = COPYING + +SCHEDTOP_DEPENDENCIES = boost ncurses + +SCHEDTOP_MAKE_ENV = $(TARGET_MAKE_ENV) OBJDIR="$(@D)/obj" LIBRARIES="-lboost_system" + +# uses __atomic_fetch_add_4 +ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y) +SCHEDTOP_MAKE_ENV += LIBRARIES+=" -latomic" +endif + +define SCHEDTOP_BUILD_CMDS + $(SCHEDTOP_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D) +endef + +define SCHEDTOP_INSTALL_TARGET_CMDS + $(SCHEDTOP_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D) \ + PREFIX=$(TARGET_DIR) install +endef + +$(eval $(generic-package))
Signed-off-by: Sergio Prado <sergio.prado@e-labworks.com> --- DEVELOPERS | 1 + package/Config.in | 1 + ...ix-build-with-Boost-Filesystem-library-V3.patch | 54 ++++++++++++++++++++++ package/schedtop/Config.in | 25 ++++++++++ package/schedtop/schedtop.mk | 30 ++++++++++++ 5 files changed, 111 insertions(+) create mode 100644 package/schedtop/0001-Fix-build-with-Boost-Filesystem-library-V3.patch create mode 100644 package/schedtop/Config.in create mode 100644 package/schedtop/schedtop.mk