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

fix includes for framework libs #630

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,3 @@ compile_commands.json

# cmps (https://github.com/leonmatthes/cmps)
.cmps/

3 changes: 2 additions & 1 deletion crates/cxx-qt-lib/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ fn main() {
qt_modules.push("Qml".to_owned());
}

ahayzen-kdab marked this conversation as resolved.
Show resolved Hide resolved
let qtbuild = qt_build_utils::QtBuild::new(qt_modules).expect("Could not find Qt installation");
let qtbuild = qt_build_utils::QtBuild::new(qt_modules)
.expect("Could not find Qt installation, please check the PATH environment variable or set the QMAKE environment variable.");

// Required for tests
qt_build_utils::setup_linker();
Expand Down
52 changes: 48 additions & 4 deletions crates/qt-build-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ pub struct QtBuild {
qmltyperegistrar_executable: Option<String>,
rcc_executable: Option<String>,
qt_modules: Vec<String>,
has_framework_libs: bool,
}

impl QtBuild {
Expand Down Expand Up @@ -232,6 +233,22 @@ impl QtBuild {
}
}

fn determine_if_has_framework_libs(qmake_executable: &String) -> bool {
let lib_path = QtBuild::static_qmake_query(qmake_executable, "QT_INSTALL_LIBS");
let target = env::var("TARGET");

match &target {
Ok(target) => {
if target.contains("apple") {
Path::new(&format!("{lib_path}/QtCore.framework")).exists()
} else {
false
}
}
Err(_) => false,
}
}

