[Buildroot] [PATCH v1] RFC newpackage: fake-hwclock
Thomas Petazzoni
thomas.petazzoni at bootlin.com
Thu Jul 19 07:03:24 UTC 2018
Hello Christopher,
Thanks for this contribution.
On Wed, 18 Jul 2018 09:18:42 -0700, Christopher McCrory wrote:
> Save/restore system clock on machines without working RTC hardware
>
> https://git.einval.com/cgi-bin/gitweb.cgi?p=fake-hwclock.git
>
> This is a RFC, am I missing anything?
See the review comments below. But first and foremost, you should not
include "personal" questions/messages in the commit description itself.
Such questions/comments should go after the "---" sign, i.e
>
> The version is 0.11, but I am not sure where this should be noted.
>
> Signed-off-by: Christopher McCrory <chrismcc at gmail.com>
> ---
... here. This
Indeed, the commit description is preserved in the Buildroot Git
history, while comments after the "---" sign are not.
Another formatting comment: the title of the commit should be just:
fake-hwclock: new package
> package/Config.in | 1 +
> package/fake-hwclock/001-start-stop.patch | 20 ++++++++++++++++++++
> package/fake-hwclock/Config.in | 9 +++++++++
> package/fake-hwclock/fake-hwclock.cronjob | 1 +
> package/fake-hwclock/fake-hwclock.mk | 31 +++++++++++++++++++++++++++++++
We will need an entry added in the DEVELOPERS file for this package.
> diff --git a/package/Config.in b/package/Config.in
> index 08a3eac48a..826e4c8717 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1981,6 +1981,7 @@ comment "Utilities"
> source "package/dialog/Config.in"
> source "package/dtach/Config.in"
> source "package/easy-rsa/Config.in"
> + source "package/fake-hwclock/Config.in"
> source "package/file/Config.in"
> source "package/gnupg/Config.in"
> source "package/gnupg2/Config.in"
> diff --git a/package/fake-hwclock/001-start-stop.patch b/package/fake-hwclock/001-start-stop.patch
> new file mode 100644
> index 0000000000..3ceb0ace4a
> --- /dev/null
> +++ b/package/fake-hwclock/001-start-stop.patch
Patches should have a description and a Signed-off-by line. Since the
upstream project uses Git, we like to have Git-formatted patches (i.e
patches generated by git format-patch).
> @@ -0,0 +1,20 @@
> +--- a/fake-hwclock.orig 2018-07-18 07:14:29.943169961 -0700
> ++++ a/fake-hwclock 2018-07-18 07:14:50.894364970 -0700
> +@@ -30,7 +30,7 @@
> + fi
> +
> + case $COMMAND in
> +- save)
> ++ save|stop)
> + if [ -e $FILE ] ; then
> + NOW_SEC=$(date -u '+%s')
> + if $FORCE || [ $NOW_SEC -ge $HWCLOCK_EPOCH_SEC ] ; then
> +@@ -45,7 +45,7 @@
> + date -u '+%Y-%m-%d %H:%M:%S' > $FILE
> + fi
> + ;;
> +- load)
> ++ start|load)
> + if [ -e $FILE ] ; then
> + SAVED="$(cat $FILE)"
> + SAVED_SEC=$(date -u -d "$SAVED" '+%s')
> diff --git a/package/fake-hwclock/Config.in b/package/fake-hwclock/Config.in
> new file mode 100644
> index 0000000000..4c59f07844
> --- /dev/null
> +++ b/package/fake-hwclock/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_FAKE_HWCLOCK
> + bool "fake-hwclock"
> + depends on BR2_INIT_SYSV
Why only BR2_INIT_SYSV and not BR2_INIT_BUSYBOX ? Do we really a
dependency on the init system here ?
> + help
> + a script to load and save creenet time for systems without a
> + hardware RTC
> +
> +comment "fake-hwclock needs SysV init"
> + depends on !BR2_INIT_SYSV
> diff --git a/package/fake-hwclock/fake-hwclock.cronjob b/package/fake-hwclock/fake-hwclock.cronjob
> new file mode 100644
> index 0000000000..cefb488719
> --- /dev/null
> +++ b/package/fake-hwclock/fake-hwclock.cronjob
> @@ -0,0 +1 @@
> +date -u '+%Y-%m-%d %H:%M:%S' > /etc/fake-hwclock.data
What about using debian/fake-hwclock.cron.hourly from the upstream Git
repository instead ?
This package will also need a .hash file, with the hash of the tarball,
and the hash of the license file.
> diff --git a/package/fake-hwclock/fake-hwclock.mk b/package/fake-hwclock/fake-hwclock.mk
> new file mode 100644
> index 0000000000..b47d6b14f5
> --- /dev/null
> +++ b/package/fake-hwclock/fake-hwclock.mk
> @@ -0,0 +1,31 @@
> +################################################################################
> +#
> +# fake-hwclock
> +#
> +################################################################################
> +
> +FAKE_HWCLOCK_VERSION = f889fd09f2d8d55819dd53785e8bd895866e6628
What about using the v0.11 tag instead, which makes it clear which
version is being used ?
> +FAKE_HWCLOCK_SITE = https://git.einval.com/git/fake-hwclock.git
> +FAKE_HWCLOCK_SITE_METHOD=git
Spaces around the "=" sign.
> +FAKE_HWCLOCK_LICENSE_FILES = COPYING
Please add FAKE_HWCLOCK_LICENSE = GPL-2.0
> +# add a daily cron job to save current time
The Debian packaging makes this an hourly cronjob, perhaps we should do
the same ?
> +ifeq ($(BR2_PACKAGE_DCRON),y)
> +FAKE_HWCLOCK_DEPENDENCIES += dcron
I don't think you need dcron to be built before.
> +define FAKE_HWCLOCK_CROBJOB
CROBJOB or CRONJOB ? :-)
> + $(INSTALL) -D -m 755 $(FAKE_HWCLOCK_PKGDIR)/fake-hwclock.cronjob $(TARGET_DIR)/etc/cron.daily/fake-hwclock
> +endef
> +FAKE_HWCLOCK_POST_INSTALL_TARGET_HOOKS += FAKE_HWCLOCK_CROBJOB
> +endif
Also, I'm not sure why this is limited to dcron. There is a
cron/crontab implementation in Busybox, which should be sufficient for
this. However, I don't think we have other packages that install
cronjobs for the moment, so it's a bit of a new thing.
Perhaps we should just unconditionally install the cronjob file
to /etc/cron.hourly/ ? Or only do so when either Busybox or dcron are
enabled ? Are there other cron implementations besides Busybox and
dcron ?
> +# This should run before everything else
> +define FAKE_HWCLOCK_INSTALL_INIT_SYSV
> + $(INSTALL) -D -m 755 $(@D)/fake-hwclock $(TARGET_DIR)/etc/init.d/S00fake-hwclock
> +endef
> +
> +# Use buildroot buildtime as a seed. Not ideal, but better than 1970-01-01 UTC
> +define FAKE_HWCLOCK_INSTALL_TARGET_CMDS
> + date -u '+%Y-%m-%d %H:%M:%S' > $(TARGET_DIR)/etc/fake-hwclock.data
> +endef
> +
> +$(eval $(generic-package))
The rest looks good to me.
Thanks!
Thomas
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
More information about the buildroot
mailing list