Hinnant's date package fails on string view

acgetchell commented 5 years ago

In file included from C:\Tools\vcpkg\installed\x64-windows\include\date/tz.h:110: 93466C:\Tools\vcpkg\installed\x64-windows\include\date/date.h(65,11): fatal error: 'string_view' file not found 93467# include 93468 ^~~~~ 934696953 warnings and 2 errors generated. 93470ninja: build stopped: subcommand failed.

From https://ci.appveyor.com/project/acgetchell/cdt-test/build/job/axeae9y9hposj673#L93467

Project:

https://github.com/acgetchell/CDT-test

CMakeLists:

https://github.com/acgetchell/CDT-test/blob/master/CMakeLists.txt

AppVeyor:

https://github.com/acgetchell/CDT-test/blob/master/.appveyor.yml

acgetchell commented 5 years ago

On MacOS it builds, but at runtime you get this:

┌─[adam][hapkido][±][master S:1 U:2 ✗][~/CDT-test/build]
└─▪ ./cdt-opt 
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: Timezone database not found at "/Users/adam/Downloads/tzdata"
cdt-opt started at Abort trap: 6

acgetchell commented 5 years ago

https://github.com/HowardHinnant/date/issues/434

qis commented 5 years ago

Apart from fixing the broken string_view support detection, the vcpkg team might want to also do something about CMAKE_CXX_STANDARD being hard-coded to 11 in the official CMakeLists.txt file.

If Microsoft wishes to support C++11 and C++17 users, then there are three options that come to mind:

Let the user set it in a custom toolchain file and don't overwrite it in the CMakeLists.txt.
Compile an additional TU with the relevant code in C++17 mode and link it in.
Provide a flavor named cpp17 or string_view that forces C++17 compilation.

NOTES:

Option 1 would make me very happy since this is the only port that we have to patch.
Option 2 may be a little bit hacky and requires that the the C++ versions are ABI compatible. It would allow users to link both, C++11 and C++17 applications to the same library.

qis commented 5 years ago

Howard fixed the source code: https://github.com/HowardHinnant/date/commit/8a563041fa65146531249ed557a6839809488521

Now we only have to adjust the custom CMakeLists.txt: https://github.com/Microsoft/vcpkg/blob/600f7fdf30434f7c1e1e15acf3df3271d6b91387/ports/date/CMakeLists.txt#L4

How should the user control the version? A flavor?

I would greatly appreciate it if vcpkg did not overwrite the CMAKE_CXX_STANDARD setting in custom toolchains.

P.S.: The date library still handles other features by only inspecting the __cplusplus define.

It can be set with /Zc:__cplusplus, but it would probably be a bad idea to export it as part of the target_link_library interface.

Edit: Some ports do not compile with MSVC and /Zc:__cplusplus. Notably, ports/harfbuzz.

Neumann-A commented 5 years ago

could target_compile_features(package PUBLIC cxx_std_11) and/or a generator expression on the CMAKE_CXX_STANDARD somewhere solve the problem?

qis commented 5 years ago

@Neumann-A That would force the user to use C++11 in all projects that link against date or he can't link against it since the string_view related functions in the tz target are never compiled.

Neumann-A commented 5 years ago

@qis target_compile_features(package PUBLIC cxx_std_11) just forces minimum CXX standard. You can still set CMAKE_CXX_STANDARD to any value you want. Furthermore, adding something like target_compile_definitions(package PUBLIC(PRIVATE?) $<$<CMAKE_CXX_STANDARD:17>:"/Zc:__cplusplus somevalidvalue" $<NOT:$<$<CMAKE_CXX_STANDARD:17>>:"/Zc:__cplusplus someothervalue") allows the user to chose their c++ version and also setting __cplusplus to the required value.

qis commented 5 years ago

@Neumann-A Ok, say the user does that.

First, he absolutely has to create his own custom toolchain to set CMAKE_CXX_STANDARD since (as far as I know), this option is not supported by the triplet. Not something the vcpkg team wants to encourage, right?

Then he has to build his own software with the same CMAKE_CXX_STANDARD option and /Zc:__cplusplus flag. This could break the usage of several other ports.

As much as I love standard conformance, it's too early to enable it globally. In my opinion, it's better to set CMAKE_CXX_STANDARD based on a chosen flavor or do the "hack" I suggested in a previous comment.

Neumann-A commented 5 years ago

Not something the vcpkg team wants to encourage, right?

