Batctl Package try 2

Hi,

after a review from Yann E. MORIN i made some adjustments to the package. I
this version is ok i'll use git send mail to send in an propper patch.

Thank you
Jens

+$(eval $(generic-package))

Jens, All,

On 2015-02-08 11:58 +0000, Jens Zettelmeyer spake thusly:
> after a review from Yann E. MORIN i made some adjustments to the package. I
> this version is ok i'll use git send mail to send in an propper patch.

git-send-email can (should) be used always, not only to send preliminary
patches for review.

Not doing so means your patch gets mangled by your mailer.

First, you sent an HTML mail, which is frowned upon, and sometime even
rejected by the list (yours managed to slip trhough, though). Using
git-send-email ensures the mail is plain-text only.

Then, what if your patch was already correct? If you had sent it with
git-send-email, we could have applied it as-is. Now, doing the way you
did means we'd have to ask you to resend it, even if it was correct.

Just always use git-send-email, it makes life easier for you and for us.
And easy life is better! ;-)

> Thank you
> Jens
> 
> diff --git a/package/batctl/Config.in b/package/batctl/Config.in
> new file mode 100644
> index 0000000..56badd3
> --- /dev/null
> +++ b/package/batctl/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_BATCTL
> + bool "batctl"
> + depends on BR2_INET_IPV6
> + depends on BR2_TOOLCHAIN_HAS_THREADS # libnl
> + select BR2_PACKAGE_LIBNL
> + help
> +  Batctl is the configuration and debugging tool for batman-adv.
> +
> +  http://www.open-mesh.org/projects/batman-adv/wiki/Using-batctl

The indentation gets broken here. As I said in my previous reply,
indentation should be:
  - one tab for the options,
  - one tab + two spaces for the help text,
like so:

   |config BR2_PACKAGE_BATCTL
   |	bool "batctl"
   |	depends on BR2_INET_IPV6
   |	depends on BR2_TOOLCHAIN_HAS_THREADS # libnl
   |	select BR2_PACKAGE_LIBNL
   |	help
   |	  Batctl is the configuration and debugging tool for batman-adv.
   |
   |	  http://www.open-mesh.org/projects/batman-adv/wiki/Using-batctl

Here, not using git-send-email (probably you cut-n-pasted in gmail's
compose window) probably broke the indentation, but there's no way we
can know if your patch is correct, or if it was your mailer breaking it.
Using git-send-email guarantees this won't happen, and that the patch is
sent un-mangled.

> diff --git a/package/batctl/batctl.hash b/package/batctl/batctl.hash
> new file mode 100644
> index 0000000..663e602
> --- /dev/null
> +++ b/package/batctl/batctl.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256 77509ed70232ebc0b73e2fa9471ae13b12d6547d167dda0a82f7a7fad7252c36
>  batctl-2014.4.0.tar.gz

Ditto, overly long lines got mangled. Using git-send-email would keep
long lines.

> diff --git a/package/batctl/batctl.mk b/package/batctl/batctl.mk
> new file mode 100644
> index 0000000..a65a5cd
> --- /dev/null
> +++ b/package/batctl/batctl.mk
> @@ -0,0 +1,23 @@
> +################################################################################
> +#
> +# batman-adv control
> +#
> +################################################################################
> +
> +BATCTL_VERSION = 2014.4.0
> +BATCTL_SOURCE = batctl-$(BATCTL_VERSION).tar.gz
> +BATCTL_SITE =
> http://downloads.open-mesh.org/batman/releases/batman-adv-$(BATCTL_VERSION)
> +BATCTL_LICENSE = GPLv2
> +BATCTL_DEPENDENCIES += libnl
> +
> +define BATCTL_BUILD_CMDS
> + $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) CC="$(TARGET_CC)" \
> + CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/libnl3" \
> + LDFLAGS="$(TARGET_LDFLAGS)"

