Debian Bug report logs - #848721
apt: please make the moo reproducible

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Chris Lamb <lamby@debian.org>

To: submit@bugs.debian.org

Subject: apt: please make the moo reproducible

Date: Mon, 19 Dec 2016 20:57:57 +0100

[Message part 1 (text/plain, inline)]

Source: apt
Version: 1.4~beta2
Severity: wishlist
Tags: patch
User: reproducible-builds@lists.alioth.debian.org
Usertags: timestamps randomness
X-Debbugs-Cc: reproducible-bugs@lists.alioth.debian.org

Hi,

Whilst working on the Reproducible Builds effort [0], we noticed
that apt could not moo reproducibly.

Patch attached.

 [0] https://reproducible-builds.org/


Regards,

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

[apt.diff.txt (text/plain, attachment)]

Message #10 received at 848721@bugs.debian.org (full text, mbox, reply):

From: David Kalnischkies <david@kalnischkies.de>

To: Chris Lamb <lamby@debian.org>, 848721@bugs.debian.org

Cc: reproducible-bugs@lists.alioth.debian.org

Subject: Re: Bug#848721: apt: please make the moo reproducible

Date: Fri, 30 Dec 2016 22:30:42 +0100

[Message part 1 (text/plain, inline)]

Good day mere mortal,

On Mon, Dec 19, 2016 at 08:57:57PM +0100, Chris Lamb wrote:
> Whilst working on the Reproducible Builds effort [0], we noticed
> that apt could not moo reproducibly.

Your prayer has been received by deity@ …


> Patch attached.

… but your sacrifice is not enough.

Given your intend is on changing the behavior of the cow itself, the cow
usually expects full dedication to the cause with mandatory minimum
service periods and probation. Enlisting your first born child¹ in the
service of the cow might be an acceptable alternative.

Failing that your prayer has to change to be considered worthy and find
support by a current member of the inner circle:


