search

mcgoo / vcpkg-rs

Build library for finding native libraries in vcpkg for Rust - Windows (msvc), Linux and macOS
https://docs.rs/vcpkg
Apache License 2.0
108 stars 22 forks source link

Support override vcpkg installed dir #49

Closed Danielmelody closed 1 year ago

Danielmelody commented 1 year ago

This might be more generic than #42 . As manifest mode is a sub case of overriding an existing vcpkg installed directory, So this pr is made.

pkgw commented 1 year ago

Thank you for your submission! This looks reasonable to me in isolation, although I don't feel like I understand the broader context of when this mode is useful. (Which is just a matter of my ignorance, not a problem with the PR.)

Danielmelody commented 1 year ago

In a hybrid C++/Rust project, one can use a vcpkg.json manifest file to manage vcpkg installations. The path for vcpkg installations can be altered to any desired location using CMake. 'vcpkg-rs' thus can be a bridge between the existing vcpkg installation and Rust, even in manifest mode.

pkgw commented 1 year ago

@waych Any feedback on this one?

waych commented 1 year ago

Hmm. My initial thought is that this is fine and can go in as is.

Thinking longer term, I'm not particularly fond of the way this extends the interface on the Config directly, because this can cause problems with dependent crates that also want to pull in vcpkg dependencies. This makes me think that perhaps we should be propagating this information down through the crate dependencies somehow so that dependent crates' vcpkg dependencies can be fulfilled by a single top level vcpkg.json manifest. Given that there isn't any good way for the top-level crate to pass this information to dependent crates (?) from build.rs, it seems perhaps it would be clearer, although maybe more inconvenient to be passing this installed path via an environment variable where all vcpkg-rs::Config's in the build tree see the same thing.

That said I think this is okay for now given the problem already exists in the config interface with the existing Config::vcpkg_root() setter that has the exact same problem. Any library crate using these will have similar challenges. This kiss approach is good enough for now and can easily be further extended with an environment variable when the time comes.

pkgw commented 1 year ago

Hmmm. I can see how environment variables can address the issue you raise, although I guess it's less "passing information from the top-level crate to dependents" and more "user specifies global-level information".

Maybe there would be some way to specify this configuration, or where to find this configuration, in the Cargo.toml metadata? I am wondering if build.rs scripts can use some magic to get information about the toplevel Cargo.toml in their workspace, not just their local file. Or some kind of devious Cargo feature?? (That's almost certainly a bad idea.)

Anyway, since I definitely don't have a strong feeling about how to do a better job of solving this problem, I'm also in favor of merging this PR.

Danielmelody commented 1 year ago

Thank you for those good reviews. I think perhaps adding environment variable like VCPKG_INSTALLED_ROOT should workaround the problem. As we already have VCPKG_ROOT playing the same role. One thing need to concern is that VCPKG_ROOT is also a env used by vcpkg itself, while VCPKG_INSTALLED_ROOT is a env defined by ourselves. Considering a real example, ffmpeg-next-sys tries to using vcpkg in their build.rs.
Defining environment VCPKG_INSTALLED_ROOT from the top crate's config.toml is the only way to pass vcpkg installed location information to ffmpeg-next-sys. So I guess it is valuable to extend environment variable at this moment.

pkgw commented 1 year ago

OK; more to do on this topic, but I've merged this piece.