Re: [groff] 01/03: contrib/hdtbl/examples/*.roff: Seed RNG.

[G. Branden Robinson]

Hi Steffen,

At 2017-12-02T01:32:01+0100, Steffen Nurpmeso wrote:
> Ok, i finally say something.
> 
> address@hidden (G. Branden Robinson) wrote:
>  |commit da6ac443f21ef09d90b40d2e8215955c916f396a
>  |Author: G. Branden Robinson <address@hidden>
>  |Date:   Sun Nov 19 22:19:52 2017 -0500
>  |
>  |    contrib/hdtbl/examples/*.roff: Seed RNG.
>  |    
>  |    Support reproducible builds by seeding hdtbl's random number generator
>  |    (in contrib/hdtbl/examples/common.roff).
>  |    
>  |    When A/Bing roff documents after a build to see what I broke with my
>  |    changes, I always have to ignore the numerous color changes caused by
>  |    contrib/hdtbl/examples/common.roff seeding its random number generator
>  |    with the current date/time and process ID.
>  |    
>  |    Zeroes turned out to be bad seeds; the tables didn't have much in the
>  |    way of color gradients.
>  |    
>  |    Fix issue https://savannah.gnu.org/bugs/?52462.
>  |    
>  |    Signed-off-by: G. Branden Robinson <address@hidden>
> 
> Yeah and i really will not look deeply into these commits no more.

I welcome code reviews.

> As a self-nominated Debian project leader i find it disturbing
> that you wanna honour reproducible-builds.org

That's not what I was referring to.  I was referring to the fact that:

$ make
$ make clean
$ make

...results in different *roff documents being generated between builds.
The _only_ documents for which that was the case were the ones in
contrib/hdtbl/examples.

I'm sorry my explanation in the commit message was inadequate; in fact I
was worried that it was already too long.

> but then (1) do not
> look out for $SOURCE_DATE_EPOCH that is (rather read:
> unfortunately!) _the_ precondition to signal reproducibility!

Well, like I said, I wasn't yet trying to tackle the problem at the
reproducible-builds.org scope yet.  Most of the changes I've been making
are small, as you've noted, and the churn in contrib/hdbtl/examples was
making me run a risk of overlooking a bug I might introduce.

> (2) change an aspect of a macro package that looks quite
> deliberate to me.

Well, yeah, it's deliberate.  The macro package author implemented his
own PRNG inside some macros defined in a directory called "examples" to
show how pretty pictures could be made with hdtbl.  But they're not part
of hdtbl itself.  I guess I'm not understanding your claim because

contrib/hdtbl/hdtbml.tmac-u and
contrib/hdtbl/hdmisc.tmac-u and

are clearly not the same thing as

contrib/hdtbl/examples/common.roff.  The former do not source the
latter, directly or indirectly.  The hdtbl macro package works without
the examples installed, which is exactly what we want.

Can you explain to me how I've broken an interface that end users are
relying upon?

As an aside, I find the implementation of a PRNG in the roff language to
be superfluous but delightful.  I wouldn't want to get rid of it.  Like
Greg Ubben's arbitrary-precision RPN calculator in sed, the world is a
better place for its existence even if it's not, and perhaps should not
be, a standard tool.

> If i where you, i would remove those changes
> and instead look for SOURCE_DATE_EPOCH in the random generator
> itself.  (3) That is likely what i will do shall i ever reach
> that point.

I agree that'd probably lead to a better fix.

> Regardless.  Really.  I am writing this because your last commit
> introduced a bug in troff source code, where an else clause now
> logically belongs to a different if than it should, if i recall
> correctly.

You mean this (my last==most recent) commit?

commit 697beedb7a27c6205fe939d51a490c510cbebe6a
Author: G. Branden Robinson <address@hidden>
Date:   Thu Nov 30 13:27:57 2017 -0500

    Make writers to stderr identify themselves.
[...]

diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp
index 9cb64e04..8a5041d2 100644
--- a/src/roff/troff/input.cpp
+++ b/src/roff/troff/input.cpp
@@ -8580,7 +8580,10 @@ static void do_error(error_type type,
   if (!get_file_line(&filename, &lineno))
     filename = 0;
   if (filename)
-    errprint("%1:%2: ", filename, lineno);
+    if (program_name)
+      errprint("%1: %2:%3: ", program_name, filename, lineno);
+    else
+      errprint("%1:%2: ", filename, lineno);
   else if (program_name)
     fprintf(stderr, "%s: ", program_name);
   switch (type) {

If so, I don't understand.  The else binds to the nearest if, so the
indentation is not misleading.  Is it too soon to thump my "goto fail;"
Bible again?

Please explain.

> The change itself is something good i would say, i had
> it TODO-commented myself, too.
> Sorry for the noise.

I don't find your comments noisy, but I do find them difficult to
understand.

-- 
Regards,
Branden

signature.asc
Description: PGP signature