Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] v.hull fails to compile in GRASS 7.8.5 #1563

Closed
okanisis opened this issue May 4, 2021 · 11 comments · Fixed by #1567
Closed

[Bug] v.hull fails to compile in GRASS 7.8.5 #1563

okanisis opened this issue May 4, 2021 · 11 comments · Fixed by #1567
Labels
bug Something isn't working

Comments

@okanisis
Copy link

okanisis commented May 4, 2021

Describe the bug
When compiling GRASS 7.8.4 against GDAL 3.3.0 the v.hull module fails.

To Reproduce

GRASS GIS 7.8.5 exported compilation log
--------------------------------------------------
Started compilation: Mon 03 May 2021 10:51:18 PM MST
--
Errors in:
/tmp/makepkg/grass/src/grass-7.8.5/vector/v.hull

and the error from the v.hull folder in the src package gives:

grass/src/grass-7.8.5/vector/v.hull % make
gcc -D_FORTIFY_SOURCE=2 -march=native -O2 -pipe -fstack-protector-strong -fno-plt   -I/tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include -I/tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include   -I/usr/include -I/usr/include -DPACKAGE=\""grassmods"\"   -I/tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include -I/tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include -DRELDIR=\"vector/v.hull\" -o OBJ.x86_64-pc-linux-gnu/chull.o -c chull.c
In file included from /usr/include/ogr_api.h:47,
                 from /tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include/grass/vect/dig_structs.h:27,
                 from /tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include/grass/vect/digit.h:3,
                 from /tmp/makepkg/grass/src/grass-7.8.5/dist.x86_64-pc-linux-gnu/include/grass/vector.h:4,
                 from chull.c:27:
