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

Port to CMake #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Port to CMake #1

wants to merge 1 commit into from

Conversation

PureTryOut
Copy link

This project uses KDE's KCalendarCore. However, that package can only be
used with build systems that support CMake dependencies, which QMake is
not one of. KCalendarCore probably shipped a pkgconfig file in the past,
but this has not been the case for a quite a while as far as I can see.

Since using QMake thus prevents using a current KCalendarCore, switch
the build system to CMake instead.

This is a recreation of https://git.sailfishos.org/mer-core/nemo-qml-plugin-calendar/merge_requests/82 as sadly you guys moved away from the FOSS Gitlab.

@neochapay
Copy link

KDE/kcalendarcore@e497e0a fixed in kde ^_^

qmake is slowly being deprecated by Qt (they don't use it themselves
anymore from Qt6 and onwards) and they recommend using CMake instead.
@PureTryOut
Copy link
Author

@neochapay nice but qmake is still being deprecated by Qt in favor of CMake (they don't even use it themselves anymore from Qt6 and onwards), so switching to CMake should still be done eventually.

Copy link
Contributor

@dcaliste dcaliste left a comment

Choose a reason for hiding this comment

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

The rpm/nemo-qml-plugin-calendar-qt5.spec file should be updated also to use CMake instead of qmake., something like:

--- a/rpm/nemo-qml-plugin-calendar-qt5.spec
+++ b/rpm/nemo-qml-plugin-calendar-qt5.spec
@@ -6,6 +6,8 @@ Release:    1
 License:    BSD
 URL:        https://github.com/sailfishos/nemo-qml-plugin-calendar
 Source0:    %{name}-%{version}.tar.bz2
+BuildRequires:  cmake
+BuildRequires:  extra-cmake-modules >= 5.75.0
 BuildRequires:  pkgconfig(Qt5Core)
 BuildRequires:  pkgconfig(Qt5Gui)
 BuildRequires:  pkgconfig(Qt5Qml)
@@ -46,14 +48,12 @@ BuildRequires:  pkgconfig(KF5CalendarCore)
 %setup -q -n %{name}-%{version}
 
 %build
-
-%qmake5 
-
+%cmake -DBUILD_TESTING=ON
 make %{?_smp_mflags}
 
 %install
 rm -rf %{buildroot}
-%qmake_install
+%make_install
 
 %files
 %defattr(-,root,root,-)

@@ -1,16 +0,0 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be deleted. It is used by the internal test tool chain @ Jolla.

include(GNUInstallDirs)
include(CTest)

set(INSTALL_QML_IMPORT_DIR "${CMAKE_INSTALL_FULL_LIBDIR}/qt/qml"
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, SailfishOS is using $LIBDIR/qt5/qml as the QML import dir. If the qt5 part is an issue, it should be set as a variable.

KF5::CalendarCore
nemocalendar)

add_test(tst_calendarevent tst_calendarevent)
Copy link
Contributor

Choose a reason for hiding this comment

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

Like in sailfishos/mkcal, tests should be installed, for instance with:

if(INSTALL_TESTS)
	install(TARGETS tst_calendarevent
		DESTINATION /opt/tests/nemo-qml-plugins-qt5/calendar)
endif()

see old common.pri.

Copy link
Author

Choose a reason for hiding this comment

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

I never understood this, why exactly? I have no problem adding it under a conditional block (so at least I have the option to not install them) but I'd like to understand why you install them in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm not working at Jolla. I'm part of the community also. Maybe @pvuorela could answer.

I guess that they want RPMs to easily deploy tests on a test infrastructure. Now, this does not necessary imply to put the installation procedure in the build system itself of the project. It could be done at the spec level for specific SailfishOS builds. I agree. But well, I guess it's historical and maybe easier also than writing install -m 644 -p... multiple times in the spec file.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do automatic nightly run for the unit tests and the setup requires that there's the tests.xml and the test binaries installed.

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.

None yet

4 participants