Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Dhole]

Hi Chris :)

On 11/09/2015 12:11 PM, Chris Lamb wrote:
> Just a few quick comment on the patch itself:
> 
> +  source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
> +  if (source_date_epoch)
> +{
> +  errno = 0;
> +  [..]
> +  return (long long) epoch;
> +}
> +  else
> +return -1;
> +}
> 
> If you are copying the surrounding code style (I don't have the context
> in front of me) then feel free to ignore this, but generally I find
> something like this much cleaner:
> 
> +  source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
> +  if (!source_date_epoch)
> +return -1
> +
> +  errno = 0;
> +  [..]
> +  return (long long) epoch;
> +}
> 
> IMHO removing indentation levels isn't for the aesthetics; it actually
> removes cognitive load on a human reading it as they don't have to
> manage/remember/push state in their mind.

That does look much cleaner, thanks for the advice! I've modified the
patch following your suggestion.

-- 
Dhole



signature.asc
Description: OpenPGP digital signature
___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Jérémy Bobbio]

Santiago Vila:
> I have very mixed feelings about this kind of patches.
> 
> I fear that by modifying gcc to hide the improper usage of __DATE__
> and __TIME__, we could be removing an incentive for maintainers and
> authors to write software which is truly reproducible, i.e. we run
> the risk of people thinking in the line of "Oh, I will not care about
> my program using __TIME__ or __DATE__ because gcc will take care of
> that".

Well, I still would like to push to add `-Wdate-time` to our default set
of CFLAGS. Even with Dhole's patch, developpers would see the warning
and ask themselves if it's the right thing to do.

I think for this precise case, that's a good compromise.

Also, I'm pretty sure that if the GCC team merges support for
SOURCE_DATE_EPOCH, we could get a similar patch merged to LLVM.

