system: allow not setting a default, system-wide time zone
diff mbox series

Message ID 20191116091004.23726-1-yann.morin.1998@free.fr
State Accepted
Headers show
Series
  • system: allow not setting a default, system-wide time zone
Related show

Commit Message

Yann E. MORIN Nov. 16, 2019, 9:10 a.m. UTC
It is valid that there is no system-wide default time zone defined, in
which case Etc/UTC is assumed.

Fixes: #12316

Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Martin Bark <martin@barkynet.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Richard Braun <rbraun@sceen.net>
Cc: Andrew Trapani <andrew.trapani@ontera.bio>
---
 package/tz/tz.mk         | 17 +++++++++++------
 package/tzdata/tzdata.mk | 19 ++++++++++++-------
 system/Config.in         |  2 ++
 3 files changed, 25 insertions(+), 13 deletions(-)

Comments

Arnout Vandecappelle Nov. 17, 2019, 2:41 p.m. UTC | #1
Hi Yann,

On 16/11/2019 10:10, Yann E. MORIN wrote:
> It is valid that there is no system-wide default time zone defined, in
> which case Etc/UTC is assumed.
> 
> Fixes: #12316
> 
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Martin Bark <martin@barkynet.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Richard Braun <rbraun@sceen.net>
> Cc: Andrew Trapani <andrew.trapani@ontera.bio>
[snip]
> diff --git a/system/Config.in b/system/Config.in
> index c87266f431..c8c5be40e0 100644
> --- a/system/Config.in
> +++ b/system/Config.in
> @@ -494,6 +494,8 @@ config BR2_TARGET_LOCALTIME
>  	    Pacific/Wallis
>  	    ...
>  
> +	  Set to empty to not install a default time zone.

 Maybe we should then also change the default to empty? Then at least this use
case gets tested in the autobuilders. Oh, maybe adding a few possible values for
this config in genrandconfig would be nice as well...

 BTW, I notice now that BR2_TARGET_TZ_ZONELIST is not used for uClibc (tz). Is
that expected? Maybe it should depend on !UCLIBC then?

 Regards,
 Arnout

> +
>  endif # BR2_TARGET_TZ_INFO
>  
>  config BR2_ROOTFS_USERS_TABLES
>
Thomas Petazzoni Nov. 18, 2019, 4:11 p.m. UTC | #2
On Sun, 17 Nov 2019 15:41:59 +0100
Arnout Vandecappelle <arnout@mind.be> wrote:

> > +	  Set to empty to not install a default time zone.  
> 
>  Maybe we should then also change the default to empty? Then at least this use
> case gets tested in the autobuilders. Oh, maybe adding a few possible values for
> this config in genrandconfig would be nice as well...

I think this kind of feature gets better tested by having runtime
tests, which can specifically test the interesting configurations.

Thomas
Arnout Vandecappelle Nov. 18, 2019, 4:18 p.m. UTC | #3
On 18/11/2019 17:11, Thomas Petazzoni wrote:
> On Sun, 17 Nov 2019 15:41:59 +0100
> Arnout Vandecappelle <arnout@mind.be> wrote:
> 
>>> +	  Set to empty to not install a default time zone.  
>>
>>  Maybe we should then also change the default to empty? Then at least this use
>> case gets tested in the autobuilders. Oh, maybe adding a few possible values for
>> this config in genrandconfig would be nice as well...
> 
> I think this kind of feature gets better tested by having runtime
> tests, which can specifically test the interesting configurations.

 I didn't express myself very well...

 The change default to empty was independent of the testing. I just thought that
it makes more sense to not install a default timezone than to install UTC as
default (net result is equivalent, but semantically no timezone makes more sense
IMO).

 And then I thought: that's nice, that way the feature also gets tested in the
autobuilders.

 Regards,
 Arnout
Yann E. MORIN Nov. 19, 2019, 8:22 p.m. UTC | #4
Arnout, All,

