reprotest: inadvertent misconfiguration in salsa-ci config

[James Addison]

Hi Chris, Vagrant,

On Tue, 27 Feb 2024 at 17:44, Vagrant Cascadian
<vagrant at reproducible-builds.org> wrote:
>
> On 2024-02-27, Chris Lamb wrote:
> >> * Update reprotest to handle a single-disabled-varations-value as a
> >>   special case - treating it as vary and/or emitting a warning.
>
> Well, I would broaden this to include an arbitrary number of negating
> options:
>
>   --variations=-time,-build_path
>
> That seems just as invalid.
>
> The one special case I could see is "--variations=-all" where you might
> want to be normalizing as much as possible.

Hmm, yep.  So when there are only subtractions, we _could_ imply that
there is an
implicit '+all' at the beginning of the 'variations' argument.

And along that line of thinking, we could emit a warning to stderr:

  $ reprotest auto --dry-run --variations=-timezone
  Implicitly expanding variations '-timezone' to '+all,-timezone'
  ...

> > On whether to magically/transparently fix this, needless to say, it's
> > considered bad practice to change the behaviour of software that has
> > already been released — I would, as a rule, subscribe to that idea.
> > However, we should bear in mind that this idea revolves around what
> > users are *expecting*, not necessarily what the software actually
> > does.
> >
> > I say that because I hazard that all 400 usages are indeed expecting
> > that `--variations=-foo` functions the same as `--variations=all,-foo`
> > (or `--vary=-foo`), and so this proposed change would merely be
> > modifying reprotest to reflect their existing expectations. It would
> > not therefore be a violation of the "don't break existing
> > functionality" dictum.
> >
> > (Saying that, the addition of a warning that we are doing so would
> > definitely not go amiss.)
>
> Hrm. Less inclined toward this approach; expectations can shift with
> time and context and culture and whatnot. That said, I agree the current
> behavior is confusing, and we should change something explicitly, rather
> than implicitly...

Changing-existing-behaviours could arguably be even more problematic for
cases like this where we're talking about continuous integration checks.

Breaking/unbreaking unrelated CI pipelines seems like something we should be
careful to avoid.

> >> * Treat removal of a variance factor from an already-empty-context
> >> as an error.
> >
> > I'm also tempted by this as well. :)  How would this be experienced by
> > most DDs? Would their new pushes to Salsa now suddenly fail in the
> > reprotest job of the pipeline? If so, that's not too awful, given that
> > the prominent error message would presumably let them know precisely
> > how to fix it.
>
> I would much prefer an error message if we can correctly identify this.

That'd be nice - perhaps something like:

  Failed to parse variations: '-timezone'; did you mean '+all,-timezone'?

I've opened a merge request[1] to explore this error-treatment approach; it
lacks useful error messaging so far, but I'll attempt to add that soon.

> Some possible expected behaviors to consider treating as invalid, and
> issue an error:
>
>   --variations=-build_path
>
>   --variations=-time,-build_path
>
> This almost makes me want to entirely deprecate --variations, and switch
> to recommending "--vary=-all,+whatever" or "--vary=-all
> --vary=+whatever" instead of ever using --variations.
>
> I'm not sure the variations syntax enables much that cannot be more
> unambiguously expressed with --vary.

I do think that supporting two command-line argument names that provide
similar operations (and use similar names!) is confusing.

However I'm inclined to limit the effect of any behaviour changes here to the
specific cases that we know are problematic (ref previous thoughts about CI
infrastructure).

> That said, the reprotest code is a bit hairy, and I am not sure what
> sort of refactoring will be needed to make this possible. In particular,
> how --auto-build is implemented, where it systematically tests each
> variation one at a time. That said, Refactoring might be needed
> regardless. :)

That's a neat bit of functionality in auto-build.  As far as I can tell, it
seems agnostic of whether the build specifications are provided by 'vary' or
'variations' -- but test coverage would be better at confirming that.

Regards,
James