From 84332a1fc1c7c948f986bfc093856e36c3cc7da0 Mon Sep 17 00:00:00 2001 From: Leon Matthes Date: Thu, 23 Jan 2025 17:15:14 +0100 Subject: [PATCH] CXX-Qt-build: Remove definitions from Interface These definitions cannot be imported into CMake, as they are unknown at configure time. It is also best practice to keep definitions in a header file, as that allows us to only include them where really needed, as pointed out by this blog post: https://www.kdab.com/setting-defines-with-cmake/ This is the method that cxx-qt-lib now uses to configure its features. Closes #1165 --- CHANGELOG.md | 4 ++ crates/cxx-qt-build/src/dependencies.rs | 71 +------------------ crates/cxx-qt-build/src/lib.rs | 19 +---- crates/cxx-qt-lib/build.rs | 39 +++++++--- crates/cxx-qt-lib/include/core/qlist.h | 3 + crates/cxx-qt-lib/include/core/qvariant.h | 3 + crates/cxx-qt-lib/include/core/qvector.h | 3 + .../include/qml/qqmlapplicationengine.h | 3 + crates/cxx-qt-lib/include/qml/qqmlengine.h | 3 + .../include/quickcontrols/qquickstyle.h | 5 +- 10 files changed, 55 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4b51c767..1b9986f70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Build warnings due to unused unsafe blocks since CXX 1.0.130 +### Removed + +- CXX-Qt-build: Interface no longer includes compiler definitions () + ## [0.7.0](https://github.com/KDAB/cxx-qt/compare/v0.6.1...v0.7.0) - 2024-10-30 ### Added diff --git a/crates/cxx-qt-build/src/dependencies.rs b/crates/cxx-qt-build/src/dependencies.rs index a829b137c..7f4c10b20 100644 --- a/crates/cxx-qt-build/src/dependencies.rs +++ b/crates/cxx-qt-build/src/dependencies.rs @@ -7,13 +7,12 @@ use serde::{Deserialize, Serialize}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::path::{Path, PathBuf}; /// When generating a library with cxx-qt-build, the library may need to export certain flags or headers. /// These are all specified by this Interface struct, which should be passed to the [crate::CxxQtBuilder::library] function. pub struct Interface { - pub(crate) compile_definitions: HashMap>, pub(crate) initializers: Vec, // The name of the links keys, whose CXX-Qt dependencies to reexport pub(crate) reexport_links: HashSet, @@ -29,7 +28,6 @@ pub struct Interface { impl Default for Interface { fn default() -> Self { Self { - compile_definitions: HashMap::new(), initializers: Vec::new(), reexport_links: HashSet::new(), exported_include_prefixes: vec![super::crate_name()], @@ -39,32 +37,6 @@ impl Default for Interface { } impl Interface { - /// Add a compile-time-definition for the C++ code built by this crate and all downstream - /// dependencies - /// - /// This function will panic if the variable has already been defined with a different value. - /// - /// Also please note that any definitions added here will only be exported throughout the cargo - /// build. Due to technical limitations, they can not be imported into CMake with the - /// cxxqt_import_crate function. - pub fn define(mut self, variable: &str, value: Option<&str>) -> Self { - use std::collections::hash_map::Entry::*; - - let entry = self.compile_definitions.entry(variable.to_owned()); - match entry { - Vacant(entry) => entry.insert(value.map(String::from)), - Occupied(entry) => { - if entry.get().as_deref() == value { - println!("Warning: Silently ignoring duplicate compiler definition for {variable} with {value:?}."); - } - panic!( - "Cxx-Qt-build - Error: Interface::define - Duplicate compile-time definition for variable {variable} with value {value:?}!" - ); - } - }; - self - } - /// Add a C++ file path that will be exported as an initializer to downstream dependencies. /// /// Initializer files will be built into object files, instead of linked into the static @@ -152,7 +124,6 @@ pub(crate) struct Manifest { pub(crate) name: String, pub(crate) link_name: String, pub(crate) qt_modules: Vec, - pub(crate) defines: Vec<(String, Option)>, pub(crate) initializers: Vec, pub(crate) exported_include_prefixes: Vec, } @@ -257,43 +228,3 @@ pub(crate) fn reexported_dependencies( } exported_dependencies } - -pub(crate) fn all_compile_definitions( - interface: Option<&Interface>, - dependencies: &[Dependency], -) -> Vec<(String, Option)> { - // For each definition, store the name of the crate that defines it so we can generate a - // nicer error message - let mut definitions: HashMap, String)> = interface - .iter() - .flat_map(|interface| &interface.compile_definitions) - .map(|(key, value)| (key.clone(), (value.clone(), crate::crate_name()))) - .collect(); - - for dependency in dependencies { - for (variable, value) in &dependency.manifest.defines { - use std::collections::hash_map::Entry::*; - let entry = definitions.entry(variable.to_owned()); - - match entry { - Vacant(entry) => { - entry.insert((value.to_owned(), dependency.manifest.name.clone())); - } - Occupied(entry) => { - let existing_value = &entry.get().0; - // Only allow duplicate definitions with the same value - if existing_value != value { - panic!("Conflicting compiler definitions requested!\nCrate {existing} exports {variable}={existing_value:?}, and crate {conflicting} exports {variable}={value:?}", - existing=entry.get().1, - conflicting = dependency.manifest.name); - } - } - } - } - } - - definitions - .into_iter() - .map(|(key, (value, _crate_name))| (key, value)) - .collect() -} diff --git a/crates/cxx-qt-build/src/lib.rs b/crates/cxx-qt-build/src/lib.rs index 144723a61..d56a8f44c 100644 --- a/crates/cxx-qt-build/src/lib.rs +++ b/crates/cxx-qt-build/src/lib.rs @@ -657,11 +657,7 @@ impl CxxQtBuilder { } } - fn setup_cc_builder( - builder: &mut cc::Build, - include_paths: &[impl AsRef], - defines: &[(String, Option)], - ) { + fn setup_cc_builder(builder: &mut cc::Build, include_paths: &[impl AsRef]) { // Note, ensure our settings stay in sync across cxx-qt and cxx-qt-lib builder.cpp(true); builder.std("c++17"); @@ -672,11 +668,6 @@ impl CxxQtBuilder { // MinGW requires big-obj otherwise debug builds fail builder.flag_if_supported("-Wa,-mbig-obj"); - // Enable any extra defines - for (variable, value) in defines { - builder.define(variable, value.as_deref()); - } - for include_path in include_paths { builder.include(include_path); } @@ -967,13 +958,11 @@ impl CxxQtBuilder { let initializers = initializers.into_iter().collect(); let exported_include_prefixes = dependencies::all_include_prefixes(interface, &dependencies); - let defines = dependencies::all_compile_definitions(Some(interface), &dependencies); let manifest = Manifest { name: crate_name(), link_name: link_name() .expect("The links key must be set when creating a library with CXX-Qt-build!"), - defines, initializers, qt_modules: qt_modules.into_iter().collect(), exported_include_prefixes, @@ -1059,11 +1048,9 @@ impl CxxQtBuilder { // to the generated files without any namespacing. include_paths.push(header_root.join(&self.include_prefix)); - let compile_definitions = - dependencies::all_compile_definitions(self.public_interface.as_ref(), &dependencies); - Self::setup_cc_builder(&mut self.cc_builder, &include_paths, &compile_definitions); + Self::setup_cc_builder(&mut self.cc_builder, &include_paths); - Self::setup_cc_builder(&mut init_builder, &include_paths, &compile_definitions); + Self::setup_cc_builder(&mut init_builder, &include_paths); // Note: From now on the init_builder is correctly configured. // When building object files with this builder, we always need to copy it first. // So remove `mut` to ensure that we can't accidentally change the configuration or add diff --git a/crates/cxx-qt-lib/build.rs b/crates/cxx-qt-lib/build.rs index 2970c8484..41f92199e 100644 --- a/crates/cxx-qt-lib/build.rs +++ b/crates/cxx-qt-lib/build.rs @@ -43,6 +43,31 @@ fn write_headers_in(subfolder: &str) { } } +fn write_definitions_header() { + // We cannot ensure that downstream dependencies set the same compile-time definitions. + // So we generate a header file that adds those definitions, which will be passed along + // to downstream dependencies with all other headers. + // + // Thanks to David Faure for reminding us of this useful trick in his blog post: + // https://www.kdab.com/setting-defines-with-cmake/ + let mut definitions = "#pragma once\n".to_owned(); + + if qt_gui_enabled() { + definitions.push_str("#define CXX_QT_GUI_FEATURE\n"); + } + + if qt_qml_enabled() { + definitions.push_str("#define CXX_QT_QML_FEATURE\n"); + } + + if qt_quickcontrols_enabled() { + definitions.push_str("#define CXX_QT_QUICKCONTROLS_FEATURE\n"); + } + + std::fs::write(header_dir().join("definitions.h"), definitions) + .expect("Failed to write cxx-qt-lib/definitions.h"); +} + fn write_headers() { println!("cargo::rerun-if-changed=include/"); std::fs::create_dir_all(header_dir()).expect("Failed to create include directory"); @@ -66,6 +91,8 @@ fn write_headers() { if qt_quickcontrols_enabled() { write_headers_in("quickcontrols"); } + + write_definitions_header(); } fn main() { @@ -323,17 +350,7 @@ fn main() { .reexport_dependency("cxx-qt"); if qt_gui_enabled() { - interface = interface - .define("CXX_QT_GUI_FEATURE", None) - .initializer("src/gui/init.cpp"); - } - - if qt_qml_enabled() { - interface = interface.define("CXX_QT_QML_FEATURE", None); - } - - if qt_quickcontrols_enabled() { - interface = interface.define("CXX_QT_QUICKCONTROLS_FEATURE", None); + interface = interface.initializer("src/gui/init.cpp"); } let mut builder = CxxQtBuilder::library(interface).include_prefix("cxx-qt-lib-internals"); diff --git a/crates/cxx-qt-lib/include/core/qlist.h b/crates/cxx-qt-lib/include/core/qlist.h index b0b86091c..8182d5b97 100644 --- a/crates/cxx-qt-lib/include/core/qlist.h +++ b/crates/cxx-qt-lib/include/core/qlist.h @@ -8,6 +8,9 @@ #include +// The definitions file is auto-generated by the build script +#include + #include #include diff --git a/crates/cxx-qt-lib/include/core/qvariant.h b/crates/cxx-qt-lib/include/core/qvariant.h index 52c8e7625..ae420f64a 100644 --- a/crates/cxx-qt-lib/include/core/qvariant.h +++ b/crates/cxx-qt-lib/include/core/qvariant.h @@ -9,6 +9,9 @@ #include +// The definitions file is auto-generated by the build script +#include + #include #include diff --git a/crates/cxx-qt-lib/include/core/qvector.h b/crates/cxx-qt-lib/include/core/qvector.h index 8a4faa3f4..191905272 100644 --- a/crates/cxx-qt-lib/include/core/qvector.h +++ b/crates/cxx-qt-lib/include/core/qvector.h @@ -8,6 +8,9 @@ #include +// The definitions file is auto-generated by the build script +#include + #include #include diff --git a/crates/cxx-qt-lib/include/qml/qqmlapplicationengine.h b/crates/cxx-qt-lib/include/qml/qqmlapplicationengine.h index cf561a4d4..2ca606f2d 100644 --- a/crates/cxx-qt-lib/include/qml/qqmlapplicationengine.h +++ b/crates/cxx-qt-lib/include/qml/qqmlapplicationengine.h @@ -6,6 +6,9 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 #pragma once +// The definitions file is auto-generated by the build script +#include + #ifdef CXX_QT_QML_FEATURE #include diff --git a/crates/cxx-qt-lib/include/qml/qqmlengine.h b/crates/cxx-qt-lib/include/qml/qqmlengine.h index cb078998e..c0803751e 100644 --- a/crates/cxx-qt-lib/include/qml/qqmlengine.h +++ b/crates/cxx-qt-lib/include/qml/qqmlengine.h @@ -6,6 +6,9 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 #pragma once +// The definitions file is auto-generated by the build script +#include + #ifdef CXX_QT_QML_FEATURE #include diff --git a/crates/cxx-qt-lib/include/quickcontrols/qquickstyle.h b/crates/cxx-qt-lib/include/quickcontrols/qquickstyle.h index 0ab3aa0ce..1fb5cf116 100644 --- a/crates/cxx-qt-lib/include/quickcontrols/qquickstyle.h +++ b/crates/cxx-qt-lib/include/quickcontrols/qquickstyle.h @@ -6,6 +6,9 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 #pragma once +// The definitions file is auto-generated by the build script +#include + #ifdef CXX_QT_QUICKCONTROLS_FEATURE #include @@ -27,4 +30,4 @@ qquickstyleSetStyle(const QString& style); } } -#endif \ No newline at end of file +#endif