Add ECMCargo module
Needs ReviewPublic

Authored by cblack on Mar 30 2020, 5:35 PM.

Details

Reviewers
None
Group Reviewers
Frameworks
Build System
Summary

The ECMCargo module allows for easy usage of Rust projects
in a CMake project. Usage looks like this:

ecm_add_cargo_workspace(
  DIRECTORY src/rs/
  NAME ikona
  FEATURES with-svgcleaner
  VENDOR_TARBALL ikona.cargo.vendor.tar.xz
  VENDOR_CONFIG cargo-vendor-config.toml
)
...
target_link_libraries(ikona
  PRIVATE Qt5::Core Qt5::Widgets Qt5::Quick Qt5::Concurrent KF5::I18n KF5::Kirigami2 KF5::ConfigWidgets "${ikona_artifacts_dir}/libikonars.so")

Diff Detail

Repository
R240 Extra CMake Modules
Branch
cblack/cargo-integration
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24871
Build 24889: arc lint + arc unit
cblack created this revision.Mar 30 2020, 5:35 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptMar 30 2020, 5:35 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
cblack requested review of this revision.Mar 30 2020, 5:35 PM

I'm not good at CMake modules, but maybe this code (which I used in a project) is useful:
https://github.com/Devolutions/CMakeRust

apol added a subscriber: apol.Mar 30 2020, 10:59 PM

Code per se looks okay, not that I've ever used a rust workspace.

It does need documentation though. See other ECM modules to see what it looks like.

I wonder if this is covering a very widespread use-case. If there's just one application that needs it, it could make sense to keep it there and once it's polished, we put it in ECM to share.

In D28444#638428, @apol wrote:

I wonder if this is covering a very widespread use-case. If there's just one application that needs it, it could make sense to keep it there and once it's polished, we put it in ECM to share.

I feel like one of the reasons KDE doesn't use Rust more is that using it in the build system side is pain. Case in point: more than half of Ikona's CMakeLists.txt is boilerplate dedicated to integrating Rust with the rest of the project. I'd like to get this in ECM to lower the barrier for other KDE applications and libraries to utilize Rust.

apol added a comment.Mar 31 2020, 12:41 AM

I feel like one of the reasons KDE doesn't use Rust more is that using it in the build system side is pain. Case in point: more than half of Ikona's CMakeLists.txt is boilerplate dedicated to integrating Rust with the rest of the project. I'd like to get this in ECM to lower the barrier for other KDE applications and libraries to utilize Rust.

Will this make all the boilerplate in ikona disappear?

Should we keep it in Ikona until it's ready for consumption then move it to ECM when someone needs it? We release ECM every month so there's not that much of downtime in such a case.

cblack added a comment.EditedMar 31 2020, 1:32 AM
In D28444#638435, @apol wrote:

Will this make all the boilerplate in ikona disappear?

It'll bring it down to the same level as everything else.

Should we keep it in Ikona until it's ready for consumption then move it to ECM when someone needs it? We release ECM every month so there's not that much of downtime in such a case.

I mean, I don't know how I feel about keeping something designed to be an API for public consumption in Ikona for an extended duration of time, buut then again that's basically what I do with Ikona's internal Rust and C APIs.

more than half of Ikona's CMakeLists.txt is boilerplate dedicated to integrating Rust with the rest of the project

I don't understand why your function needs a list of Rust files, even though those are already handled by Cargo. Looks boilerplate-ish...

more than half of Ikona's CMakeLists.txt is boilerplate dedicated to integrating Rust with the rest of the project

I don't understand why your function needs a list of Rust files, even though those are already handled by Cargo. Looks boilerplate-ish...

CMake needs to know when the Rust source has changed so it can rebuild it

CMake needs to know when the Rust source has changed so it can rebuild it

Changes to Rust code need to be tracked by Cargo, not by CMake

CMake needs to know when the Rust source has changed so it can rebuild it

Changes to Rust code need to be tracked by Cargo, not by CMake

CMake needs to know what files Cargo wants to build in order to invoke it only when Rust files change. There's no reason to invoke Cargo every time make is ran when CMake has the ability to keep track of dirty files.

CMake needs to know what files Cargo wants to build in order to invoke it only when Rust files change. There's no reason to invoke Cargo every time make is ran when CMake has the ability to keep track of dirty files.

I disagree here; I'd rather invoke the tool that is meant to do the job than
a) reimplement tracking in CMake;
b) duplicate file lists for no real win.
Extra concerns for the developer, barely any difference for the machine.

cblack updated this revision to Diff 79430.Apr 5 2020, 8:31 PM

Address feedback

cblack retitled this revision from WIP/RFC: Add ECMCargo module to Add ECMCargo module.Apr 5 2020, 8:31 PM
cblack edited the summary of this revision. (Show Details)

I don't have enough CMake knowledge to give code comments, but here are some things I can notice in case they help in review:

  1. The function specifically mentions Cargo workspace, which is basically a Rust alternative to TEMPLATE = subdirs in qmake. It doesn't seem though that the current implementation has code specific for this project type, and it can probably work just as well for normal executable and library projects. Am I missing something? If it can't, how would developers use this ECM module if they don't need the workspace? Can it be generalized?
  2. In the implementation that I mentioned, the resulting target gets a name that we can use directly, whereas in your usage example you have to manually use an artifact variable to actually be able to link. You also manually specify the shared library extension; will this require a platform-dependent wrapper in end user's CMakeLists to be able to use this on e.g. Windows?

Thank you for your work :)

cblack updated this revision to Diff 79521.Apr 6 2020, 6:57 PM

Add STATIC_LIBNAME and DYNAMIC_LIBNAME options