Re: [PATCH cygport] Add initial support for SOURCE_DATE_EPOCH

[Christian Franke <Christian.Franke@t-online.de>]

Jon Turney wrote:
> On 28/08/2023 16:12, Christian Franke via Cygwin-apps wrote:
>> Christian Franke wrote:
>>> A small step towards reproducible packaging...
>>>
>
> Thanks very much for this. Sorry for taking so long to look at it.
>

No problem.


> A few questions and suggestions interspersed
>
> [...]
>>
>> -- 
>> Regards,
>> Christian
>>
>>
>> 0001-Add-initial-support-for-SOURCE_DATE_EPOCH.patch
>>
>>  From 73dde4d2dabb74b7b9ee40655204f84e1d4086d6 Mon Sep 17 00:00:00 2001
>> From: Christian Franke<christian.franke@t-online.de>
>> Date: Mon, 28 Aug 2023 16:24:36 +0200
>> Subject: [PATCH] Add initial support for SOURCE_DATE_EPOCH
>>
>> If specified, set the header timestamps of executables and
>> patch files to SOURCE_DATE_EPOCH.
>> Suppress the header timestamp of .gz files.
>> Instruct tar to avoid more recent modification times and
>> to sort all entries by name.
>> ---
>>   bin/cygport.in           | 17 +++++++++++++++--
>>   lib/pkg_pkg.cygpart      | 20 ++++++++++++++++++--
>>   lib/src_postinst.cygpart | 22 +++++++++++++++++++---
>>   3 files changed, 52 insertions(+), 7 deletions(-)
>>
>> diff --git a/bin/cygport.in b/bin/cygport.in
>> index 3f89ac67..e2fe1785 100755
>> --- a/bin/cygport.in
>> +++ b/bin/cygport.in
>> @@ -231,8 +231,9 @@ source ${_privlibdir}/check_funcs.cygpart
>>   ###
>>     # check now for all mandatory programs
>> -for _myprog in bzip2 cat chmod cp diff diffstat dos2unix file find 
>> gawk grep gzip \
>> -               install ln mkdir mv patch rm rsync sed sort tar xargs 
>> which xz
>> +for _myprog in bzip2 cat chmod cp date diff diffstat dos2unix file 
>> find gawk grep \
>> +               gzip install ln mkdir mv patch rm rsync sed sort tar 
>> touch which \
>> +               xargs xz
>>   do
>>       if ! check_prog ${_myprog}
>>       then
>> @@ -490,6 +491,18 @@ do
>>   done
>>   unset restrict
>>   +if [ "${SOURCE_DATE_EPOCH+y}" = "y" ]
>> +then
>> +    if [ -n "$(echo "${SOURCE_DATE_EPOCH}" | sed -e 's/^$/X/' -e 
>> 's/[0-9]//g')" ]
>> +    then
>> +        error "Malformed SOURCE_DATE_EPOCH: '${SOURCE_DATE_EPOCH}'"
>
> This error message should perhaps say what a well-formed 
> SOURCE_DATE_EPOCH looks like: an integer number of seconds since the 
> unix epoch?
>

error "SOURCE_DATE_EPOCH must be an integer number (seconds since the 
unix epoch)" ?


