-
Notifications
You must be signed in to change notification settings - Fork 89
qt-build-utils: build system changes to have a QtInstallation #1230
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
qt-build-utils: build system changes to have a QtInstallation #1230
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 73 73
Lines 12681 12681
=========================================
Hits 12681 12681 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5f33455
to
8dcd7d5
Compare
634fb0d
to
1ad252a
Compare
fefe5fc
to
155f7a1
Compare
155f7a1
to
86a7f22
Compare
328cc00
to
01dc659
Compare
cdb31a3
to
7015b0f
Compare
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.
Some nitpicks, overall looking great!
// } | ||
fn link_modules(&self, builder: &mut cc::Build, qt_modules: &[String]); | ||
/// Find the path to a given Qt tool for the Qt installation | ||
fn try_find_tool(&self, tool: QtTool) -> Option<PathBuf>; |
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.
This should probably return an anyhow::Result
instead of an Option.
Otherwise it can be heard to decipher why a required tool could not be found.
crates/qt-build-utils/src/lib.rs
Outdated
/// rather than `"QtCore"`). | ||
// | ||
// TODO: is there a better name for this method or a sane way to create a default QtInstallation? | ||
pub fn new_with_default_installation(qt_modules: Vec<String>) -> anyhow::Result<Self> { |
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.
This should be new
and the other one should be with_installation
as "automatically doing the right thing" should be the behavior of new
.
b0f57b4
to
6c94838
Compare
I somehow managed to close this PR with an ill-formed force-push 🤯 |
This is part of one of the steps towards #1125 and rewriting qt-build-utils.