Opened 16 months ago

Closed 8 months ago

Last modified 8 months ago

#5278 closed defect (fixed)

GHA PG16 tests failing on all warnings being treated as errors

Reported by: robe Owned by: robe
Priority: medium Milestone: PostGIS 3.4.0
Component: build Version: master
Keywords: Cc:


So I enabled PG16 on github actions and it's failing because of all sorts of warnings. Most notably in

gserialized_spgist_2d.c (use of error: passing argument 1 of 'DatumGetPointer' makes integer from pointer without a cast [-Werror=int-conversion] )

lwgeom_triggers.c:113:25: note: in expansion of macro 'DirectFunctionCall1'
  113 |   out = PointerGetDatum(DirectFunctionCall1(LWGEOM_addBBOX, in));
      |                         ^~~~~~~~~~~~~~~~~~~
In file included from lwgeom_triggers.c:26:
/usr/local/pgsql/include/server/postgres.h:670:29: note: expected 'const void *' but argument is of type 'Datum' {aka 'long unsigned int'}
  670 | PointerGetDatum(const void *X)

gserialized_spgist_3d.c:433:9: note: shadowed declaration is here
  433 |  BOX3D *box = DatumGetBox3DP(in->datums[0]);
      |         ^~~
In file included from ../libpgcommon/lwgeom_pg.h:22,
                 from gserialized_gist_2d.c:51:
/usr/local/pgsql/include/server/utils/geo_decls.h:239:25: note: expected 'const BOX *' but argument is of type 'BOX2DF *'
  239 | BoxPGetDatum(const BOX *X)
      |              ~~~~~~~~~~~^
gserialized_spgist_3d.c:484:10: error: declaration of 'box' shadows a previous local [-Werror=shadow=compatible-local]

I didn't think PG16 changed that recently, so these might exist in earlier. Looks like only PG14 GHA is testing in tests mode which treats all warnings as errors, and that one is running fine.

Change History (10)

comment:1 by Regina Obe <lr@…>, 16 months ago

In 2405b53/git:

Change PG16 to not treat warnings as errors. References #5278

comment:2 by strk, 16 months ago

Resolution: fixed
Status: newclosed

GHA is all green now, I guess this ticket can be closed

comment:3 by robe, 16 months ago

Resolution: fixed
Status: closedreopened

No GHA is green because I changed the tests from usan_clang/tests to coverage.

coverage GHA script doesn't treat warnings as errors. But we do have a ton of warnings now which weren't present with PG15.

A lot of stuff they seem to be doing in the PG16 cycle is replacing their home grown functions with standard C ones. So I think that is what is causing these warnings. I haven't investigated in detail yet.

comment:4 by strk, 13 months ago

The reported shadowed BOX in gserialized_spgist_3d.c:433 seems to be a real error to me as the box received as parameter by the picksplit 3d function is ONLY used for the SRID and nothing else. \cc @pramsey

comment:5 by robe, 8 months ago

Owner: changed from strk to robe
Status: reopenednew

comment:6 by Regina Obe <lr@…>, 8 months ago

Resolution: fixed
Status: newclosed

In 9401093/git:

FIX GHA 16 latest warnings
Closes #5278 for PostGIS 3.4.0

  • Turn back on warnings notices on latest (switch back tests mode)
  • Fix shadow warnings on gserialized gist c files
  • Fix shawow warning on mvt
  • Fix double-free warning on address_standardizer
  • Fix strncopy length depends on bounds of source instead of dest

comment:7 by strk, 8 months ago

The change in postgis/gserialized_spgist_3d.c looks dangerous. It would be good if Paul could take a look. The BOX3D "box" variable is first initialized with in→datums[0]and then again in a loop over in→nTuples. Should it not be initialized upfront at all ? Should it be declared constant ? I would do both things but I'm not sure how easy it is to test these things.

comment:8 by strk, 8 months ago

We seem to assume there's always at least one tuple and we use that to fetch the SRID, I'll make that more explicit

comment:9 by Sandro Santilli <strk@…>, 8 months ago

In 3874270/git:

const-correct getOctant

References #5278

comment:10 by Paul Ramsey <pramsey@…>, 8 months ago

In 1985836/git:

Change local var names to avoid shadows, references #5278

Note: See TracTickets for help on using tickets.