[Data] Unconditionally includes of SQLParser.h

[Krzmbrzl]

The use of the SQL parser is optional but the SQLParser header is included unconditionally in e.g. the Statement.h header.

This is problematic as SQLParser.h itself includes

poco/Data/include/Poco/Data/SQLParser.h

Lines 26 to 28 in 48d7a3e

	#include "sql-parser/src/SQLParser.h"
	#include "sql-parser/src/SQLParserResult.h"
	#include "sql-parser/src/util/sqlhelper.h"

but these are only considered if the parser is active

poco/Data/CMakeLists.txt

Lines 4 to 8 in 48d7a3e

	if (NOT POCO_DATA_NO_SQL_PARSER)
	file(GLOB_RECURSE SRCS_PARSER "src/sql-parser/src/*.cpp")
	LIST(REMOVE_ITEM SRCS_PARSER "${CMAKE_CURRENT_SOURCE_DIR}/src/sql-parser/src/parser/conflict_test.cpp")
	POCO_SOURCES_AUTO(SRCS ${SRCS_PARSER})
	endif()

and thus these don't end up getting installed along the other stuff if the parser is disabled. However, as soon as that happens compilation fails as the parser's header includes these non-existent files.

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.

[Krzmbrzl]

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 🤔

[aleks-f]

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.:

poco/Data/include/Poco/Data/SQLParser.h

Lines 23 to 28 in 48d7a3e

	#ifndef POCO_DATA_NO_SQL_PARSER
	#include "sql-parser/src/SQLParser.h"
	#include "sql-parser/src/SQLParserResult.h"
	#include "sql-parser/src/util/sqlhelper.h"

[Cheney-W]

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.

[aleks-f]

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?

[Cheney-W]

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.

[aleks-f]

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:

poco/Data/CMakeLists.txt

Line 44 in 48d7a3e

$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>

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

[Cheney-W]

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()

[FreelancerCGN]

@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")

[Cheney-W]

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.