Re: [PATCH v2] Add '--shuffle' argument support

[Eli Zaretskii]

> From: Sergei Trofimovich <slyich@gmail.com>
> Date: Fri, 18 Feb 2022 23:50:09 +0000
> Cc: Sergei Trofimovich <siarheit@google.com>
> 
> * Makefile.am (make_SRCS): Add src/shuf.h and src/shuf.c file references.
> * builddos.bat: Add shuf.o into build script.
> * doc/make.1: Add '--shuffle' option description.
> * doc/make.texi: Add '--shuffle' option description.
> * src/dep.h (DEP): Add 'shuf' field to store shuffled order.
> * src/filedef.h (struct file): Add 'was_shuffled' bit flag to guard against
>   circular dependencies.
> * src/implicit.c (pattern_search): Reshuffle dependencies for targets
>   dynamically extended with dependencies from implicit rules.
> * src/job.c (child_error): Print shuffle parameter used for dependency
>   shuffling.
> * src/main.c: Add '--shuffle' option handling.
> * src/remake.c (update_goal_chain): Change goal traversal order to shuffled.
> * src/shuf.c: New file with shuffle infrastructure.
> * src/shuf.h: New file with shuffle infrastructure declarations.
> * tests/scripts/options/shuffle-reverse: New file with basic tests.

This should also update build_w32.bat, which is used to build Make on
MS-Windows.  I think NEWS should also call out the new feature.

> +Note that @code{make --shuffle clean all install} does reorder goals
> +similar to how @code{make -j clean all install} runs all targets in

These should use @kbd, not @code, since you are describing commands
the user will type.  I also recommend to wrap each one in @w{..}, so
that these (long) command isn't broken between 2 lines.

> +@samp{--shuffle=} accepts an optional value:

I think nowadays it's customary to use @option as markup for
command-line options (unless Make wants to keep its manual compatible
to very old versions of Texinfo -- this is up to Paul to decide).

> +  /* TODO: could we make this shuffle more deterministic and less
> +     dependent on current filesystem state?  */
> +  if (! file->was_shuffled)
> +    shuffle_file_deps_recursive (file);

Should this TODO be resolved before installing the feature?

> +  /* Handle shuffle mode argument.  */
> +  if (shuffle_mode_arg)
> +  {
> +    if (streq (shuffle_mode_arg, "none"))
> +      sm = sm_none;
> +    else if (streq (shuffle_mode_arg, "identity"))
> +      sm = sm_identity;
> +    else if (streq (shuffle_mode_arg, "reverse"))
> +      sm = sm_reverse;
> +    else if (streq (shuffle_mode_arg, "random"))
> +      sm = sm_random;
> +    /* Assume explicit seed if starts from a digit.  */
> +    else if (ISDIGIT (shuffle_mode_arg[0]))
> +      {
> +        sm = sm_random;
> +        shuffle_seed = atoi (shuffle_mode_arg);
> +      }
> +    else
> +      {
> +        O (error, NILF, _("error: unsupported --shuffle= option."));
> +        die (MAKE_FAILURE);
> +      }
> +    set_shuffle_mode (sm, shuffle_seed);
> +
> +    /* Write fixed seed back to argument list to propagate fixed seed
> +       to child $(MAKE) runs.  */
> +    free (shuffle_mode_arg);
> +    shuffle_mode_arg = xstrdup (shuffle_get_str_value ());
> +  }

Should this be factored out and put on shuf.c?

> +  switch (mode)
> +    {
> +    case sm_random:
> +      if (seed == 0)
> +        seed = (int)time(NULL);
> +      srand (seed);
> +      shuffler = random_shuffle_array;
> +      sprintf(shuffle_str_value, "%d", seed);
               ^^
Stylistic comment: I believe our style is to leave one space between
the function name and the opening parenthesis (here and elsewhere in
the patch).

> +random_shuffle_array (void ** a, size_t len)
> +{
> +  for (unsigned int i = 0; i < len; i++)
     ^^^^^^^^^^^^^^^^^^^
This requires some minimal level of ANSI C support that I'm not sure
Make already requires.  Older compilers will error out here.

> +  /* TODO: below is almost identical to goaldeps shuffle.  The only 
> difference
> +     is a type change.  Worth deduplicating?  */

Another TODO.

> +  /* Shuffle dependencies. */
> +  /* TODO: this is a naive recursion.  Is it good enough?  Or better change 
> it
> +     to queue based implementation?  */

And another one.

> --- /dev/null
> +++ b/tests/scripts/options/shuffle-reverse

This test doesn't seem to test the random option.  I understand why
this is not easy, but shouldn't it still be tested, if only to make
sure the option works, and also that different seeds produce different
outcomes?

Thanks.