[PATCH 1/2] Fix pe timestamp comment

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Fix pe timestamp comment

Bernhard M. Wiedemann
correct the statement to reflect commit eeb14e5a
---
 bfd/peXXigen.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index e0b494a289..bfa21166ae 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -877,7 +877,8 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
   H_PUT_16 (abfd, filehdr_in->f_magic, filehdr_out->f_magic);
   H_PUT_16 (abfd, filehdr_in->f_nscns, filehdr_out->f_nscns);
 
-  /* Only use a real timestamp if the option was chosen.  */
+  /* Use a real timestamp by default, unless the no-insert-timestamp
+     option was chosen.  */
   if ((pe_data (abfd)->insert_timestamp))
     H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
   else
--
2.16.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

Bernhard M. Wiedemann
in order to make builds reproducible.
See https://reproducible-builds.org/ for why this is good
and https://reproducible-builds.org/specs/source-date-epoch/
for the definition of this variable.

This helps making xen efi binaries build reproducibly by default
much more elegantly than
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
---
 bfd/peXXigen.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index bfa21166ae..778dbf1191 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -880,7 +880,14 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
   /* Use a real timestamp by default, unless the no-insert-timestamp
      option was chosen.  */
   if ((pe_data (abfd)->insert_timestamp))
-    H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
+    {
+      time_t build_time;
+      const char *source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+      if (source_date_epoch == NULL
+          || (build_time = strtoll (source_date_epoch, NULL, 10)) <= 0)
+        build_time = time (0);
+      H_PUT_32 (abfd, build_time, filehdr_out->f_timdat);
+    }
   else
     H_PUT_32 (abfd, 0, filehdr_out->f_timdat);
 
--
2.16.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

John Darrington-4
I don't agree that this is an elegant solution to the problem.

SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
practical way to solve the problem.

Binutils already has a --enable-deterministic-archives option so we
should use that instead.

J'

On Fri, Oct 26, 2018 at 07:59:27AM +0200, Bernhard M. Wiedemann wrote: in order to make builds reproducible.
     See https://reproducible-builds.org/ for why this is good
     and https://reproducible-builds.org/specs/source-date-epoch/
     for the definition of this variable.
     
     This helps making xen efi binaries build reproducibly by default
     much more elegantly than
     https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
     ---
      bfd/peXXigen.c | 9 ++++++++-
      1 file changed, 8 insertions(+), 1 deletion(-)
     
     diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
     index bfa21166ae..778dbf1191 100644
     --- a/bfd/peXXigen.c
     +++ b/bfd/peXXigen.c
     @@ -880,7 +880,14 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
        /* Use a real timestamp by default, unless the no-insert-timestamp
           option was chosen.  */
        if ((pe_data (abfd)->insert_timestamp))
     -    H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
     +    {
     +      time_t build_time;
     +      const char *source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
     +      if (source_date_epoch == NULL
     +          || (build_time = strtoll (source_date_epoch, NULL, 10)) <= 0)
     +        build_time = time (0);
     +      H_PUT_32 (abfd, build_time, filehdr_out->f_timdat);
     +    }
        else
          H_PUT_32 (abfd, 0, filehdr_out->f_timdat);
     
     --
     2.16.4

--
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

Bernhard M. Wiedemann
On 26/10/2018 12.54, John Darrington wrote:
> I don't agree that this is an elegant solution to the problem.
>
> SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
> practical way to solve the problem.
>
> Binutils already has a --enable-deterministic-archives option so we
> should use that instead.

Yes, there exists a --no-insert-timestamp CLI param in this case, but
the problem is that there exist 'ld' versions out there (that are
supported by xen), that do not support this parameter. So you cannot
always add it, because ld will (and probably should) error out on
unknown params.

Env-vars are much nicer in this regard, because older versions will just
ignore it, so it can always be set and help make results more
reproducible for everyone who wants it.

A different way to approach this, could be to adjust ld/emultempl/pep.em
and make the default of the insert_timestamp bool depend on if
SOURCE_DATE_EPOCH is set.

Ciao
Bernhard M.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

Bernhard M. Wiedemann
In reply to this post by John Darrington-4
On 26/10/2018 12.54, John Darrington wrote:
> I don't agree that this is an elegant solution to the problem.
>
> SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
> practical way to solve the problem.
>
> Binutils already has a --enable-deterministic-archives option so we
> should use that instead.

I still would like some nice solution be merged. If you know a better
way, than my patch, I'd like to test that.

enable-deterministic-archives is not even related to this, so may want
to have another look.

Ciao
Bernhard M.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

John Darrington-4
On Wed, Oct 31, 2018 at 10:50:07AM +0100, Bernhard M. Wiedemann wrote:
     On 26/10/2018 12.54, John Darrington wrote:
     > I don't agree that this is an elegant solution to the problem.
     >
     > SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
     > practical way to solve the problem.
     >
     > Binutils already has a --enable-deterministic-archives option so we
     > should use that instead.
     
     I still would like some nice solution be merged. If you know a better
     way, than my patch, I'd like to test that.
     
     enable-deterministic-archives is not even related to this, so may want
     to have another look.


Hi Bernhard,


