corrosion-rs / corrosion

Marrying Rust and CMake - Easy Rust and C/C++ Integration!
https://corrosion-rs.github.io/corrosion/
MIT License
1.07k stars 103 forks source link

fix: add a check to include, default to no change in actions #485

Closed bconn98 closed 7 months ago

bconn98 commented 8 months ago

While creating the Vcpkg port, I discovered that the CMake script tries to include the tool during the build process immediately after a note recommending that users not do this. To create the Vcpkg port, I cannot have Rust available. To resolve this, I need to optionally disable the include as it is recommended.

jschwe commented 8 months ago

I discovered that the CMake script tries to include the tool during the build process immediately after a note recommending that users not do this.

I believe you may be misunderstanding something here. Are you perhaps referring to the following line:

It is strongly encouraged to install Corrosion separately and use find_package(Corrosion REQUIRED) instead if that works with your workflow.

The intention behind this is that Corrosion supports CMake versions before 3.19 by shipping a Rust tool to do parsing of JSON metadata. If Corrosion is installed, this Rust tool only needs to be built once (before the installation). When using Corrosion as a submodule, the Rust tool needs to be built every time a new cmake build directory is configured. This also means that for CMake < 3.19 Rust (and that include line) is absolutely required to make the build and install step work! For CMake >= 3.19 the comment is outdated.

For CMake 3.19 the include could indeed be disabled, but I'd like to have a better understanding of why it is required.

Could you provide some more information on which CMake versions are supposed to be supported by vcpkg / vcpkg-Corrosion, and why Rust is not available at vcpkg install time? Could you explain the situation where Rust is not available at the time vcpkg installs Corrosion, but Rust would be available when the user uses Corrosion?

bconn98 commented 8 months ago

It maybe easiest to answer the following question

Could you provide some more information on which CMake versions are supposed to be supported by vcpkg / vcpkg-Corrosion, and why Rust is not available at vcpkg install time? Could you explain the situation where Rust is not available at the time vcpkg installs Corrosion, but Rust would be available when the user uses Corrosion?

The Vcpkg CI/CD requires that every port (project) that it installs can be installed in their base container which does not support Rust/Cargo/Rustup. Users that install Corrosion using Vcpkg and attempt to find_package(Corrosion CONFIG REQUIRED) would see the standard Corrosion error message when it's unable to find a Rust installation.

When using Corrosion as a submodule, the Rust tool needs to be built every time a new cmake build directory is configured.

I think that each Vcpkg install is treated similar to a submodule. I was able to verify that this change works in a small test application. I didn't commit & push the changes before wiping my OS the other day, but I can recreate it pretty easily if you'd prefer to see and test it out.

I believe you may be misunderstanding something here. Are you perhaps referring to the following line:

Sounds like I did misunderstand. However, I think the ambiguity of the variable name allows users to disable the include functionality for whatever reason they may choose.

jschwe commented 8 months ago

. I was able to verify that this change works in a small test application. I didn't commit & push the changes before wiping my OS the other day, but I can recreate it pretty easily if you'd prefer to see and test it out.

Your current MR just targets the master branch (which already removed support for older CMake version), but it might still take a while until the next major corrosion version is released.

If you want a vcpkg release with a v0.4 release, could you test your changes with Corrosion stable/v0.4 and a CMake version between 3.15 and 3.18? I believe that your current PR is not sufficient to prevent failures when using these older CMake versions when Rust is not installed.

Otherwise, if it's not urgent from your side, we could also merge this PR as is, and when Corrosion v0.5 is released add the vcpkg support.

bconn98 commented 8 months ago

I think it's fine to wait until 0.5. As far as the CMake version, the current Vcpkg requires CMake 3.27.1. I backed down my cmake install from 3.25 to 3.15.3, but Vcpkg pulls down 3.27.1 (its minimum) to perform the install/build. In the short term, vcpkg allows for patch files which allows me to make the change in the "short" term. The team there wants to make sure that it gets flowed back into source though.

bconn98 commented 8 months ago

I can confirm though, that without that install line it causes unexpected failures using CMake 3.15.

jschwe commented 8 months ago

the current Vcpkg requires CMake 3.27.1. I

great, then that isn't a problem on vcpkg side, even with corrosion v0.4. I would not backport this MR to the v0.4 branch, so you would need to keep the patch on the vcpkg side until v0.5 is released.

bconn98 commented 8 months ago

Sounds good!