RTimothyEdwards / magic

Magic VLSI Layout Tool
Other
498 stars 103 forks source link

make all fails silently if database.h fails to build #149

Open proppy opened 2 years ago

proppy commented 2 years ago

assuming csh is not present:

# ./configure
...
checking for csh... no
...

make all will fails silently and exit with 0:

# make all
make all
--- errors and warnings logged in file make.log
--- making header file database/database.h
# echo $?
echo $?
0

while make.log contains the appropriate error message:

--- making header file database/database.h
./scripts/makedbh database/database.h.in database/database.h
/bin/bash: ./scripts/makedbh: /bin/csh: bad interpreter: No such file or directory
make[1]: *** [Makefile:48: database/database.h] Error 126
make[1]: Leaving directory '/tmp/1/magic'
proppy commented 2 years ago

this is likely caused by: https://github.com/RTimothyEdwards/magic/blob/master/Makefile#L22

which invert grep for specific keywords like Stop in the log while in that case make outputs Error.

RTimothyEdwards commented 2 years ago

But if the lack of "csh" now prevents "configure" from completing, then it doesn't matter, does it?

proppy commented 2 years ago

I'd argue that it would be best for make to always surface build error messages (and exit with the corresponding exit code) as it makes it challenging to debug in CI environment (where the capability to easily be able to access the filesystem to inspect make.log should not be taken for granted)

RTimothyEdwards commented 2 years ago

It's very likely that the reason that I did the grep for "Error" was because the compile was not stopping on an error, which made it hard to find the needle in the output haystack. I'd suppose that with the change to halt the build on an error, there's no longer any particular need to filter the terminal output.

proppy commented 2 years ago

+1, something like:

diff --git a/Makefile b/Makefile
index e1d61fb..dc0efc4 100644
--- a/Makefile
+++ b/Makefile
@@ -18,12 +18,10 @@ INSTALL_CAD_DIRS = windows doc ${TECH}
 all:        $(ALL_TARGET)

 standard:
-        @echo --- errors and warnings logged in file make.log
-        @${MAKE} mains 2>&1 | tee -a make.log | egrep -i "(.c:|Stop.|---)"
+        @${MAKE} mains

 tcl:
-        @echo --- errors and warnings logged in file make.log
-        @${MAKE} tcllibrary 2>&1 | tee -a make.log | egrep -i "(.c:|Stop.|---)"
+        @${MAKE} tcllibrary

 force: clean all

and

diff --git a/scripts/configure b/scripts/configure
index 549324e..5c91663 100755
--- a/scripts/configure
+++ b/scripts/configure
@@ -9504,9 +9504,6 @@ echo

 echo "Use 'make' to compile and 'make install' to install."
 echo
-echo "Errors may not be printed to stdout:  see files 'make.log' "
-echo "  and 'install.log' for complete error summary."
-echo
 echo "-----------------------------------------------------------"
 echo

diff --git a/scripts/configure.in b/scripts/configure.in
index 79b4cd8..1e5ace6 100644
--- a/scripts/configure.in
+++ b/scripts/configure.in
@@ -1910,9 +1910,6 @@ echo

 echo "Use 'make' to compile and 'make install' to install."
 echo
-echo "Errors may not be printed to stdout:  see files 'make.log' "
-echo "  and 'install.log' for complete error summary."
-echo
 echo "-----------------------------------------------------------"
 echo

should allow the error logs and status code to propagate nicely (in combination with #148 fixes).

RTimothyEdwards commented 2 years ago

I can see the need to remove the egrep statement, but is there any particular reason not to tee to a log file?

proppy commented 2 years ago

I think in that case you'd want to either:

to make sure you propagate the make exit code back to the parent make invocation; otherwise it will just propagate tee success.

RTimothyEdwards commented 2 years ago

Ah, right, the obscure pipefail thing.

RTimothyEdwards commented 2 years ago

@proppy : Given that SHELL is set to /bin/sh, which is not necessarily bash, is it better to insist that SHELL be bash, or just forget about logging the output?

proppy commented 2 years ago

maybe it's best to keep the Makefile simple and document in the README how to get it to log a file if needed with make 2>&1 | tee make.log, wdyt?

RTimothyEdwards commented 2 years ago

Works for me.

RTimothyEdwards commented 2 years ago

Should be all fixed now.

RTimothyEdwards commented 2 years ago

Note that the currently posted version on github will fail to compile with NULL graphics. Version 8.3.284 will fix this, as well as correctly declining to link "magicdnull" to any graphics libraries.