> From: Sergei Trofimovich <sly...@gmail.com>
> Date: Fri, 18 Feb 2022 23:50:09 +0000
> Cc: Sergei Trofimovich <siarh...@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.
