WillAyd commented on code in PR #233: URL: https://github.com/apache/iceberg-cpp/pull/233#discussion_r2352754420
########## CMakeLists.txt: ########## @@ -60,7 +60,6 @@ include(CMakeParseArguments) include(IcebergBuildUtils) include(IcebergSanitizer) include(IcebergThirdpartyToolchain) -include(GenerateExportHeader) Review Comment: Meson does not have this feature. Rather than generate something in the build directory, I created the export header directly in the source and modeled the structure of it after similar visibility.h files in Apache Arrow ########## test/test_common.cc: ########## @@ -28,7 +28,7 @@ #include <nlohmann/json.hpp> #include "iceberg/json_internal.h" -#include "iceberg/test/test_config.h" +#include "test_config.h" Review Comment: The reason why this needed to change is that there is an inconsistency between the source directory layout and the build directory layout when it comes to the CMake configuration. The test_config.h file is located in the `test` directory in the source tree, but CMake installs it to `iceberg/test` in the build directory. Meson does not allow you to modify the structure of the build directory like that; generally where you place the `meson.build` file in the source tree will reflect where files from that are output into the build directory. I think the ideal approach would be to move the tests to `iceberg/test` so that the source, build (and potentially install) trees all have the same layout. If we want to do that however, I think that's best to a separate PR -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
