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
KDAB · Jan 27, 2025 · 7d96ffe · 7d96ffe
1 parent d5b0c98
commit 7d96ffe
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 98 deletions.
diff --git 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 (<https://github.com/KDAB/cxx-qt/issues/1165>)
+
 ## [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
@@ -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<String, Option<String>>,
     pub(crate) initializers: Vec<PathBuf>,
     // The name of the links keys, whose CXX-Qt dependencies to reexport
     pub(crate) reexport_links: HashSet<String>,
@@ -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<String>,
-    pub(crate) defines: Vec<(String, Option<String>)>,
     pub(crate) initializers: Vec<PathBuf>,
     pub(crate) exported_include_prefixes: Vec<String>,
 }
@@ -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<String>)> {
-    // 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, (Option<String>, 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
@@ -657,11 +657,7 @@ impl CxxQtBuilder {
         }
     }
 
-    fn setup_cc_builder(
-        builder: &mut cc::Build,
-        include_paths: &[impl AsRef<Path>],
-        defines: &[(String, Option<String>)],
-    ) {
+    fn setup_cc_builder(builder: &mut cc::Build, include_paths: &[impl AsRef<Path>]) {
         // 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
@@ -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
@@ -8,6 +8,9 @@
 
 #include <cstdint>
 
+// The definitions file is auto-generated by the build script
+#include <cxx-qt-lib/definitions.h>
+
 #include <QtCore/QList>
 
 #include <QtCore/QByteArray>

diff --git a/crates/cxx-qt-lib/include/core/qvariant.h b/crates/cxx-qt-lib/include/core/qvariant.h
@@ -9,6 +9,9 @@
 
 #include <cstdint>
 
+// The definitions file is auto-generated by the build script
+#include <cxx-qt-lib/definitions.h>
+
 #include <QtCore/QVariant>
 
 #include <QtCore/QByteArray>

diff --git a/crates/cxx-qt-lib/include/core/qvector.h b/crates/cxx-qt-lib/include/core/qvector.h
@@ -8,6 +8,9 @@
 
 #include <cstdint>
 
+// The definitions file is auto-generated by the build script
+#include <cxx-qt-lib/definitions.h>
+
 #include <QtCore/QVector>
 
 #include <QtCore/QByteArray>

diff --git 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 <cxx-qt-lib/definitions.h>
+
 #ifdef CXX_QT_QML_FEATURE
 
 #include <memory>

diff --git 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 <cxx-qt-lib/definitions.h>
+
 #ifdef CXX_QT_QML_FEATURE
 
 #include <memory>

diff --git 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 <cxx-qt-lib/definitions.h>
+
 #ifdef CXX_QT_QUICKCONTROLS_FEATURE
 
 #include <memory>
@@ -27,4 +30,4 @@ qquickstyleSetStyle(const QString& style);
 }
 }
 
-#endif
+#endif