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

master/edge: Linux builds would break without intrusive patching #5957

Closed
guruz opened this issue Aug 14, 2017 · 21 comments
Closed

master/edge: Linux builds would break without intrusive patching #5957

guruz opened this issue Aug 14, 2017 · 21 comments
Assignees
Labels
p2-high Escalation, on top of current planning, release blocker
Milestone

Comments

@guruz
Copy link
Contributor

guruz commented Aug 14, 2017

https://build.opensuse.org/public/build/isv:ownCloud:testpilot:edge/xUbuntu_16.10/i586/testpilotcloud-client-overlays/_log

I don't see any obvious changes in the client sources so it must have been from the OBS templates? Or the underlying OBS Qt or?

[  112s] CMake Error at cmake/modules/QtVersionAbstraction.cmake:8 (find_package):
[  112s]   By not providing "FindQt5Core.cmake" in CMAKE_MODULE_PATH this project has
[  112s]   asked CMake to find a package configuration file provided by "Qt5Core", but
[  112s]   CMake did not find one.
[  112s] 
[  112s]   Could not find a package configuration file provided by "Qt5Core" with any
[  112s]   of the following names:
[  112s] 
[  112s]     Qt5CoreConfig.cmake
[  112s]     qt5core-config.cmake
[  112s] 
[  112s]   Add the installation prefix of "Qt5Core" to CMAKE_PREFIX_PATH or set
[  112s]   "Qt5Core_DIR" to a directory containing one of the above files.  If
[  112s]   "Qt5Core" provides a separate development package or SDK, be sure it has
[  112s]   been installed.
[  112s] Call Stack (most recent call first):

Detected first as QtWebKit problem in #5945 (comment)

@guruz guruz added the p2-high Escalation, on top of current planning, release blocker label Aug 14, 2017
@guruz guruz added this to the 2.4.0 milestone Aug 14, 2017
@guruz
Copy link
Contributor Author

guruz commented Aug 14, 2017

