search

taocpp / taopq

C++ client library for PostgreSQL
Boost Software License 1.0
264 stars 40 forks source link

Make ${PostgreSQL_INCLUDE_DIRS} private to avoid unintentionally accessing PostgreSQL's headers #53

Closed iyanging closed 3 years ago

iyanging commented 3 years ago

fixes #52

d-frey commented 3 years ago

I probably don't understand how CMake works. Our headers include libpq-fe.h, so the dependency towards libpq requires their headers to be visible when our headers are used. I thought that this means that libpq's headers are a public requirement. This is also supported by the src/test/pq/CMakeLists.txt change you added, as our binaries should depend on taoPQ and that should automatically add the requirement for libpq. But now you added it manually, which should not be necessary IMHO. I'm confused. @uilianries Can you explain this to me?

Update: The failing tests now also support my theory AFAICT.

iyanging commented 3 years ago

The reason why I make libpq visible to tests is that tests directly use it through libpq-fe.h. But I am also confused why CI cannot compile them successfully when I can compile them in my PC.

uilianries commented 3 years ago

so the dependency towards libpq requires their headers to be visible when our headers are used

If they need PostgreSQL so yes, it should to be public. If a public header includes a pq header, so it must be public. I though there was no link between pq headers and dependencies, was more internally only.

The test should not need to include pq directly, it should be exported by taopq. If it works or not in your environment, but fails on CI, we have an environment problem then.

Let me try to reproduce your case here.

iyanging commented 3 years ago

My Bad... It's include/tao/pq/exception.hpp who include libpq-fe.h.

You're right, libpq needs to be public.

d-frey commented 3 years ago

It's not just include/tao/pq/exception.hpp:

 frey@fiasko:~/work/git/taocpp/taopq:main  grep++ libpq-fe.h
./include/tao/pq/exception.hpp:#include <libpq-fe.h>
./include/tao/pq/result.hpp:#include <libpq-fe.h>
./include/tao/pq/notification.hpp:#include <libpq-fe.h>
./include/tao/pq/transaction.hpp:#include <libpq-fe.h>
./include/tao/pq/table_reader.hpp:#include <libpq-fe.h>
./include/tao/pq/oid.hpp:#include <libpq-fe.h>
./include/tao/pq/connection.hpp:#include <libpq-fe.h>
./src/lib/pq/result.cpp:#include <libpq-fe.h>
./src/lib/pq/table_writer.cpp:#include <libpq-fe.h>
./src/lib/pq/table_reader.cpp:#include <libpq-fe.h>
 frey@fiasko:~/work/git/taocpp/taopq:main
d-frey commented 3 years ago

I'll close this one then, as this is clearly not the solution. Please re-open #52 if necessary and figure out what the actual problem is.

uilianries commented 3 years ago

Thank you for investigating and sharing.