Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support override vcpkg installed dir #49

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Conversation

Danielmelody
Copy link
Contributor

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.

src/lib.rs Outdated Show resolved Hide resolved
@pkgw
Copy link
Collaborator

pkgw commented Feb 3, 2023

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.)

src/lib.rs Show resolved Hide resolved
@Danielmelody
Copy link
Contributor Author

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
Copy link
Collaborator

pkgw commented Feb 5, 2023

@waych Any feedback on this one?

@waych
Copy link
Collaborator

waych commented Feb 6, 2023

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
Copy link
Collaborator

pkgw commented Feb 7, 2023

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
Copy link
Contributor Author

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 pkgw merged commit b52cfe1 into mcgoo:master Feb 11, 2023
@pkgw
Copy link
Collaborator

pkgw commented Feb 11, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants