KRunner: port away from deprecated KF5 API
ClosedPublic

Authored by kossebau on Oct 27 2019, 8:57 AM.

Details

Summary

Plasma::Package has been deprecated since 5.28, but it's used
in the public API for AbstractRunner.

Removed the underlying member variable which is never set
(since apol's commit 8d77a33b6eae26).

Once krunner is ported to the new deprecation macros, I guess
the package() method will need something like

#if KRUNNER_ENABLE_DEPRECATED_SINCE(5, 28)

Technically the method wasn't marked as deprecated between 5.28 and 5.64
but the class it returned, was, so that's pretty much the same.

Test Plan

Builds

Diff Detail

Repository
R308 KRunner
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfaure created this revision.Oct 27 2019, 8:57 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 27 2019, 8:57 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Oct 27 2019, 8:57 AM
kossebau requested changes to this revision.EditedOct 27 2019, 6:05 PM

One of the Interesting challenges with all the cross-library deprecation visibility control setup.

Let me try to collect requirements:

  • Plasma::Package is part of ABI of Plasma API, cannot be removed in normal builds, only made optionally invisible in API when building against
  • Plasma::Package is part of ABI of KRunner API, cannot be removed in normal builds, only made optionally invisible in API when building against
  • if Plasma::Package is made invisible with Plasma API, it also needs to be invisible with KRunner API

Current patch as proposed has an issue for build time of KRunner itself, because KF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x053f00 results in PLASMA_ENABLE_DEPRECATED_SINCE(5, 28) evaluating to FALSE during the build of KRunner. Which means,the symbol for AbstractRunner::package() will be missing and thus ABI being broken -> not allowed.

At the same time #if PLASMA_ENABLE_DEPRECATED_SINCE(5, 28) with the declaration in the header is needed indeed, as Plasma::Package is only visible to the compiler with this condition, and given consumers of KRunner API have control over that we need to handle it.

So, KF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x053f00 is not possibly in total with KRunner, as its required implementation code has a hard dependency on Plasma API deprecated before that.
Two options:
a) set the module specific PLASMA_DISABLE_DEPRECATED_BEFORE_AND_AT=0x050500 to override the default from the group KF one
b) remove visibility wrappers around originalPlasma::Package API

I personally favour a) here.

Edit: When designing ECMGenerateExportHeader, I also considered adding support for sublib granularity (next to the group and lib as there is now) when it comes to visibility control, so one could have the option to e.g. have overrules for features. And Plasma::Package could then have their own macro set, and here we could limit the exception by some e.g. PLASMA_PACKAGE_DISABLE_DEPRECATED_BEFORE_AND_AT=0x050500 and have the rest of deprecated Plasma API still invisible by the 0x053f00 limit. Something which still could be added on top of what we have now, if there is demand. There is no open end for complexity ;)

Given we have officially deprecated API in KRunner, it should indeed also get ported to ecm_generate_export_header. I would have now done this myself, but actually would like to see other people are easily able to use that macro API, so it's not just something compatible to my state of brain ;)

This revision now requires changes to proceed.Oct 27 2019, 6:05 PM

Thanks for the detailed analysis. I'm ok with option A. Let's not complicate this by adding submodule granularity.

I have submitted the removal of the "package" member variable separately, that one is obvious and almost unrelated.

dfaure updated this revision to Diff 69242.Nov 3 2019, 11:08 PM

rebase, add PLASMA_DISABLE_DEPRECATED_BEFORE_AND_AT=0x050500

kossebau accepted this revision.Nov 3 2019, 11:49 PM

Untested, but looks okay, besides the unneeded #f in the sources.

src/abstractrunner.cpp
327 ↗(On Diff #69242)

This one should be removed.

We have defined

add_definitions(-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x053f00)
add_definitions(-DPLASMA_DISABLE_DEPRECATED_BEFORE_AND_AT=0x050500) # needed because we use Plasma::Package in the API

with this in the build of KRunner itselt PLASMA_ENABLE_DEPRECATED_SINCE(5, 28) is always true, and people building KRunner do not have the option to change this.

This condition is only interesting in the header currently, as this is what then is used by 3rd-party building against KRunner and being allowed to set different visibility limits.

This revision is now accepted and ready to land.Nov 3 2019, 11:49 PM
kossebau commandeered this revision.Nov 5 2019, 10:23 AM
kossebau edited reviewers, added: dfaure; removed: kossebau.

@dfaure I allow myself to take over here given your are off the next days and I would like to get this off the table :)

This revision now requires review to proceed.Nov 5 2019, 10:23 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 5 2019, 10:36 AM
This revision was automatically updated to reflect the committed changes.