szaszm commented on code in PR #1337:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1337#discussion_r879152231
##########
cmake/Date.cmake:
##########
@@ -28,15 +28,12 @@ if (WIN32)
FetchContent_Populate(tzdata)
endif()
- install(DIRECTORY ${tzdata_SOURCE_DIR}/
- DESTINATION tzdata
- COMPONENT tzdata
- )
+ file(COPY
${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/cldr-common-38.1/common/supplemental/windowsZones.xml
+ DESTINATION ${tzdata_SOURCE_DIR})
- install(FILES
${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/cldr-common-38.1/common/supplemental/windowsZones.xml
+ install(DIRECTORY ${tzdata_SOURCE_DIR}/
DESTINATION tzdata
- COMPONENT tzdata
- )
+ COMPONENT tzdata)
Review Comment:
I don't really like writing into any SOURCE_DIR. Consider copying it all to
a build directory, outside of FETCHCONTENT_BASE_DIR, but inside
CMAKE_CURRENT_BUILD_DIR, and using that for the unit tests.
##########
cmake/Date.cmake:
##########
@@ -60,7 +57,7 @@ if (NOT date_src_POPULATED)
target_compile_options(date-tz PRIVATE $<IF:$<CXX_COMPILER_ID:MSVC>,/w,-w>)
target_compile_definitions(date-tz PRIVATE AUTO_DOWNLOAD=0
HAS_REMOTE_API=0)
if (WIN32)
- target_compile_definitions(date-tz PRIVATE INSTALL=. PUBLIC
USE_OS_TZDB=0)
+ target_compile_definitions(date-tz PRIVATE INSTALL=. PUBLIC
USE_OS_TZDB=0 PUBLIC TZ_DATA_DIR="${tzdata_SOURCE_DIR}")
Review Comment:
I prefer that this reference to the source dir only in unit tests only, and
not the normal binaries. If a user installs binaries only, then any reference
to source directories are going to be invalid and useless.
A way to separate them could be having another target for tests only, which
depends on date-tz and has this definition.
--
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]
