[simdjson] Fix dynamic usage

[NancyLi1013]

Describe the pull request

• What does your PR fix? Fixes [simdjson] unresolved external symbol on Visual Studio 2019 32/64 bit #16062

Fix dynamic build usage on console application.

1>simdjson.test.obj : error LNK2001: unresolved external symbol "class simdjson::internal::atomic_ptr<class simdjson::implementation const > simdjson::active_implementation" (?active_implementation@simdjson@@3V?$atomic_ptr@$$CBVimplementation@simdjson@@@internal@1@A)
1>simdjson.test.obj : error LNK2001: unresolved external symbol "struct simdjson::internal::error_code_info const * const simdjson::internal::error_codes" (?error_codes@internal@simdjson@@3QBUerror_code_info@12@B)

Note: No feature needs to test.

[lemire]

We shall issue a patch release within simdjson, so patching should not be necessary.

Give us a day or so.

[lemire]

This should be fixed by Version 0.8.2 https://github.com/simdjson/simdjson/releases/tag/v0.8.2

Please note that you may need to specify a x64 target when compiling under Visual Studio 2017 even if you have a 64-bit Windows. If you are getting warnings during the build of this type...

The simdjson library is designed for 64-bit processors and it seems that you are not compiling for a known 64-bit platform. All fast kernels will be disabled and performance may be poor.

Then your compiler is targeting x86 platforms even if you have a 64-bit OS. The simdjson library is designed for 64-bit systems.

If you have further problems, we will address them. It should not be needed to patch simdjson.

[ras0219-msft]

Thank you for the quick response @lemire, @NancyLi1013 please try using the newer published version.

Also, not to tell you how to write your library, however please be aware that extending namespace std is undefined behavior by the standard:

https://en.cppreference.com/w/cpp/language/extending_std
https://stackoverflow.com/questions/37541022/what-are-the-reasons-that-extending-the-std-namespace-is-considered-undefined-be

This does include poly-fills like your std::string_view.

[lemire]

@ras0219-msft Thanks Robert. We are aware. The string_view functionality is somewhat of an edge case. It was provided as a de facto standard feature in some standard libraries before C++17 and was finally standardized in C++17. We believe that it is a key feature that we want to rely upon. The alternatives are not great. We could roll our own, we could require C++17, we could return C pointers. Some users are not yet ready to adopt C++17, so we provide a temporary bridge to people who have older compilers (supporting only C++11). We worked hard to make it transparent to users. It does not affect Visual Studio 2017 and 2019 users since in both cases, string_view is available. We will drop this polyfill as soon as C++17 is sufficiently widespread as a requirement. At this time, C++17 is still consider by many as bleeding edge.

[JackBoosY]

Wait for #16270 merge.

[ras0219-msft]

I believe this has been superceded by #16277. Thanks everyone!

[NancyLi1013]

@ras0219-msft

Thanks for your information.

This PR also includes another fix. So I think this is still necessary.

[NancyLi1013]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[lemire]

@NancyLi1013 Can you elaborate on why this fix is needed?

If SIMDJSON_BUILD_STATIC evaluates to false, then we set SIMDJSON_USING_LIBRARY=1 as a compile-time definition in CMake:

if(SIMDJSON_BUILD_STATIC)
  ...
else()
  target_compile_definitions(simdjson INTERFACE SIMDJSON_USING_LIBRARY=1)
endif()

Your patch is...

if(VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic")
     vcpkg_replace_string(${CURRENT_PACKAGES_DIR}/include/simdjson.h
         "#if SIMDJSON_USING_LIBRARY"
         "#if 1"
     )
 endif()

This should not be needed, you should already have SIMDJSON_USING_LIBRARY be 1 if SIMDJSON_BUILD_STATIC evaluates to false.

If there is an issue, I would much rather fix it upstream than have the packager patching it up. It is more sustainable.

[NancyLi1013]

@lemire

Thanks for your help and support.

I also noticed these codes, but it seems it's invalid in a VS/MSBuild project. It can work in CMake Project.

[lemire]

@NancyLi1013 Thanks.

Of course, vcpkg is welcome to apply all the patches it wants... I would just encourage you to bring back the fixes upstream so that vcpkg does not need the patches.

Cheers!!!

[JackBoosY]

@lemire I will make a PR to fix that in upstream.

[lemire]

Though it is still untested, I have a minimally invasive fix proposal upstream at simdjson/simdjson#1457

[lemire]

@NancyLi1013 @JackBoosY @ras0219-msft I would recommend you go forward with the patch right now.

For our next release (0.9), I hope to convince you that it isn't necessary. I really dislike the idea of a downstream code patch based on build configuration. It feels quite dirty to me.

[lemire]

That is, it makes less and less sense for me to patch the 0.8 version since we are quite close to a 0.9 release.

cc @jkeiser

[lemire]

PR simdjson/simdjson#1457 shows that there should be no need to patch the source code during installation.

We will include this fix in the next release of our library (0.9) which should make your life easier.

Meanwhile, for 0.8, the current patch should do fine but it should become unnecessary.

Thanks for the feedback.

[strega-nil-ms]

Cool, LGTM! Thanks @lemire, @NancyLi1013 :)