On 2019-11-17 15:41 +0100, Arnout Vandecappelle spake thusly:
> On 16/11/2019 10:10, Yann E. MORIN wrote:
> > It is valid that there is no system-wide default time zone defined, in
> > which case Etc/UTC is assumed.
> > 
> > Fixes: #12316
> > 
> > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> > Cc: Martin Bark <martin@barkynet.com>
> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Cc: Richard Braun <rbraun@sceen.net>
> > Cc: Andrew Trapani <andrew.trapani@ontera.bio>
> [snip]
> > diff --git a/system/Config.in b/system/Config.in
> > index c87266f431..c8c5be40e0 100644
> > --- a/system/Config.in
> > +++ b/system/Config.in
> > @@ -494,6 +494,8 @@ config BR2_TARGET_LOCALTIME
> >  	    Pacific/Wallis
> >  	    ...
> >  
> > +	  Set to empty to not install a default time zone.
> 
>  Maybe we should then also change the default to empty?

I disagree. I prefer there is an explicit default, which makes it
obvious what it means, rather than use an implicit setting.

> Then at least this use
> case gets tested in the autobuilders. Oh, maybe adding a few possible values for
> this config in genrandconfig would be nice as well...

If you enable the randomisation in genrandconfig, then there is no
longer any reason to set the default to empty, is there?

>  BTW, I notice now that BR2_TARGET_TZ_ZONELIST is not used for uClibc (tz). Is
> that expected? Maybe it should depend on !UCLIBC then?

I think this should be done in a further patch. Having a value that is
not used in the uClibc case is not a regression: it's been like that for
years now, since 2014 with commit 337fbd549c.

