vfx-rs / ptex-bind

High-level cxx-based bindings for Ptex https://github.com/wdas/ptex https://crates.io/crates/ptex https://crates.io/crates/ptex-sys
https://crates.io/crates/ptex
Apache License 2.0
5 stars 2 forks source link

ptex 2.4 and cppmm updates #1

Closed davvid closed 2 years ago

davvid commented 3 years ago

@anderslanglands cppmm question: I manually hacked up my cppmm build to emit find_package(Ptex REQUIRED) and use target_link_libraries(... PRIVATE Ptex::Ptex_dynamic) in order to use Ptex's imported targets.

This is different than what cppmm emits by default for its autogenerated CMakeLists.txt.

I was able to build ptex-c with those changes, but I was a little unclear on what we're supposed to do next in order to build ptex-sys. I'll read the code in the openexr-bind project a bit to see if there are any hints there. Does the cargo part assume that we've installed globally or setup our environment accordingly in order to find ptex-c?

Oh.. back to the cppmm question -- how do you feel about a --cmake-use-find-package option that takes a string target_link_libraries argument as a parameter? When cppmm sees that flag it'll use find_package and it'll emit the user-supplied target_link_libraries arguments.

That seems like it would be sufficient to handle the use case here were we want to customize the generated cmakelists.txt. If you think that sounds like a good idea (or if you have other suggestions) let me know and I'll see if I extend cppmm for it.

luke-titley commented 3 years ago

I was able to build ptex-c with those changes, but I was a little unclear on what we're supposed to do next in order to build ptex-sys. I'll read the code in the openexr-bind project a bit to see if there are any hints there. Does the cargo part assume that we've installed globally or setup our environment accordingly in order to find ptex-c?

Hey. So for openexr, I manually wrote a build.rs that finds openexr and builds it for the -sys crate. I'm using environment variables (following anders lead on that), but it could be changed to use cmake find if that's how you wanted to do it for ptex.

_Oh.. back to the cppmm question -- how do you feel about a --cmake-use-find-package option that takes a string target_link_libraries argument as a parameter? When cppmm sees that flag it'll use find_package and it'll emit the user-supplied target_linklibraries arguments.

That seems like it would be sufficient to handle the use case here were we want to customize the generated cmakelists.txt. If you think that sounds like a good idea (or if you have other suggestions) let me know and I'll see if I extend cppmm for it.

From my perspective that sounds like a good idea. The only alternative I can think of, is if we make it possible to provide a template CMakelists.txt file for cppmm to use. Might be more flexible...

anderslanglands commented 3 years ago

Hi David! Thanks for kicking the tires!

Yeah I've been thinking about CMake as well - the current setup was good to get something up and running but we probably want to rely on CMake for setting up all the relevant options.

I think all our target libraries are using modern CMake these days? i.e. they're creating proper configs that can be found with CMAKE_PREFIX_PATH?

If so I guess the best idea would indeed be to have extra options to asttoc. Probably:

--find-package <package name>
--target-link-libraries <multiple> <library> <names>

e.g.

asstoc --find-package OpenEXR --target-link-libraries OpenEXR::OpenEXR OpenEXR::OpenEXRUtil

It would be really nice if we could pass the same arguments to astgen to find the headers - need to check how to do that.

Then when it comes to actually building the -sys crate (and its dependencies), then rather than specifying OPENEXR_ROOT like we do currently, build.rs should check CMAKE_PREFIX_PATH instead and if it's set, ignore the local build.

That still leaves us with having to specify the full list of libraries to link in build.rs, unless we can get that from CMake from there as well.

davvid commented 3 years ago

Excellent, leveraging CMAKE_PREFIX_PATH sounds perfect. I'll rebase and pickup the updated glibcxx abi macros and drop the s/::std/std/ edit I made.

anderslanglands commented 3 years ago

Hi David, this PR on cppmm adds support for "modern" cmake: https://github.com/vfx-rs/cppmm/pull/96 and this PR on openexr-bind shows how I've currently setup the build.rs in order to be able to use it to handle both submodule-built and pre-installed target libraries: https://github.com/vfx-rs/openexr-bind/pull/39

davvid commented 3 years ago

Thanks for the --fp and --tll options -- they work great. I updated bind.sh here to leverage that new feature.

davvid commented 2 years ago

Alrighty, thanks again for cppmm @anderslanglands. I added a test.rs similarly to how openexr-bind handles it (by copying it into the generated ptex-sys/src/ directory) and both cargo build and cargo test are passing now, so I think this is good to merge as a starting point.

We don't have the generated std_string::new() that cppmm uses in its test examples here yet, though, but that's something that can be added in later once I see how that's getting setup.

I'll plan to merge this in this week.