Re: [PATCH] GNU_AR_DETERMINISTIC env var, --enable-deterministic-archives

[nick clifton <nickc@redhat.com>]

Hi Roland,

   Sorry for the delay in responding.  I was waiting to see if any of 
the other maintainers had anything to say on this issue.


> ar member headers are one of the last remaining common barriers to
> reproducible builds (of development packages).  Hence we have the D option.
> But it is devilishly difficult to get the option override--or even AR and
> RANLIB settings pointing to a wrapper script--through configure et al into
> all corners of e.g. the gcc build.
>
> Instead, this change lets the user set the environment variable

Eww, I hate using environment variables like this.  It makes debugging 
so much harder when you cannot rely upon the command line to describe 
how a program was invoked.

I appreciate that the main thrust of this change is to make reproducible 
builds possible without having to do a lot of work changing package 
build scripts.  But I think that that is a poor reason to introduce a 
debugging nightmare like this.

In my opinion it is better to take on the work of changing all of the 
packages one at a time, even if this will be a lot of effort.  On the 
plus side if you do this you can also simplify the package build scripts 
at the same time.  Perhaps by providing a library of useful command 
lines that the scripts can access.  Then future changes like this can be 
simply made by just updating the command line in the library.

We have always resisted using environment variables to control program 
behaviour in projects like gcc and the binutils and I see no reason why 
this policy should change.


That said I have some other comments on the patch (see below).  They are 
more along the lines of commentary on the code for possible future 
reference, rather than acceptance of the patch.


> Users can still set GNU_AR_DETERMINISTIC=0 to reverse
> the build-time default.

But they cannot use a command line option to do this, which strikes me 
as wrong.  If we are going to allow deterministic behaviour to be a 
default then we must also provide a command line option to disable it.

Also - there needs to be some way for the user to find out how the 
particular ar/ranlib that they are using was configured.  Ie they need 
to be able to find out if the default behaviour is for deterministic or 
non-deterministic archives.


> Since D is incompatible with u and some packages run "ar cru",
> when the environment variable or build-time default sets D, we
> just silently ignore u.

I am not happy with this either.  Since "u" has been explicitly 
requested by the user (well the package build script) we should not 
silently ignore it.  If "D" and "u" have both been specified on the 
command line then a fatal warning makes sense.  If on the other hand "D" 
is just the default behaviour then a warning message would make more 
sense.  Then if the user really does want the "u" behaviour and no 
warnings they can add the "not-D" command line option to their build script.


> Ok for trunk?

No, sorry.



> +      const char *env = getenv ("GNU_AR_DETERMINISTIC");
> +      if (env != NULL&&  env[0] != '\0')
> +        {
> +          if (isdigit(env[0]))
> +            {
> +              char *endp = NULL;
> +              long int value = strtol (env,&endp, 0);
> +              if (endp != NULL)
> +                deterministic = value != 0;
> +            }
> +          else
> +            deterministic = 1;

So setting "GNU_AR_DETERMINISTIC=no" will enable determinism...  I admit 
that it matches your proposed new documentation, but it still seems 
counter intuitive.


Cheers
   Nick