On the other hand, this patch fixes an existing issue (#12316).

Regards,
Yann E. MORIN.
Thomas Petazzoni Nov. 27, 2019, 8:49 p.m. UTC | #5
On Sat, 16 Nov 2019 10:10:04 +0100
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> It is valid that there is no system-wide default time zone defined, in
> which case Etc/UTC is assumed.
> 
> Fixes: #12316
> 
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Martin Bark <martin@barkynet.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Richard Braun <rbraun@sceen.net>
> Cc: Andrew Trapani <andrew.trapani@ontera.bio>
> ---
>  package/tz/tz.mk         | 17 +++++++++++------
>  package/tzdata/tzdata.mk | 19 ++++++++++++-------
>  system/Config.in         |  2 ++
>  3 files changed, 25 insertions(+), 13 deletions(-)

Applied to master, thanks.

Thomas
Peter Korsgaard Dec. 3, 2019, 1:54 p.m. UTC | #6
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > It is valid that there is no system-wide default time zone defined, in
 > which case Etc/UTC is assumed.

 > Fixes: #12316

 > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
 > Cc: Martin Bark <martin@barkynet.com>
 > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
 > Cc: Richard Braun <rbraun@sceen.net>
 > Cc: Andrew Trapani <andrew.trapani@ontera.bio>

Committed to 2019.02.x and 2019.08.x, thanks.

Patch
diff mbox series

diff --git a/package/tz/tz.mk b/package/tz/tz.mk
index 1e797046d2..135726c2ce 100644
--- a/package/tz/tz.mk
+++ b/package/tz/tz.mk
@@ -8,6 +8,16 @@  TZ_DEPENDENCIES = host-tzdata host-tzdump
 TZ_LICENSE = Public domain
 
 TZ_LOCALTIME = $(call qstrip,$(BR2_TARGET_LOCALTIME))
+ifneq ($(TZ_LOCALTIME),)
+define TZ_SET_LOCALTIME
+	if [ ! -f $(TARGET_DIR)/usr/share/zoneinfo/uclibc/$(TZ_LOCALTIME) ]; then \
+		printf "Error: '%s' is not a valid timezone, check your BR2_TARGET_LOCALTIME setting\n" \
+			"$(TZ_LOCALTIME)"; \
+		exit 1; \
+	fi
+	ln -sf ../usr/share/zoneinfo/uclibc/$(TZ_LOCALTIME) $(TARGET_DIR)/etc/TZ
+endef
+endif
 
 define TZ_BUILD_CMDS
 	(cd $(HOST_DIR)/share/zoneinfo/posix/; \
@@ -25,12 +35,7 @@  define TZ_INSTALL_TARGET_CMDS
 		$(TARGET_DIR)/usr/share/zoneinfo/iso3166.tab
 	mkdir -p $(TARGET_DIR)/usr/share/zoneinfo/uclibc
 	cp -a $(@D)/output/* $(TARGET_DIR)/usr/share/zoneinfo/uclibc
-	if [ ! -f $(TARGET_DIR)/usr/share/zoneinfo/uclibc/$(TZ_LOCALTIME) ]; then \
-		printf "Error: '%s' is not a valid timezone, check your BR2_TARGET_LOCALTIME setting\n" \
-			"$(TZ_LOCALTIME)"; \
-		exit 1; \
-	fi
-	ln -sf ../usr/share/zoneinfo/uclibc/$(TZ_LOCALTIME) $(TARGET_DIR)/etc/TZ
+	$(TZ_SET_LOCALTIME)
 endef
 
 $(eval $(generic-package))
diff --git a/package/tzdata/tzdata.mk b/package/tzdata/tzdata.mk
index 4d097dbee9..b656bc7f7f 100644
--- a/package/tzdata/tzdata.mk
+++ b/package/tzdata/tzdata.mk
@@ -26,6 +26,17 @@  TZDATA_ZONELIST = $(call qstrip,$(BR2_TARGET_TZ_ZONELIST))
 endif
 
 TZDATA_LOCALTIME = $(call qstrip,$(BR2_TARGET_LOCALTIME))
+ifneq ($(TZDATA_LOCALTIME),)
+define TZDATA_SET_LOCALTIME
+	if [ ! -f $(TARGET_DIR)/usr/share/zoneinfo/$(TZDATA_LOCALTIME) ]; then \
+		printf "Error: '%s' is not a valid timezone, check your BR2_TARGET_LOCALTIME setting\n" \
+			"$(TZDATA_LOCALTIME)"; \
+		exit 1; \
+	fi
+	ln -sf ../usr/share/zoneinfo/$(TZDATA_LOCALTIME) $(TARGET_DIR)/etc/localtime
+	echo "$(TZDATA_LOCALTIME)" >$(TARGET_DIR)/etc/timezone
+endef
+endif
 
 # No need to extract for target, we're using the host-installed files
 TZDATA_EXTRACT_CMDS =
@@ -37,13 +48,7 @@  define TZDATA_INSTALL_TARGET_CMDS
 	for zone in posix/*; do \
 	    ln -sfn "$${zone}" "$${zone##*/}"; \
 	done
-	if [ ! -f $(TARGET_DIR)/usr/share/zoneinfo/$(TZDATA_LOCALTIME) ]; then \
-		printf "Error: '%s' is not a valid timezone, check your BR2_TARGET_LOCALTIME setting\n" \
-			"$(TZDATA_LOCALTIME)"; \
-		exit 1; \
-	fi
-	ln -sf ../usr/share/zoneinfo/$(TZDATA_LOCALTIME) $(TARGET_DIR)/etc/localtime
-	echo "$(TZDATA_LOCALTIME)" >$(TARGET_DIR)/etc/timezone
+	$(TZDATA_SET_LOCALTIME)
 endef
 
 define HOST_TZDATA_BUILD_CMDS
diff --git a/system/Config.in b/system/Config.in
index c87266f431..c8c5be40e0 100644
--- a/system/Config.in
+++ b/system/Config.in
@@ -494,6 +494,8 @@  config BR2_TARGET_LOCALTIME
 	    Pacific/Wallis
 	    ...
 
+	  Set to empty to not install a default time zone.
+
 endif # BR2_TARGET_TZ_INFO
 
 config BR2_ROOTFS_USERS_TABLES