if let Ok(qmake_env_var) = env::var("QMAKE") {
match verify_candidate(qmake_env_var.trim()) {
Ok((executable_name, version)) => {
Expand All @@ -242,6 +259,9 @@ impl QtBuild {
rcc_executable: None,
version,
qt_modules,
has_framework_libs: determine_if_has_framework_libs(
&executable_name.to_string(),
),
});
}
Err(e) => {
Expand All @@ -265,6 +285,9 @@ impl QtBuild {
rcc_executable: None,
version,
qt_modules,
has_framework_libs: determine_if_has_framework_libs(
&executable_name.to_string(),
),
});
}
// If QT_VERSION_MAJOR is specified, it is expected that one of the versioned
Expand Down Expand Up @@ -294,8 +317,12 @@ impl QtBuild {

/// Get the output of running `qmake -query var_name`
pub fn qmake_query(&self, var_name: &str) -> String {
QtBuild::static_qmake_query(&self.qmake_executable, var_name)
}

fn static_qmake_query(qmake_executable: &String, var_name: &str) -> String {
std::str::from_utf8(
&Command::new(&self.qmake_executable)
&Command::new(qmake_executable)
.args(["-query", var_name])
.output()
.unwrap()
Expand All @@ -319,6 +346,9 @@ impl QtBuild {

match std::fs::read_to_string(prl_path) {
Ok(prl) => {
if self.has_framework_libs {
builder.flag(&format!("-F{}", lib_path));
}
for line in prl.lines() {
if let Some(line) = line.strip_prefix("QMAKE_PRL_LIBS = ") {
parse_cflags::parse_libs_cflags(
Expand Down Expand Up @@ -369,10 +399,15 @@ impl QtBuild {
}
}

format!(
let prl_file = format!(
"{}/{}Qt{}{}.prl",
lib_path, prefix, version_major, qt_module
)
);
println!(
"cargo:warning=Qt .prl file not found at expected path, falling back to {}",
prl_file
);
prl_file
}

/// Tell Cargo to link each Qt module.
Expand Down Expand Up @@ -448,12 +483,21 @@ impl QtBuild {
/// Get the include paths for Qt, including Qt module subdirectories. This is intended
/// to be passed to whichever tool you are using to invoke the C++ compiler.
pub fn include_paths(&self) -> Vec<PathBuf> {
let root_path = self.qmake_query("QT_INSTALL_HEADERS");
let mut paths = Vec::new();

if self.has_framework_libs {
let lib_root_path: String = self.qmake_query("QT_INSTALL_LIBS");

for qt_module in &self.qt_modules {
paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers"));
}
}
let root_path: String = self.qmake_query("QT_INSTALL_HEADERS");
for qt_module in &self.qt_modules {
paths.push(format!("{root_path}/Qt{qt_module}"));
}
Comment on lines 496 to 498
Copy link
Contributor

@Be-ing Be-ing Jul 31, 2023

Choose a reason for hiding this comment

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

Is this correct when Qt is installed as a framework? This allows using include paths like #include <QString> instead of #include <QtCore/QString>. Should we skip this part for frameworks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never included using #include <QtCore/QString>, always using #include <QString> like it's shown in the Qt documentation. So, I don't see the problem. Or I misunderstand your comment ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code should likely be moved into an else block of the if self.has_framework_libs statement. If it wasn't sufficient before to get the include paths with frameworks, it's probably okay to not add these include paths when using frameworks.

Copy link
Contributor Author

@jacquetc jacquetc Aug 4, 2023

Choose a reason for hiding this comment

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

I did a few researches :
The module-style includes (#include <QObject>)are essential in MacOS framework style. Using the old #include <QtCore/qobject.h> or #include <QtCore/QObject> is not possible any longer. The old style still runs well on Windows and Linux thought.

I was testing only on qml_minimal example project, so this problem didn't appear.
On a hunch, I ran QMAKE=/Users/cyril/Qt/6.5.2/macos/bin/qmake cargo run -p qml-minimal-no-cmake , so Qt's official install on MacOs with framework style

It gave me these errors:
cargo:warning=In file included from /Users/cyril/Devel/cxx-qt/target/debug/build/cxx-qt-lib-c19809693f328b69/out/cxxbridge/sources/cxx-qt-lib/src/core/qlist/qlist_i32.rs.cc:1:
cargo:warning=/Users/cyril/Devel/cxx-qt/target/debug/build/cxx-qt-lib-c19809693f328b69/out/cxx-qt-lib/qlist.h:11:10: fatal error: 'QtCore/QList' file not found
cargo:warning=#include <QtCore/QList>
cargo:warning= ^~~~~~~~~~~~~~
cargo:warning=1 error generated.
exit status: 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh curious, i found doing #include <QtCore/QObject> useful as it allows you to quickly determine which modules you need to include and link or why a library needs eg QtQuick as you can search for it quickly.

Guess we'll have to drop doing that if this doesn't work in macOS anymore (i can create a separate PR to do this), unless there is a different include folder that can be used that does include the module name + the header name ?

Copy link
Contributor

@Be-ing Be-ing Aug 7, 2023

Choose a reason for hiding this comment

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

I looked at the CI output from another projecting using the Qt official binary installer and the include paths are:

-isystem /Users/build/Qt/online/6.3.2/macos/lib/QtCore.framework/Headers
-isystem /Users/build/Qt/online/6.3.2/macos/lib/QtCore.framework/Headers/6.3.2
-isystem /Users/build/Qt/online/6.3.2/macos/lib/QtCore.framework/Headers/6.3.2/QtCore

So it seems we need:

        if self.has_framework_libs {
            let lib_root_path: String = self.qmake_query("QT_INSTALL_LIBS");

            let qt_version = &self.qt_version;
            for qt_module in &self.qt_modules {
                paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers"));
                paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers/{qt_version}"));
                paths.push(format!("{lib_root_path}/Qt{qt_module}.framework/Headers/{qt_version}/Qt{qt_module}"));
            }
        }

Please try that code with both #include <QString> and #include <QtCore/QString> style includes.

I also saw -isystem /Users/build/Qt/online/6.3.2/macos/include in the include paths, so forget what I was saying about moving the QT_INSTALL_HEADERS path into an else block; leave that code as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,
I'm back, I'll be able to test that this week.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jacquetc Have you had a chance to test the code I posted above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No yet, I'll take time tomorrow at noon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested, exactly the same error.

paths.push(root_path);

paths.iter().map(PathBuf::from).collect()
}

Expand Down
Loading