@jnweiger Since when does this fail btw? Is it indirectly related to your NSIS changes? (even though it's Linux not Windows)

@jnweiger
Copy link
Contributor

I don't see any changes on my side. The relevant files debian.control, *.dsc, *.spec are identical in the 2.3.3 and 2.4.0 templates.

Hard to tell since when it is broken. OBS does not archive any build logs. Manual bisecting needed to find out.

This is where the log between nightly and edge differs:
nightly:

[   93s] -- Build of crashreporter disabled.
[   93s] 5a9286a68ce09d80f603e552a528d57f3b853bef
[   93s] -- GIT_SHA1 5a9286a68ce09d80f603e552a528d57f3b853bef
[   93s] CMake Warning at shell_integration/CMakeLists.txt:16 (find_package):
[   93s]   By not providing "FindKF5.cmake" in CMAKE_MODULE_PATH this project has

edge:

[  130s] -- Build of crashreporter disabled.
[  130s] 20a979173f648a134d97497165a71130861c9066
[  130s] -- GIT_SHA1 20a979173f648a134d97497165a71130861c9066
[  130s] CMake Error at cmake/modules/QtVersionAbstraction.cmake:8 (find_package):
[  130s]   By not providing "FindQt5Core.cmake" in CMAKE_MODULE_PATH this project has

The former is non-fatal, the later is fatal.

@jnweiger
Copy link
Contributor

jnweiger commented Aug 14, 2017

Seems some remainders of QT4 compatibility got removed in master:

--- isv:ownCloud:testpilot:nightly/testpilotcloud-client-overlays/testpilotcloudclient-2.3.3-nightly20170814/cmake/
modules/QtVersionAbstraction.cmake   2017-08-14 07:02:46.000000000 +0200
+++ isv:ownCloud:testpilot:edge/testpilotcloud-client-overlays/testpilotcloudclient-2.4.0-nightly20170814/cmake/mod
ules/QtVersionAbstraction.cmake      2017-08-14 06:43:46.000000000 +0200
@@ -5,67 +5,47 @@
 include (MacroOptionalFindPackage)
 include (MacroLogFeature)
 
-option(BUILD_WITH_QT4 "Build with Qt4 no matter if Qt5 was found" OFF)
+find_package(Qt5Core REQUIRED)
+find_package(Qt5Network REQUIRED)
+find_package(Qt5Xml REQUIRED)
+find_package(Qt5Concurrent REQUIRED)
+if(UNIT_TESTING)
+    find_package(Qt5Test REQUIRED)
+endif()
 
-if( BUILD_WITH_QT4 )
-    message(STATUS "Search for Qt5 was disabled by option BUILD_WITH_QT4")
-else( BUILD_WITH_QT4 )
-    find_package(Qt5Core QUIET)
-endif( BUILD_WITH_QT4 )
...

AFAIK we have to build nautilus and nemo plugins and friends with system QT.

@guruz
Copy link
Contributor Author

guruz commented Aug 14, 2017

@jnweiger But that BUILD_WITH_QT4 was already in April in 9aeb587 by @ogoffart .. are you sure those master nightlies did not work since April?

AFAIK we have to build nautilus and nemo plugins and friends with system QT.

I don't know how this works in the OBS magic, should be @dschmidt or maybe @ogoffart that could know

@ogoffart
Copy link
Contributor

nautilous and nemo plugin don't depend on Qt.
The dolphin plugin need to be build with the system Qt and also need KF5

@jnweiger
Copy link
Contributor

jnweiger commented Aug 15, 2017

error in Leap 42.2 (system libQt5Core5-5.6.1-2.1 installed at build time) :

[   11s] CMake Error at cmake/modules/QtVersionAbstraction.cmake:11 (find_package):
[   11s]   By not providing "FindQt5Concurrent.cmake" in CMAKE_MODULE_PATH this
[   11s]   project has asked CMake to find a package configuration file provided by
[   11s]   "Qt5Concurrent", but CMake did not find one.
[   11s] 
[   11s]   Could not find a package configuration file provided by "Qt5Concurrent"
[   11s]   with any of the following names:
[   11s] 
[   11s]     Qt5ConcurrentConfig.cmake
[   11s]     qt5concurrent-config.cmake

@jnweiger
Copy link
Contributor

error in Ubuntu 16.04 (no system qt installed at build time):

[   85s] CMake Error at cmake/modules/QtVersionAbstraction.cmake:8 (find_package):
[   85s]   By not providing "FindQt5Core.cmake" in CMAKE_MODULE_PATH this project has
[   85s]   asked CMake to find a package configuration file provided by "Qt5Core", but
[   85s]   CMake did not find one.
[   85s] 
[   85s]   Could not find a package configuration file provided by "Qt5Core" with any
[   85s]   of the following names:
[   85s] 
[   85s]     Qt5CoreConfig.cmake
[   85s]     qt5core-config.cmake

@jnweiger
Copy link
Contributor

error in Ubuntu 16.04 (system libqt5core5a-5.5.1 enforced at build time):

[   64s]   By not providing "FindQt5WebKitWidgets.cmake" in CMAKE_MODULE_PATH this
[   64s]   project has asked CMake to find a package configuration file provided by
[   64s]   "Qt5WebKitWidgets", but CMake did not find one.
[   64s] 
[   64s]   Could not find a package configuration file provided by "Qt5WebKitWidgets"
[   64s]   with any of the following names:
[   64s] 
[   64s]     Qt5WebKitWidgetsConfig.cmake
[   64s]     qt5webkitwidgets-config.cmake

@guruz
Copy link
Contributor Author

guruz commented Aug 15, 2017

@jnweiger I thought we should always use our own oc-qt562 package?

@jnweiger
Copy link
Contributor

jnweiger commented Aug 15, 2017

@guruz yes, 9aeb587 killed the overlays.

cmake .. -DBUILD_CLIENT=OFF -DBUILD_SHELL_INTEGRATION=ON ...

cannot survive with the current QtVersionAbstraction.cmake
We need this feature, so that overlays that don't depend on QT can be built. (Or those that depend on a System QT version like dolphin.)
I'd suggest to put all the include(QtVersionAbstraction) and result analysis in
if(BUILD_CLIENT)
...
endif()

@ogoffart
Copy link
Contributor

I'd suggest to put all the include(QtVersionAbstraction) and result analysis in
if(BUILD_CLIENT)

Yes, that sounds sensible

@jnweiger
Copy link
Contributor

jnweiger commented Sep 4, 2017

Workaround with a packaging.diff

Nightly master happily builds:

@guruz & team: please review the packaging.diff here.

@guruz
Copy link
Contributor Author

guruz commented Sep 11, 2017

Even though it might work, the https://gitea.int.owncloud.com/ownbrander/scripting/src/master/client/linux/templates/client/2.4.0/@LINUX_PACKAGE_SHORTNAME@-client/packaging.diff looks quite ugly :(

So there is two build modes now?

Is this really what you intended like this @ogoffart ?

Why is executableVersionOutput moved?

Can we "upstream" this...

@guruz
Copy link
Contributor Author

guruz commented Sep 11, 2017

@guruz guruz mentioned this issue Sep 11, 2017
9 tasks
@jnweiger
Copy link
Contributor

One question I can answer:

executableVersionOutput() generates the blurb text for the gui client. We also want to have the same blurb text printed with owncloudcmd --version

The method is originally found in src/gui/application.cpp which is not part of owncloudcmd. @dschmidt first moved it to src/libsync/utility.cpp which is easily accessible in both gui and cmd client.
I had trouble finding a good place after libsync/utility.cpp disappeared. It is currently in src/common/utility.cpp, but most of the code does not work from there. I failed to include theme.h -- this needs a helpful wizard to help expand my c++ foo. Sorry for the ugly hacks. All the FIXME there are bad code.

A much saner version was https://gitea.int.owncloud.com/ownbrander/scripting/src/d9a95f80fb9c3a217042638e6fd06e535353074f/client/linux/templates/client/2.4.0/@LINUX_PACKAGE_SHORTNAME@-client-overlays-dolphin/packaging.diff

@jnweiger
Copy link
Contributor

Maybe this answers the 'Mode' question:

We use the same CMakeList.txt files for multiple build targets

  • the main package builds the gui-client, but none of the shell integration modules
cmake .. \
%if 0%{have_doc}
  -DWITH_DOC=TRUE \
%else
  -DWITH_DOC=FALSE \
%endif
  -DCMAKE_FIND_ROOT_PATH="@OC_QT_ROOT@" \
%if "%{prerelease}" != ""
  -DMIRALL_VERSION_SUFFIX="%{prerelease}" \
%endif
  -DMIRALL_VERSION_BUILD="@BUILD_NUMBER@" \
  -DKDE_INSTALL_USE_QT_SYS_PATHS=1 \
  -DCMAKE_C_FLAGS:STRING="%{optflags}" \
  -DCMAKE_CXX_FLAGS:STRING="%{optflags}" \
  -DCMAKE_SKIP_RPATH=OFF \
  -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DCMAKE_INSTALL_PREFIX=@CLIENT_ROOT@ \
  -DCMAKE_INSTALL_DATAROOTDIR:PATH=%{_datadir} \
  -DDATA_INSTALL_DIR:PATH=%{_datadir} \
  -DCMAKE_INSTALL_DOCDIR:PATH=%{_docdir} \
  -DSYSCONF_INSTALL_DIR=%{_sysconfdir} \
%if %{_lib} == lib64
  -DLIB_SUFFIX=64 \
%endif
%if ! %{is_owncloud_client}
  -DOEM_THEME_DIR=$PWD/../@THEME@/@OEM_SUB_DIR@ \
%endif
  -DQTKEYCHAIN_INCLUDE_DIR=@OC_QT_ROOT@/include/qt5keychain \
  -DQTKEYCHAIN_LIBRARY=@OC_QT_ROOT@/%{_lib}/libqt5keychain.so \
  -DGIT_SHA1="HACK FIXME" \
  -DBUILD_SHELL_INTEGRATION=OFF \
  -DPACKAGE="%{name}" \
  -DCMAKE_SKIP_BUILD_RPATH=OFF -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON -DCMAKE_INSTALL_RPATH="@CLIENT_ROOT@/%{_lib};@OC_QT_ROOT@/%{_lib}" -DCMAKE_INSTALL_RPATH_USE_LINK_PATH=ON \
  %cmake_args
  • the overlays package builds the nautilus and friends shell integration modules
cmake .. \
%if "%{prerelease}" != ""
  -DMIRALL_VERSION_SUFFIX="%{prerelease}" \
  -DMIRALL_VERSION_BUILD="@BUILD_NUMBER@" \
%endif
  -DKDE_INSTALL_USE_QT_SYS_PATHS=1 \
  -DCMAKE_C_FLAGS:STRING="%{optflags}" \
  -DCMAKE_CXX_FLAGS:STRING="%{optflags}" \
  -DCMAKE_SKIP_RPATH=OFF \
  -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DCMAKE_INSTALL_PREFIX=%{_prefix} \
%if %{_lib} == lib64
  -DLIB_SUFFIX=64 \
%endif
%if ! %{is_owncloud_client}
  -DOEM_THEME_DIR=$PWD/../@THEME@/@OEM_SUB_DIR@ \
%endif
  -DGIT_SHA1="HACK FIXME" \
  -DBUILD_SHELL_INTEGRATION=ON \
  -DBUILD_CLIENT=OFF
  • the overlays-dolphin builds the dolphin shell integration using
dh_auto_configure -- \
		"$${git_defs[@]}" \
		-DCMAKE_INSTALL_PREFIX=/usr \
		-DCMAKE_INSTALL_DOCDIR=/usr/share/doc \
		-DCMAKE_INSTALL_DATAROOTDIR:PATH=/usr/share \
		-DDATA_INSTALL_DIR:PATH=/usr/share \
		-DSYSCONF_INSTALL_DIR=/etc \
		-DWITH_DOC=FALSE \
		-DCMAKE_SKIP_RPATH=FALSE \
		-DCMAKE_INSTALL_RPATH_USE_LINK_PATH=TRUE \
		-DOEM_THEME_DIR=$(CURDIR)/@THEME@/@OEM_SUB_DIR@ \
		-DBUILD_SHELL_INTEGRATION=ON \
		-DBUILD_SHELL_INTEGRATION_ICONS=OFF \
		-DBUILD_SHELL_INTEGRATION_NAUTILUS=OFF \
		-DBUILD_SHELL_INTEGRATION_DOLPHIN=ON \
		-DBUILD_CLIENT=OFF
	```

We want this in separate builds, as we need different contexts:
* the main gui client builds with  our qt-5.6.2 installed,
* the dolphin shell extenstion bulds with default system qt installed.

@jnweiger
Copy link
Contributor

@dschmidt why is our GIT_SHA specfied as "HACK FIXME" ?

@jnweiger jnweiger changed the title master/edge: Linux builds broken because of Qt master/edge: Linux builds would break without heavy patching Sep 13, 2017
@jnweiger jnweiger changed the title master/edge: Linux builds would break without heavy patching master/edge: Linux builds would break without intrusive patching Sep 13, 2017
@ckamm ckamm self-assigned this Sep 22, 2017
@ckamm
Copy link
Contributor

ckamm commented Sep 22, 2017

I'm looking at upstreaming the patches.

@ckamm
Copy link
Contributor

ckamm commented Sep 22, 2017

@jnweiger Looking at packaging.diff I'm wondering whether the admin subdirectory was removed from cmake intentionally

ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 22, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 25, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 25, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 25, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 25, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 25, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@guruz
Copy link
Contributor Author

guruz commented Sep 25, 2017

I answered the admin/ thing on the PR, @jnweiger pls comment.

ckamm added a commit that referenced this issue Sep 27, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 27, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 27, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 27, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
ckamm added a commit that referenced this issue Sep 27, 2017
Extracted and adjusted from packaging.diff by @jnweiger and @dschmidt
@ckamm
Copy link
Contributor

ckamm commented Sep 28, 2017

Merged!

@ckamm ckamm closed this as completed Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-high Escalation, on top of current planning, release blocker
Projects
None yet
Development

No branches or pull requests

5 participants