Bug#848721: apt: please make the moo reproducible

[David Kalnischkies]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/reproducible-bugs/attachments/20161230/f741ded8/attachment.sig>