All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH v9] package/sysdig: New package
@ 2015-03-26  7:48 Angelo Compagnucci
  2015-03-26 18:38 ` Ryan Barnett
  0 siblings, 1 reply; 5+ messages in thread
From: Angelo Compagnucci @ 2015-03-26  7:48 UTC (permalink / raw)
  To: buildroot

Sysdig is open source, system-level exploration:
capture system state and activity from a running Linux
instance, then save, filter and analyze.

Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
---
Changes v8 -> v9:
  - Added SYSDIG_MAKE_ENV to correctly compile kernel driver (thanks Ryan Barnett)
  - Added an explicit SYSDIG_POST_INSTALL_TARGET_HOOKS for installing kernel driver
    as stated in the official documentation [1]
  - Fixed jsoncpp dependency
  - The patch should be in a good final shape now

[1] https://github.com/draios/sysdig/wiki/How%20to%20Install%20Sysdig%20from%20the%20Source%20Code

Changes v7 -> v8:
  - sysdig now alphabetically ordered in Config.in

Changes v6 -> v7:
  - Fixing a nasty mistake in Config.in

Changes v5 -> v6:
  - Patching kernel module makefile to be compatible with buildroot.
  - Patching cmakefile to remove unneed installation of DKMS infrastructure.
  - Removing of unneeded post installation script.
  - Added -DUSE_BUNDLED_JSONCPP = NO
  - Package is now at the bare minimum.

Changes v4 -> v5:
  - Adjusted to 80 columns for sysdig.mk header

Changes v3 -> v4:
  - Changed "depends on" to "select" and fixed selected packages
    dependencies.
  - moved "comment" section to the bottom

Changes v2 -> v3:
  - Changed "depends on" and "select" to simplify package

Changes v1 -> v2:
  - Changed "depends on" with "select" for dependencies (suggested by Baruch)
  - Added comment "sysdig needs a Linux kernel to be built" (suggested by Baruch)
  - Upgreded to recently released 0.1.99

 package/Config.in                                  |  1 +
 .../0001-makefile-driver-compile-options.patch     | 23 ++++++++++++++++
 .../sysdig/0002-remove-dkms-module-updater.patch   | 31 ++++++++++++++++++++++
 package/sysdig/Config.in                           | 21 +++++++++++++++
 package/sysdig/sysdig.mk                           | 24 +++++++++++++++++
 5 files changed, 100 insertions(+)
 create mode 100644 package/sysdig/0001-makefile-driver-compile-options.patch
 create mode 100644 package/sysdig/0002-remove-dkms-module-updater.patch
 create mode 100644 package/sysdig/Config.in
 create mode 100644 package/sysdig/sysdig.mk

diff --git a/package/Config.in b/package/Config.in
index e4ee95d..aaf12ec 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -94,6 +94,7 @@ endif
 	source "package/spidev_test/Config.in"
 	source "package/strace/Config.in"
 	source "package/stress/Config.in"
+	source "package/sysdig/Config.in"
 	source "package/sysprof/Config.in"
 	source "package/tinymembench/Config.in"
 	source "package/trace-cmd/Config.in"
diff --git a/package/sysdig/0001-makefile-driver-compile-options.patch b/package/sysdig/0001-makefile-driver-compile-options.patch
new file mode 100644
index 0000000..74c74d8
--- /dev/null
+++ b/package/sysdig/0001-makefile-driver-compile-options.patch
@@ -0,0 +1,23 @@
+Updated Makefile compile options
+
+This patch updates linux kernel module (driver) of sysdig to be
+compatible with buildroot compile flags.
+
+Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
+
+--- a/driver/Makefile.in
++++ b/driver/Makefile.in
+@@ -6,10 +6,10 @@ KERNELDIR 		?= /lib/modules/$(shell uname -r)/build
+
+ TOP := $(shell pwd)
+ all:
+-	$(MAKE) -C $(KERNELDIR) M=$(TOP) modules
++	$(MAKE) $(LINUX_MAKE_FLAGS) -C $(LINUX_DIR) M=$(TOP) modules
+
+ clean:
+-	$(MAKE) -C $(KERNELDIR) M=$(TOP) clean
++	$(MAKE) $(LINUX_MAKE_FLAGS) -C $(LINUX_DIR) M=$(TOP) clean
+
+ install: all
+-	$(MAKE) -C $(KERNELDIR) M=$(TOP) modules_install
++	$(MAKE) $(LINUX_MAKE_FLAGS) -C $(LINUX_DIR) M=$(TOP) modules_install
diff --git a/package/sysdig/0002-remove-dkms-module-updater.patch b/package/sysdig/0002-remove-dkms-module-updater.patch
new file mode 100644
index 0000000..8c0e739
--- /dev/null
+++ b/package/sysdig/0002-remove-dkms-module-updater.patch
@@ -0,0 +1,31 @@
+Rmove DKMS module updater
+
+This patch disables the in target installation of DKMS module updater
+mechanism unneeded in buildroot.
+
+Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
+
+--- a/driver/CMakeLists.txt
++++ b/driver/CMakeLists.txt
+@@ -39,21 +39,3 @@ add_custom_target(install_driver
+ 	WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
+ 	VERBATIM)
+ 
+-install(FILES ${CMAKE_CURRENT_BINARY_DIR}/Makefile.dkms
+-	RENAME Makefile
+-	DESTINATION "src/sysdig-${SYSDIG_VERSION}")
+-
+-install(FILES
+-	${CMAKE_CURRENT_BINARY_DIR}/dkms.conf
+-	dynamic_params_table.c
+-	event_table.c
+-	flags_table.c
+-	main.c
+-	ppm.h
+-	ppm_events.c
+-	ppm_events.h
+-	ppm_events_public.h
+-	ppm_fillers.c
+-	ppm_ringbuffer.h
+-	syscall_table.c
+-	DESTINATION "src/sysdig-${SYSDIG_VERSION}")
diff --git a/package/sysdig/Config.in b/package/sysdig/Config.in
new file mode 100644
index 0000000..caf7ef8
--- /dev/null
+++ b/package/sysdig/Config.in
@@ -0,0 +1,21 @@
+config BR2_PACKAGE_SYSDIG
+	bool "sysdig"
+	select BR2_PACKAGE_ZLIB
+	select BR2_PACKAGE_LUAJIT
+	select BR2_PACKAGE_JSONCPP
+	depends on BR2_LINUX_KERNEL
+	depends on BR2_INSTALL_LIBSTDCPP # libjson
+	depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS
+	help
+	  Sysdig is open source, system-level exploration:
+	  capture system state and activity from a running Linux instance,
+	  then save, filter and analyze.
+	  Think of it as strace + tcpdump + lsof + awesome sauce.
+	  With a little Lua cherry on top.
+
+	  http://sysdig.org
+
+comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
+	depends on !BR2_LINUX_KERNEL
+	depends on !BR2_INSTALL_LIBSTDCPP
+	depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS
diff --git a/package/sysdig/sysdig.mk b/package/sysdig/sysdig.mk
new file mode 100644
index 0000000..e05124b
--- /dev/null
+++ b/package/sysdig/sysdig.mk
@@ -0,0 +1,24 @@
+################################################################################
+#
+# sysdig
+#
+################################################################################
+
+SYSDIG_VERSION = 0.1.99
+SYSDIG_SITE = $(call github,draios,sysdig,$(SYSDIG_VERSION))
+SYSDIG_LICENSE = GPLv2
+SYSDIG_LICENSE_FILES = COPYING
+SYSDIG_CONF_OPTS = -DUSE_BUNDLED_LUAJIT=OFF -DUSE_BUNDLED_ZLIB=OFF \
+	-DUSE_BUNDLED_JSONCPP=OFF
+SYSDIG_DEPENDENCIES = zlib luajit jsoncpp linux
+SYSDIG_SUPPORTS_IN_SOURCE_BUILD = NO
+
+SYSDIG_MAKE_ENV = LINUX_DIR='$(LINUX_DIR)' LINUX_MAKE_FLAGS='$(LINUX_MAKE_FLAGS)'
+
+define SYSDIG_INSTALL_DRIVER
+	cd $(@D)/buildroot-build; $(MAKE) $(SYSDIG_MAKE_ENV) install_driver
+endef
+
+SYSDIG_POST_INSTALL_TARGET_HOOKS += SYSDIG_INSTALL_DRIVER
+
+$(eval $(cmake-package))
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [Buildroot] [PATCH v9] package/sysdig: New package
  2015-03-26  7:48 [Buildroot] [PATCH v9] package/sysdig: New package Angelo Compagnucci
@ 2015-03-26 18:38 ` Ryan Barnett
  2015-03-26 20:37   ` Angelo Compagnucci
  0 siblings, 1 reply; 5+ messages in thread
From: Ryan Barnett @ 2015-03-26 18:38 UTC (permalink / raw)
  To: buildroot

Hi Angelo,

On Thu, Mar 26, 2015 at 2:48 AM, Angelo Compagnucci
<angelo.compagnucci@gmail.com> wrote:
> Sysdig is open source, system-level exploration:
> capture system state and activity from a running Linux
> instance, then save, filter and analyze.
>
> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>

It would be good to carry forward previous reviews when you submit a
new version of a patch. In your v8 of this patch Yegor Yefremov
reviewed your patch so after your Signed-off-by line you should put
this:

Reviewed-by: Yegor Yefremov <yegorslists@googlemail.com>

I have a found a few other things when testing v9 and I have listed them below.

[...]

> diff --git a/package/sysdig/0002-remove-dkms-module-updater.patch b/package/sysdig/0002-remove-dkms-module-updater.patch
> new file mode 100644
> index 0000000..8c0e739
> --- /dev/null
> +++ b/package/sysdig/0002-remove-dkms-module-updater.patch
> @@ -0,0 +1,31 @@
> +Rmove DKMS module updater

Minor typo: 'Rmove' should be 'Remove'.

> +
> +This patch disables the in target installation of DKMS module updater
> +mechanism unneeded in buildroot.
> +
> +Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> +
> +--- a/driver/CMakeLists.txt
> ++++ b/driver/CMakeLists.txt
> +@@ -39,21 +39,3 @@ add_custom_target(install_driver
> +       WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
> +       VERBATIM)
> +
> +-install(FILES ${CMAKE_CURRENT_BINARY_DIR}/Makefile.dkms
> +-      RENAME Makefile
> +-      DESTINATION "src/sysdig-${SYSDIG_VERSION}")
> +-
> +-install(FILES
> +-      ${CMAKE_CURRENT_BINARY_DIR}/dkms.conf
> +-      dynamic_params_table.c
> +-      event_table.c
> +-      flags_table.c
> +-      main.c
> +-      ppm.h
> +-      ppm_events.c
> +-      ppm_events.h
> +-      ppm_events_public.h
> +-      ppm_fillers.c
> +-      ppm_ringbuffer.h
> +-      syscall_table.c
> +-      DESTINATION "src/sysdig-${SYSDIG_VERSION}")
> diff --git a/package/sysdig/Config.in b/package/sysdig/Config.in
> new file mode 100644
> index 0000000..caf7ef8
> --- /dev/null
> +++ b/package/sysdig/Config.in
> @@ -0,0 +1,21 @@
> +config BR2_PACKAGE_SYSDIG
> +       bool "sysdig"
> +       select BR2_PACKAGE_ZLIB
> +       select BR2_PACKAGE_LUAJIT
> +       select BR2_PACKAGE_JSONCPP
> +       depends on BR2_LINUX_KERNEL
> +       depends on BR2_INSTALL_LIBSTDCPP # libjson
> +       depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS
> +       help
> +         Sysdig is open source, system-level exploration:
> +         capture system state and activity from a running Linux instance,
> +         then save, filter and analyze.
> +         Think of it as strace + tcpdump + lsof + awesome sauce.
> +         With a little Lua cherry on top.
> +
> +         http://sysdig.org
> +
> +comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
> +       depends on !BR2_LINUX_KERNEL
> +       depends on !BR2_INSTALL_LIBSTDCPP
> +       depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS

When you have a toolchain that doesn't have one of the required
dependencies, this comment doesn't show up. This is due to have 3
separate depends. Instead the depends should look like this:

comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
        depends on !BR2_LINUX_KERNEL || !BR2_INSTALL_LIBSTDCPP \
                  || !BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS

(Note indentation above should be tabs instead of spaces)

> diff --git a/package/sysdig/sysdig.mk b/package/sysdig/sysdig.mk
> new file mode 100644
> index 0000000..e05124b
> --- /dev/null
> +++ b/package/sysdig/sysdig.mk
> @@ -0,0 +1,24 @@
> +################################################################################
> +#
> +# sysdig
> +#
> +################################################################################
> +
> +SYSDIG_VERSION = 0.1.99
> +SYSDIG_SITE = $(call github,draios,sysdig,$(SYSDIG_VERSION))
> +SYSDIG_LICENSE = GPLv2
> +SYSDIG_LICENSE_FILES = COPYING
> +SYSDIG_CONF_OPTS = -DUSE_BUNDLED_LUAJIT=OFF -DUSE_BUNDLED_ZLIB=OFF \
> +       -DUSE_BUNDLED_JSONCPP=OFF
> +SYSDIG_DEPENDENCIES = zlib luajit jsoncpp linux
> +SYSDIG_SUPPORTS_IN_SOURCE_BUILD = NO
> +
> +SYSDIG_MAKE_ENV = LINUX_DIR='$(LINUX_DIR)' LINUX_MAKE_FLAGS='$(LINUX_MAKE_FLAGS)'
> +
> +define SYSDIG_INSTALL_DRIVER
> +       cd $(@D)/buildroot-build; $(MAKE) $(SYSDIG_MAKE_ENV) install_driver
> +endef
> +
> +SYSDIG_POST_INSTALL_TARGET_HOOKS += SYSDIG_INSTALL_DRIVER
> +
> +$(eval $(cmake-package))

With these final fixups I think we are about done with some issues. I
will look for you v10

Thanks,
-Ryan

-- 
Ryan Barnett / Sr Software Engineer
Airborne Information Systems / Security Systems and Software
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
ryan.barnett at rockwellcollins.com
www.rockwellcollins.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Buildroot] [PATCH v9] package/sysdig: New package
  2015-03-26 18:38 ` Ryan Barnett
