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 option to use system-wide qtsingleapplication #292

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

Conversation

eclipseo
Copy link

Only tested on Linux.

The goal is to use system-wide qtsingleapplication.

@Krakonos
Copy link
Member

Thanks for the PR. Unfortunately, I merged the PR #294 this now has a conflict (easy to fix).
Also, what distro actually has qtsingleapplication system-wide? Anyway, I think the correct way to do this is to build the internally bundled qtsingleapplication as a static library and then link it or the system one using the same mechanism.

@gbschenkel
Copy link

QtSingleApplication is deprecated for more then 10 years, is from Qt4, it doesn't "exist" on Qt5 and Qt6.
This guy has ported the concept to Qt5 and Qt6.
https://github.com/itay-grudev/SingleApplication

@eclipseo
Copy link
Author

eclipseo commented May 26, 2024

set(QTSINGLEAPPLICATION_LIBRARIES ${QtSingleApplication_LIBRARIES})
else()
set(QTSINGLEAPPLICATION_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/3rdparty/qtsingleapplication-2.6_1-opensource/src)
set(QTSINGLEAPPLICATION_LIBRARIES "QtSingleApplication")
Copy link

Choose a reason for hiding this comment

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

This line needs to be removed without replacement. (The bundled QtSingleApplication is added directly to the list of sources, not built as a library target, so there is nothing to link in this case. So QTSINGLEAPPLICATION_LIBRARIES should just be left unset.)

@Krakonos
Copy link
Member

Krakonos commented Sep 8, 2024

@kkofler Thanks for looking into it.

@eclipseo However, I'm in doubts if it makes sense to do this work. Is there any benefit to using the system-wide lib? It's reasonably small, it's already included in the sources and necessary for other platforms. Adding a bunch of conditional compilations introduces possible complexity and possible bugs.

@kkofler
Copy link

kkofler commented Sep 8, 2024

For some context: I am the primary point of contact for the Fedora package of Merkaartor, for which @eclipseo has written this patch. I have built Merkaartor 0.20.0 updates for Fedora yesterday: https://bodhi.fedoraproject.org/updates/?packages=merkaartor

In Fedora, we try to build against system libraries whenever possible, see:

That is why the Fedora package carries this patch.

How I have noticed the fault that I have commented on (the line that needs to be removed) is that I have made the system QtSingleApplication usage optional in the specfile in order to be able to support a Qt 6 build of Merkaartor also on older Fedora releases where the system QtSingleApplication is only built for Qt 4 and 5, in particular, the "old stable" release Fedora 39 that has about 2 months of update support left. (The current Fedora 40 has a system qtsingleapplication-qt6 available.) I could have just applied the patch conditionally, but since the patch is supposed to allow building both with and without system QtSingleApplication, I did just that, which is how I noticed that the build fails without system QtSingleApplication because of a bogus -lQtSingleApplication that is not available.

Also note that QtSingleApplication itself bundled QtLockedFile, which is also packaged separately in Fedora (and the system QtSingleApplication is built against the system QtLockedFile), so we are technically speaking about two bundled libraries, not one.

@Krakonos
Copy link
Member

Krakonos commented Sep 9, 2024

@kkofler Thanks for the explanation. I understand the motivation, but though in case of unmaintained libraries, I don't think it's reasonable to ask users/packagers to keep them in the distribution.

In any case, I'll review in more detail and see what needs to be done. I don't fancy adding more conditional compilation here, so I'd rather just create a subproject of the singleapplication and link one or the other.

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.

4 participants