depends on your view. In the vcpkg.cmake toolchain file there is the variable: VCPKG_CHAINLOAD_TOOLCHAIN_FILE to load user defined toolchain files which can be set in a triplet for example (see also https://github.com/Microsoft/vcpkg/blob/master/docs/users/triplets.md)

In general the user is free to define and modify triplets as he or she pleases. The defaults are just there to define reasonable defaults which work out of the box without much thinking.

ras0219-msft commented 5 years ago

I think there are a few things to consider here:

Most libraries don't care about the standards version because they're programmed against 98/11
Some libraries have a minimum standard version, but end up ABI compatible as long as that minimum is met
A very small set of libraries break ABI when changing the standard version

Unfortunately, because of (3), it is important that the user can control the standard version on a triplet-wide basis. I think this is best implemented as the toolchain file setting CMAKE_CXX_STANDARD.

This means that for our default toolchain files, we should add a triplet option (such as VCPKG_CMAKE_CXX_STANDARD) which will avoid users needing to supply their own toolchain just to set the standard version.

Libraries then should always follow the CMAKE_CXX_STANDARD setting if it was set by the toolchain (even if that results in a build failure). In the case where CMAKE_CXX_STANDARD is not set, I think it is acceptable for the library to make its own decision about what the appropriate default choice is (even if that won't necessarily be the same choice for each library).

For now, we strongly discourage libraries from use target_compile_features(package PUBLIC cxx_std_11).

In the future we may need to forcibly align all defaults to be the same, however I think that is a change we will be able to make when the need arises.

NancyLi1013 commented 5 years ago

@acgetchell, the PR has been merged to the master. And it should be fixed now. Could you please update the port to the latest version and try it again?

acgetchell commented 5 years ago

Thanks @NancyLi1013 !

Problem still persists:

https://ci.appveyor.com/project/acgetchell/cdt-test/build/job/j1el69t5nwklhe4u#L93727

emptyVoid commented 5 years ago

@NancyLi1013, now it fails to link when consumed by the code compiled under C++17:

Error LNK2001 unresolved external symbol "class date::time_zone const * __cdecl date::locate_zone(class std::basic_string_view<char,struct std::char_traits<char> >)" (?locate_zone@date@@YAPEBVtime_zone@1@V?$basic_string_view@DU?$char_traits@D@std@@@std@@@Z)

Neumann-A commented 5 years ago

@emptyVoid: steps for workaround:

create toolchain file doing nothing more than setting CMAKE_CXX_STANDARD
define custom vcpkg triplet with VCPKG_CHAINLOAD_TOOLCHAIN_FILE defined to the toolchain above (or define it only for the date port)
rebuild required vcpkg packages
when configuring your project also define VCPKG_CHAINLOAD_TOOLCHAIN_FILE as above

emptyVoid commented 5 years ago

@Neumann-A, sorry I got no idea what "define costume vcpkg triplet" means.

Neumann-A commented 5 years ago

@emptyVoid I mean custom ;) Just define your own triplet.

emptyVoid commented 5 years ago

@Neumann-A, your workaround doesn't work.

I created custom.cmake toolchain file with a single line set(CMAKE_CXX_STANDARD 17).

Added x64-windows-custom.cmake triplet file with these lines:

set(VCPKG_TARGET_ARCHITECTURE x64)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE dynamic)
set(VCPKG_CHAINLOAD_TOOLCHAIN_FILE path/to/custom.cmake)

And tried building date using vcpkg install date --triplet x64-windows-custom.

The build failed with this in config-x64-windows-custom-out.log:

-- The CXX compiler identification is unknown
CMake Error at CMakeLists.txt:2 (project):
  No CMAKE_CXX_COMPILER could be found.

I guess it's due to this condition: https://github.com/microsoft/vcpkg/blob/79fd23fe99ec0ea30aba678bb92249e838473db2/scripts/cmake/vcpkg_configure_cmake.cmake#L86-L87

Neumann-A commented 5 years ago

Ok alternative workaround: add set(VCPKG_CXX_FLAGS /std:c++17) to your custom triplet and remove the line with VCPKG_CHAINLOAD_TOOLCHAIN_FILE. This should compile any package in your triplet in c++17 mode.

JackBoosY commented 5 years ago

Hi @acgetchell. does this issue bother you after using these workarounds?

qis commented 5 years ago

Regarding No CMAKE_CXX_COMPILER could be found.:

On Windows, chain-loading a toolchain file disables the automatic compiler detection in Vcpkg. You'd have to use vcpkg only from the visual studio command prompt that sets up the compiler for you.

What I do at work and in private is just overwrite the original vcpkg toolchain. Not the cleanest solution, but I have not seen any other way to do fancy stuff like LTCG for release builds.

https://github.com/qis/toolchains/blob/master/windows.cmake

@emptyVoid What you can do is simply change your triplet to something like this:

set(VCPKG_TARGET_ARCHITECTURE x64)
set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE static)

set(VCPKG_CXX_FLAGS "/std:c++17")
# or
if(PORT STREQUAL "date")
  set(VCPKG_CXX_FLAGS "/std:c++17")
endif()

acgetchell commented 4 years ago

Fixed by https://github.com/microsoft/vcpkg/pull/9006

microsoft / vcpkg

Hinnant's date package fails on string view #4864