@ 2015-03-26 20:37   ` Angelo Compagnucci
  2015-03-27  0:46     ` Ryan Barnett
  2015-03-27 21:57     ` Arnout Vandecappelle
  0 siblings, 2 replies; 5+ messages in thread
From: Angelo Compagnucci @ 2015-03-26 20:37 UTC (permalink / raw)
  To: buildroot

Dear Ryan Barnett,

2015-03-26 19:38 GMT+01:00 Ryan Barnett <ryan.barnett@rockwellcollins.com>:
> Hi Angelo,
>
> On Thu, Mar 26, 2015 at 2:48 AM, Angelo Compagnucci
> <angelo.compagnucci@gmail.com> wrote:
>> Sysdig is open source, system-level exploration:
>> capture system state and activity from a running Linux
>> instance, then save, filter and analyze.
>>
>> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
>
> It would be good to carry forward previous reviews when you submit a
> new version of a patch. In your v8 of this patch Yegor Yefremov
> reviewed your patch so after your Signed-off-by line you should put
> this:
>
> Reviewed-by: Yegor Yefremov <yegorslists@googlemail.com>

Are you sure? IMO, If the patch changes it should be reviewed again.

>
> I have a found a few other things when testing v9 and I have listed them below.
>
> [...]
>
>> diff --git a/package/sysdig/0002-remove-dkms-module-updater.patch b/package/sysdig/0002-remove-dkms-module-updater.patch
>> new file mode 100644
>> index 0000000..8c0e739
>> --- /dev/null
>> +++ b/package/sysdig/0002-remove-dkms-module-updater.patch
>> @@ -0,0 +1,31 @@
>> +Rmove DKMS module updater
>
> Minor typo: 'Rmove' should be 'Remove'.

good catch!

>
>> +
>> +This patch disables the in target installation of DKMS module updater
>> +mechanism unneeded in buildroot.
>> +
>> +Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
>> +
>> +--- a/driver/CMakeLists.txt
>> ++++ b/driver/CMakeLists.txt
>> +@@ -39,21 +39,3 @@ add_custom_target(install_driver
>> +       WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
>> +       VERBATIM)
>> +
>> +-install(FILES ${CMAKE_CURRENT_BINARY_DIR}/Makefile.dkms
>> +-      RENAME Makefile
>> +-      DESTINATION "src/sysdig-${SYSDIG_VERSION}")
>> +-
>> +-install(FILES
>> +-      ${CMAKE_CURRENT_BINARY_DIR}/dkms.conf
>> +-      dynamic_params_table.c
>> +-      event_table.c
>> +-      flags_table.c
>> +-      main.c
>> +-      ppm.h
>> +-      ppm_events.c
>> +-      ppm_events.h
>> +-      ppm_events_public.h
>> +-      ppm_fillers.c
>> +-      ppm_ringbuffer.h
>> +-      syscall_table.c
>> +-      DESTINATION "src/sysdig-${SYSDIG_VERSION}")
>> diff --git a/package/sysdig/Config.in b/package/sysdig/Config.in
>> new file mode 100644
>> index 0000000..caf7ef8
>> --- /dev/null
>> +++ b/package/sysdig/Config.in
>> @@ -0,0 +1,21 @@
>> +config BR2_PACKAGE_SYSDIG
>> +       bool "sysdig"
>> +       select BR2_PACKAGE_ZLIB
>> +       select BR2_PACKAGE_LUAJIT
>> +       select BR2_PACKAGE_JSONCPP
>> +       depends on BR2_LINUX_KERNEL
>> +       depends on BR2_INSTALL_LIBSTDCPP # libjson
>> +       depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS
>> +       help
>> +         Sysdig is open source, system-level exploration:
>> +         capture system state and activity from a running Linux instance,
>> +         then save, filter and analyze.
>> +         Think of it as strace + tcpdump + lsof + awesome sauce.
>> +         With a little Lua cherry on top.
>> +
>> +         http://sysdig.org
>> +
>> +comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
>> +       depends on !BR2_LINUX_KERNEL
>> +       depends on !BR2_INSTALL_LIBSTDCPP
>> +       depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS
>
> When you have a toolchain that doesn't have one of the required
> dependencies, this comment doesn't show up. This is due to have 3
> separate depends. Instead the depends should look like this:
>
> comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
>         depends on !BR2_LINUX_KERNEL || !BR2_INSTALL_LIBSTDCPP \
>                   || !BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS

Good catch, but the message should be viewed only on supported arhcs,
so it should be:

 comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
         depends on !BR2_LINUX_KERNEL || !BR2_INSTALL_LIBSTDCPP
         depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS

> (Note indentation above should be tabs instead of spaces)
>
>> diff --git a/package/sysdig/sysdig.mk b/package/sysdig/sysdig.mk
>> new file mode 100644
>> index 0000000..e05124b
>> --- /dev/null
>> +++ b/package/sysdig/sysdig.mk
>> @@ -0,0 +1,24 @@
>> +################################################################################
>> +#
>> +# sysdig
>> +#
>> +################################################################################
>> +
>> +SYSDIG_VERSION = 0.1.99
>> +SYSDIG_SITE = $(call github,draios,sysdig,$(SYSDIG_VERSION))
>> +SYSDIG_LICENSE = GPLv2
>> +SYSDIG_LICENSE_FILES = COPYING
>> +SYSDIG_CONF_OPTS = -DUSE_BUNDLED_LUAJIT=OFF -DUSE_BUNDLED_ZLIB=OFF \
>> +       -DUSE_BUNDLED_JSONCPP=OFF
>> +SYSDIG_DEPENDENCIES = zlib luajit jsoncpp linux
>> +SYSDIG_SUPPORTS_IN_SOURCE_BUILD = NO
>> +
>> +SYSDIG_MAKE_ENV = LINUX_DIR='$(LINUX_DIR)' LINUX_MAKE_FLAGS='$(LINUX_MAKE_FLAGS)'
>> +
>> +define SYSDIG_INSTALL_DRIVER
>> +       cd $(@D)/buildroot-build; $(MAKE) $(SYSDIG_MAKE_ENV) install_driver
>> +endef
>> +
>> +SYSDIG_POST_INSTALL_TARGET_HOOKS += SYSDIG_INSTALL_DRIVER
>> +
>> +$(eval $(cmake-package))
>
> With these final fixups I think we are about done with some issues. I
> will look for you v10
>
> Thanks,
> -Ryan
>
> --
> Ryan Barnett / Sr Software Engineer
> Airborne Information Systems / Security Systems and Software
> MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
> ryan.barnett at rockwellcollins.com
> www.rockwellcollins.com



-- 
Profile: http://it.linkedin.com/in/compagnucciangelo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Buildroot] [PATCH v9] package/sysdig: New package
  2015-03-26 20:37   ` Angelo Compagnucci
@ 2015-03-27  0:46     ` Ryan Barnett
  2015-03-27 21:57     ` Arnout Vandecappelle
  1 sibling, 0 replies; 5+ messages in thread
From: Ryan Barnett @ 2015-03-27  0:46 UTC (permalink / raw)
  To: buildroot

On Thu, Mar 26, 2015 at 3:37 PM, Angelo Compagnucci
<angelo.compagnucci@gmail.com> wrote:
> Dear Ryan Barnett,
>
> Are you sure? IMO, If the patch changes it should be reviewed again.

You do bring up a good point. However, I was basing this on what Yann
did with his hash improvement series:

http://patchwork.ozlabs.org/patch/453147/

> Good catch, but the message should be viewed only on supported arhcs,
> so it should be:
>
>  comment "sysdig needs a toolchain w/ C++ and a Linux kernel to be built"
>          depends on !BR2_LINUX_KERNEL || !BR2_INSTALL_LIBSTDCPP
>          depends on BR2_PACKAGE_LUAJIT_ARCH_SUPPORTS

Yes I agree with this.

Thanks,
-Ryan

-- 
Ryan Barnett / Sr Software Engineer
Airborne Information Systems / Security Systems and Software
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
ryan.barnett at rockwellcollins.com
www.rockwellcollins.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Buildroot] [PATCH v9] package/sysdig: New package
  2015-03-26 20:37   ` Angelo Compagnucci
  2015-03-27  0:46     ` Ryan Barnett
