From 0c2fc00544b89314643561dcb6d78f35eb98da68 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Mon, 6 Sep 2021 17:47:04 -0400 Subject: [PATCH] [SV 60795] Don't remake phony included makefiles and show errors Change the handling of included makefiles which are phony targets to be similar to double-colon rules with no prerequisites: simply don't build them at all during the remake a makefile phase. Ensure that any included makefile which is needed but not built results in an error. Update the documentation to make this clear. Add tests to verify this behavior. * doc/make.texi (Remaking Makefiles): Clarify double-colon exception. Document that phony targets are handled the same way. (Phony Targets): Ditto. * src/main.c (main): Check for phony targets when skipping goals. Rather than throwing out skipped but failed goals keep them separately then report them as errors. * src/read.c (eval): Set the file location on included makefiles even when there's no error. * tests/scripts/features/include: Add tests for handling included makefiles with both phony and double-colon rules to rebuild them. --- doc/make.texi | 64 +++++++++------ src/main.c | 137 +++++++++++++++++++-------------- src/read.c | 4 +- tests/scripts/features/include | 79 ++++++++++++++++++- 4 files changed, 199 insertions(+), 85 deletions(-) diff --git a/doc/make.texi b/doc/make.texi index f9fce45f..627d4f14 100644 --- a/doc/make.texi +++ b/doc/make.texi @@ -1379,16 +1379,20 @@ of preventing implicit rule look-up to do so. For example, you can write an explicit rule with the makefile as the target, and an empty recipe (@pxref{Empty Recipes, ,Using Empty Recipes}). -If the makefiles specify a double-colon rule to remake a file with -a recipe but no prerequisites, that file will always be remade -(@pxref{Double-Colon}). In the case of makefiles, a makefile that has a -double-colon rule with a recipe but no prerequisites will be remade every -time @code{make} is run, and then again after @code{make} starts over -and reads the makefiles in again. This would cause an infinite loop: -@code{make} would constantly remake the makefile, and never do anything -else. So, to avoid this, @code{make} will @strong{not} attempt to -remake makefiles which are specified as targets of a double-colon rule -with a recipe but no prerequisites.@refill +If the makefiles specify a double-colon rule to remake a file with a recipe +but no prerequisites, that file will always be remade (@pxref{Double-Colon}). +In the case of makefiles, a makefile that has a double-colon rule with a +recipe but no prerequisites will be remade every time @code{make} is run, and +then again after @code{make} starts over and reads the makefiles in again. +This would cause an infinite loop: @code{make} would constantly remake the +makefile and restart, and never do anything else. So, to avoid this, +@code{make} will @strong{not} attempt to remake makefiles which are specified +as targets of a double-colon rule with a recipe but no prerequisites. + +Phony targets (@pxref{Phony Targets}) have the same issue: they are never +considered up-to-date and so an included file marked as phony would cause +@code{make} to restart continuously. To avoid this @code{make} will not +attempt to remake makefiles which are marked phony. If you do not specify any makefiles to be read with @samp{-f} or @samp{--file} options, @code{make} will try the default makefile names; @@ -2760,15 +2764,15 @@ subdirs: @end group @end example -There are problems with this method, however. First, any error -detected in a sub-make is ignored by this rule, so it will continue -to build the rest of the directories even when one fails. This can be -overcome by adding shell commands to note the error and exit, but then -it will do so even if @code{make} is invoked with the @code{-k} -option, which is unfortunate. Second, and perhaps more importantly, -you cannot take advantage of @code{make}'s ability to build targets in -parallel (@pxref{Parallel, ,Parallel Execution}), since there is only -one rule. +There are problems with this method, however. First, any error detected in a +sub-make is ignored by this rule, so it will continue to build the rest of the +directories even when one fails. This can be overcome by adding shell +commands to note the error and exit, but then it will do so even if +@code{make} is invoked with the @code{-k} option, which is unfortunate. +Second, and perhaps more importantly, you cannot take full advantage of +@code{make}'s ability to build targets in parallel (@pxref{Parallel, ,Parallel +Execution}), since there is only one rule. Each individual makefile's targets +will be built in parallel, but only one sub-directory will be built at a time. By declaring the sub-directories as @code{.PHONY} targets (you must do this as the sub-directory obviously always exists; otherwise it won't @@ -2799,12 +2803,18 @@ The implicit rule search (@pxref{Implicit Rules}) is skipped for @code{.PHONY} is good for performance, even if you are not worried about the actual file existing. -A phony target should not be a prerequisite of a real target file; if it -is, its recipe will be run every time @code{make} goes to update that -file. As long as a phony target is never a prerequisite of a real -target, the phony target recipe will be executed only when the phony -target is a specified goal (@pxref{Goals, ,Arguments to Specify the -Goals}). +A phony target should not be a prerequisite of a real target file; if it is, +its recipe will be run every time @code{make} considers that file. As long as +a phony target is never a prerequisite of a real target, the phony target +recipe will be executed only when the phony target is a specified goal +(@pxref{Goals, ,Arguments to Specify the Goals}). + +You should not declare an included makefile as phony. Phony targets are not +intended to represent real files, and because the target is always considered +out of date make will always rebuild it then re-execute itself +(@pxref{Remaking Makefiles, ,How Makefiles Are Remade}). To avoid this, +@code{make} will not re-execute itself if an included file marked as phony is +re-built. Phony targets can have prerequisites. When one directory contains multiple programs, it is most convenient to describe all of the programs in one @@ -13203,3 +13213,7 @@ tar.zoo: $(SRCS) $(AUX) @printindex fn @bye + +@c Local Variables: +@c eval: (setq fill-column 78) +@c End: diff --git a/src/main.c b/src/main.c index 183c77a1..03db83cc 100644 --- a/src/main.c +++ b/src/main.c @@ -2192,8 +2192,10 @@ main (int argc, char **argv, char **envp) /* Update any makefiles if necessary. */ FILE_TIMESTAMP *makefile_mtimes; + struct goaldep *skipped_makefiles = NULL; char **aargv = NULL; const char **nargv; + int any_failed = 0; int nargc; enum update_status status; @@ -2226,21 +2228,34 @@ main (int argc, char **argv, char **envp) while (d != 0) { - struct file *f; + int skip = 0; + struct file *f = d->file; - for (f = d->file->double_colon; f != NULL; f = f->prev) - if (f->deps == 0 && f->cmds != 0) - break; + /* Check for makefiles that are either phony or a :: target with + commands, but no dependencies. These will always be remade, + which will cause an infinite restart loop, so don't try to + remake it (this will only happen if your makefiles are written + exceptionally stupidly; but if you work for Athena, that's how + you write your makefiles.) */ - if (f) + if (f->phony) + skip = 1; + else + for (f = f->double_colon; f != NULL; f = f->prev) + if (f->deps == NULL && f->cmds != NULL) + { + skip = 1; + break; + } + + if (!skip) + { + makefile_mtimes[mm_idx++] = file_mtime_no_search (d->file); + last = d; + d = d->next; + } + else { - /* This makefile is a :: target with commands, but no - dependencies. So, it will always be remade. This might - well cause an infinite loop, so don't try to remake it. - (This will only happen if your makefiles are written - exceptionally stupidly; but if you work for Athena, that's - how you write your makefiles.) */ - DB (DB_VERBOSE, (_("Makefile '%s' might loop; not remaking it.\n"), f->name)); @@ -2250,17 +2265,19 @@ main (int argc, char **argv, char **envp) else read_files = d->next; - /* Free the storage. */ - free_goaldep (d); + if (d->error && ! (d->flags & RM_DONTCARE)) + { + /* This file won't be rebuilt, was not found, and we care, + so remember it to report later. */ + d->next = skipped_makefiles; + skipped_makefiles = d; + any_failed = 1; + } + else + free_goaldep (d); d = last ? last->next : read_files; } - else - { - makefile_mtimes[mm_idx++] = file_mtime_no_search (d->file); - last = d; - d = d->next; - } } } @@ -2280,6 +2297,23 @@ main (int argc, char **argv, char **envp) db_level = orig_db_level; } + /* Report errors for makefiles that needed to be remade but were not. */ + while (skipped_makefiles != NULL) + { + struct goaldep *d = skipped_makefiles; + const char *err = strerror (d->error); + + OSS (error, &d->floc, _("%s: %s"), dep_name (d), err); + + skipped_makefiles = skipped_makefiles->next; + free_goaldep (d); + } + + /* If we couldn't build something we need but otherwise we succeeded, + reset the status. */ + if (any_failed && status == us_success) + status = us_none; + switch (status) { case us_question: @@ -2293,20 +2327,16 @@ main (int argc, char **argv, char **envp) /* No makefiles needed to be updated. If we couldn't read some included file that we care about, fail. */ { - int any_failed = 0; struct goaldep *d; for (d = read_files; d != 0; d = d->next) if (d->error && ! (d->flags & RM_DONTCARE)) { /* This makefile couldn't be loaded, and we care. */ - OSS (error, &d->floc, - _("%s: %s"), dep_name (d), strerror (d->error)); + const char *err = strerror (d->error); + OSS (error, &d->floc, _("%s: %s"), dep_name (d), err); any_failed = 1; } - - if (any_failed) - die (MAKE_FAILURE); break; } @@ -2315,9 +2345,6 @@ main (int argc, char **argv, char **envp) { /* Nonzero if any makefile was successfully remade. */ int any_remade = 0; - /* Nonzero if any makefile we care about failed - in updating or could not be found at all. */ - int any_failed = 0; unsigned int i; struct goaldep *d; @@ -2327,11 +2354,9 @@ main (int argc, char **argv, char **envp) { /* This makefile was updated. */ if (d->file->update_status == us_success) - { - /* It was successfully updated. */ - any_remade |= (file_mtime_no_search (d->file) - != makefile_mtimes[i]); - } + /* It was successfully updated. */ + any_remade |= (file_mtime_no_search (d->file) + != makefile_mtimes[i]); else if (! (d->flags & RM_DONTCARE)) { FILE_TIMESTAMP mtime; @@ -2346,33 +2371,30 @@ main (int argc, char **argv, char **envp) makefile_status = MAKE_FAILURE; } } - else - /* This makefile was not found at all. */ - if (! (d->flags & RM_DONTCARE)) - { - const char *dnm = dep_name (d); - size_t l = strlen (dnm); - /* This is a makefile we care about. See how much. */ - if (d->flags & RM_INCLUDED) - /* An included makefile. We don't need to die, but we - do want to complain. */ - error (&d->floc, l, - _("Included makefile '%s' was not found."), dnm); - else - { - /* A normal makefile. We must die later. */ - error (NILF, l, - _("Makefile '%s' was not found"), dnm); - any_failed = 1; - } - } + /* This makefile was not found at all. */ + else if (! (d->flags & RM_DONTCARE)) + { + const char *dnm = dep_name (d); + + /* This is a makefile we care about. See how much. */ + if (d->flags & RM_INCLUDED) + /* An included makefile. We don't need to die, but we + do want to complain. */ + OS (error, &d->floc, + _("Included makefile '%s' was not found."), dnm); + else + { + /* A normal makefile. We must die later. */ + OS (error, NILF, _("Makefile '%s' was not found"), dnm); + any_failed = 1; + } + } } if (any_remade) goto re_exec; - if (any_failed) - die (MAKE_FAILURE); + break; } @@ -2535,6 +2557,9 @@ main (int argc, char **argv, char **envp) free (aargv); break; } + + if (any_failed) + die (MAKE_FAILURE); } /* Set up 'MAKEFLAGS' again for the normal targets. */ diff --git a/src/read.c b/src/read.c index 4ef94c9a..58c69a25 100644 --- a/src/read.c +++ b/src/read.c @@ -904,9 +904,7 @@ eval (struct ebuffer *ebuf, int set_default) | (set_default ? 0 : RM_NO_DEFAULT_GOAL)); struct goaldep *d = eval_makefile (files->name, flags); - - if (errno) - d->floc = *fstart; + d->floc = *fstart; free_ns (files); files = next; diff --git a/tests/scripts/features/include b/tests/scripts/features/include index 16b2e8e3..54ad12c1 100644 --- a/tests/scripts/features/include +++ b/tests/scripts/features/include @@ -235,6 +235,29 @@ inc1: foo; echo > $@ '', "#MAKEFILE#:3: inc1: $ERR_no_such_file\n#MAKE#: *** No rule to make target 'foo', needed by 'inc1'. Stop.\n", 512); rmfiles('inc1'); + + # Check that included double-colon targets with no prerequisites aren't + # built. This should fail as hello.mk doesn't exist + + run_make_test(q! +.PHONY: default +default:;@echo 'FOO=$(FOO)' +include hello.mk +hello.mk:: ; echo 'FOO=bar' > $@ +!, + '', "#MAKEFILE#:4: hello.mk: $ERR_no_such_file", 512); + + # Check that included phony targets aren't built. + # This should fail as hello.mk doesn't exist + + run_make_test(q! +.PHONY: default +default:;@echo 'FOO=$(FOO)' +include hello.mk +hello.mk: ; echo 'FOO=bar' > $@ +.PHONY: hello.mk +!, + '', "#MAKEFILE#:4: hello.mk: $ERR_no_such_file", 512); } if (defined $ERR_unreadable_file) { @@ -276,15 +299,69 @@ all:; # https://savannah.gnu.org/bugs/?57676 run_make_test(q! +default:; @echo $(hello) -include hello.mk $(shell echo hello=world >hello.mk) include hello.mk -default:; @echo $(hello) !, '', "world\n"); unlink('hello.mk'); +# Check that included double-colon targets with no prerequisites aren't built. +# This should succeed since hello.mk already exists + +touch('hello.mk'); + +run_make_test(q! +.PHONY: default +default:;@echo 'FOO=$(FOO)' +include hello.mk +hello.mk:: ; echo 'FOO=bar' > $@ +!, + '', 'FOO='); + +unlink('hello.mk'); + +# Check that included double-colon targets with no prerequisites aren't built. +# This should succeed due to -include + +run_make_test(q! +.PHONY: default +default:;@echo 'FOO=$(FOO)' +-include hello.mk +hello.mk:: ; echo 'FOO=bar' > $@ +!, + '', 'FOO='); + +# Check that phony targets aren't built. +# This should succeed since hello.mk already exists + +touch('hello.mk'); + +run_make_test(q! +.PHONY: default +default:;@echo 'FOO=$(FOO)' +include hello.mk +hello.mk: ; echo 'FOO=bar' > $@ +.PHONY: hello.mk +!, + '', 'FOO='); + +unlink('hello.mk'); + +# Check that included double-colon targets with no prerequisites aren't built. +# This should succeed due to -include + +run_make_test(q! +.PHONY: default +default:;@echo 'FOO=$(FOO)' +-include hello.mk +hello.mk: ; echo 'FOO=bar' > $@ +.PHONY: hello.mk +!, + '', 'FOO='); + # Check the default makefiles... this requires us to invoke make with no # arguments. Also check MAKEFILES