Re: [Buildroot] [PATCH 1/2] nodogsplash package added

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello,

First of all, thanks a lot for your contribution! Please see below a
number of comments. It would be good if you could address those
comments and send an updated version.

On Sun,  2 Jan 2022 20:15:24 +0200
azxxza22 <amrtmoh2006@gmail.com> wrote:

> Signed-off-by: azxxza22 <amrtmoh2006@gmail.com>

We need contributions to be made under a real name. Could you adjust
this?

>  package/Config.in                  |  1 +
>  package/nodogsplash/Config.in      |  9 ++++++++
>  package/nodogsplash/nodogsplash.mk | 37 ++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+)

You also need to add an entry in the DEVELOPERS file, so that we know
who maintains this package.


> diff --git a/package/nodogsplash/Config.in b/package/nodogsplash/Config.in
> new file mode 100644
> index 0000000000..7a68bcccb0
> --- /dev/null
> +++ b/package/nodogsplash/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_NODOGSPLASH
> +	bool "nodogsplash"
> +	depends on BR2_PACKAGE_LIBMICROHTTPD

This should be:

	select BR2_PACKAGE_LIBMICROHTTPD

It would be useful to test the package with ./utils/test-pkg to make
sure you have captured all toolchain dependencies.

> +	help
> +	   Nodogsplash is a Captive Portal that offers 
> +	   a simple way to provide restricted access to the Internet by 
> +	   showing a splash page to the user before Internet access is granted

I think this last line is too long. Could you run "make check-package"
and make sure you haven't introduced new warnings?

> +	   https://github.com/nodogsplash/nodogsplash/
> diff --git a/package/nodogsplash/nodogsplash.mk b/package/nodogsplash/nodogsplash.mk
> new file mode 100644
> index 0000000000..3075b7e9f1
> --- /dev/null
> +++ b/package/nodogsplash/nodogsplash.mk
> @@ -0,0 +1,37 @@
> +################################################################################
> +#
> +# nodogsplash
> +#
> +################################################################################

Empty new line needed here.

> +NODOGSPLASH_VERSION = v4.5.1

Should be without the "v".

> +NODOGSPLASH_SITE = git://github.com/nodogsplash/nodogsplash.git
> +NODOGSPLASH_SITE_METHOD = git

Instead of those two lines, use:

NODOGSPLASH_SITE = $(call github,nodogsplash,nodogsplash,v$(NODOGSPLASH_VERSION))

> +NODOGSPLASH_LICENSE = GPL-2.0
> +NODOGSPLASH_LICENSE_FILES = COPYING
> +NODOGSPLASH_DEPENDENCIES = libmicrohttpd
> +NODOGSPLASH_INSTALL_STAGING = YES

Why is this needed in staging? Does it install some library that
another package will link against ?

> +
> +define NODOGSPLASH_BUILD_CMDS
> +	$(MAKE1) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -C $(@D)

Why is $(MAKE1) used? Isn't $(MAKE) working?

Ideally, this should be:

	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D)

> +endef
> +
> +define NODOGSPLASH_INSTALL_EXTRA_FILES
> +	mkdir -p $(TARGET_DIR)/usr/bin
> +	mkdir -p $(TARGET_DIR)/etc/nodogsplash/htdocs/images
> +	cp $(@D)/resources/nodogsplash.conf $(TARGET_DIR)/etc/nodogsplash/
> +	cp $(@D)/resources/splash.html $(TARGET_DIR)/etc/nodogsplash/htdocs/
> +	cp $(@D)/resources/splash.css $(TARGET_DIR)/etc/nodogsplash/htdocs/
> +	cp $(@D)/resources/status.html $(TARGET_DIR)/etc/nodogsplash/htdocs/
> +	cp $(@D)/resources/splash.jpg $(TARGET_DIR)/etc/nodogsplash/htdocs/images/

Use:

	$(INSTALL) -D -m 0644 src dst

it will automatically create any missing destination directory for you.

> +endef
> +
> +NODOGSPLASH_POST_INSTALL_TARGET_HOOKS += NODOGSPLASH_INSTALL_EXTRA_FILES

There's no need for a post-install target hook, just put the commands
directly inside NODOGSPLASH_INSTALL_TARGET_CMDS.

> +
> +define NODOGSPLASH_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/nodogsplash $(TARGET_DIR)/usr/bin
> +	$(INSTALL) -D -m 0755 $(@D)/ndsctl $(TARGET_DIR)/usr/bin

Please use full destination paths, i.e:

	$(INSTALL) -D -m 0755 $(@D)/nodogsplash $(TARGET_DIR)/usr/bin/nodogsplash
	$(INSTALL) -D -m 0755 $(@D)/ndsctl $(TARGET_DIR)/usr/bin/ndsctl

But I see the Makefile of the upstream project has an install target
which seems to do the right thing. Why don't you use it?

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot