search

lxqt / lxqt-qtplugin

LXQt Qt platform integration plugin
https://lxqt.github.io
GNU Lesser General Public License v2.1
24 stars 14 forks source link

Improves libmf-qt versioned .so handling #48

Closed luis-pereira closed 5 years ago

luis-pereira commented 5 years ago

We don't need to manually bump the libfm-qt ABI versions. We detect it at build time.

An enhanced version b1abe51484ad253a768f57594570bc3e3f16e362. Ref: lxqt/lxqt/issues/391

tsujan commented 5 years ago

Very nice! LGTM.

EDIT 1: Also, tested it after renaming /usr/lib/libfm-qt.so and encountered no problem.

EDIT 2: This means the build order is important now (for upgrading).

luis-pereira commented 5 years ago

@tsujan Yes, build order in crucial when upgrading. Maybe @agaida can write a note to packagers.

luis-pereira commented 5 years ago

Updated: Fix typo. 

-        message(wARNING " libfm-qt found, but no configuration found. Check your libfm-qt installation. Unversioned libfm-qt will be used.")
+        message(WARNING "libfm-qt found, but no configuration found. Check your libfm-qt installation. Unversioned libfm-qt will be used.")
tsujan commented 5 years ago

@luis-pereira The're no need to that: https://github.com/lxqt/lxqt/wiki/Building-from-source#generalbuild-order. It's just that , if people didn't do it in the correct order, they should do from now on.

luis-pereira commented 5 years ago

@tsujan Good.

agaida commented 5 years ago

@luis-pereira - the question goes a bit deeper: a) is it possible to have libfm-qt found, but no versioned file - i guess no b) is it possible to have libfm-qt found and don't fine configurations? How can it be? c) given that libfm-qt.so should be a link to the real file - what does it change - the soversion is a built in

So i would change only a bit of the cmake thing and make libfm-qt optional, because it is not a real build dependency. And i don't know if i would pick up any libfm-qt at run-time if the ḿaintainers don't want it - just to prevent any questions.

tsujan commented 5 years ago

So i would change only a bit of the cmake thing and make libfm-qt optional

It is optional.

luis-pereira commented 5 years ago

@luis-pereira - the question goes a bit deeper: b) is it possible to have libfm-qt found and don't fine configurations? How can it be?

Yes. it's possible.

So i would change only a bit of the cmake thing and make libfm-qt optional, because it is not a real build dependency. And i don't know if i would pick up any libfm-qt at run-time if the ḿaintainers don't want it - just to prevent any questions.

libfm-qt is already optional. if it's not found then everything behaves exactly as before.

tsujan commented 5 years ago

The differences between this and https://github.com/lxqt/lxqt-qtplugin/commit/b1abe51484ad253a768f57594570bc3e3f16e362:

(1) With this patch, there's no need to change anything in lxqt-qtplugin when libfm-qt's version is bumped but lxqt-qtplugin should be recompiled after libfm-qt.

(2) With the previous patch, we should edit lxqt-qtplugin's cmake file and recompile it when libfm-qt's version is bumped, although the order of compilation isn't important.

So, IMO, we gain something with this PR.

agaida commented 5 years ago

I know - i suggested this change and mentioned that nothing has to be changed in in lxqt-qtplugin :D

What i don't get is: How to create a situation where libfm-qt is found but no config. How is that possible?

tsujan commented 5 years ago

How to create a situation where libfm-qt is found but no config. How is that possible?

It may not be possible but the code should be comprehensive — as it is here. However, I don't have @luis-pereira's knowledge about these matters.

agaida commented 5 years ago

I'm mostly fine with the PR but i would like the libfm-qt part a bit more strict - dunno if it make much sense to do so, but it will make the behaviour more predictable - so less possible bug reports. Will work on it today.

luis-pereira commented 5 years ago

What i don't get is: How to create a situation where libfm-qt is found but no config. How is that possible?

If there is no fm-qt-targets-*.cmake files and the package isn't REQUIRED and we not link with libfm-qt CMake will not complain about it. My check for the configuration is defensive programming to avoid that a broken installation will go undetected.

What do you mean by more predictable ?

agaida commented 5 years ago