Ditto the indentation: _CMDS are indented by one tab, and continuation
lines are indented by one additional tab, like so:

   |define BATCTL_BUILD_CMDS
   |	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) CC="$(TARGET_CC)" \
   |		CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/libnl3" \
   |		LDFLAGS="$(TARGET_LDFLAGS)"
   |endef

> +endef
> +
> +define BATCTL_INSTALL_TARGET_CMDS
> + $(INSTALL) -m 755 -D $(@D)/batctl $(TARGET_DIR)/usr/sbin/batctl

Ditto the indentation: one tab.

Otherwise, there's nothing that really stands out.

Please, resend using git-send-email. ;-)

Regards,
Yann E. MORIN.

> +endef
> +
> +$(eval $(generic-package))

> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Dear Jens Zettelmeyer,

Thanks for your contribution!

I won't repeat the comments made by Yann E. Morin in his review, since
they are all valid. I will only make additional comments.

First, the title of the patch should be:

	batctl: new package

On Sun, 8 Feb 2015 11:58:01 +0000, Jens Zettelmeyer wrote:
> Hi,
> 
> after a review from Yann E. MORIN i made some adjustments to the package. I
> this version is ok i'll use git send mail to send in an propper patch.

The commit log should not contain such informations, as the commit log
is preserved forever.

> diff --git a/package/batctl/Config.in b/package/batctl/Config.in
> new file mode 100644
> index 0000000..56badd3
> --- /dev/null
> +++ b/package/batctl/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_BATCTL
> + bool "batctl"
> + depends on BR2_INET_IPV6
> + depends on BR2_TOOLCHAIN_HAS_THREADS # libnl
> + select BR2_PACKAGE_LIBNL
> + help
> +  Batctl is the configuration and debugging tool for batman-adv.
> +
> +  http://www.open-mesh.org/projects/batman-adv/wiki/Using-batctl

A Kconfig comment is missing here to tell the user about the IPv6 and
thread dependencies:

comment "batctl needs a toolchain w/ IPv6, threads"
	depends on !BR2_INET_IPV6 || !BR2_TOOLCHAIN_HAS_THREADS


> diff --git a/package/batctl/batctl.mk b/package/batctl/batctl.mk
> new file mode 100644
> index 0000000..a65a5cd
> --- /dev/null
> +++ b/package/batctl/batctl.mk
> @@ -0,0 +1,23 @@
> +################################################################################
> +#
> +# batman-adv control

This should just be the name of the package, i.e: "batctl".

Yes, I know it's silly, but that's the rule :)

> +#
> +################################################################################
> +
> +BATCTL_VERSION = 2014.4.0
> +BATCTL_SOURCE = batctl-$(BATCTL_VERSION).tar.gz

This assignment is not needed, as it is the default value.

> +BATCTL_SITE =
> http://downloads.open-mesh.org/batman/releases/batman-adv-$(BATCTL_VERSION)
> +BATCTL_LICENSE = GPLv2
> +BATCTL_DEPENDENCIES += libnl

+= not really needed here, it could be just =

> +
> +define BATCTL_BUILD_CMDS
> + $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) CC="$(TARGET_CC)" \
> + CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/libnl3" \
> + LDFLAGS="$(TARGET_LDFLAGS)"

If possible, please use $(TARGET_CONFIGURE_OPTS) :

	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) \
		$(TARGET_CONFIGURE_OPTS) \
		CFLAGS="$(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/libnl3"

TARGET_CONFIGURE_OPTS is passing CC, CFLAGS, LDFLAGS, LD, and many
other useful variables. We are just passing CFLAGS afterwards to
override the default (which is just $(TARGET_CFLAGS)).

> +define BATCTL_INSTALL_TARGET_CMDS
> + $(INSTALL) -m 755 -D $(@D)/batctl $(TARGET_DIR)/usr/sbin/batctl
> +endef

No 'make install' target in the project Makefile? If it exists and it's
working, please use it.

Thanks,

Thomas