Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve #618 (Specify TZDB location via CMake) #623

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

connorjak
Copy link

I went ahead and renamed INSTALL to TZDB_INSTALL_PATH to reduce possibility of name conflicts.

NOTE: A common issue may be that you have multiple executables with different relative paths. I resolve that by inserting a copy of the tzdata folder at the same relative path from the each executable.

Related: #618

I went ahead and renamed `INSTALL` to `TZDB_INSTALL_PATH` to reduce possibility of name conflicts.
@connorjak
Copy link
Author

NOTE: This would require a documentation update for set_install() at https://howardhinnant.github.io/date/tz.html

@HowardHinnant
Copy link
Owner

At this time I would rather not change the API, even that of the build macros, unless it is truly urgent. The rationale for this position is that this is a period of stability for this library while C++ vendors implement and ship its C++20 equivalent. In the best of all worlds, a year from now this library will be used only for legacy projects where stability is the highest priority.

@connorjak
Copy link
Author

Got it, I can revert TZDB_INSTALL_PATH to INSTALL.

@HowardHinnant
Copy link
Owner

Thanks.

@connorjak
Copy link
Author

I did the revert in the latest commit; I'd say this PR is ready to merge.

@@ -96,6 +104,9 @@ endif()
if ( DISABLE_STRING_VIEW )
target_compile_definitions( date INTERFACE HAS_STRING_VIEW=0 -DHAS_DEDUCTION_GUIDES=0 )
endif()
if ( USE_CUSTOM_TZDB_INSTALL_PATH )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be this in the

if( BUILD_TZ_LIB )

block?

I would put it in this line:

     if ( USE_SYSTEM_TZ_DB AND NOT WIN32 AND NOT MANUAL_TZ_DB )
         target_compile_definitions( date-tz PRIVATE PUBLIC USE_OS_TZDB=1 )
     else()
          target_compile_definitions( date-tz PUBLIC INSTALL=${USE_CUSTOM_TZDB_INSTALL_PATH} USE_OS_TZDB=0 )
     endif()

Se how I removed the other: INSTALL=. together with USE_OS_TZDB, that is IMHO an error, will not be considered

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I should move it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants