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

[IMPROVEMENT] Use Corrosion to build Rust code #1630

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

emilazy
Copy link
Contributor

@emilazy emilazy commented Aug 1, 2024

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.
  • I have mentioned this change in the changelog.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

Currently, using the CMake Ninja generator fails when Rust code is enabled, because the dependencies and outputs of the Rust code compilation are not fully declared. I tried to fix this with the BYPRODUCTS feature, but it seemed like it wouldn’t really work. Instead, I ported the Rust build to Corrosion, which is a CMake package designed to integrate CMake and Cargo. Corrosion is used by other projects that combine C++ and Rust, like the work‐in‐progress Rust port of the fish shell. Now people can get faster, more reliable builds using Ninja, and the CMake code is simpler and cleaner.

Developers and users do not need to explicitly install Corrosion, thanks to CMake’s FetchContent feature. However, if Corrosion is installed on the system, that version will be used.

I bumped the minimum CMake requirement to 3.15.0, as Corrosion requires this version and CMake was warning that compatibility for versions as old as the current declaration will be removed in the future. However, if this is not desired, I could lower it back down so that users on older versions of CMake can still build the project as long as they disable Rust, and the policy_max argument of cmake_minimum_required could be used to fix the warning.

@emilazy
Copy link
Contributor Author

emilazy commented Aug 1, 2024

The CI failure looks like an FFmpeg compatibility error rather than anything this should have caused, although I’m not sure why it would be happening only on macOS.

@emilazy emilazy force-pushed the push-rozmtsmxykmp branch from de9cef5 to 6d6537c Compare August 2, 2024 16:11
@emilazy
Copy link
Contributor Author

emilazy commented Aug 2, 2024

Pushed a change to use the more elegant FIND_PACKAGE_ARGS argument to FetchContent_Declare to achieve the behaviour of using the system Corrosion package if present. This bumps up the required CMake version to compile the Rust code a little further, although the required version is still older than the one present in e.g. Debian stable. I can undo this if it‘s not preferred.

@PunitLodha PunitLodha self-requested a review August 12, 2024 14:34
Copy link
Member

@PunitLodha PunitLodha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good improvement. Thanks for adding Corrosion!

@PunitLodha PunitLodha merged commit b92ca87 into CCExtractor:master Aug 12, 2024
15 of 16 checks passed
IshanGrover2004 pushed a commit to IshanGrover2004/ccextractor-fork that referenced this pull request Aug 23, 2024
@emilazy emilazy deleted the push-rozmtsmxykmp branch November 17, 2024 08:46
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.

2 participants