Predictable like in the state before the changes - we picked up libfm-qt.so any time it was installed - and that lead to situations like "on my workstation i have the LXQt file dialog, on my notebooks not" and similar. Imho - if a distribution/maintainer decide not to pick up our dialog for what reasons ever, it shouldn't picked up -- even if it is installed.

Edit: Thats why i insisted on "optional" - please read it as in "libfm-qt should be only picked up in cmake if the maintainer say so"

tsujan commented 5 years ago

if a distribution/maintainer decide not to pick up our dialog for what reasons ever

That could happen only if he doesn't compile lxqt-qtplugin after upgrading to a new release of libfm-qt and that will be his mistake. If there isn't a new release and libfm-qt is upgraded to its latest git version, there will be no need to do anything with lxqt-qtplugin.

IMO, this PR is the most that can be done for distros with dev/devel packages.

luis-pereira commented 5 years ago

@agaida Humm We might have different viewpoints at a deeper level. Our dialog is part of LXQt platform/desktop plugin. It should not be optional. The platform plugin purpose is to provide a coherent user experience. Making some part of the plugin optional breaks the platform plugin purpose.

In a perfect world the dialog code would be located in lxqt-qtplugin not someplace where. In our real world it's in lifm-qt. The actual code location is the reason, IMO, we are having this discussion.

agaida commented 5 years ago

@luis-pereira - we don't have such different viewpoints i guess. Maybe i see it more from the perspective as a packager - with my debian hat on. And from that viewpoint i wouldn't spend so much time on possible wrong build orders and so on - code maybe expresses it better than words - should be easy.

And with my packager hat on i prefer things to fail predictable over cmake magic - if something not needed isn't there - the build has to fail, but not silently drop some options - easy as can be. Will fork you branch, maybe it will be far more clear then :)

tsujan commented 5 years ago

In a perfect world the dialog code would be located in lxqt-qtplugin

It wasn't a perfect world to PCMan ;) The dialog code was there before (as far as I remember) but PCMan moved it to libfm-qt. IMO, that was a good change and he had a good reason to do so.

I think, practically, we're discussing about nothing ;)

luis-pereira commented 5 years ago

I'm Ok with that. I don't understand the CMake magic part. I don't see any magic.

@tsujan I know and I agree with it. But still, the code logically belongs to lxqt-qtplugin. The fact that it's located at libfm-qt is a implementation detail.

One thing that I don't understand is why can't we simply link to libfm-qt.

tsujan commented 5 years ago

This is the commit: https://github.com/lxqt/lxqt-qtplugin/commit/73d20ab01995ca869f8d967535d4969409b22ec3

"This speeds up the loading of the QPA plugin and also avoid loading libfm-qt in Qt programs having QT_NO_GLIB=1"

tsujan commented 5 years ago

I have yet another reason why that commit was good: libfm-qt stuff should be in libfm-qt ;)

luis-pereira commented 5 years ago

I have yet another reason why that commit was good: libfm-qt stuff should be in libfm-qt ;)

No doubt about that.

luis-pereira commented 5 years ago

This is the commit: 73d20ab

"This speeds up the loading of the QPA plugin and also avoid loading libfm-qt in Qt programs having QT_NO_GLIB=1"

From a theoretical perspective that is the solution. But.... performance matter a lot.

agaida commented 5 years ago

https://github.com/lxqt/lxqt-qtplugin/pull/49 - alternative approach. We want people to use fm-qt. No magic, no fallbacks - if not all dependencies are found just fail to build.

PCMan commented 5 years ago

@luis-pereira @agaida @tsujan We can remove these stuff from CMake altogether. FYI: https://github.com/lxqt/lxqt-qtplugin/blob/master/src/lxqtplatformtheme.cpp#L249

Currently libfm-qt is detected and dynamically loaded at runtime.

  1. If glib is disabled, it's not loaded.
  2. If the lib is not installed, it's not loaded
  3. It's initialized by calling a C function rather than a C++ one, which often has no ABI issues if function signature remains the same.

It should be safe to just remove libfm-qt stuff from CMakeLists.txt and it should still build. Also, if libfm-qt does not exist or is not loaded, we fallback to Qt file dialog as usual. Nothing will break. If not, it's a bug and I can help fix it.

tsujan commented 5 years ago

@PCMan We know that. The whole discussion arose because libfm.so was inside dev packages of distros that have dev packages. Dev packages should not be required to be installed for something to work.