| Message ID | 20220102181525.9494-1-amrtmoh2006@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] nodogsplash package added | expand |
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
diff --git a/package/Config.in b/package/Config.in index 2eda8f6ad7..152a2e9e60 100644 --- a/package/Config.in +++ b/package/Config.in @@ -2270,6 +2270,7 @@ endif source "package/nfacct/Config.in" source "package/nftables/Config.in" source "package/nginx/Config.in" + source "package/nodogsplash/Config.in" if BR2_PACKAGE_NGINX menu "External nginx modules" source "package/nginx-dav-ext/Config.in" 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 + 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 + + 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 +# +################################################################################ +NODOGSPLASH_VERSION = v4.5.1 +NODOGSPLASH_SITE = git://github.com/nodogsplash/nodogsplash.git +NODOGSPLASH_SITE_METHOD = git +NODOGSPLASH_LICENSE = GPL-2.0 +NODOGSPLASH_LICENSE_FILES = COPYING +NODOGSPLASH_DEPENDENCIES = libmicrohttpd +NODOGSPLASH_INSTALL_STAGING = YES + +define NODOGSPLASH_BUILD_CMDS + $(MAKE1) CC="$(TARGET_CC)" LD="$(TARGET_LD)" -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/ +endef + +NODOGSPLASH_POST_INSTALL_TARGET_HOOKS += NODOGSPLASH_INSTALL_EXTRA_FILES + +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 + +endef + +$(eval $(generic-package)) +
Signed-off-by: azxxza22 <amrtmoh2006@gmail.com> --- package/Config.in | 1 + package/nodogsplash/Config.in | 9 ++++++++ package/nodogsplash/nodogsplash.mk | 37 ++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 package/nodogsplash/Config.in create mode 100644 package/nodogsplash/nodogsplash.mk