Bugfix: configure: Quote SUPPRESS_WARNINGS sufficiently to preserve brackets

luke-jr commented at 11:58 pm on March 21, 2022: member

The regex includes [/ ] which is supposed to match either a forward slash or a space, but m4 treats the brackets as special characters and effectively strips them out, leading to -isystem /usr/include paths except for in the typical scenario where it is the final parameter in the flag string.

The regex includes [/ ] which is supposed to match either a forward slash or a
space, but m4 treats the brackets as special characters and effectively strips
them out, leading to -isystem /usr/include paths except for in the typical
scenario where it is the final parameter in the flag string.

556ee6f2fa

DrahtBot added the label Build system on Mar 22, 2022

hebasto commented at 7:08 am on March 22, 2022: member

Tested this PR rebased on master on Ubuntu 22.04:

0$ ./configure --quiet --with-incompatible-bdb --enable-suppress-external-warnings CC=clang CXX=clang++
1$ make
2... massive Qt warnings

hebasto commented at 7:11 am on March 22, 2022: member

cc @vasild

Bugfix: configure: Only avoid -isystem for exact /usr/include path 5a157eb370

in configure.ac:1178 in 556ee6f2fa outdated

1174@@ -1175,7 +1175,7 @@ dnl Do not change "-I/usr/include" to "-isystem /usr/include" because that
1175 dnl is not necessary (/usr/include is already a system directory) and because
1176 dnl it would break GCC's #include_next.
1177 AC_DEFUN([SUPPRESS_WARNINGS],
1178-         [$(echo $1 |${SED} -E -e 's/(^| )-I/\1-isystem /g' -e 's;-isystem /usr/include([/ ]|$);-I/usr/include\1;g')])
1179+         [[$(echo $1 |${SED} -E -e 's/(^| )-I/\1-isystem /g' -e 's;-isystem /usr/include([/ ]|$);-I/usr/include\1;g')]])

hebasto commented at 9:17 am on March 22, 2022:

@luke-jr

Does

0         [$(echo $1 |${SED} -E -e 's/(^| )-I/\1-isystem /g' -e 's;-isystem /usr/include( |$);-I/usr/include\1;g')])

work for you?

luke-jr commented at 2:14 pm on March 22, 2022:

Yes, but we should probably still quote the command properly.

I don’t understand why -isystem breaks things for some paths… Hopefully @vasild can comment

vasild commented at 1:55 pm on March 28, 2022:

If -isystem /usr/include is used with gcc, then the compilation would fail because that somehow breaks the #include_next <...> directives used by gcc headers themselves. For example, on my system with gcc 10:

t.cc:

0#include <iostream>
1
2int main(int, char**)
3{
4    return 0;
5}

 0g++10 -isystem /usr/include t.cc -c -o t.o
 1In file included from /usr/local/lib/gcc10/include/c++/ext/string_conversions.h:41,
 2                 from /usr/local/lib/gcc10/include/c++/bits/basic_string.h:6557,
 3                 from /usr/local/lib/gcc10/include/c++/string:55,
 4                 from /usr/local/lib/gcc10/include/c++/bits/locale_classes.h:40,
 5                 from /usr/local/lib/gcc10/include/c++/bits/ios_base.h:41,
 6                 from /usr/local/lib/gcc10/include/c++/ios:42,
 7                 from /usr/local/lib/gcc10/include/c++/ostream:38,
 8                 from /usr/local/lib/gcc10/include/c++/iostream:39,
 9                 from t.cc:1:
10/usr/local/lib/gcc10/include/c++/cstdlib:75:15: fatal error: stdlib.h: No such file or directory
11   75 | #include_next <stdlib.h>
12      |               ^~~~~~~~~~
13compilation terminated.
14*** Error code 1

hebasto commented at 5:36 pm on March 22, 2022: member

Tested 556ee6f2fa0e2da2bb12fe05a885431f61db2e47. Still not working for me.

Actually, my idea was to substitute paths like /usr/include/some-path but not /usr/include.

Btw, is there a decent reason why the base commit is so old?

luke-jr commented at 5:57 pm on March 22, 2022: member

Tested https://github.com/bitcoin/bitcoin/commit/556ee6f2fa0e2da2bb12fe05a885431f61db2e47. Still #24633#pullrequestreview-916745471 for me.

That’s the original version… test the new one? (5a157eb3703e1e65ed285f13a824562ab12aa3ff)

Actually, my idea was to substitute paths like /usr/include/some-path but not /usr/include.

The current PR code should do that (allowing for trailing slash(es) on the end).

luke-jr requested review from hebasto on Mar 22, 2022

hebasto approved

hebasto commented at 6:24 am on March 23, 2022: member

ACK 5a157eb3703e1e65ed285f13a824562ab12aa3ff, tested on Ubuntu 22.04 with clang 14.0.

fanquake requested review from vasild on Mar 23, 2022

vasild commented at 2:34 pm on March 28, 2022: member

I merged this (5a157eb370) on latest master (0a14a16efe0f4a26f3f5dc8eeba2a2c11093b320) and it does not break anything: warnings from QT appear without --enable-suppress-external-warnings and are silenced when that option is used (FreeBSD, clang 14). All good.

How to reproduce the problem this PR aims to solve?

Why is the commit Bugfix: configure: Only avoid -isystem for exact /usr/include path necessary? Do we have a case where -I/usr/include/foo is used and it is not replaced by -isystem /usr/include/foo and warnings popped up from there?

I am not sure whether the CI failure is related to this PR or not.

luke-jr commented at 5:04 am on March 29, 2022: member

How to reproduce the problem this PR aims to solve?

I encountered it trying to test #24558 with --enable-suppress-external-warnings

Why is the commit Bugfix: configure: Only avoid -isystem for exact /usr/include path necessary?

Otherwise we get warnings from Qt includes I guess. #24633#pullrequestreview-916745471

vasild approved

vasild commented at 8:41 am on March 29, 2022: member

ACK 5a157eb3703e1e65ed285f13a824562ab12aa3ff

I checked that, without this PR, the generated ./configure ends up with ...include(/ |$) instead of the intended ...include([/ ]|$). To reproduce, I used BDB_CFLAGS="-I/usr/include -I/foo" in the ./configure environment and it was changed to -isystem /usr/include -isystem /foo (wrong). This PR fixes that.

Subdirectories of /usr/include should be with -isystem since e.g. Qt may be installed in /usr/include/qt5. Having -isystem /usr/include/whatever is fine with gcc’s #include_next.

Thank you!

fanquake merged this on Mar 29, 2022

fanquake closed this on Mar 29, 2022

sidhujag referenced this in commit a4c08f8f66 on Apr 3, 2022

DrahtBot locked this on Mar 29, 2023

Bugfix: configure: Quote SUPPRESS_WARNINGS sufficiently to preserve brackets #24633