>> +    fi
>> +    case $(peflags --version 2>/dev/null | sed -n '1s/^.* //p') in
>> +        4.6.[6-9]|4.[7-9]*|[5-9]*) ;;
>> +        *) error "SOURCE_DATE_EPOCH requires peflags 4.6.6 or later"
>> +    esac
>> +fi
>> +
>> ################################################################################
>>   #
>> diff --git a/lib/pkg_pkg.cygpart b/lib/pkg_pkg.cygpart
>> index 2a2bb663..4e6a7cd2 100644
>> --- a/lib/pkg_pkg.cygpart
>> +++ b/lib/pkg_pkg.cygpart
>> @@ -42,7 +42,7 @@ TAR_COMPRESSION_EXT="${TAR_COMPRESSION_EXT:-xz}"
>>   #****
>>     __tar() {
>> -    local TAR_COMPRESSION_OPT;
>> +    local TAR_COMPRESSION_OPT TAR_SOURCE_DATE_OPTS;
>>         # We could use --auto-compress, but this also constrains the 
>> extension
>>       # to the currently valid set. We could probe if tar supports the
>> @@ -65,7 +65,14 @@ __tar() {
>>               error "tar option for 
>> TAR_COMPRESSION_EXT='${TAR_COMPRESSION_EXT}' unknown"
>>               ;;
>>       esac
>> -    tar ${TAR_COMPRESSION_OPT} --owner=Guest:501 --group=None:513 
>> -cvf "$@"
>> +
>> +    if [ -n "${SOURCE_DATE_EPOCH}" ]
>> +    then
>> +        # Ensure reproducible sort order and last modification times 
>> <= SOURCE_DATE_EPOCH
>> +        TAR_SOURCE_DATE_OPTS="--sort=name 
>> --mtime=@${SOURCE_DATE_EPOCH} --clamp-mtime"
>
> I'm slightly concerned that maybe this is masking problems elsewhere, 
> at least when making the source archive: In 2eb7c0eb I started making 
> an effort so that if the "source inputs" have fixed, upstream 
> timestamps, we'll preserve those in the output the source package.
>
> (Obviously that's not always going to be the case, e.g. where cygport 
> patches are from a local git checkout, so maybe that's not a real 
> problem...)

If the following condition holds, the timestamps of "source inputs" are 
not affected:
newest_source_timestamp < SOURCE_DATE_EPOCH < build_time


>
>> +    fi
>> +
>> +    tar ${TAR_COMPRESSION_OPT} ${TAR_SOURCE_DATE_OPTS} 
>> --owner=Guest:501 --group=None:513 -cvf "$@"
>>   }
>>     __pkg_binpkg() {
>> @@ -319,6 +326,7 @@ __pkg_diff() {
>>       local difflevel;
>>       local exclude;
>>       local optional_patchfiles;
>> +    local source_date;
>>         default_excludes="CYGWIN-PATCHES aclocal.m4~ aclocal.m4t 
>> autom4te.cache
>>           config.cache config.guess config.log config.status config.sub
>> @@ -498,6 +506,14 @@ __pkg_diff() {
>>         sed -b -e '/^diff -u/d' -i ${optional_patchfiles} 
>> ${patchdir}/${src_patchfile};
>>   +    if [ -n "${SOURCE_DATE_EPOCH}" ]
>> +    then
>> +        # Ensure that the timestamp comment of the new file is 
>> reproducible
>
> "timestamp comments in the generated patch files" or suchlike?

OK.


>
>
>> +        source_date=$(date -d @"${SOURCE_DATE_EPOCH}" -u +'%Y-%m-%d 
>> %H:%M:%S.000000000 +0000')
>> +        sed -b -e  "s|^\(+++ [^\t]*\t\).*\$|\1${source_date}|" \
>> +            -i ${optional_patchfiles} ${patchdir}/${src_patchfile}
>> +    fi
>> +
>>       diffstat -p${difflevel} ${optional_patchfiles} 
>> ${patchdir}/${src_patchfile};
>>   }
>>   diff --git a/lib/src_postinst.cygpart b/lib/src_postinst.cygpart
>> index dd947311..53eaa71a 100644
>> --- a/lib/src_postinst.cygpart
>> +++ b/lib/src_postinst.cygpart
>> @@ -41,7 +41,7 @@ __prep_fonts_dir() {
>>       for catalogue in ${D}${cataloguedir}/*
>>       do
>>           fontdir=$(readlink -f ${catalogue})
>> -        find ${D}${fontdir} -name '*.pcf' -exec gzip -q '{}' +
>> +        find ${D}${fontdir} -name '*.pcf' -exec gzip -q 
>> ${SOURCE_DATE_EPOCH:+-n} '{}' +
>>           mkfontscale ${D}${fontdir}
>>           mkfontdir ${D}${fontdir}
>>       done
>> @@ -775,7 +775,7 @@ __prepman() {
>>           while read -d $'\0' manpage
>>           do
>>               echo "        ${manpage##*/}";
>> -            gzip -q "${manpage}";
>> +            gzip -q ${SOURCE_DATE_EPOCH:+-n} "${manpage}";
>>           done
>>       fi
>>   }
>> @@ -819,7 +819,7 @@ __prepinfo() {
>>           while read -d $'\0' infopage
>>           do
>>               echo "        ${infopage##*/}";
>> -            gzip -q "${infopage}";
>> +            gzip -q ${SOURCE_DATE_EPOCH:+-n} "${infopage}";
>>           done
>>       fi
>>   }
>> @@ -989,6 +989,12 @@ __prepstrip_one() {
>>         objdump=${objcopy/copy/dump}
>>   +    if [ -n "${SOURCE_DATE_EPOCH}" ]
>> +    then
>> +        # Let objcopy preserve the timestamps
>
> I found this comment confusing.
>
> "Do not embed timestamps in archives"

OK.


>
> Why to we need "--preserve-dates"? The input file presumably has a 
> timestamp when the build actually took place, which we're going to 
> clamp to SOURCE_DATE_EPOCH when we make the archive?

Option --preserve-dates also preserves the PE header timestamps. This is 
missing in the objcopy documentation, The timestamp of an original exe 
is set to SOURCE_DATE_EPOCH by peflags and then preserved by objcopy.


>
>
> It seems like maybe "--enable-deterministic-archives" should be in 
> LDFLAGS, if the build produces static or unstripped archives?

$ ld --enable-deterministic-archives foo.o
ld: unrecognized option '--enable-deterministic-archives'

Ld supports --no-insert-timestamp which is a different approach which 
would work without peflags.


>
>> +        objcopy+=" --enable-deterministic-archives --preserve-dates"
>> +    fi
>> +
>>       # Static libraries should not be fully stripped, but we can
>>       # still provide split debuginfo if desired
>>       case "${exe}" in
>> @@ -1074,6 +1080,16 @@ __prepstrip_one() {
>>       # keep sticky bit if present
>>       chmod u+w,a+x "${exe}";
>>   +    if [ -n "${SOURCE_DATE_EPOCH}" ]
>> +    then
>> +        case "${exe}" in
>> +        *.exe|*.dll|*.so|*.so.*|*.oct|*.mex|*.cmxs)
>> +            # Ensure PE header timestamp is reproducible and 
>> checksum is correct
>> +            # objcopy later inherits the timestamp to debug info and 
>> stripped file
>> +            peflags --checksum=1 --timestamp=${SOURCE_DATE_EPOCH} 
>> ${exe} >/dev/null ;;
>> +        esac
>> +    fi
>> +
>>       if defined_CYGPORT_RESTRICT_debuginfo_
>>       then
>>           ${objcopy} --strip-all "${exe}";
>
>