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 support cmake unity build #5109

Merged
merged 5 commits into from
Nov 14, 2022

Conversation

tnixeu
Copy link
Contributor

@tnixeu tnixeu commented Oct 29, 2022

This PR prepares building with CMAKE_UNITY_BUILD enabled. This makes builds often a bit faster and the executables often a bit smaller.

I tried to keep the changes as small as possible and without renaming variables. This is why I exclude the file causing troubles from the unity build or added macros in order to avoid defining a variable multiple times.

I timed the builds with time ninja

Unity build: nextcloud 5.6M

real    0m42,137s
user    3m36,684s
sys     0m17,226s

Normal build: nextcloud 5.8M

real    0m46,048s
user    9m13,199s
sys     0m45,951s

@tnixeu tnixeu force-pushed the prepare_cmake_unity_build branch from 133f0e0 to 40c607a Compare October 29, 2022 09:15
@tnixeu
Copy link
Contributor Author

tnixeu commented Oct 29, 2022

I logged in with this version, synced a view files. It seems fine by me.

None of my changes should currently have an impact at runtime, because the Appimage is probably not yet built with CMAKE_UNITY_BUILD turned on.

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

thanks a lot that is nice
would you like to also try activating it for the AppImage ?
I can help and guide you if you want

Comment on lines 49 to 53
#ifndef VERSION_C
#define VERSION_C
static const char versionC[] = "version";

#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

please can you replace them by the following code

namespace {
constexpr auto versionC= "version";
}

can you also apply the same solution to the few other occurrences

Copy link
Collaborator

Choose a reason for hiding this comment

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

please ignore that comment
it is indeed not compiling
so please use

namespace {
#ifndef VERSION_C
#define VERSION_C
constexpr auto versionC= "version";
#endif
}

@mgallien
Copy link
Collaborator

mgallien commented Nov 9, 2022

I am also getting a build failure if the dolphin native plugin is getting build due to a conflict between signals macro from Qt headers and some code in this header /usr/include/glib-2.0/gio/gdbusintrospection.h
maybe you could also add this file src/3rdparty/kmessagewidget/kmessagewidget.cpp to the list of files not included in unity builds ?

To solve the build failures I added the following lines src/gui/CMakeLists.txt

set_property(SOURCE ../3rdparty/kmessagewidget/kmessagewidget.cpp PROPERTY SKIP_UNITY_BUILD_INCLUSION ON)
set_property(SOURCE ../3rdparty/kirigami/wheelhandler.cpp PROPERTY SKIP_UNITY_BUILD_INCLUSION ON)

@mgallien
Copy link
Collaborator

mgallien commented Nov 9, 2022

I logged in with this version, synced a view files. It seems fine by me.

None of my changes should currently have an impact at runtime, because the Appimage is probably not yet built with CMAKE_UNITY_BUILD turned on.

You would need to add the extra configure option in this file https://github.com/nextcloud/desktop/blob/master/admin/linux/build-appimage.sh

@tnixeu tnixeu force-pushed the prepare_cmake_unity_build branch 3 times, most recently from 44b2139 to eb5ed39 Compare November 9, 2022 22:28
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #5109 (5eadb26) into master (0fe476e) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5109      +/-   ##
==========================================
+ Coverage   57.56%   57.58%   +0.01%     
==========================================
  Files         138      138              
  Lines       17399    17399              
==========================================
+ Hits        10016    10019       +3     
+ Misses       7383     7380       -3     
Impacted Files Coverage Δ
src/libsync/discovery.cpp 88.33% <0.00%> (+0.30%) ⬆️

@mgallien mgallien force-pushed the prepare_cmake_unity_build branch from eb5ed39 to f496c97 Compare November 10, 2022 14:16
Signed-off-by: tnixeu <4436784+tnixeu@users.noreply.github.com>
Signed-off-by: tnixeu <4436784+tnixeu@users.noreply.github.com>
Signed-off-by: tnixeu <4436784+tnixeu@users.noreply.github.com>
Signed-off-by: tnixeu <4436784+tnixeu@users.noreply.github.com>
Signed-off-by: tnixeu <4436784+tnixeu@users.noreply.github.com>
@mgallien mgallien force-pushed the prepare_cmake_unity_build branch from f496c97 to 5eadb26 Compare November 14, 2022 07:41
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

thanks

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5109-5eadb269a614567ce5f7b5ac0e2b97387ec9c25b-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@mgallien mgallien merged commit 81f6844 into nextcloud:master Nov 14, 2022
@mgallien mgallien added this to the 3.7.0 milestone Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants