search

pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
463 stars 133 forks source link

CMake install target does not install dll on windows #314

Closed bvstrien closed 3 months ago

bvstrien commented 3 months ago

Since version 2.3.0 (commit https://github.com/pboettch/json-schema-validator/commit/0d60d48a586965b56d1f5c89a1e7a5cedbbdb8f2) the install target on windows is broken, as predicted by the following comment:

https://github.com/pboettch/json-schema-validator/blob/349cba9f7e3cb423bbc1811bdd9f6770f520b468/src/CMakeLists.txt#L58

The problem seems to be the use of the non-existent ${CMAKE_INSTALL_RUNTIMEDIR}, replacing it with ${CMAKE_INSTALL_BINDIR} fixed it for me. The documentation for install(TARGETS) also suggests RUNTIME should be set to ${CMAKE_INSTALL_BINDIR}.

I've attached a patch, applying this fix. fix_cmake.patch

pboettch commented 3 months ago

Could you create a PR instead of adding a patch to this issue?

pboettch commented 3 months ago

@LecrisUT What do you think?

LecrisUT commented 3 months ago

I haven't seen any good windows CMake scripts that I can reference and I am not familiar enough with the ecosystem. Maybe there it does make sense to have cpack and stuff.

Anyway regarding CMAKE_INSTALL_RUNTIMEDIR -> CMAKE_INSTALL_BINDIR. Indeed the former does not exist (must have misread RUNSTATEDIR). The latter expands to bin, is that an appropriate design on windows or should it be lib. If you have any reference @bvstrien it would be greatly appreciated

bvstrien commented 3 months ago

I don't have a direct reference. But I consume this library via vcpkg, and microsoft provides clear guidance there: Installation directory layout conventions

In short, the bin dir should contain the dll files, as they should be next to the exe files, and the lib dir should contain static libraries and import libraries. That now seems to be the case again, and vcpkg installs it properly after the fix.

LecrisUT commented 3 months ago

Thanks, didn't know it followed GNUInstallDirs. About vcpkg, does it need to be uploaded or packaged like in conan or does it take it dynamically?

bvstrien commented 3 months ago

I'm not quite sure what you're asking, but it basically automates downloading, building and deploying the packages using the library-provided build tools. See their "portfile" here, which calls vcpkg_cmake_install(), which presumably just ends up calling cmake --install.

LecrisUT commented 3 months ago

Thanks, you've answered my question :D. Yeah you need to upload some metadata similar to homebrew and other packages. Cheers for the assist @bvstrien