>  static std::string getMooLine() {					/*{{{*/
> -   time_t const timenow = time(NULL);
> +   time_t const timenow = GetTimeNow();

On a nitpick level the cow feels kinda tricked by a method named
GetTimeNow to report not always the time now, but what an external has
forced. GetSecondsSinceEpoch() might be better.

>     struct tm special;
>     localtime_r(&timenow, &special);
>     enum { NORMAL, PACKAGEMANAGER, APPRECIATION, AGITATION, AIRBORN } line;
> @@ -163,7 +164,8 @@ bool DoMooApril(CommandLine &)						/*{{{*/
>  									/*}}}*/
>  bool DoMoo(CommandLine &CmdL)						/*{{{*/
>  {
> -   time_t const timenow = time(NULL);
> +   const time_t timenow = GetTimeNow();

Asking the lord of time two times in the same meditation is considered
bad style especially now that this can generate errors.
The cow suggests passing time forward as parameter.


> +   const char *source_date_epoch = getenv("SOURCE_DATE_EPOCH");
> +
> +   if (!source_date_epoch) {
> +      return time(NULL);
> +   }

The cow dislikes the '!' as it is easy to miss and prefers the far
noisier "== nullptr" here. It also prefers its curly brackets on new
lines even through even its current followers tend to dislike it, but
inertia (as it is so common for cows to be "cowheaded").

The followers advice is dropping the curly brackets for these one-line
ifs to make them all happy.

> +   errno = 0;
> +   char *endptr;
> +   const unsigned long long epoch = strtoull(source_date_epoch, &endptr, 10);

The speaker wonders how reproducible such a call is if it is effected by
LANG – or is that just "bad environment setup"?

And while we are at it a minor nitpick: We prefer the const-modifier to be set
after the type.

> +   if ((errno == ERANGE && (epoch == ULLONG_MAX || epoch == 0))
> +         || (errno != 0 && epoch == 0)) {
> +      _error->Error("SOURCE_DATE_EPOCH: strtoull: %s\n", strerror(errno));
> +   }

Using _error->Errno here would be more consistent.


> +   if (endptr == source_date_epoch) {
> +      _error->Error("SOURCE_DATE_EPOCH: No digits were found: %s\n", endptr);
> +   }
> +   if (*endptr != '\0') {
> +       _error->Error("SOURCE_DATE_EPOCH: Trailing garbage: %s\n", endptr);
> +   }

At least some of the if's might be better of as else if's as they can
produce incorrect messages e.g. if the cow itself operates it:

E: SOURCE_DATE_EPOCH: No digits were found: moo
E: SOURCE_DATE_EPOCH: Trailing garbage: moo

Also, far more strange perhaps:

E: SOURCE_DATE_EPOCH: No digits were found: m00
E: SOURCE_DATE_EPOCH: Trailing garbage: m00

The message style is a bit uncommon for apt in general. Unlikely that
a human is going to encounter them a lot, but perhaps something like:

Environment variable SOURCE_DATE_EPOCH has invalid value "%s" (not
starting with digits, trailing garbage, …).


> +   if (epoch > ULONG_MAX) {
> +      _error->Error("SOURCE_DATE_EPOCH: %llu must be <= %lu", epoch, ULONG_MAX);
> +   }

Why is that so that we add pain for our grand-grand-grand-…-children but
support our grand-grand-grand-…-parents parsing a long long future and
past, but accepting only a long future in the end?

The cow fears this is an attempt at making it say something which could
be interpreted as "You don't need to talk about the (far) future, as
there is none for mankind", but much like the god of the christian
faith(s) it likes to put enormous trust into mankind to fix their ways
desperate an endless stream of facts to the contrary.

We hence do expect an explanatory comment at the minimum.


> +   return (time_t) epoch;

we should be using a c++-style cast here: static_cast<time_t>(epoch)

It makes no practical difference here, but its good style to declare
explicitly what cast is needed to avoid problems down the road if the
"this obviously never changes" code ever gets rewritten.

With all these errors previously I wonder if its a good idea to still be
using epoch value btw.


Also: As this is adding a non-trivial amount of code to a much beloved
and very important feature it would be in your best interest to add
some very basic tests for this [for you] obviously very important
behaviour² so future incarnations do not (re)break reproducibility of
moo.  You may add them to test/integration/test-00-commands-have-help
as a (reproducible) cow is obviously a great help.


Moo,

David Kalnischkies, Metatron of the Cow

¹ the cow accepts followers of any age, race, origin and gender but its
followers primarily consist of slowly ageing caucasian human males at
the moment so the cow would prefer a higher biodiversity.

² out of character: you might have guessed from the message style that
I don't consider it 100% serious and mission critical but if for some
reason that is false judgement just say so.

[signature.asc (application/pgp-signature, inline)]

Message #15 received at 848721@bugs.debian.org (full text, mbox, reply):

From: Chris Lamb <lamby@debian.org>

To: David Kalnischkies <david@kalnischkies.de>, 848721@bugs.debian.org

Cc: reproducible-bugs@lists.alioth.debian.org

Subject: Re: Bug#848721: apt: please make the moo reproducible

Date: Sat, 31 Dec 2016 16:54:17 +0000

[Message part 1 (text/plain, inline)]

David Kalnischkies wrote:

> Good day mere mortal

I must say I very much enjoyed receiving this code review…  *grin*

> > Patch attached.
> 
> … but your sacrifice is not enough.

Hah! Okay, updated patch attached:

> On a nitpick level the cow feels kinda tricked by a method named
> GetTimeNow […] GetSecondsSinceEpoch() might be better.

Renamed.

> The cow suggests passing time forward as parameter.

I did initially try this but it makes the code/patch rather ugly in
that one needs to pass the time_t value to DooMooX as well as to
getMooLine and printMooLine…  Yuck.

(Your good point about generating errors has been addressed by simply
calling exit(2) on error; if the value is bad, let's just bail out.)

> > +   if (!source_date_epoch) {
> 
> The cow dislikes the '!' as it is easy to miss and prefers the far
> noisier "== nullptr" here.

Agreed.

> > +   const unsigned long long epoch = strtoull(source_date_epoch, &endptr, 10);
> 
> The speaker wonders how reproducible such a call is if it is effected by
> LANG – or is that just "bad environment setup"?

(It is not.)

> And while we are at it a minor nitpick: We prefer the const-modifier to be set
> after the type.

Updated throughout.

> > +   if ((errno == ERANGE && (epoch == ULLONG_MAX || epoch == 0))
> > +         || (errno != 0 && epoch == 0)) {
> > +      _error->Error("SOURCE_DATE_EPOCH: strtoull: %s\n", strerror(errno));
> 
> Using _error->Errno here would be more consistent.

So, I've made some bigger changes around here:

a) Use std::stringstream to parse the input. This is C++ after all
   and means we don't need a cast at the end of the method, the code
   is much shorter and far more readable. (eg. ss.eof() instead of
   checking for a null pointer, etc.)

b) Call exit(2) on failure as described above.

c) Use _error->Fatal as its now a fatal error due to "b".


Regards,

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

[0001-Make-the-moo-reproducible-Closes-848721.patch (text/x-patch, attachment)]

Message #18 received at 848721-submitter@bugs.debian.org (full text, mbox, reply):

From: David Kalnischkies <david@kalnischkies.de>

To: 848721-submitter@bugs.debian.org

Subject: Bug#848721 in apt marked as pending

Date: Thu, 19 Jan 2017 17:25:44 +0000

Control: tag 848721 pending

Hello,

Bug #848721 in apt reported by you has been fixed in the Git repository. You can
see the commit message below, and you can check the diff of the fix at:

    https://anonscm.debian.org/cgit/apt/apt.git/diff/?id=25a14d4

(this message was generated automatically based on the git commit message)
---
commit 25a14d4ccfceb2698edce01092bc6a1dbe9fb217
Author: David Kalnischkies <david@kalnischkies.de>
Date:   Thu Jan 19 11:50:41 2017 +0100

    make the moo reproducible
    
    Normal cows moo every time they feel like it and it might be a "moo",
    "moo!" or "moo?". This is completely unacceptable behaviour in our super
    cow through as as a superior being it has to show its superiority over
    the common cows and the meager easter eggs by being fully reproducible!
    
    The second version of Chris' patch is modified to include an array of
    tests for this feature – which doubles as explanation for some of the
    moo lines by giving more exact dates – and falling back to current time
    if the environment is invalid + passing time around instead of having an
    invalid environment be an unrecoverable error (aka: Guru Meditation) as
    that is more inline with how apt usually behaves: The wisdom of super cow
    should be available to everyone, even to the most misfortune users not
    capable of having a valid environment variable.
    
    That makes the code slightly more ugly, so instead of requiring a young
    follower to produce a third version a high priest of the cult applied the
    finishing touches as he is used to the pain by now – and another round
    with the slowpoke high priest would have been a serious threat to the
    Debian release schedule which the cow would not approve.
    
    Closes: #848721
    Signed-off-by: Super Cow
    Thanks: Chris Lamb for initial patch and guru meditation

Message #25 received at 848721-close@bugs.debian.org (full text, mbox, reply):

From: Julian Andres Klode <jak@debian.org>

To: 848721-close@bugs.debian.org

Subject: Bug#848721: fixed in apt 1.4~rc1

Date: Mon, 06 Feb 2017 15:03:31 +0000

Source: apt
Source-Version: 1.4~rc1

We believe that the bug you reported is fixed in the latest version of
apt, which is due to be installed in the Debian FTP archive.

A summary of the changes between this version and the previous one is
attached.

Thank you for reporting the bug, which will now be closed.  If you
have further comments please address them to 848721@bugs.debian.org,
and the maintainer will reopen the bug report if appropriate.

Debian distribution maintenance software
pp.
Julian Andres Klode <jak@debian.org> (supplier of updated apt package)

(This message was generated automatically at their request; if you
believe that there is a problem with it please contact the archive
administrators by mailing ftpmaster@ftp-master.debian.org)


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Format: 1.8
Date: Mon, 06 Feb 2017 14:41:23 +0100
Source: apt
Binary: apt libapt-pkg5.0 libapt-inst2.0 apt-doc libapt-pkg-dev libapt-pkg-doc apt-utils apt-transport-https
Architecture: source
Version: 1.4~rc1
Distribution: unstable
Urgency: medium
Maintainer: APT Development Team <deity@lists.debian.org>
Changed-By: Julian Andres Klode <jak@debian.org>
Description:
 apt        - commandline package manager
 apt-doc    - documentation for APT
 apt-transport-https - https download transport for APT
 apt-utils  - package management related utility programs
 libapt-inst2.0 - deb package format runtime library
 libapt-pkg-dev - development files for APT's libapt-pkg and libapt-inst
 libapt-pkg-doc - documentation for APT development
 libapt-pkg5.0 - package management runtime library
Closes: 838441 846476 848721 850759 852460 853761 853762
Changes:
 apt (1.4~rc1) unstable; urgency=medium
 .
   [ David Kalnischkies ]
   * don't show update stats if cache generation is disabled
   * don't lock dpkg in 'apt-get clean'
   * don't lock dpkg in update commands
   * avoid validate/delete/load race in cache generation
   * fix 'install --no-download' mode
   * remove 'old' FAILED files in the next acquire call (Closes: 846476)
   * stop rred from leaking debug messages on recovered errors (Closes: #850759)
   * make the moo reproducible.
     Thanks to Chris Lamb for initial patch and guru meditation (Closes: #848721)
   * update release mappings in documentation
   * avoid malloc if option whitelist is disabled (default)
 .
   [ Julian Andres Klode ]
   * basehttp: Only read Content-Range on 416 and 206 responses (LP: #1657567)
   * test suite: Do not exit 0 in trap for QUIT
   * Only merge acquire items with the same meta key (Closes: #838441)
 .
   [ Zhou Mo ]
   * po: update Simplified Chinese program translation
 .
   [ Jean-Pierre Giraud ]
   * French manpages translation update (Closes: 852460)
 .
   [ victory ]
   * Japanese manpages & program translation update
 .
   [ Frans Spiesschaert ]
   * Dutch program translation update (Closes: #853761)
   * Dutch manpage translation update (Closes: #853762)
Checksums-Sha1:
 f2a6fa3a43768b12eabd30ee602342bd5a4de564 2557 apt_1.4~rc1.dsc
 efb98c1f28db703ca0c40aa0dcefb852d83c739e 2074296 apt_1.4~rc1.tar.xz
 e4415fae0fb9d95148eb1c5f375f980dfef1a221 6739 apt_1.4~rc1_source.buildinfo
Checksums-Sha256:
 44439d7539c6a8129d35db3e74acadd1939e862fccbc226d6aac222b1d84b2a2 2557 apt_1.4~rc1.dsc
 81d3ea631510b1be7dccc1e0b111ae848f5ad956bc4ee88a2a8025c7c11869d5 2074296 apt_1.4~rc1.tar.xz
 e508137139b44f0220b1adb19454d33ed9ad0413b4fc0968c65c8d579b90c6f0 6739 apt_1.4~rc1_source.buildinfo
Files:
 25cbf49babe77eef36e9baa01ad287e8 2557 admin important apt_1.4~rc1.dsc
 57fb2eda30b0749dcf2ecbbac25aa0a7 2074296 admin important apt_1.4~rc1.tar.xz
 c85d869ace06bf0b0d962ca42731b9cd 6739 admin important apt_1.4~rc1_source.buildinfo

-----BEGIN PGP SIGNATURE-----

iQJDBAEBCgAtFiEEzeVhi4gF/W4gLOnC1zw55WWAs4YFAliYfTQPHGpha0BkZWJp
YW4ub3JnAAoJENc8OeVlgLOGbG8P/0NwgmB49Wz9UoFvuShPMP9s6cWxFvViV3ns
TcoIhuCmQhvg7EKYlTyktGgf/E64LAbPsQd7gd68cI6Jy2anZNpQN3S/y8mUOvjr
/3Ne650uwlFm7e9iBnaOs+fnpuRJSk4b5WK8usGv7lmz98c4Nlxzn7AqwGMjTid9
/XuMTRaq8bKeNVZLZUKis8HueGzvAH1Pm5Uh6RCIRIp+YfNAt1KX6rP+skkmN2zT
0g0yU0j1JC0XRVNrojpXZiPvI9YPYjGW3NpnKdkCSZdwemfn2a57NldWs4YnBmL+
VqtoNiXUo1jMRMvEpEVGSE9+g3V366hkKrIgkJOdqHkVNCspyoCKb2LMA7Z7c+GU
y13aI+AOIEYsp95rckouTEHDyUK03tdxhYasYPotuJCNRQGdQZkO4jTo2PJwwtiM
u2jZVziZH4nTtEKJOVDgiGvJbcV4HWAz6dwVI0figMJJJnHng4kaOCOqnlnEVM88
x9XZc4CRCUtrvehzi/6kdu3Dk7tgNglIun8w7p7nQsnB9P0KCawkfBYdxF9UlW21
Ccr376ua3yqx7R+hft+bsHoMweEM791DvONXG7HyXLEPItYTxbKKMxh0POw8ARB/
N78oNtfGiR2tocce74iG/vivWL1xSvy3N8QWAX+tFwYRfA3XcM2A8Jxg5jxIeWwQ
b4FbnLr8
=rvG0
-----END PGP SIGNATURE-----

Send a report that this bug log contains spam.