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

configuration header #313

Open
cbeck88 opened this issue Mar 15, 2018 · 2 comments
Open

configuration header #313

cbeck88 opened this issue Mar 15, 2018 · 2 comments

Comments

@cbeck88
Copy link

cbeck88 commented Mar 15, 2018

Currently, when we install the tz library using cmake, we select a bunch of configuration options, for example:

cmake -DENABLE_DATE_TESTING=NO \
             -DUSE_SYSTEM_TZ_DB=YES \
             -DTZ_CXX_STANDARD=14 \
             -DBUILD_TZ_STATIC=YES \
             -DCMAKE_CXX_FLAGS=-fPIC \
             -DCMAKE_INSTALL_PREFIX=/usr \
             .. \

However, we can't then directly use the headers as is, we also have to add a bunch of defines to the build system, per installation instructions here: https://howardhinnant.github.io/date/tz.html#Installation

So we end up writing our own configuration header like so:

#pragma once

#define USE_OS_TZDB 1

#include "date/date.h"
#include "date/tz.h"

and we tell developers that it is a bug if they include date/tz.h directly in their code and not using our in-tree configuration header.

This also means that the configuration header in-tree needs to be kept in sync with the code that is invoking cmake.

It would be nicer if, for example, the date library cmake would write its own configuration header, e.g. install tz_config.h alongside tz.h. And tz.h would itself include this header, which would contain the defines that we selected at installation time. This is the pattern followed by other common open source libraries like libpng and freetype. And it avoids the possibility of ODR violations and crashes if the installation options and the options in my hand-written header above get out of sync.

Would you guys be interested in a Pull Request that introduces such a configuration header into your cmake?

@jeremy-coulon
Copy link

I wish there was other CMake options in order to be able to load TZ database from a custom location.
Currently options are:

  • USE_SYSTEM_TZ_DB
  • USE_TZ_DB_IN_DOT

I would like another option (e.g. TZ_AUTO_DOWNLOAD).

Then I could set all options to OFF and specify my tzdata installation at runtime with "void set_install(const std::string& s);" API.

@jeremy-coulon
Copy link

if( USE_SYSTEM_TZ_DB AND NOT WIN32 )
        # cannot set USE_OS_TZDB to 1 on Windows
        target_compile_definitions( tz PUBLIC -DUSE_OS_TZDB=1 )
else( )
        target_compile_definitions( tz PUBLIC -DUSE_OS_TZDB=0 )
endif( )
if( TZ_AUTO_DOWNLOAD )
        target_compile_definitions( tz PRIVATE -DAUTO_DOWNLOAD=1 )
        target_compile_definitions( tz PUBLIC -DHAS_REMOTE_API=1 )
        find_package( CURL REQUIRED )
        include_directories( SYSTEM ${CURL_INCLUDE_DIRS} )
        set( OPTIONAL_LIBRARIES ${CURL_LIBRARIES} )
else( )
        target_compile_definitions( tz PRIVATE -DAUTO_DOWNLOAD=0 )
        target_compile_definitions( tz PUBLIC -DHAS_REMOTE_API=0 )
endif( )

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

No branches or pull requests

3 participants