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

use CLI C++ code generator with CMake, put crates in cargo workspace #173

Closed
wants to merge 10 commits into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Jul 24, 2022

depends on #172

Be-ing added 6 commits July 23, 2022 15:40
Previously, the C++ code was autogenerated by cxx-qt-build from
applications' build.rs, then repeatedly recompiled for each
application. Now, the C++ code is generated once and built as
a library which is linked to each application. This is a
prerequisite for removing the need for applications to have
build.rs to instead use the CLI C++ code generator. Also, this
avoids redundant compilation of the autogenerated C++ which is
constant acrosss applications.
Somehow it links with lld but not GNU ld??
This moves the build of the Rust libraries from CMake configure time to
CMake build time, resolving KDAB#106.

In addition to behaving in a more conventional manner for a C++ build
system, this reduces the build time of this repository by avoiding
redundant compilation of the Rust code that does the C++ code
generation.
This was previously used for communicating the C++ files to build to
CMake, but that is now done with the C++ library and the standard output
of the CLI tool.
@Be-ing Be-ing marked this pull request as draft July 24, 2022 00:03
@Be-ing Be-ing changed the title use CLI C++ code generator with CMake use CLI C++ code generator with CMake, put crates in cargo workspace Jul 24, 2022
@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 24, 2022

Windows builds are down to 24 minutes without cache populated and 14 minutes with the cache populated. That's a nice improvement from the 25-45 minutes we had before. Locally, with sccache and ccache disabled, this builds in 3 minutes versus 6 minutes for the main branch. 🚀

@jason-yau
Copy link

jason-yau commented Jul 24, 2022

The function cxx_qt_create_target has too many parameters. As developers using this CXX-Qt, it's best to simplify it. For example, we can compute the MANIFEST_PATH by the function. As for CRATES, I have a question. Do you want to import multiple CRATES? Whether or not in this case, I think the CRATES can be removed and computed internally by the function.

@jason-yau
Copy link

jason-yau commented Jul 24, 2022

How about cxxqt_add_library(<name> <source>...) or cxxqt_add_library(TARGET <name> BRIDGE_MODULE_FILES <source>...)? This looks like add_library in CMake, so developers can guess how to use this function。
For example:

cxxqt_add_library(rustlibrary
    src/lib.rs
    path/to/cxxqt/bridge/source.rs
)

or

cxxqt_add_library(
    TARGET
         rustlibrary
    BRIDGE_MODULE_FILES
        src/lib.rs
        path/to/cxxqt/bridge/source.rs
)

And then we can link the rust library as below:

target_link_libraries(cpp-exe
    PRIVATE
        rustlibrary
)

I prefer the first one because it is almost identical to add_library in CMake.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 24, 2022

I like your suggestions @jason-yau, however, now that I have this working, I am questioning whether this is something we should keep. I want to replace the static cxx-qt-lib C++ library and CLI code generator with a rewritten cxx-qt-build crate to be used from build.rs. Currently in the main branch, the applications' build.rs script generates the C++ files but does not build them. Instead, there is a convoluted process serializing the C++ source code into JSON files so CMake can compile it. That was the reason that the Rust libraries were compiled at CMake configure time; CMake's configure stage had to be aware of what C++ files it needed to compile.

Going forward, I want the Rust libraries' build.rs scripts to not just generate the C++ code, but also compile it and statically link it into the Rust library with the cc crate. This will obviate the need to build and distribute the static C++ library generated from cxx-qt-lib, the CxxQt.cmake module, and the CLI executable. It will also eliminate the possibility of the cxx-qt Rust libraries, the generated C++ library, and the CLI code generator to be different, incompatible versions. Instead, CMake applications could simply use Corrosion to build and link the Rust library at CMake build time (which this branch is using currently, plus more CMake logic to link the cxx-qt-lib C++ library and set the #include paths). In addition to simplifying using cxx-qt with CMake, this approach has the advantage of making cxx-qt easier to use from other C++ build systems; it wouldn't be different from linking any other Rust library into a C++ build system.

Previously I was thinking that doing the build with Cargo was a more long-term goal, however, now that I think about the practicalities of:

  1. Us autogenerating and publishing a C++ library,
  2. Downstream applications and packagers building and installing that C++ library,
  3. Downstream applications and packagers building and installing a CLI executable written in Rust, and
  4. Downstream applications and packagers distributing and using a CMake module from us

I think handling this all from Cargo would be much simpler for downstreams. Initially I was thinking of distributing the cxx-qt-lib C++ library and CLI tool with vcpkg. However, vcpkg currently has no infrastructure for building Rust. The reason this wasn't done before is because it is more complicated to implement for us. We have to:

  1. Run cxx on cxx-qt-lib to generate C++ code, compile it, and link it into the Rust library. Using a Cargo workspace (as this branch does currently), I think this will only need to be done once even when the application uses multiple crates with cxxqt::bridge modules. I think that would keep build times down and avoid potential issues with duplicate symbols.
  2. Run cxx, our code generation step, and Qt's moc on the application code, then compile and link the generated C++ code to the Rust library using the cc crate

