[Buildroot] [PATCH 1/1] dcron: new package

Thomas Petazzoni thomas.petazzoni at free-electrons.com
Fri Apr 18 21:42:17 UTC 2014


Hello,

Thanks for your contribution. I have a few comments below.

On Wed, 12 Mar 2014 09:44:53 +0100, Alvaro G. M wrote:

> diff --git a/package/dcron/Config.in b/package/dcron/Config.in
> new file mode 100644
> index 0000000..70dadcd
> --- /dev/null
> +++ b/package/dcron/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_DCRON
> +	bool "dcron"

Have you checked that dcron really has no dependencies on toolchain
features? Could you test building it with the following toolchain
configurations:

 http://autobuild.buildroot.org/toolchains/configs/free-electrons/br-arm-basic.config
 http://autobuild.buildroot.org/toolchains/configs/free-electrons/br-arm-full-nothread.config
 http://autobuild.buildroot.org/toolchains/configs/free-electrons/bfin-uclinux.config


> diff --git a/package/dcron/dcron.mk b/package/dcron/dcron.mk
> new file mode 100644
> index 0000000..cfff875
> --- /dev/null
> +++ b/package/dcron/dcron.mk
> @@ -0,0 +1,36 @@
> +################################################################################
> +#
> +# dcron
> +#
> +################################################################################
> +
> +DCRON_VERSION = 4.5
> +DCRON_SITE = http://www.jimpryor.net/linux/releases/
> +DCRON_LICENSE = GPLv2

How do you know it's GPLv2 ? Unfortunately the source code isn't very
specific about the version of the GPL license being used. The only
information I've found is:

May be distributed under the GNU General Public License

> +
> +DCRON_MAKE_OPT = CC="$(TARGET_CC)" CXX="$(TARGET_CXX)" AR="$(TARGET_AR)"

You should instead use $(TARGET_CONFIGURE_OPTS), which already has all
these definitions.

> +
> +define DCRON_PERMISSIONS
> +/usr/sbin/crond			 f 4755	0 0 - - - - -

Is it really needed to install this daemon suid-root? I don't think we
install other daemons suid-root.

> +/usr/bin/crontab		 f 755  0 0 - - - - -
> +/etc/cron.d/system		 f 755  0 0 - - - - -

These are not needed, as these permissions are normal ones, that
can simply be defined by installation commands (below).

> +endef
> +
> +define DCRON_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) DESTDIR=$(TARGET_DIR) -C $(@D) $(DCRON_MAKE_OPT)

Is DESTDIR= really needed during the build step?

Also, use TARGET_CONFIGURE_OPTS instead of DCRON_MAKE_OPT.

> +endef
> +
> +define DCRON_INSTALL_TARGET_CMDS
> +	$(INSTALL) -m0700 $(@D)/crond $(TARGET_DIR)/usr/sbin/crond

Use -D

> +	$(INSTALL) -m4755 $(@D)/crontab $(TARGET_DIR)/usr/bin/crontab

Ditto. And why 4755 instead of 755 ?

> +	$(INSTALL) -D -m0644 $(@D)/extra/root.crontab $(TARGET_DIR)/etc/cron.d/system
> +	$(INSTALL) -d -m0755 $(TARGET_DIR)/var/spool/cron/crontabs \
> +	        $(TARGET_DIR)/etc/cron.daily $(TARGET_DIR)/etc/cron.hourly \
> +	        $(TARGET_DIR)/etc/cron.monthly $(TARGET_DIR)/etc/cron.weekly
> +endef
> +
> +define DCRON_CLEAN_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) clean
> +endef

<pkg>_CLEAN_CMDS no longer exist, so this part should be removed.

Could you take these comments into account and submit an updated
version?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


More information about the buildroot mailing list