Re: Detecting typos in makefiles (was: Re: CVS commit: src/share/mk)
On Sun, 15 Jan 2023 19:14:14 +0100, Roland Illig writes:
>It is really unfortunate that make didn't catch this typo by itself.
>
>.if ${MACHINE_ARCH} =3D=3D "x86_64" || \
> ${MACHINE_ARCH} =3D=3D "i386" || \
> ${MACHINE_ARCH} =3D=3D "alpha" || \\
> !empty(MACHINE_ARCH:Maarch64*) || \
> !empty(MACHINE_ARCH:Msparc*)
>
>(The 'alpha' line has a trailing '\\' instead of the usual '\'.)
Yes warnings would be good I think.
FWIW the above is why I encouage use of :N to reduce the complexity of
conditionals (eaier to extend without screwing up) as in:
.if ${MACHINE_ARCH:Nalpha:Ni386:Nx86_64} == ""
is equivalent to the first 3 lines above.
Detecting typos in makefiles (was: Re: CVS commit: src/share/mk)
Am 15.01.2023 um 11:51 schrieb Nick Hudson:
Module Name:src
Committed By: skrll
Date: Sun Jan 15 10:51:04 UTC 2023
Modified Files:
src/share/mk: bsd.own.mk
Log Message:
Really switch aarch64 and sparc binutils to 2.39
To generate a diff of this commit:
cvs rdiff -u -r1.1293 -r1.1294 src/share/mk/bsd.own.mk
It is really unfortunate that make didn't catch this typo by itself.
.if ${MACHINE_ARCH} == "x86_64" || \
${MACHINE_ARCH} == "i386" || \
${MACHINE_ARCH} == "alpha" || \\
!empty(MACHINE_ARCH:Maarch64*) || \
!empty(MACHINE_ARCH:Msparc*)
(The 'alpha' line has a trailing '\\' instead of the usual '\'.)
Here is what happened:
1. In the condition '${SOMETHING} || \\', the trailing '\\' is a bare
word, which in the case of a simple '.if' directive is interpreted as
'defined(\\)'. So if there had been a variable named '\\', the branch
would have been taken.
2. If the branch is not taken, make ignores all text until the next
'.elif' or '.else' or '.endif' line.
3. If the branch is taken, the line below the '.if' line is indented
with a few spaces and then starts with '!'. This line is interpreted as
a dependency using the dependency operator '!'. The left-hand side of
the dependency line is empty, saying 'nothing depends on these sources'.
Except for the case of automatically generated makefiles, this doesn't
make sense.
4. The '!' dependency has 3 sources:
4a. The first and third sources are archive members. In the first
source, the archive is named 'empty' and the member is named
'MACHINE_ARCH:Maarch64*'.
4b. The second source is '||'.
There are a few things that make could warn about:
* The variable name '\\' is very rare.
* The bare word '\\' occurs in a condition that otherwise uses "proper
expressions".
* The dependency line starts with spaces.
* The dependency line has no targets, only sources.
* The dependency line contains an archive member. Archive members,
while required by POSIX, are seldom used.
* The archive filename is 'empty', which is missing the typical archive
filename suffix '.a'.
* The archive member contains a ':', which is uncommon for filenames.
* The dependency source '||' has a name that is neither a usual filename
nor a usual variable name.
None of these things generates any warning right now. Should they?
Roland
Re: Detecting typos in makefiles (was: Re: CVS commit: src/share/mk)
That's very funny. Yes I think we should warn. I don't think that \\ should
be a valid variable name.
Best,
christos
> On Jan 15, 2023, at 1:14 PM, Roland Illig wrote:
>
> Am 15.01.2023 um 11:51 schrieb Nick Hudson:
>> Module Name: src
>> Committed By:skrll
>> Date:Sun Jan 15 10:51:04 UTC 2023
>>
>> Modified Files:
>> src/share/mk: bsd.own.mk
>>
>> Log Message:
>> Really switch aarch64 and sparc binutils to 2.39
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.1293 -r1.1294 src/share/mk/bsd.own.mk
>
> It is really unfortunate that make didn't catch this typo by itself.
>
> .if ${MACHINE_ARCH} == "x86_64" || \
>${MACHINE_ARCH} == "i386" || \
>${MACHINE_ARCH} == "alpha" || \\
>!empty(MACHINE_ARCH:Maarch64*) || \
>!empty(MACHINE_ARCH:Msparc*)
>
> (The 'alpha' line has a trailing '\\' instead of the usual '\'.)
>
> Here is what happened:
>
> 1. In the condition '${SOMETHING} || \\', the trailing '\\' is a bare
> word, which in the case of a simple '.if' directive is interpreted as
> 'defined(\\)'. So if there had been a variable named '\\', the branch
> would have been taken.
>
> 2. If the branch is not taken, make ignores all text until the next
> '.elif' or '.else' or '.endif' line.
>
> 3. If the branch is taken, the line below the '.if' line is indented
> with a few spaces and then starts with '!'. This line is interpreted as
> a dependency using the dependency operator '!'. The left-hand side of
> the dependency line is empty, saying 'nothing depends on these sources'.
> Except for the case of automatically generated makefiles, this doesn't
> make sense.
>
> 4. The '!' dependency has 3 sources:
>
> 4a. The first and third sources are archive members. In the first
> source, the archive is named 'empty' and the member is named
> 'MACHINE_ARCH:Maarch64*'.
>
> 4b. The second source is '||'.
>
>
> There are a few things that make could warn about:
>
> * The variable name '\\' is very rare.
>
> * The bare word '\\' occurs in a condition that otherwise uses "proper
> expressions".
>
> * The dependency line starts with spaces.
>
> * The dependency line has no targets, only sources.
>
> * The dependency line contains an archive member. Archive members,
> while required by POSIX, are seldom used.
>
> * The archive filename is 'empty', which is missing the typical archive
> filename suffix '.a'.
>
> * The archive member contains a ':', which is uncommon for filenames.
>
> * The dependency source '||' has a name that is neither a usual filename
> nor a usual variable name.
>
>
> None of these things generates any warning right now. Should they?
>
> Roland
signature.asc
Description: Message signed with OpenPGP