To implement this, I am thinking of extracting qttypes' build.rs script into a new crate and adding a feature to find and invoke moc (qmetaobject doesn't need that because it reimplements moc in Rust). I could then rewrite qttypes' build.rs to use the new crate so we can share code. I propose calling this new crate qt-build. Initially I was thinking of calling it qt-sys, but that would be a bit unconventional for a crate that doesn't provide FFI bindings on its own.

From there I think it would be a relatively small step to do the entire build with Cargo without even needing CMake. This would make it feasible to gradually rewrite existing C++ applications in Rust piece by piece, and eventually as an end goal have a Rust application.

What do you think?

@ahayzen
Copy link

ahayzen commented Jul 24, 2022

Note CMake is likely to remain our primary use case / build system, as this is the preferred build system of Qt, there are large existing projects that we need to add to easily (if you are starting from scratch use an all Rust system like Slint), and there are other tools / functions within Qt's CMake that we need to hook into in the future with Qt 6 etc. (implementing these additional tools / functions in some kind of Rust cc wrapper thing seems overkill). But making it possible to build a basic Qt project (just calling moc etc) with cargo is still kinda interesting but not the primary goal.

The existing CMake API below is already much nicer than we have :-)

cxx_qt_create_target(
    TARGET ${APP_NAME}_cxxqt
    BRIDGE_MODULE_FILES
        src/rustfile.rs
    MANIFEST_PATH Cargo.toml
    CRATES crate-name
)

But I do agree with Jason, what is the CRATES bit for, would you ever have multiple crates? (seems no). Also as Jason suggested copying CMake's add_library() method and having the following API seems even better :-)

cxxqt_add_library(rustlibrary
    src/lib.rs
    path/to/cxxqt/bridge/source.rs
)

As for distribution, using a tarbull / git repository for cxx-qt-lib's project didn't seem too bad initially, or the alternate route of cxx-qt-cli / cxx-qt-template having an option to create this project / template for you (eg creating the CxxQt.cmake file, putting the cxx-qt-lib files into a 3rdparty folder, adding the CMakeLists file for cxx-qt-lib, then allowing the developer to easily include CxxQt.cmake and just add_subdirectory on the cxx-qt-lib project. Similar to a typical `3rdparty/cxx-qt-lib`` folder most projects have.) Then the tooling can self update this as cargo updates the tool etc.

@jason-yau
Copy link

@Be-ing We can use cxx-qt-cli to generate cxx-qt-lib C++ files and cxx.h. This will tell CMake which files need to be compiled and which paths need to be included during the CMake configuration.

@Be-ing
Copy link
Contributor Author

Be-ing commented Jul 27, 2022

To clarify, my primary motivation for moving responsibilities from CMake to Cargo is not to do the whole build from Cargo, but rather to simplify the CMake integration and make it easier to use cxxqt with other C++ build systems. From there, doing the whole build with Cargo without needing CMake at all would be a relatively small step and a nice bonus, but not the primary purpose.

In #174 I have successfully moved the build of cxx-qt-lib's C++ code from CMake to Cargo and got Cargo to link to Qt (although CMake is still doing the final link of the application, this was needed for the Cargo test executables to link). The next step after that, which I will work on next week, will be refactoring cxx-qt-build to also compile the C++ code it generates. When that is done, integrating a cxx-qt staticlib into a CMake project will be as simple as using any other Rust library with Corrosion; there won't be a need for a CMake module specific to cxx-qt.

I am closing this now that I have shown this is feasible in #174.

@Be-ing Be-ing closed this Jul 27, 2022
@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 25, 2022

Superseded by #218

Be-ing added a commit to Be-ing/cxx-qt that referenced this pull request Aug 25, 2022
This was a good try for improving the build process:
KDAB#173
However, the practicalities of distributing the Rust exectuable,
plus a C++ library, plus a CMake module, would not be worth it.
Now that cxx-qt-build has been refactored so it compiles the
generated C++ code from build.rs scripts, there is no need for a
separate CLI tool.
Be-ing added a commit to Be-ing/cxx-qt that referenced this pull request Aug 25, 2022
This was a good try for improving the build process:
KDAB#173
However, the practicalities of distributing the Rust exectuable,
plus a C++ library, plus a CMake module, would not be worth it.
Now that cxx-qt-build has been refactored so it compiles the
generated C++ code from build.rs scripts, there is no need for a
separate CLI tool.
ahayzen-kdab pushed a commit to Be-ing/cxx-qt that referenced this pull request Aug 26, 2022
This was a good try for improving the build process:
KDAB#173
However, the practicalities of distributing the Rust exectuable,
plus a C++ library, plus a CMake module, would not be worth it.
Now that cxx-qt-build has been refactored so it compiles the
generated C++ code from build.rs scripts, there is no need for a
separate CLI tool.
ahayzen-kdab pushed a commit that referenced this pull request Aug 26, 2022
This was a good try for improving the build process:
#173
However, the practicalities of distributing the Rust exectuable,
plus a C++ library, plus a CMake module, would not be worth it.
Now that cxx-qt-build has been refactored so it compiles the
generated C++ code from build.rs scripts, there is no need for a
separate CLI tool.
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