[Buildroot] [PATCH 1/1] tzdata: Improve right/posix installation

[Mylene Josserand]

Hello,


On 26/10/2016 00:09, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 25 Oct 2016 23:48:26 +0200, Mylène Josserand wrote:
>> Improve the right/posix timezone installation by creating new options
>> to choose between these two versions. As one version is necessary, the
>> default one is posix.
>> Now, the user can install both versions or one of both. It reduces the
>> image size when only one version is needed.
>>
>> Signed-off-by: Mylène Josserand <mylene.josserand at free-electrons.com>
>
> Thanks Mylène for your patch. It would be good to explain in the commit
> log why you are proposing this change.


Okay, I will try to be more explicit in the V2.


>> diff --git a/package/tzdata/tzdata.mk b/package/tzdata/tzdata.mk
>> index de7d1be..d1ed21c 100644
>> --- a/package/tzdata/tzdata.mk
>> +++ b/package/tzdata/tzdata.mk
>> @@ -32,10 +32,20 @@ TZDATA_EXTRACT_CMDS =
>>  define TZDATA_INSTALL_TARGET_CMDS
>>  	$(INSTALL) -d -m 0755 $(TARGET_DIR)/usr/share/zoneinfo
>>  	cp -a $(HOST_DIR)/usr/share/zoneinfo/* $(TARGET_DIR)/usr/share/zoneinfo
>> -	cd $(TARGET_DIR)/usr/share/zoneinfo;    \
>> -	for zone in posix/*; do                 \
>> -	    ln -sfn "$${zone}" "$${zone##*/}";  \
>> -	done
>> +	cd $(TARGET_DIR)/usr/share/zoneinfo;     \
>> +	if [ -n "$(BR2_TARGET_TZ_POSIX)" ]; then \
>> +		for zone in posix/*; do \
>> +			ln -sfn "$${zone}" "$${zone##*/}"; \
>> +		done \
>> +	fi;
>> +	# If "right" is the only version enabled
>> +	cd $(TARGET_DIR)/usr/share/zoneinfo; \
>> +	if [ -n "$(BR2_TARGET_TZ_RIGHT)" ] && [ -z "$(BR2_TARGET_TZ_POSIX)" ]; then \
>> +		for zone in right/*; do \
>> +			ln -sfn "$${zone}" "$${zone##*/}"; \
>> +		done \
>> +	fi;
>
> It's a bit late now, so I'm not sure what's going on here. But for
> sure, I'd like this to be handled using make conditionals and not shell
> conditionals.

The current version creates links to have /usr/share/zoneinfo/Etc/ which 
is linked to /usr/share/zoneinfo/posix/Etc/, for example.
In case we have only "right" version installed, these links must be 
updated to use the "right" folder instead of the "posix" one. This is 
what I added here.

>> +
>>  	if [ -n "$(TZDATA_LOCALTIME)" ]; then                           \
>>  	    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" \
>> @@ -51,8 +61,12 @@ endef
>>  define HOST_TZDATA_BUILD_CMDS
>>  	(cd $(@D); \
>>  		for zone in $(TZDATA_ZONELIST); do \
>> -			$(ZIC) -d _output/posix -y yearistype.sh $$zone || exit 1; \
>> -			$(ZIC) -d _output/right -L leapseconds -y yearistype.sh $$zone || exit 1; \
>> +			if [ -n "$(BR2_TARGET_TZ_POSIX)" ]; then \
>> +				$(ZIC) -d _output/posix -y yearistype.sh $$zone || exit 1; \
>> +			fi; \
>> +			if [ -n "$(BR2_TARGET_TZ_RIGHT)" ]; then \
>> +				$(ZIC) -d _output/right -L leapseconds -y yearistype.sh $$zone || exit 1; \
>> +			fi; \
>>  		done; \
>
> Ditto here, we need to use more the make language and less the shell
> language.
>
> Perhaps:
>
> ifeq ($(BR2_TARGET_TZ_POSIX),y)
> define HOST_TZDATA_BUILD_TZ_POSIX
> 	cd $(@D) ; \
> 	$(foreach zone,$(TZDATA_ZONELIST),\
> 		$(ZIC) -d output/posix -y yearistype.sh $(zone)
> 	)
> endef
> endif
>
> ifeq ($(BR2_TARGET_TZ_RIGHT),y)
> define HOST_TZDATA_BUILD_TZ_RIGHT
> 	cd $(@D) ; \
> 	$(foreach zone,$(TZDATA_ZONELIST),\
> 		$(ZIC) -d output/right -L leapseconds -y yearistype.sh $(zone)
> 	)
> endef
> endif
>
> define HOST_TZDATA_BUILD_CMDS
> 	$(HOST_TZDATA_BUILD_TZ_POSIX)
> 	$(HOST_TZDATA_BUILD_TZ_RIGHT)
> endef
>
>>  	)
>>  endef


okay, I see. I did not know that the make language should be preferred. 
I will keep it in mind for next times.


>> diff --git a/system/Config.in b/system/Config.in
>> index f9a3bda..dcc9f6b 100644
>> --- a/system/Config.in
>> +++ b/system/Config.in
>> @@ -429,6 +429,7 @@ config BR2_TARGET_TZ_INFO
>>  	select BR2_PACKAGE_TZDATA if BR2_TOOLCHAIN_USES_GLIBC
>>  	select BR2_PACKAGE_TZDATA if BR2_TOOLCHAIN_USES_MUSL
>>  	select BR2_PACKAGE_TZ if BR2_TOOLCHAIN_USES_UCLIBC
>> +	select BR2_TARGET_TZ_POSIX if ! BR2_TARGET_TZ_RIGHT
>
> No space after !
>

Noted.

>>  	help
>>  	  Say 'y' here to install timezone info.
>>
>> @@ -461,6 +462,20 @@ config BR2_TARGET_LOCALTIME
>>  	  If empty, no local time will be set, and the dates will be
>>  	  expressed in UTC.
>>
>> +config BR2_TARGET_TZ_POSIX
>> +	tristate "Install the Posix timezone"
>
> Don't use tristate, there is no meaning in Buildroot for an option to
> be enabled as a module.
>
> Also, you want to have:
>
> 	default y
>
> in order to preserve the existing behavior.
>

Okay

>> +	help
>> +	  The time zone can be provided by two versions : a posix and a right version.
>> +	  The "posix" version is based on the Coordinated Universal Time (UTC).
>> +	  This is the default one.
>
> Lines are too long, they should be wrapper at 72 characters. Also, I
> think a better wording would be:
>
> 	  Each time zone is provided in two versions: a "posix" version
> 	  and a "right" version. This option enables the installation
> 	  of the "posix" version of all time zone data. The "posix"
> 	  version is based on the Coordinated Universal Time (UTC), and
> 	  does not take leap seconds into account.
>

Thank you for the new description.

>> +
>> +config BR2_TARGET_TZ_RIGHT
>> +	tristate "Install the Right timezone"
>
> Same comments as above: use bool, and add "default y".
>
>> +	help
>> +	  The time zone can be provided by two versions : a posix and a right version.
>> +	  The "right" version is based on the International Atomic Time (TAI) and
>> +	  includes the leap seconds.
>
> Similar rework of the help text would be useful.


Thanks for the review!

Best regards,

-- 
Mylène Josserand, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com