-- 
Lunar.''`. 
lu...@debian.org: :Ⓐ  :  # apt-get install anarchism
`. `'` 
  `-   


signature.asc
Description: Digital signature
___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Holger Levsen]

Hi,

Dhole, thanks for keeping up the good work on this! +kudos too! :)

On Dienstag, 10. November 2015, Santiago Vila wrote:
> > Out of interest, would you extend this argument to argue for an
> > arbitrary build path?
> In principle, yes. I have yet to see why an executable should be
> different just because it was built in one path or in another one.

While I agree in theory, I do disagree in practise and today.

I totally agree keeping this on a todo list somewhere visible, but we also 
want reproducible builds now and using a fixed path is the only solution we 
found in a year of looking for it.

So, I don't want perfect to be the blocker of very good (in the case of 
reproducible builds).


cheers,
Holger


signature.asc
Description: This is a digitally signed message part.
___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Santiago Vila]

B1;2802;0cOn Tue, Nov 10, 2015 at 12:41:51AM +, Chris Lamb wrote:
> Santiago Vila wrote:
> 
> > So, I don't think that this patch would really be "beneficial to our
> > project", as it will only serve to artificially "improve" the statistics.
> 
> Out of interest, would you extend this argument to argue for an
> arbitrary build path?

In principle, yes. I have yet to see why an executable should be
different just because it was built in one path or in another one.

___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Chris Lamb]

Santiago Vila wrote:

> So, I don't think that this patch would really be "beneficial to our
> project", as it will only serve to artificially "improve" the statistics.

Out of interest, would you extend this argument to argue for an
arbitrary build path?


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-

___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Santiago Vila]

On Mon, Nov 09, 2015 at 01:02:40AM +0100, Dhole wrote:
> It would be very beneficial to our project (and other free software
> projects working on reproducible builds) if gcc supported this feature.

Hi.

I have very mixed feelings about this kind of patches.

I fear that by modifying gcc to hide the improper usage of __DATE__
and __TIME__, we could be removing an incentive for maintainers and
authors to write software which is truly reproducible, i.e. we run
the risk of people thinking in the line of "Oh, I will not care about
my program using __TIME__ or __DATE__ because gcc will take care of
that".

I also think that this patch will not really make any software to be
reproducible, and it will only serve to "make up" the statistics.

If you build a package twice with llvm and the results differ, then I
would consider the package to be still unreproducible, no matter how
much we patch gcc.

Sure, we can patch llvm as well, but really, what is at fault here is
the program using __DATE__ and __TIME__, not the compiler.

So, I don't think that this patch would really be "beneficial to our
project", as it will only serve to artificially "improve" the statistics.

It would be quite paradoxical indeed that we "make Debian reproducible"
while the software that we distribute is still not reproducible by itself.

"So, do you propose to modify and patch those 500 packages instead?" you might 
ask.

Well, I do not propose anything as we are all volunteers and nobody
can tell anybody else what he/she should do, but if I were alone in
this project, that would be my goal, yes, even if it takes a lot more
time than patching a single package (gcc). To me, that would be the
right thing to do.

Thanks.

___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Re: [Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Chris Lamb]

Dhole,

First of all thanks so much for working on this patch and especially for
taking extra attention before posting it to GCC list given the high
volume, etc. there.

Just a few quick comment on the patch itself:

+  source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+  if (source_date_epoch)
+{
+  errno = 0;
+  [..]
+  return (long long) epoch;
+}
+  else
+return -1;
+}

If you are copying the surrounding code style (I don't have the context
in front of me) then feel free to ignore this, but generally I find
something like this much cleaner:

+  source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+  if (!source_date_epoch)
+return -1
+
+  errno = 0;
+  [..]
+  return (long long) epoch;
+}

IMHO removing indentation levels isn't for the aesthetics; it actually
removes cognitive load on a human reading it as they don't have to
manage/remember/push state in their mind.


Regards,

-- 
  ,''`.
 : :'  : Chris Lamb
 `. `'`  la...@debian.org / chris-lamb.co.uk
   `-

___
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

[Reproducible-builds] GCC patch reviewed. Proposed mail for gcc-patches mailing list

[Dhole]

Hi all!

Back in summer I worked on a patch for GCC to honor the
SOURCE_DATE_EPOCH env variable when embedding timestamps with the C/C++
macros __DATE__ and __TIME__. I sent the patch to the gcc-patches
mailing list, and after some replies with suggestions and opinions it
didn't reach further.
In this year's DebConf I got in touch with doko (Debian gcc package
maintainer) and he offered to help me with the patch. I have addressed a
comment made by a gcc contributor in the original patch email thread, I
have organized it better and improved the style based on doko advices
and reviews. Before sending the patch to the gcc mailing list I'm
copying the message I intend to write here, in case anyone has comments
and/or improvements. Finally I'll wait for doko's approval before
sending it to the gcc-patches ML.

Cheers,
Dhole

---

Hi!

A while back I submitted a patch to the gcc-patches mailing list
to allow the embedded timestamps by C/C++ macros to be set externally
[0]. The attached patch adresses a comment from Joseph Myers : move the getenv calls into the gcc/ directory;
and it also provides better organization and style following the advice
and comments of the current Debian gcc package maintainer Matthias Klose
 (I'm sending the patch with him as co-author).

As a reminder for the motivation behind this patch, we are working on
the reproducible builds project which aims to provide users with a way
to reproduce bit-for-bit identical binary packages from the source and
build environment. The project involves Debian as well as several other
free software projects. See  for more
information.

Quoting from the original email:

> In order to do this, we need to make the build processes deterministic.
> As you can imagine, gcc is quite involved in producing Debian packages.
> One issue we encounter in many packages that fail to build reproducibly
> is the use of the __DATE__, __TIME__ C macros [1], right now we have 456
> affected packages that would need patching (either removing the macros,
> or passing a known date externally).

> A solution for toolchain packages that embed timestamps during the build
> process has been proposed for anyone interested and it consists of the
> following:
> The build environment can export an environment variable called
> SOURCE_DATE_EPOCH with a known timestamp in Unix epoch format (In our
> case, we use the last date of the package's debian changelog). The
> toolchain package running during the build can check if the exported
> variable is set and if so, instead of embedding the local date/time,
> embed the date/time from SOURCE_DATE_EPOCH.

The proposal to use SOURCE_DATE_EPOCH has now been gathered in a more
formal specification [2], so that any project can adhere to it to
achieve reproducible builds when dealing with timestamps.

> It would be very beneficial to our project (and other free software
> projects working on reproducible builds) if gcc supported this feature.

I'm attaching a patch for gcc-5.2.0 that enables this feature: it
modifies the behavior of the macros __DATE__ and __TIME__ when
SOURCE_DATE_EPOCH is exported.

Any comments, suggestions or other ideas to improve this patch are
mostly welcomed.

I'm willing to extend the documentation if the patch feels appropriate.

Thanks for your attention!


PD: Implementation details:

The error output in gcc/c-family/c-common.c (get_source_date_epoch) is
handled by an fprintf() to stderr followed by an exit (EXIT_FAILURE). I
am not sure this is the right approach for error handling, as I have
found many usages of the error() function in the code. If implemented
with error(), the output looks like this:

"""
$ SOURCE_DATE_EPOCH=123a123 g++ test_cpp.cpp -o test_cpp
In file included from :0:0:
/usr/include/stdc-predef.h:1:0: error: Environment variable
$SOURCE_DATE_EPOCH: Trailing garbage: a123

 /* Copyright (C) 1991-2014 Free Software Foundation, Inc.
 ^
"""

For this reason I used fprintf(stderr, "error message") to report the
errors.


[0] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02210.html
[1] https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros
[2] https://reproducible-builds.org/specs/source-date-epoch/

Best regards,
Dhole
gcc/c-family/ChangeLog:

2015-10-10  Matthias Klose  
Eduard Sanou  
* gcc/c-family/c-common.c (get_source_date_epoch): New function, gets 
the environment variable SOURCE_DATE_EPOCH and parses it as long long
with error handling.
* gcc/c-family/c-common.h (get_source_date_epoch): Prototype.
* gcc/c-family/c-lex.c (c_lex_with_flags): set 
parse_in->source_date_epoch.

libcpp/ChangeLog:

2015-10-10  Matthias Klose  
Eduard Sanou  
* libcpp/include/cpplib.h (cpp_init_source_date_epoch): Prototype.
* libcpp/init.c (cpp_init_source_date_epoch): New function.
* libcpp/internal.h: Added source_date_epoch variable to struct
cpp_reader to store a