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

cxx-qt-lib: use +whole_archive for std_types so that cargo-only works #593

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

ahayzen-kdab
Copy link
Collaborator

When building with cargo we need to ensure that the statics aren't optimised out for registering types with Qt 5.

Otherwise we cannot use numbers in QML.

Closes #592

@ahayzen-kdab ahayzen-kdab force-pushed the 592-fix-cargo-only-example branch 13 times, most recently from 75cecee to 81c0e8e Compare June 28, 2023 14:31
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

I think the wording can be improved a bit, but otherwise looks good 👍

crates/cxx-qt-build/src/lib.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
crates/cxx-qt-build/src/lib.rs Outdated Show resolved Hide resolved
When building a staticlib -bundle means that the native static library
is simply not included into the archive and some higher level build
system will need to add it later during linking of the final binary.
https://doc.rust-lang.org/rustc/command-line-arguments.html#option-l-link-lib

Otherwise when we build with CMake the libqt-static-initializers.a
is not bundled into the final libqt_minimal.a so is then missing
from the final binary.

Closes KDAB#597
Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

LGTM

@Be-ing
Copy link
Contributor

Be-ing commented Jun 28, 2023

I'm concerned that compiling the same C++ code unconditionally with cxx-qt-build could lead to duplicate symbol errors if a user tries to have multiple crates in one project use cxx-qt. If the C++ code is moved to the cxx-qt or cxx-qt-lib crates and multiple crates in the dependency graph use cxx-qt, IIUC they would all share the same build of the C++ code that would only need to be compiled once. But if multiple crates run cxx-qt-build in their build.rs, then those crates are linked into one staticlib (like Firefox does #148), this might cause duplicate symbol errors.

@ahayzen-kdab
Copy link
Collaborator Author

Note that i tried to create a multi crate project, but this doesn't work as you run into

error: link modifiers combination `+bundle,+whole-archive` is unstable when generating rlibs

I tried to have a project where i had the following crate structure each with their own QObject and cxx-qt-build, and then main using public methods from sub1 and sub2.

main (staticlib)
  - sub1 (rlib)
  - sub2 (rlib)

@Be-ing
Copy link
Contributor

Be-ing commented Jun 28, 2023

But if multiple crates run cxx-qt-build in their build.rs, then those crates are linked into one staticlib (like Firefox does #148), this might cause duplicate symbol errors.

Considering #598 shows this already doesn't work, there's no regression by merging this.

When building with cargo we need to ensure that the statics aren't
optimised out for registering types with Qt 5.

Otherwise we cannot use numbers in QML.

Also move std_types from -lib into -build, which potentially allows
us to have cxx-qt-lib separate in the future.

Closes KDAB#592
@Be-ing Be-ing enabled auto-merge (rebase) June 29, 2023 19:38
@Be-ing Be-ing merged commit b5e05c2 into KDAB:main Jun 29, 2023
6 checks passed
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.

Cargo only example fails in main with Qt 5 due to std types confusion
3 participants