search

jkvargas / russimp-sys

Assimp raw bindings
Other
2 stars 22 forks source link

Vcpkg integration #15

Closed JonCB closed 3 years ago

JonCB commented 3 years ago

An attempt to fix #14.

The solution here is using cargo-vcpkg to manage vcpkg. When youcargo vcpkg build it will walk the dependency tree for vcpkg dependencies and then build anything it finds. This will place the vcpkg directory inside the target directory (I haven't tested but assume it would play nice with cargo workspaces as well).

To resolve the correct libdir and libname, i've reworked the build script to use the rust vcpkg build dependency to probe for assimp build information. The good thing about this is that the same code will work regardless of whether the developer is using per-project dependencies with cargo vcpkg or has a central vcpkg install (via a provided VCPKG_ROOT). In theory if it can't find vcpkg or it can't find assimp inside vcpkg it will default to the values originally provided so i believe this should maintain the same build behaviour for people not using vcpkg (in theory someone could use vcpkg on linux or mac and this should work just as well for them... obviously haven't tested this).

I also switched the cargo instruction from specifying rustc flags to the builtin "rustc-link-lib". Wasn't critical but it seemed like a good idea at the time... i can obviously roll that back if there was an explicit reason to specify flags instead.

Something else worth noting... vcpkg by default will output cargo directives and i haven't squelched that behaviour so if the vcpkg probe finds assimp it will output rustc-link-lib twice for the assimp library. This doesn't seem to cause a problem so I left it in place.

JonCB commented 3 years ago

Something worth noting, the tests on windows currently fail because of a rogue doctest being generated by bindgen. I'd assumed that this was normal but lin-build suggests otherwise. I'll have a hunt to see if i can figure out where it's coming from.

jkvargas commented 3 years ago

Hey mate. First let me thank you a lot for checking this up on windows. Can I ask you to enable the windows build inside .github/workflows/russimp-sys.yml before we merge this? That way we can be sure that everything is going as planned :)

JonCB commented 3 years ago

Yeah no problem. I'm not entirely sure why we're not seeing the doctest issue in linux... the version it's using should have the offending comment (line 506-511 of postprocess.h). How wedded to bindgen generating documentation are you? I can tell bindgen to not generate them and that works but obviously if you want the docs then that's a non-starter.

jkvargas commented 3 years ago

You can disable the docs for now. This will be used just on russimp mainly, and if more information is needed than we can refer to assimp source code.

jkvargas commented 3 years ago

Also, can you please raise the release version to 0.2.0? :)

JonCB commented 3 years ago

Ugh... no libclang. Documentation says that LLVM should be installed as part of VS but obviously i'm getting the directory wrong. I'll give it more of a look in the morning.

JonCB commented 3 years ago

Finally.

As it stands, this will inevitably start failing when github updates VS on the "windows-latest" box. There is probably a way to get the VS install dir as an environment variable but i don't know enough about github actions to be sure.

If it breaks before that happens, you should be able to use the documentation pointed to at https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#preinstalled-software to find the updated install path for visual studio.