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

Add includemocs.py and create one moc file per cpp file #59230

Merged
merged 17 commits into from
Oct 30, 2024

Conversation

m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented Oct 26, 2024

This creates an include moc file per source file, which is supposed to improve build times for incremental builds and have a few other positive effects.
https://planet.kde.org/friedrich-kossebau-2023-06-28-include-also-moc-files-of-headers/
It should also help to fix the requirement for /bigobj in some environments (not sure about the exact conditions under which this is required)

Copied from https://github.com/KDABLabs/KDToolBox/tree/master/qt/includemocs

Fixes #59157

What this does (reviewer guide)

  1. adds a helper script scripts/includemocs.py which can add moc include lines and detect missing ones (see link above for source)
  2. add moc include lines (that's where all the files are affected)
  3. adds include guards in various header files where they were missing
  4. removes unused private variables
  5. removes qt_add_cpp calls, they are not needed with automoc in place

@github-actions github-actions bot added this to the 3.42.0 milestone Oct 26, 2024
@m-kuhn m-kuhn changed the title Add includemocs.py Add includemocs.py and create one moc file per cpp file Oct 26, 2024
@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 26, 2024

To reduce deviation from master and make future backporting easier, I'd propose to backport this too.

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 26, 2024

Nice addition, for whatever reason, we now get warnings about unused private variables which were hidden before.

Copy link

github-actions bot commented Oct 26, 2024

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 3a7182b)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 3a7182b)

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

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

Lgtm! will you resurrect the warnings as Error flag?

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 28, 2024

Lgtm! will you resurrect the warnings as Error flag?

Thanks for the review @nyalldawson
It's gone

@m-kuhn m-kuhn force-pushed the mocdefs branch 2 times, most recently from a6f19ab to afd266c Compare October 29, 2024 19:40
it warns erroneously about moc_include headers
@m-kuhn m-kuhn merged commit f436bfa into qgis:master Oct 30, 2024
30 checks passed
@m-kuhn m-kuhn deleted the mocdefs branch October 30, 2024 21:14
@nicogodet
Copy link
Member

@m-kuhn It still fails on my machine
D:\QGIS_src\src\core\expression\qgsexpressionfunction.cpp : fatal error C1128: le nombre de sections a dépassé la limite du format de fichier objet : compilez avec /bigobj
Same point as before

I compile with Ninja and ccache, should I delete my build folder and retry ?
Ninja compiles more than 3000 files so I suspect it will not change the outcome

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 31, 2024

That's on a different file now (not the moc any more)

Can you add this file to the list that you cited before:

QGIS/src/core/CMakeLists.txt

Lines 2183 to 2189 in 42a3bcf

if(MSVC)
set_source_files_properties(
${CMAKE_CURRENT_BINARY_DIR}/qgsexpression_texts.cpp
${CMAKE_CURRENT_BINARY_DIR}/qgis_core_autogen/mocs_compilation.cpp
proj/qgscoordinatereferencesystem.cpp
PROPERTIES COMPILE_FLAGS "/bigobj"
)

@nicogodet
Copy link
Member

diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt
index f64c6e76cb1..ed2f1ae7765 100644
--- a/src/app/CMakeLists.txt
+++ b/src/app/CMakeLists.txt
@@ -477,16 +477,21 @@ if(PEDANTIC)
 endif()

 if(MSVC)
-# -wd4091 Avoid 'typedef' ignored on left of '' when no variable is declared warning in dbghelp.h
-set_source_files_properties(
-  qgisapp.cpp
-  main.cpp
-  qgscrashhandler.cpp
-  PROPERTIES COMPILE_FLAGS -wd4091)
+  # -wd4091 Avoid 'typedef' ignored on left of '' when no variable is declared warning in dbghelp.h
+  set_source_files_properties(
+    qgisapp.cpp
+    main.cpp
+    qgscrashhandler.cpp
+    PROPERTIES COMPILE_FLAGS -wd4091
+  )
+  set_source_files_properties(
+    qgisapp.cpp
+    PROPERTIES COMPILE_FLAGS "/bigobj"
+  )
 endif()

 if(HAVE_OPENCL)
-    include_directories(SYSTEM ${OpenCL_INCLUDE_DIRS})
+   include_directories(SYSTEM ${OpenCL_INCLUDE_DIRS})
 endif()

 if(ENABLE_MODELTEST)
diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
index 808540650d2..0c1ad171c11 100644
--- a/src/core/CMakeLists.txt
+++ b/src/core/CMakeLists.txt
@@ -2183,7 +2183,9 @@ endif()
 if(MSVC)
   set_source_files_properties(
       ${CMAKE_CURRENT_BINARY_DIR}/qgsexpression_texts.cpp
+      expression/qgsexpressionfunction.cpp
+      textrenderer/qgsfontmanager.cpp
       proj/qgscoordinatereferencesystem.cpp
       PROPERTIES COMPILE_FLAGS "/bigobj"
   )

It builds with theses diffs
I can push them (with or without little formatting)

@m-kuhn
Copy link
Member Author

m-kuhn commented Oct 31, 2024

Please do (with or without formatting)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QGIS - Visual Studio build - Projects missing build options (/bigobj)
3 participants