Re: [PATCH v3 1/5] xen: add XEN_BUILD_POSIX_TIME

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2021 22:57, Bobby Eshleman wrote:
> On Fri, Jan 22, 2021 at 12:27:29PM +0100, Jan Beulich wrote:
>> On 22.01.2021 01:51, Bobby Eshleman wrote:
>>>  export XEN_BUILD_DATE      ?= $(shell LC_ALL=C date)
>>>  export XEN_BUILD_TIME      ?= $(shell LC_ALL=C date +%T)
>>>  export XEN_BUILD_HOST      ?= $(shell hostname)
>>> +export XEN_BUILD_POSIX_TIME        ?= $(shell echo 
>>> $${SOURCE_DATE_EPOCH:-$(shell date +%s)})
>>
>> ... the use of SOURCE_DATE_EPOCH here when it's not used for
>> XEN_BUILD_TIME (the two could also do with living side by
>> side) and ...
>>
> 
> XEN_BUILD_TIME is of the form "HH:MM:SS" and SOURCE_DATE_EPOCH / date
> +%s are unix timestamps (seconds since epoch).  On Linux, `date -d`
> could be used to equalize the two timestamps... I'm not sure about
> FreeBSD, as -d is not required by POSIX.
> 
> I could place them side-by-side if that's preferred.  I placed it
> afterwards here so that there wasn't one oddly aligned "?=" assignment
> in the middle of the others, as in rev2 it was requested their alignment
> be retained here.

Personally I'd prefer if the time ones were all together.
The odd alignment isn't uncommon when introducing new items
with unexpectedly long names into a previously aligned list
of items. Of course you will want to drop the excess blanks
in any event - one each suffice as separators.

>>> --- a/xen/include/xen/compile.h.in
>>> +++ b/xen/include/xen/compile.h.in
>>> @@ -1,5 +1,6 @@
>>>  #define XEN_COMPILE_DATE   "@@date@@"
>>>  #define XEN_COMPILE_TIME   "@@time@@"
>>> +#define XEN_COMPILE_POSIX_TIME     @@posix_time@@
>>>  #define XEN_COMPILE_BY             "@@whoami@@"
>>>  #define XEN_COMPILE_DOMAIN "@@domain@@"
>>>  #define XEN_COMPILE_HOST   "@@hostname@@"
>>
>> ... the lack of quotes here when all neighboring items have
>> them.
>>
> 
> XEN_COMPILE_POSIX_TIME is used as a long, while the others are used as
> strings.  Should this be commented?

Not sure about commenting, but at the very least the
description will imo want to point out the (intentional)
difference. You shouldn't imply readers know in advance
what a subsequent patch may use this for.

Jan