[Data] Unconditionally includes of SQLParser.h about poco HOT 9 OPEN

However, when downstream or individual programs call this header file, they are not obligated to pass this macro at compile time, and may not necessarily know that it needs to be passed. This can lead to the inclusion of the aforementioned header files, potentially causing issues with header file not found.

from poco.

Firstly, add the installation of headers from the sql-parser in the file https://github.com/pocoproject/poco/blob/devel/Data/CMakeLists.txt. This would ensure that the headers can be found, at least when POCO_DATA_NO_SQL_PARSER is set to off.

I'm not sure what "installation of headers from the sql-parser" exactly refers to - path to headers is there unconditionally:

Secondly, redesign a forward-working macro to replace POCO_DATA_NO_SQL_PARSER, such as POCO_DATA_SQL_PARSER or another more appropriate name. which defaults to ON. It defaults to ON, and in the header files, check if it is defined before including headers like sql-parser/src/SQLParser.h.

Sounds like inverse logic resulting in the same situation where SQLParser is present by default, and must be explicitly excluded. While you are helping the clients who are using the build without SQLParser, what about the users who want it and now explicitly have to enable it?

As far as I see at the moment, only an additional level of indirection can resolve this situation - in order to be able to remove the #include SQLParser.h in Statement.h, all SQLParser dependencies should be moved to a separate class, which would allow forward declaration in Statement.h and removal of the #include SQLParser.h. I suggest to think it over and propose the change through pull request - it will be much easier to discuss. Since this would be a breaking change, it can't go in before 1.14

from poco.

Or maybe the issue is that the header files in the sql-parser subdirectory don't get installed at all. Regardless of whether or not the parser is enabled 🤔

from poco.

Therefore, I think it would probably make sense to add some conditional including at some point. Most logical to me would seem to guard the includes of SQLParser.h itself.

But there is conditional including in the SQLParser.h itself.:

from poco.

What exactly are you proposing to do here? Wrap #include "SQLParser.h" into ifdefs? I don't see how would that solve the described problem?

from poco.

Firstly, add the installation of headers from the sql-parser in the file https://github.com/pocoproject/poco/blob/devel/Data/CMakeLists.txt. This would ensure that the headers can be found, at least when POCO_DATA_NO_SQL_PARSER is set to off.

Secondly, redesign a forward-working macro to replace POCO_DATA_NO_SQL_PARSER, such as POCO_DATA_SQL_PARSER or another more appropriate name. which defaults to ON. It defaults to ON, and in the header files, check if it is defined before including headers like sql-parser/src/SQLParser.h.

from poco.

The "installation of headers from the sql-parser" I referred to is the operation that directly copies specific header files to a designated location, as demonstrated in the following code:

install(
	DIRECTORY include/Poco
	DESTINATION include
	COMPONENT Devel
	PATTERN ".svn" EXCLUDE
)

The mentioned $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src> only adds that path to the include directories during compilation and does not actually copy files.

I fully agree with the solution you proposed. Before it is completed and released, I will add the following patch to this PR to ensure that the issue of missing header files does not occur again in vcpkg.

diff --git a/Data/CMakeLists.txt b/Data/CMakeLists.txt
index 72da6df..12a2870 100644
--- a/Data/CMakeLists.txt
+++ b/Data/CMakeLists.txt
@@ -49,6 +49,20 @@ target_include_directories(Data
 POCO_INSTALL(Data)
 POCO_GENERATE_PACKAGE(Data)
 
+if (NOT POCO_DATA_NO_SQL_PARSER)
+    file(GLOB HEADERS "${CMAKE_CURRENT_LIST_DIR}/src/sql-parser/src/*.h")
+    install(FILES ${HEADERS} DESTINATION "include/Poco/Data/sql-parser/src")
+    
+    file(GLOB HEADERS "${CMAKE_CURRENT_LIST_DIR}/src/sql-parser/src/parser/*.h")
+    install(FILES ${HEADERS} DESTINATION "include/Poco/Data/sql-parser/src/parser")
+    
+    file(GLOB HEADERS "${CMAKE_CURRENT_LIST_DIR}/src/sql-parser/src/sql/*.h")
+    install(FILES ${HEADERS} DESTINATION "include/Poco/Data/sql-parser/src/sql")
+    
+    file(GLOB HEADERS "${CMAKE_CURRENT_LIST_DIR}/src/sql-parser/src/util/*.h")
+    install(FILES ${HEADERS} DESTINATION "include/Poco/Data/sql-parser/src/util")
+endif()
+
 if(ENABLE_TESTS)
 	add_subdirectory(testsuite)
 endif()

from poco.

@Cheney-W

Thank you for your fix. But even if you

• modify the file "Data/CMakeLists.txt" according your patch
• and set "-DPOCO_DATA_NO_SQL_PARSER=ON" for cmake

the file "Config.h" (after the installation with "cmake --build . --target install") won't be updated with

#define POCO_DATA_NO_SQL_PARSER

(in my case it's "/usr/local/include/Poco/Config.h")

from poco.

I have modified my fix solution in vcpkg. The current result is that the header files under sql-parser are always installed, regardless of whether the value of POCO_DATA_NO_SQL_PARSER is ON or OFF. This ensures that the sql-parser header files can always be found whenever there is a call to them.

from poco.