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

cmake/FindPostgreSQL: put the server include dir first #217

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

medvied
Copy link
Contributor

@medvied medvied commented Oct 25, 2023

An issue with PG_VERSION_NUM definition for PostgreSQL < 15 has been revealed by #211: PG_VERSION_NUM is defined for PostgreSQL 16.

Inside the build environment for PostgreSQL 15 it looks like the following:

> grep -R PG_VERSION_NUM /usr/include/
/usr/include/postgresql/pg_config.h:#define PG_VERSION_NUM 160000
/usr/include/postgresql/15/server/pg_config.h:#define PG_VERSION_NUM 150004
/usr/include/postgresql/15/server/fmgr.h:       PG_VERSION_NUM / 100, \

And the build command for, for example, src/hnsw/external_index.c looks like this:

/usr/bin/cc -DLANTERN_USE_USEARCH -Dlantern_EXPORTS
-I/tmp/lantern/./third_party/usearch/c -isystem /usr/include/postgresql
-isystem /usr/include/postgresql/15/server -isystem /tmp/lantern/src
-isystem /tmp/lantern -Wall -Wextra -Wno-conversion -Wno-unknown-pragmas
-fPIC -MD -MT CMakeFiles/lantern.dir/src/hnsw/external_index.c.o -MF
CMakeFiles/lantern.dir/src/hnsw/external_index.c.o.d -o
CMakeFiles/lantern.dir/src/hnsw/external_index.c.o -c
/tmp/lantern/src/hnsw/external_index.c

We see

-isystem /usr/include/postgresql

followed by

-isystem /usr/include/postgresql/15/server

which means that PG_VERSION_NUM would be defined as 160000.

We see that the first file is from libpq-dev, which is not being used by Lantern.

> dpkg -S /usr/include/postgresql/pg_config.h
libpq-dev: /usr/include/postgresql/pg_config.h
> dpkg -S /usr/include/postgresql/15/server/pg_config.h
postgresql-server-dev-15: /usr/include/postgresql/15/server/pg_config.h

This patch changes the order of directories for header search to use the server dir first, so the right PG_VERSION_NUM would be defined for builds for the older PostgreSQL versions.

An issue with PG_VERSION_NUM definition for PostgreSQL < 15 has been
revealed by lanterndata#211: PG_VERSION_NUM is defined for PostgreSQL 16.

Inside the build environment for PostgreSQL 15 it looks like the
following:

```
> grep -R PG_VERSION_NUM /usr/include/
/usr/include/postgresql/pg_config.h:#define PG_VERSION_NUM 160000
/usr/include/postgresql/15/server/pg_config.h:#define PG_VERSION_NUM 150004
/usr/include/postgresql/15/server/fmgr.h:       PG_VERSION_NUM / 100, \
```

And the build command for, for example, `src/hnsw/external_index.c`
looks like this:

```
/usr/bin/cc -DLANTERN_USE_USEARCH -Dlantern_EXPORTS
-I/tmp/lantern/./third_party/usearch/c -isystem /usr/include/postgresql
-isystem /usr/include/postgresql/15/server -isystem /tmp/lantern/src
-isystem /tmp/lantern -Wall -Wextra -Wno-conversion -Wno-unknown-pragmas
-fPIC -MD -MT CMakeFiles/lantern.dir/src/hnsw/external_index.c.o -MF
CMakeFiles/lantern.dir/src/hnsw/external_index.c.o.d -o
CMakeFiles/lantern.dir/src/hnsw/external_index.c.o -c
/tmp/lantern/src/hnsw/external_index.c
```
We see
```
-isystem /usr/include/postgresql
```
followed by
```
-isystem /usr/include/postgresql/15/server
```
which means that `PG_VERSION_NUM` would be defined as `160000`.

We see that the first file is from `libpq-dev`, which is not being used
by Lantern.
```
> dpkg -S /usr/include/postgresql/pg_config.h
libpq-dev: /usr/include/postgresql/pg_config.h
> dpkg -S /usr/include/postgresql/15/server/pg_config.h
postgresql-server-dev-15: /usr/include/postgresql/15/server/pg_config.h
```
This patch changes the order of directories for header search to use the
server dir first, so the right `PG_VERSION_NUM` would be defined for
builds for the older PostgreSQL versions.
@Ngalstyan4 Ngalstyan4 merged commit 0652b41 into lanterndata:main Oct 25, 2023
@medvied medvied deleted the postgresql-include-order-fix branch October 25, 2023 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants