The manual page says that in -j mode when the shell does not have ErrCtl
(and none of the default shells has that), the command prefix '-'
"affects the entire job", but this seems to be wrong. At least, there
is no change in the output from before, when all commands had been in
the same target.
The first parameter of the macro was always stdout, and there was no
apparent reason to pass anything else there.
Let the compiler decide whether to inline this or not, it's not
time-critical.
Since Var_Export1 is neither exported by the module nor does it belong
to the Var type, the previous function name was misleading. The 1 in
the function name was not as expressive as possible. The new name
aligns nicely with UnexportVar, which is a very young name as well.
Now that errors in the main targets and in their dependencies have the
same effect on the .END node and its dependencies, the two variables can
be merged.
Whether in -k mode or not, the exit status tells whether all requested
targets were made or not. If a dependency could not be made, the main
target was not made as well, therefore the exit status must be nonzero
in such a case.
This part of the code lacked proper unit tests until today. The unit
test deptgt-end-fail.mk is compatible with make>=2003 at least, allowing
to compare the output over time.
In 2003, in the ok-ok-ok-ok case, "Making all from all-dep." was printed
twice in a row, for whatever reason ... (40 minutes later) ... If I had
just made the two commands for 'all' and '.END' more distinguishable.
Back in 2003, the local variables for .END had not been initialized,
instead the .END node was run with the local variables of the last
preceding node. In this case, that node was 'all', therefore ${.TARGET}
had obviously expanded to 'all'.
Somewhere in 2004, the shell commands were no longer run with the -e
flag, which resulted in the "exit status $?" line to be printed in cases
that had stopped early before.
Somewhere in 2005, the local variables for the .END node had been fixed.
The variable ${.TARGET} now had the value '.END', just as expected. In
addition, the dependencies for the .END node were made, although without
getting their proper local variables. This resulted in the output
"Making out of nothing" instead of the expected "Making end-dep out of
nothing".
Still in 2005, in the test case "all=ok all-dep=ok end=ok end-dep=ERR",
the error code of the failed 'end-dep' was first reported as "*** Error
code 1 (continuing)". To compensate for this improvement, a new bug had
been introduced. The test case "all=ok all-dep=ok end=ERR end-dep=ERR"
had properly exited with status 1 on 2005-01-01, but on 2006-01-01 it
exited with status 0, thereby ignoring errors in the .END node.
Somewhere in 2008, some of the error messages (but not all) were
directed to stderr instead of stdout. The actual output stayed the same
though.
Somewhere in 2011, the dependency of the .END node got its own local
variables, and ${.TARGET} now expanded to 'end-dep', as expected.
Somewhere in 2016, the two empty lines between the "*** Error code 1
(continuing)" and the "Stop." got compressed into a single empty line.
On 2020-12-07 (that is, today), the exit status 1 has been restored in
the error cases, after it had been wrong for at least 14 years.
Adding an individual test for each of the 16 combinations would have
been too much manual work, and it's not easy to come up with a good
naming scheme for all the tests, keeping them short and expressive at
the same time.
Earlier versions of make didn't know the -v option to print the expanded
value of a variable. To make the test runnable by older makes as well,
switch to -V instead, which has been available much longer.
Makefiles are text files, they must not contain null bytes.
The previous code in this area was rotten anyway. It assumed that
buf_end could be NULL even if buf_ptr was a valid pointer, which is no
longer true, probably since a few years already.
Continuing parsing after a null byte does not make sense. If there's a
null byte in a text file, that file is corrupted, and parsing it leads
to unintended effects easily. Therefore the only sensible action is to
stop parsing immediately.
The check whether cf->readMore could be null was outdated as well, which
previously made the fatal error impossible to reach. Because of the
missing unit tests, nobody noticed this though.
The "exit status 0" in opt-file.exp is worring but that's due to another
bug and will be fixed in a follow-up commit.
Several parts of make intentionally depend on the guarantee that
snprintf and vsnprintf do not overflow their buffer. If an
implementation cannot provide this guarantee, refuse to use it.
Concatenating identifiers makes it difficult to spot them when searching
the code. This gets rid of the special case for OP_MEMBER and MEMBER.
The same pattern is applied in the DEBUG macro, by the way.
The previous output format had a %-20s conversion specifier. This
produced different output depending on the length of the pathname, which
was too difficult to normalize. By moving the directory name to the
end, it is no longer necessary to fill up any space, and the numbers are
always aligned properly.
As a result, 3 of the unit tests no longer need any special
postprocessing of their output.
This is only used interactively, not in the official builds, therefore
the additional dependency on Perl doesn't matter. The same result could
have been achieved in any other programming language, but probably not
as concisely.
Before, the conditions in the output had been expanded, which made them
illegible. The expanded conditions were unrealistic as well since the
evaluation flags differ between a condition and normal evaluation
(VARE_WANTRES, VARE_UNDEFERR).
The function name had been too ambiguous since it didn't mention the
particular directory that was initialized. Instead of that function,
Dir_InitCur is called directly from main_Init.
The pseudo CachedDir entry ".DOTLAST" is initialized at the very
beginning. The observable behavior is unchanged since this a
memory-only object with no connection to the file system.
The special path entry is called .DOTLAST, therefore the local variable
should have the same name.
A variable named 'base' must not point to the slash of a pathname. It
may only point to the character after the slash, everything else is
confusing, even if it's only for a brief moment.
Calling CachedDir_Assign requires that the variable be initialized. On
most systems, NULL is represented as all-zero bits already. This change
is only for the few other systems.
Add some comments explaining the implementation of Dir_AddDir since that
is tricky to read from the code alone.
Previously, the reference count for a newly created CacheDir had been
set to 1 in CacheNewDir. This was wrong because at that point, the
object had not been referenced by any nonlocal variable. The reference
count is no longer incremented at this point.
All callers of CacheNewDir either append the newly created CachedDir to
a SearchPath via Lst_Append and CachedDir_Ref, or they assign it to a
global variable via CachedDir_Assign.
Since the reference count is no longer wrongly incremented, it does not
need to be decremented more than necessary in Dir_End. To keep the code
simple and maintainable, all assignments to global variables are now
handled by CachedDir_Assign. Adding a CachedDir to a list is still done
manually via Lst_Append, and the corresponding code for decrementing is
in SearchPath_Clean and SearchPath_Free. These details may be cleaned
up in a follow-up commit.
As a result, when OpenDirs_Done is called in the unit tests, the list of
open directories is empty. It had been non-empty in a single unit test
before (dep-wildcards.mk), as a result of calling Dir_Expand.
The additional debug logging for the reference counting is not enabled
by default since it contains memory addresses, which makes the output
dependent on the memory allocator.
The function CachedDir_Destroy has been merged into CachedDir_Undef,
which had only been used in Dir_End before. The new name emphasizes
that it corresponds to CachedDir_Ref.
The memory management for dotLast is quite simple. It is initialized
exactly once main_Init > Init_Objdir > Dir_InitDir and freed exactly
once in main_CleanUp > Dir_End. Previously, dotLast was not freed at all.
The first call to CachedDir_Unref decremented the refCount to 0 but
didn't free anything. Next, CachedDir_Destroy was called, which
decremented the reference count to -1, therefore skipping the actual
freeing. This was probably an implementation mistake.
Since Dir_End is called at the very end of main_CleanUp, no code
accesses dotLast after it has been freed.
In a makefile with repeated ".CURDIR=." lines, Dir_AddDir is called with
a NULL path, once per line. Since the path is NULL, the search for
OpenDirs_Find is skipped and the directory is always read from disk.
The freshly read directory has a refCount of 1, and the refCount never
raises above 2.
In Dir_InitCur, the directory of the previous .CURDIR has a refCount of
2, which is decremented twice and then freed. After this, the new
directory is placed in the global 'cur', after incrementing its refCount
to 2.
It still seems wrong that the refCount of 'cur' is 2 instead of 1, but
it works well.
The function Dir_Destroy is an implementation detail of the cached
directories, and it should not be exported to the other modules. The
search paths, on the other hand, are the high-level API that may be used
by the other modules, as the concept of search paths is documented in
the manual page.
Previously, the documentation of that test was much too short to explain
all the effects that happened in the bug situation from 2020-06-28 until
2020-07-02.
When compiling make in all-in-one mode, these variable names conflict.
They could have been merged into a single variable, but that would have
required to make it a global variable for the other modules as well.
The parse module has a similar variable called 'fatals'. All these can
possibly be merged into a single variable, but not now.
When compiling make in all-in-one mode, the variable name nfds conflicts
with the local variable name nfds in meta_compat_parent.
The variable name jobfds was misleading. It has nothing to do with file
descriptors, it's just an array of jobs.
This test is just meant to cover the existing code, it still needs to be
cleaned up to serve as a tutorial and to highlight the really
interesting points.
This means no more unnecessary void pointers in function signatures and
no more abstraction level at checking a single element of a list. In
most cases it is more appropriate to define a function that operates on
the list as a whole, thereby hiding implementation details like the
ListNode from the caller.
Having an enum whose constants must be ordered in a certain way may be
unexpected to casual readers. Hide this implementation detail in
separate functions.
Fix one bug, find 4 new ones. All these bugs have been around since
2005-05-08, when dependencies on the .BEGIN, .END and .INTERRUPT nodes
were implemented. Before that, checking gn->made == ERROR was
appropriate, but adding the dependencies made ABORTED a new possible
error value from Compat_Make.
Given only the state names and their individual documentation, it is
hard to see the full picture. To make this easier, provide typical
examples of the ways that a GNode takes through these states.
The output from the directory cache made the regular NetBSD build fail
because the pathname to the working directory differs, thus affecting
the spacing.
This time, document why the directory cache needs to be excluded from
the output.
This avoids passing invisible void pointers around in Lst_Append. It
also provides a convenient place to document what it means to "add a
candidate to the searcher".
Having a simple list of candidates is not enough. It is currently
possible to construct endless loops with huge memory usage, as
demonstrated in suff-transform-endless.mk.
To fix this, a straight-forward idea is to remember which candidates
have already been searched and to not search them again.
This also fixes a small inconsistency in the code. Most parameters had
been named slst (the s came from a time when Candidate was named Src),
except for Suff_FindDeps, where the variable was named srcs. The
confusing thing about this was that the name srcs is used throughout the
file for a different purpose. Only in FindThem there were two
parameters of the same type, which made this even more confusing.
The suffix code still doesn't behave as expected. Make sure that at
least setting the main target works as expected. It does, and the added
debug logging provides further hints for debugging the suffix handling
code.
The configuration section of unit-tests/Makefile is already complicated
enough to read, due to the excessive use of regular expressions.
Therefore, to keep the structuring elements at a minimum, inline the
.for loop.
Previously, the variables section had been omitted. This was because
the variables had been output in hashcode order until 2020-10-18, and
because some of the variable values are specific to the test environment
or the individual run (MACHINE_ARCH, MAKE.PPID).
The compiler cannot check these since all lists and list nodes are
aliases to each other.
Maybe it's time to add type-generic lists to the code, to delegate these
checks to the compiler.
In plain C, bit 31 of an enum is not guaranteed to play well with binary
operators. Bit 23 was unused until now, so shift them all down.
The order of the bits is only used in Targ_PrintType.
The names of the functions AddSources and AddLevel were not as precise
and guiding as possible. Merging them into a single function makes the
code indented a little more, but on the other hand provides a function
with an easy to grasp purpose.