chull.c:34:19: error: expected ‘;’, identifier or ‘(’ before ‘_Bool’
   34 | { BFALSE, BTRUE } bool;
      |                   ^~~~
make: *** [../../include/Make/Compile.make:32: OBJ.x86_64-pc-linux-gnu/chull.o] Error 1

System description (please complete the following information):

  • Operating System: Arch Linux (5.11.6 kernel)
  • GRASS GIS 7.8.4
  • GDAL 3.3.0

Additional context
Downgrading GDAL to 3.2.2 lets GRASS 7.8.4 compile.

@okanisis okanisis added the bug Something isn't working label May 4, 2021
@neteler
Copy link
Member

neteler commented May 4, 2021

The code in question is

/*Define Boolean type */
typedef enum
{ BFALSE, BTRUE } bool;

typedef enum

but I don't see any connection to GDAL so far...

@okanisis okanisis changed the title [Bug] v.hull fails in compile against gdal 3.3.0 [Bug] v.hull fails to compile in GRASS 7.8.5 May 4, 2021
@okanisis
Copy link
Author

okanisis commented May 4, 2021

I updated the title so it's not specific - cause I really don't know the cause of the underlying issue.

@nilason
Copy link
Contributor

nilason commented May 4, 2021

bool is a C99 addition, which actually needs #include <stdbool.h> (but perhaps some compilers are too generous, thus the _Bool in the error message) . Testing this I could only reproduce the reported error (using clang) by explicitly include the stdbool.h, which isn't included in chull.c at least. Now, there shouldn't be any reason anymore to (re-)typedef a local bool type. The code in chull.c should probably be modified to use standard C bool type, alternatively use the TRUE and FALSE defined in gis.h.
But I'm curious which compiler and version caused this reported error, @saultdon ?

@neteler
Copy link
Member

neteler commented May 5, 2021

Do we want this fixed in the upcoming 7.8.6 release or no hurry?

@okanisis
Copy link
Author

okanisis commented May 5, 2021

If it matters too, in the PKGBUILD[1] there is a line that exports CPP as:

  # Ancient autoconf used upstream can't handle CPPFLAGS correctly, so set CPP to ignore warnings
  CPP="gcc -E -w" \

I'll get you the compiler version today when I get back home (out on the trapline right now with sketchy service).

[1] https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=grass

@okanisis
Copy link
Author

okanisis commented May 5, 2021

I have GCC 10.2.0

@nilason
Copy link
Contributor

nilason commented May 6, 2021

Thanks for the additional info @saultdon!

bool is a C99 addition, which actually needs #include <stdbool.h>

bool is a macro for _Bool and the use of bool needs the inclusion of stdbool.h for compiling C code. This is a deliberate design for C99 to avoid problems (conflicts with legacy/existing code which may have defined a local bool) for example with this case of chull.h. However, compiling this code as C++ (which has bool as a built-in type) re-produces exactly the problem reported.
Although the GRASS code may be updated to use the C99 bool type, that is not really a fix for this reported issue, which is most likely caused by compiling the C code as C++ code.

E.g. use of locally defined bool (as in chull.c):
works with clang -x c ...
fails with clang -x c++ ...

I would suggest that this is a downstream/packaging issue, nothing that should promptly be addressed in GRASS.

@nilason
Copy link
Contributor

nilason commented May 6, 2021

Now I see where this conflict originates. It is indeed GDAL 3.3.0 related, as ogr_api.h now includes the stdbool.h.

This will indeed become a general problem the moment GDAL 3.3 will be more widely spread, which may be sooner than later, and should be addressed in GRASS.

nilason added a commit to nilason/grass that referenced this issue May 6, 2021
@nilason
Copy link
Contributor

nilason commented May 6, 2021

@saultdon If you can, please test the proposed fix #1567 against GDAL 3.3. It builds fine on GDAL 3.2, but I have no 3.3 around at the moment.

@okanisis
Copy link
Author

okanisis commented May 6, 2021

Build works with patch against GDAL 3.3.0

% gdalinfo --version
GDAL 3.3.0, released 2021/04/26

@david-geiger
Copy link

Also confirmed hare on Mageia Cauldron #1567 fixes build with gdal 3.3.0.

nilason added a commit that referenced this issue May 19, 2021
Enables globally the C99 'bool' type with its values 'true' and 'false'.

The old boolean constants 'TRUE' and 'FALSE' are still valid but
redefined to 'true' and 'false' to emphasize their equivalence.

(Re-)definitions of TRUE/FALSE in modules are removed.

This change was originally triggered by a conflict of v.hull -- defining
a local bool type -- with GDAL 3.3 which introduced the inclusion of
stdbool.h (#1563).
nilason added a commit that referenced this issue May 19, 2021
Enables globally the C99 'bool' type with its values 'true' and 'false'.

The old boolean constants 'TRUE' and 'FALSE' are still valid but
redefined to 'true' and 'false' to emphasize their equivalence.

(Re-)definitions of TRUE/FALSE in modules are removed.

This change was originally triggered by a conflict of v.hull -- defining
a local bool type -- with GDAL 3.3 which introduced the inclusion of
stdbool.h (#1563).
a0x8o added a commit to a0x8o/grass that referenced this issue May 19, 2021
Enables globally the C99 'bool' type with its values 'true' and 'false'.

The old boolean constants 'TRUE' and 'FALSE' are still valid but
redefined to 'true' and 'false' to emphasize their equivalence.

(Re-)definitions of TRUE/FALSE in modules are removed.

This change was originally triggered by a conflict of v.hull -- defining
a local bool type -- with GDAL 3.3 which introduced the inclusion of
stdbool.h (OSGeo#1563).
a0x8o added a commit to a0x8o/grass that referenced this issue May 19, 2021
Enables globally the C99 'bool' type with its values 'true' and 'false'.

The old boolean constants 'TRUE' and 'FALSE' are still valid but
redefined to 'true' and 'false' to emphasize their equivalence.

(Re-)definitions of TRUE/FALSE in modules are removed.

This change was originally triggered by a conflict of v.hull -- defining
a local bool type -- with GDAL 3.3 which introduced the inclusion of
stdbool.h (OSGeo#1563).
ninsbl pushed a commit to ninsbl/grass that referenced this issue Oct 26, 2022
Enables globally the C99 'bool' type with its values 'true' and 'false'.

The old boolean constants 'TRUE' and 'FALSE' are still valid but
redefined to 'true' and 'false' to emphasize their equivalence.

(Re-)definitions of TRUE/FALSE in modules are removed.

This change was originally triggered by a conflict of v.hull -- defining
a local bool type -- with GDAL 3.3 which introduced the inclusion of
stdbool.h (OSGeo#1563).
ninsbl pushed a commit to ninsbl/grass that referenced this issue Feb 17, 2023
Enables globally the C99 'bool' type with its values 'true' and 'false'.

The old boolean constants 'TRUE' and 'FALSE' are still valid but
redefined to 'true' and 'false' to emphasize their equivalence.

(Re-)definitions of TRUE/FALSE in modules are removed.

This change was originally triggered by a conflict of v.hull -- defining
a local bool type -- with GDAL 3.3 which introduced the inclusion of
stdbool.h (OSGeo#1563).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants