<div dir="ltr"><div><div><div><div><div>Hi Thomas,<br><br></div>Thank you for your reply and your comments.<br><br></div>Here is a new version of the package.<br> <br></div>Concerning the work flow, I sent the first patch via "git send-email", which was a bit laconic.. Is it ok if I just attach the patch file to a "regular" email ? <br>
<br></div>Best regards,<br><br></div>Pierre <br><div><div><div><br> <br></div></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 30, 2013 at 6:59 PM, Thomas De Schampheleire <span dir="ltr"><<a href="mailto:patrickdepinguin+buildroot@gmail.com" target="_blank">patrickdepinguin+buildroot@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Pierre,<br>
<br>
Thanks for your contribution! Below are some comments...<br>
<div><div class="h5"><br>
On Tue, Jul 30, 2013 at 4:03 PM, <<a href="mailto:p.floury@gmail.com">p.floury@gmail.com</a>> wrote:<br>
> From: pierre <<a href="mailto:pierre.floury@gmail.com">pierre.floury@gmail.com</a>><br>
><br>
> ---<br>
> package/Config.in | 1 +<br>
> package/tracecmd/Config.in | 9 +++++++++<br>
> package/tracecmd/<a href="http://tracecmd.mk" target="_blank">tracecmd.mk</a> | 24 ++++++++++++++++++++++++<br>
> 3 files changed, 34 insertions(+)<br>
> create mode 100644 package/tracecmd/Config.in<br>
> create mode 100644 package/tracecmd/<a href="http://tracecmd.mk" target="_blank">tracecmd.mk</a><br>
><br>
> diff --git a/package/Config.in b/package/Config.in<br>
> index d980871..4a7bc5d 100644<br>
> --- a/package/Config.in<br>
> +++ b/package/Config.in<br>
> @@ -39,6 +39,7 @@ source "package/memstat/Config.in"<br>
> source "package/netperf/Config.in"<br>
> source "package/oprofile/Config.in"<br>
> source "package/perf/Config.in"<br>
> +source "package/tracecmd/Config.in"<br>
> source "package/ramspeed/Config.in"<br>
> source "package/rt-tests/Config.in"<br>
> source "package/strace/Config.in"<br>
> diff --git a/package/tracecmd/Config.in b/package/tracecmd/Config.in<br>
> new file mode 100644<br>
> index 0000000..cb24816<br>
> --- /dev/null<br>
> +++ b/package/tracecmd/Config.in<br>
> @@ -0,0 +1,9 @@<br>
> +config BR2_PACKAGE_TRACECMD<br>
> + bool "trace-cmd"<br>
> + help<br>
> + trace-cmd - command line reader for ftrace<br>
> + need to have ftrace enable into your kernel:<br>
> +<br>
> + <a href="http://git.kernel.org/cgit/linux/kernel/git/rostedt/trace-cmd.git" target="_blank">http://git.kernel.org/cgit/linux/kernel/git/rostedt/trace-cmd.git</a><br>
<br>
</div></div>The help text should not repeat the name of the command.<br>
Also, please remove the colon (:) and make the second line a real<br>
sentence, for example:<br>
"To do anything useful with this, you should enable ftrace in your<br>
kernel configuration"<br>
<div class="im"><br>
> +<br>
> +comment "trace-cmd - command line reader for ftrace"<br>
<br>
</div>This comment should not be there. We only put comments when packages<br>
are hidden due to missing dependencies.<br>
<div class="im"><br>
> diff --git a/package/tracecmd/<a href="http://tracecmd.mk" target="_blank">tracecmd.mk</a> b/package/tracecmd/<a href="http://tracecmd.mk" target="_blank">tracecmd.mk</a><br>
> new file mode 100644<br>
> index 0000000..3ddff28<br>
> --- /dev/null<br>
> +++ b/package/tracecmd/<a href="http://tracecmd.mk" target="_blank">tracecmd.mk</a><br>
> @@ -0,0 +1,24 @@<br>
> +#############################################################<br>
> +#<br>
> +# tracecmd<br>
> +#<br>
> +#############################################################<br>
> +<br>
> +TRACECMD_VERSION = 8c10a774f1f8586cd8b0e<br>
> +TRACECMD_SITE = <a href="http://git.kernel.org/cgit/linux/kernel/git/rostedt/trace-cmd.git" target="_blank">http://git.kernel.org/cgit/linux/kernel/git/rostedt/trace-cmd.git</a><br>
<br>
</div>This version is tagged, so you'd preferable use that for the version:<br>
trace-cmd-v2.2.1<br>
<div class="im"><br>
> +TRACECMD_SITE_METHOD = git<br>
> +TRACECMD_INSTALL_STAGING = YES<br>
> +TRACECMD_LICENSE = GPL<br>
> +# No license file, the license is in the installed header<br>
> +TRACECMD_LICENSE_FILES = COPYING<br>
<br>
</div>What do you mean with this comment 'No license file'? There is a<br>
COPYING file, so I don't see the point.<br>
<br>
Also, the license should be as specific as possible, so GPLv2, GPLv2+<br>
etc. See <a href="http://buildroot.uclibc.org/downloads/manual/manual.html#legal-info-list-licenses" target="_blank">http://buildroot.uclibc.org/downloads/manual/manual.html#legal-info-list-licenses</a><br>
for the list of known abbreviations.<br>
<br>
In the case of tracecmd, there both are GPLv2 as LGPLv2.1 files, so<br>
both licenses should be specified, as well as the license files<br>
COPYING and COPYING.LIB.<br>
<div class="im"><br>
> +define TRACECMD_BUILD_CMDS<br>
> + $(MAKE) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D) all<br>
> +endef<br>
> +<br>
> +define TRACECMD_INSTALL_TARGET_CMDS<br>
> + $(INSTALL) -D -m 0755 $(@D)/trace-cmd $(TARGET_DIR)/usr/bin<br>
> + $(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/lib/trace-cmd/plugins<br>
> + $(INSTALL) -D -m 0755 $(@D)/plugin_*.so $(TARGET_DIR)/usr/lib/trace-cmd/plugins<br>
> +endef<br>
> +<br>
> +$(eval $(generic-package))<br>
<br>
</div>If you send an updated version, please also fix the typo in the title:<br>
pacakge -> package.<br>
<br>
Best regards,<br>
Thomas<br>
</blockquote></div><br></div>