@ 2015-03-27 21:57     ` Arnout Vandecappelle
  1 sibling, 0 replies; 5+ messages in thread
From: Arnout Vandecappelle @ 2015-03-27 21:57 UTC (permalink / raw)
  To: buildroot

On 26/03/15 21:37, Angelo Compagnucci wrote:
> Dear Ryan Barnett,
> 
> 2015-03-26 19:38 GMT+01:00 Ryan Barnett <ryan.barnett@rockwellcollins.com>:
>> > Hi Angelo,
>> >
>> > On Thu, Mar 26, 2015 at 2:48 AM, Angelo Compagnucci
>> > <angelo.compagnucci@gmail.com> wrote:
>>> >> Sysdig is open source, system-level exploration:
>>> >> capture system state and activity from a running Linux
>>> >> instance, then save, filter and analyze.
>>> >>
>>> >> Signed-off-by: Angelo Compagnucci <angelo.compagnucci@gmail.com>
>> >
>> > It would be good to carry forward previous reviews when you submit a
>> > new version of a patch. In your v8 of this patch Yegor Yefremov
>> > reviewed your patch so after your Signed-off-by line you should put
>> > this:
>> >
>> > Reviewed-by: Yegor Yefremov <yegorslists@googlemail.com>
> Are you sure? IMO, If the patch changes it should be reviewed again.

 It basically depends on how much you still changed after that reviewed-by was
given. In this case, it indeed probably changed too much to keep the tag.

 For Yegor's convenience, however, it could be good to add something like:

Previous-version-reviewed-by: ...

(At least in my case, I tend to look more carefully at the patches I reviewed
before, and I tend to forget which ones I reviewed in the past.)


 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-03-27 21:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-26  7:48 [Buildroot] [PATCH v9] package/sysdig: New package Angelo Compagnucci
2015-03-26 18:38 ` Ryan Barnett
2015-03-26 20:37   ` Angelo Compagnucci
2015-03-27  0:46     ` Ryan Barnett
2015-03-27 21:57     ` Arnout Vandecappelle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.