I don't think  the decision is mine to make.   I was simply voicing an
opinion.

I agree that the name of the option --enable-deterministic-archives is
too specific to logically include this change.   I would be in favour of
a new option for binutils: --enable-deterministic   which by default
would imply --enable-deterministic-archives

I would be suprised if, in the future, no other sources of non-determinism
are discovered.  So I think an option which can cover them all would
be the best way to go.

J'
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Fix pe timestamp comment

Nick Clifton
In reply to this post by Bernhard M. Wiedemann
Hi Bernhard,

> diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
> index e0b494a289..bfa21166ae 100644
> --- a/bfd/peXXigen.c
> +++ b/bfd/peXXigen.c
> @@ -877,7 +877,8 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
>     H_PUT_16 (abfd, filehdr_in->f_magic, filehdr_out->f_magic);
>     H_PUT_16 (abfd, filehdr_in->f_nscns, filehdr_out->f_nscns);
>  
> -  /* Only use a real timestamp if the option was chosen.  */
> +  /* Use a real timestamp by default, unless the no-insert-timestamp
> +     option was chosen.  */
>     if ((pe_data (abfd)->insert_timestamp))
>       H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
>     else

This specific patch is OK, although it does need a ChangeLog entry
to go with it.  I can create one if you wish, but if you are going
to apply the patch yourself then please post the changelog update
to this list at the same time.

Cheers
   Nick


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

Nick Clifton
In reply to this post by Bernhard M. Wiedemann
Hi Bernhard,

> This helps making xen efi binaries build reproducibly by default
> much more elegantly than
> https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html

> +    {
> +      time_t build_time;
> +      const char *source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
> +      if (source_date_epoch == NULL
> +          || (build_time = strtoll (source_date_epoch, NULL, 10)) <= 0)
> +        build_time = time (0);
> +      H_PUT_32 (abfd, build_time, filehdr_out->f_timdat);
> +    }

I *really* don't like this solution to the problem.  There are several
reasons:

   * There is no documentation on this environment variable, so
     users are not going to know that it exists.

   * There is already a well established way of customizing the
     behaviour of the linker: command line options.

   * Possibly the most important: It is really hard to debug problems
     reported by users when there are environment variables controlling
     the behaviour of the program.  Most users will not even bother to
     include the environment variables in the bug report, and even if
     they do, they could be misleading if the build system involved
     overrides these variables for its own purposes.

My suggestion would be to modify the already existing --insert-timestamp
option, so that it can take an option argument specifying the time to be
inserted.  That ought to work and would, in my opinion, be much better
than using environment variables.

Cheers
   Nick


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Allow to override build date with SOURCE_DATE_EPOCH

Bernhard M. Wiedemann
On 05/11/2018 18.29, Nick Clifton wrote:
>
> I *really* don't like this solution to the problem.  There are several
> reasons:
>
>   * There is no documentation on this environment variable, so
>     users are not going to know that it exists.

That can be added. I did not want to go though the effort without
knowing if the change would be accepted.

>   * There is already a well established way of customizing the
>     behaviour of the linker: command line options.

When you look at the discussion in
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01820.html
leading to
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
you will see, that one problem with command line options is, that they
only work with new versions and break on old versions, so every caller
has to spend effort on detecting availability of options.

The other general downside for distributions like openSUSE is that such
CLI options need to be added in many places - a quasi infinite number
(if you consider all the software that is yet to be written).
OTOH an environment variable we can set once in a single place and you
are done.
https://github.com/ImageMagick/ImageMagick/pull/1270 illustrates this
concept nicely.
It does not matter as much in this case, because we do not ship that
many PE binaries.


>   * Possibly the most important: It is really hard to debug problems
>     reported by users when there are environment variables controlling
>     the behaviour of the program.  Most users will not even bother to
>     include the environment variables in the bug report, and even if
>     they do, they could be misleading if the build system involved
>     overrides these variables for its own purposes.

I can understand this concern.
Maybe there could be some --debug mode where it dumps all relevant
inputs (env vars, config files, CLI options, versions)

And then, the effect here is very limited in scope, so I would not
expect many reported problems.

> My suggestion would be to modify the already existing --insert-timestamp
> option, so that it can take an option argument specifying the time to be
> inserted.  That ought to work and would, in my opinion, be much better
> than using environment variables.

In terms of reproducible-builds, it would be equivalent to the current
--no-insert-timestamp, so I see little value in that.

One idea was to just check if an environment variable is set to decide
on if to include a timestamp. Saves on parsing and avoids trouble with
y2038.


Ciao
Bernhard M.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Fix pe timestamp comment

Nick Clifton
In reply to this post by Nick Clifton
Hi Bernhard,

>> This specific patch is OK, although it does need a ChangeLog entry
>> to go with it.  I can create one if you wish, but if you are going
>> to apply the patch yourself then please post the changelog update
>> to this list at the same time.
>
> please add the ChangeLog entry. I don't want push access.

Applied with this changelog entry.

Cheers
  Nick

bfd/ChangeLog
2018-11-09  Bernhard M. Wiedemann  <[hidden email]>

        * peXXigen.c (_bfd_XXi_only_swap_filehdr_out): Correct comment
        concerning timestamp insertion.