-
Notifications
You must be signed in to change notification settings - Fork 89
WIP: cxx-qt-build: use init method rather than WHOLE_ARCHIVE for CMake #964
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
Conversation
712d09a
to
7b515c0
Compare
int | ||
main(int argc, char* argv[]) | ||
{ | ||
cxx_qt::init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to document this in the book and changelog
// https://doc.qt.io/qt-6/qtresource-qtcore-proxy.html#Q_INIT_RESOURCE | ||
let qrc_name = file_stem.replace('-', "_"); | ||
// Note this cannot be inside a namespace | ||
format!(" Q_INIT_RESOURCE({qrc_name};") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to check this works with qrc files as we don't have any in our tests?
crates/cxx-qt-build/src/lib.rs
Outdated
} | ||
|
||
// Generate init method | ||
cc_builder_whole_archive.file(self.write_init_method()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appears we still need the +whole-archive
internally though? Unless is there a way of removing this too 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might only be Cargo builds that now need it? As seems they can't tell that the extern's from Rust are linking through to the init method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but this also means we don't need the init
method at all in our cargo-only builds, right?
Initialization can just work "as-is" in that case, as we don't have a problem there iirc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it means we don't need the init
currently in Cargo, although it'd be nice to remove this whole-archive and have the linker figure things out.
However i wonder if we still document that you should use it, as later it gives us a place to register/setup anything globally. Eg what if we wanted to setup Qt logging or something?
7b515c0
to
f6a5730
Compare
f6a5730
to
d96c28e
Compare
This reverts commit 40f5ad1. We have changed to using an init method rather than whole-archive which fixes Windows CI.
d96c28e
to
d8157b3
Compare
The build system has changed instead as described in #983 |
No description provided.