Add ECMGenerateExportHeader, for improved handling of deprecated API
ClosedPublic

Authored by kossebau on Sep 8 2019, 3:24 PM.

Details

Summary

Generates additional macros in the export header which can be used for
fine-grained disabling of warnings & visibility as well as excluding from
the build.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
addgenerateexportheader
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17519
Build 17537: arc lint + arc unit
kossebau created this revision.Sep 8 2019, 3:24 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptSep 8 2019, 3:24 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Sep 8 2019, 3:24 PM
kossebau added a comment.EditedSep 8 2019, 3:39 PM

Implementation prototype for T11490.

Current version works already quite well, developed against applying it to a few tier-1 and tier-2 KDE Frameworks modules.
A few corner casess still need care, but otherwise already pretty much a prototype which is not too far from a production version.

So feel invited to investigate by reading at least the API dox and check the examples given there.

krop added a subscriber: krop.Sep 8 2019, 4:10 PM

Tests needed :)

In D23789#527769, @cgiboudeaux wrote:

Tests needed :)

Sure :) Will be done, once I know investment makes sense. For now interested in feedback whether the principle approach makes sense and is welcome/wanted, and what people think about the API of the macro as well as the generated API/macros for use in the code.

kossebau updated this revision to Diff 65678.Sep 9 2019, 3:27 PM
  • change BEFORE to BEFORE_AND_AT, to match actual Qt behaviour (though do not copy confusing name, but be more precise using AND_AT
  • add NO_DEFINITION_EXPORT_TO_BUILD_INTERFACE option
kossebau retitled this revision from WIP: Add ECMGenerateExportHeaders, for improved handling of deprecated API to RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API.Sep 9 2019, 5:07 PM
dfaure added a subscriber: dfaure.Sep 10 2019, 8:11 PM

Great work. Not really easy to grasp at first sight (because it handles BC for no-compat builds of the lib itself, which we never did before) but this is certainly quite comprehensive.

Since we don't yet have any source compat to worry about for these macros (unlike Qt, which needed _X variants for that), how about simply adding a third argument to FOO_DEPRECATED_VERSION right away, to allow specifying a hint in the compiler warning, provided the compiler supports C++14?

kio's src/core/job_base.h:77 would become

KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()") KJobUiDelegate *ui() const;

If your first version just ignores the argument, at least we'll start writing those hints and we can work on making cmake's GenerateExportHeaders support that, or fork it here to support it.

modules/ECMGenerateExportHeader.cmake
76

typo: pther -> other

86

(minor) small inconsistency, here you use '&' as separator between #if and #endif, while on line 80 you use '/'.

88

I'm having trouble parsing the "only" in this sentence.

deprecated API only... that seems redundant, it's only API that gets deprecated :-)

Maybe you mean "to disable only at build time"?
Or "declaration-only code" ?

and it's always about build time... the choice is between building the library and building the users of the library, isn't it? It's all "build time" of someone...

108

"The tricks [....] does not work" is invalid grammar: either "The trick" (singular) or "do not work" (plural).

kossebau added a comment.EditedSep 11 2019, 1:33 PM

@dfaure Thanks for first in-detail feedback, good to get a feeling this is not totally insane over-engineered stuff to other people'e eyes :)

Great work. Not really easy to grasp at first sight (because it handles BC for no-compat builds of the lib itself, which we never did before) but this is certainly quite comprehensive.

It's basically trying to solve 2 problems at the same time, as they both would be solved by the same macro techniques:
a) allow control over warnings and API visibility to people building against the library (incl. 2 level settings by additional library group level)
b) allow building of the library itself with legacy code stripped

If there are suggestions for better wording of variables/arguments, or improved ordering/threading of the documentation, all eager to hear. Myself still too much into thinking about details to be able to simulate a first-contact experience again, where ideas are taken from docs and term semantics :)

Since we don't yet have any source compat to worry about for these macros (unlike Qt, which needed _X variants for that), how about simply adding a third argument to FOO_DEPRECATED_VERSION right away, to allow specifying a hint in the compiler warning, provided the compiler supports C++14?

kio's src/core/job_base.h:77 would become

KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()") KJobUiDelegate *ui() const;

If your first version just ignores the argument, at least we'll start writing those hints and we can work on making cmake's GenerateExportHeaders support that, or fork it here to support it.

As there might not be always a simple hint text, I would still like to also keep the just-version variant of that macro. So if we overload the macro name to support both 2 (major, minor) and 3 (major, minor, message) arguments, any simple way to do default argument value with C++ preprocessor macros? Never wrote such code myself, all things I just found do complicated dances around __VA_ARGS__ of which I none could resolve to a simple application for our single-parameter default value yet.

So, any C++ macro magicians around to be able to tell how to support both

KIOCORE_DEPRECATED_VERSION(5, 0)
KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()")

while also making sure there is only one argument, and not many? (at least that would be my ambition here to catch this ourselves, instead of just taking __VA_ARGS__ and pass it on, to fail later if >1 args are used.

kossebau updated this revision to Diff 65854.Sep 11 2019, 3:57 PM
kossebau marked 3 inline comments as done.
  • reword documentation based on dfaure's comments
  • bump since-version to 5.64.0, as currently targetted introduction version

Hope is review can be finished at 5.63 tagging time, so this macro would be
added right after that and all the ideally prepared patches for the KF
modules, so there is a full KF release cycle of testing in git master before
things get rolled out to users.

kossebau added inline comments.Sep 11 2019, 3:58 PM
modules/ECMGenerateExportHeader.cmake
88

I tried to reword things a bit, also changed all other places talking about build time to be explicit.

I wouldn't offer both variants, but rather recommend always adding a hint. At worse that hint can be "See method documentation" or an empty string. I think we're just over-complicating things otherwise, for no actual gain, since providing a hint is good practice anyway.

Quick update:
Currently still busy trying to get unit tests done, half-way through that. ETA begin of upcoming week.
Next plan: see how only having the 3-arg-FOO_DEPRECATED_VERSION(major. minor, message) would work by using that in the experimental patches done for some KF repos.

Quick Update (week 39):
took me a bit longer to design and write tests, not trained to write such things in cmake code, all its magic & caches & scopes + own undiscovered typos make this not (yet?) fun, and I am not certain I have done things properly, so will be looking forward to feedback.
Currently cleaning up and documenting the code, added tests should land tonight with an update here (and they already found an error in the so far prototype code :) ).

(FTR: Meanwhile saw that Qt actually also introduced QT_DEPRECATED_VERSION_X next to QT_DEPRECATED_VERSION, in the very same commit. Not sure what version of qglobal.h file I had looked or why I came to claim something else)

@chehrlic Hi. As I just discovered, you are the author of the macros for Qt (commit) which I took as inspiration/blue print when designing this here. So, curious what you think of your work now and if you can already point out things where you see potential for improvements. And also curious what you think of this approach here :)

Actual questions I have:

  • why is QT_DEPRECATED_WARNINGS_SINCE not officially documented? like, any plans to change that macro to something else?
  • why has all Qt code not yet been adapted to QT_DEPRECATED_VERSION/QT_DEPRECATED_VERSION_X, are there places where those macros should not be used, but the version-less ones?
  • why did you go for both QT_DEPRECATED_VERSION & QT_DEPRECATED_VERSION_X, are there places where no message will be wanted?
kossebau updated this revision to Diff 66670.Sep 23 2019, 3:44 PM

add first set of unit tests covering the main functionality.
Feedback on principal approach wanted!

Also add some fixes to actual macro code found by the tests already :)

Actual questions I have:

  • why is QT_DEPRECATED_WARNINGS_SINCE not officially documented? like, any plans to change that macro to something else?

Just forgot it (and also the reviewers) I would guess. There is no plan to change it, at least none I'm aware of. I'm looking for an automatic generation of this macro. Since Qt6 switches to CMake I can maybe borrow some stuff from you ;)

  • why has all Qt code not yet been adapted to QT_DEPRECATED_VERSION/QT_DEPRECATED_VERSION_X, are there places where those macros should not be used, but the version-less ones?

Because noone wanted to do the work and it was added late in the Qt5 lifetime -> A lot of stuff was deprecated for a long time already (in Qt4 times) and there is was a replacement since Qt5.0.0 so the macro was not needed (even though a lot of people got very angry about it). I added it for some new signals which created a lot of discussion since the old ones are widely used.

  • why did you go for both QT_DEPRECATED_VERSION & QT_DEPRECATED_VERSION_X, are there places where no message will be wanted?

Just for consistency - we've QT_DEPRECATED and QT_DEPRECATED_X but I think QT_DEPRECATED should be deprecated by itself. I hope no reviewer will accept a QT_DEPRECATED anymore.

The main problem with the macro is that you have to create a new one for each version so it will become lengthy. But I could not find a better solution somewhere - in the end it's only macro magic from the 80s... :(
But for a library such a macro should be mandatory - your blog post explains the problem very good.

Thanks for your reply, Christian :)

Actual questions I have:

  • why is QT_DEPRECATED_WARNINGS_SINCE not officially documented? like, any plans to change that macro to something else?

Just forgot it (and also the reviewers) I would guess. There is no plan to change it, at least none I'm aware of. I'm looking for an automatic generation of this macro.

Okay. So will keep it documented as part of public API here then.

Since Qt6 switches to CMake I can maybe borrow some stuff from you ;)

Sure :) As others commented and what I think as well, this should be even candidate for being added to upstream in some variant, once it has proven to work out.

  • why has all Qt code not yet been adapted to QT_DEPRECATED_VERSION/QT_DEPRECATED_VERSION_X, are there places where those macros should not be used, but the version-less ones?

Because noone wanted to do the work and it was added late in the Qt5 lifetime -> A lot of stuff was deprecated for a long time already (in Qt4 times) and there is was a replacement since Qt5.0.0 so the macro was not needed (even though a lot of people got very angry about it). I added it for some new signals which created a lot of discussion since the old ones are widely used.

Can you point to those discussions? Would be curious to learn what people's thought are.

  • why did you go for both QT_DEPRECATED_VERSION & QT_DEPRECATED_VERSION_X, are there places where no message will be wanted?

Just for consistency - we've QT_DEPRECATED and QT_DEPRECATED_X but I think QT_DEPRECATED should be deprecated by itself. I hope no reviewer will accept a QT_DEPRECATED anymore.

So you would also think that message-less variants will not be needed I understand. Okay. From what I tested with experimentally deploying the macros on some KF modules I almost always could add a useful message, so running short of reasons for keeping a message-less variant :)

The main problem with the macro is that you have to create a new one for each version so it will become lengthy. But I could not find a better solution somewhere - in the end it's only macro magic from the 80s... :(

Also had no better idea, but then I am also nowhere a C++ macro magician, I operate by manual mostly :) Thankfully with the proposed cmake-generated approach here it's more a matter of adding another version to the DEPRECATION_VERSIONS list, and nobody looks into generated code anyway.

  • why has all Qt code not yet been adapted to QT_DEPRECATED_VERSION/QT_DEPRECATED_VERSION_X, are there places where those macros should not be used, but the version-less ones?

Because noone wanted to do the work and it was added late in the Qt5 lifetime -> A lot of stuff was deprecated for a long time already (in Qt4 times) and there is was a replacement since Qt5.0.0 so the macro was not needed (even though a lot of people got very angry about it). I added it for some new signals which created a lot of discussion since the old ones are widely used.

Can you point to those discussions? Would be curious to learn what people's thought are.

https://lists.qt-project.org/pipermail/development/2019-March/035343.html

It's most likely a discussion when a deprecated / replaced function should really create a warning (esp. when the replacement was just recently added) and when the function should be removed completely - should a function deprecated in Qt5 really be removed in Qt6 or wait for Qt7 or don't remove it at all.

  • why did you go for both QT_DEPRECATED_VERSION & QT_DEPRECATED_VERSION_X, are there places where no message will be wanted?

Just for consistency - we've QT_DEPRECATED and QT_DEPRECATED_X but I think QT_DEPRECATED should be deprecated by itself. I hope no reviewer will accept a QT_DEPRECATED anymore.

So you would also think that message-less variants will not be needed I understand. Okay. From what I tested with experimentally deploying the macros on some KF modules I almost always could add a useful message, so running short of reasons for keeping a message-less variant :)

I think it should be enough to have one macro nowadays :)

Quick Update (week 40):
Locally have added experimental code to even set the proper attribute for GCC compiler, so we get e.g.:

/home/koder/Kode/kdegit/kf5/frameworks/kparts/tests/notepad.cpp: In constructor ‘NotepadPart::NotepadPart(QWidget*, QObject*, const QVariantList&)’:
/home/koder/Kode/kdegit/kf5/frameworks/kparts/tests/notepad.cpp:46:55: warning: ‘KAboutData& KAboutData::setProgramIconName(const QString&)’ is deprecated: Use QApplication::setWindowIcon [-Wdeprecated-declarations]

Still need to clean up the code and update the patch.

Open questions you might want to comment on already:
a) Also plan to use the version info and prefix the message text with a "Since major.minor.", as this info also is interesting usually.
b) A thing I am unsure about is: CMake's generate_export_header does the compiler detection in cmake code and then generates code for that very compiler. Which then ends up with the installed/deployed include files. Which might be an issue for people who would like to use different compiler on the same system, both building against the same generated export header file.
Draft code currently moves compiler detection for deprecated(text) support in generated code, as I have no clue whether one can expect all compilers to digest what the system compiler when it comes to this attribute?

Which might be an issue for people who would like to use different compiler on the same system, both building against the same generated export header file.

I see the theoretical problem, but how could this ever be a problem in practice?
On Unix all compilers support the same syntax (__attribute__ ((__deprecated__))), so you'd have to be on Windows, build a library with mingw, and then try to use it with MSVC, or vice-versa? I wonder if this even works. AFAIK it doesn't (hence the compiler-specific binary installers for Qt, for instance).

Oh and if C++14 support is enabled, we could use [[deprecated("use foo instead")]] which is standard and portable :-)
(requires C++17 for enums and namespaces)

I see the theoretical problem, but how could this ever be a problem in practice?
On Unix all compilers support the same syntax (__attribute__ ((__deprecated__))), so you'd have to be on Windows, build a library with mingw, and then try to use it with MSVC, or vice-versa? I wonder if this even works. AFAIK it doesn't (hence the compiler-specific binary installers for Qt, for instance).

No idea yet, I have no overview about which compilers support what, so far only tested with my local gnu compiler to have an test example how the respective values/macros need to be propagated :) Sounds promising then we could be able to leave complicated logic to cmake time

I found confirmation in cmake's Tests/RunCMake/GenerateExportHeader/reference/

$ grep -r LIBSHARED_DEPRECATED\  . | grep -w define
./UNIX_DeprecatedOnly/libshared_export.h:#  define LIBSHARED_DEPRECATED __attribute__ ((__deprecated__))
./Win32-Clang/libshared_export.h:#  define LIBSHARED_DEPRECATED __attribute__ ((__deprecated__))
./Win32/libshared_export.h:#  define LIBSHARED_DEPRECATED __declspec(deprecated)
./UNIX/libshared_export.h:#  define LIBSHARED_DEPRECATED __attribute__ ((__deprecated__))
./MinGW/libshared_export.h:#  define LIBSHARED_DEPRECATED __attribute__ ((__deprecated__))
kossebau updated this revision to Diff 67220.Oct 2 2019, 8:27 PM
  • extend *_VERSION_DEPRECATED to expect third argument "text" first experimental consumption time support for "text" output with GCC compiler
  • make *_NO_DEPRECATED a proper shortcut for DISABLE_BEFORE_AND_AT=CURRENT
  • extend unit test to cover *_NO_DEPRECATED

I found confirmation in cmake's Tests/RunCMake/GenerateExportHeader/reference/

Not sure what you exactly mean, can you please specify confirmation for what? And what this recommends us to do? :)

kossebau updated this revision to Diff 67324.Oct 4 2019, 2:24 PM
  • extend unit tests to cover library group macro variants

Sadly blows up by all combinations unit test time to >1 min on old machine

kossebau retitled this revision from RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API to Add ECMGenerateExportHeaders, for improved handling of deprecated API.Oct 4 2019, 4:46 PM
dfaure added a comment.Oct 8 2019, 5:19 AM

I found confirmation in cmake's Tests/RunCMake/GenerateExportHeader/reference/

Not sure what you exactly mean, can you please specify confirmation for what? And what this recommends us to do? :)

It confirms that only MSVC uses a different way to express this, which is why nobody handles the mix-and-match case where you'd generate the header for one compiler and then consume it with another.
(unless I'm wrong and mix-and-match between mingw and MSVC is actually possible - AFAIK it's not)

kossebau updated this revision to Diff 67523.Oct 9 2019, 12:39 AM
  • add missing group defaulting for warning settings, got lost in rebase before

So, given the lack of further change proposals or objections, I would proceed to push this in the next days (Thurday evening or Friday morning), to have 3 weeks of pre-5.64 real world testing by CI and people running from git master.
See also https://mail.kde.org/pipermail/kde-frameworks-devel/2019-October/094708.html

dfaure added inline comments.Oct 9 2019, 5:29 AM
modules/ECMGenerateExportHeader.cmake
270

Really? Isn't doWhat excluded only if EXCLUDE_DEPRECATED_BEFORE_AND_AT is set to 5.12.0?
With 5.0.0 it's still available, no?

I'm also surprised this paragraph talks about doWhat but not doBar, they both get disabled together, right?

277

comma followed by uppercase letter is unusual grammar

kossebau marked 2 inline comments as done.Oct 9 2019, 1:54 PM
kossebau added inline comments.
modules/ECMGenerateExportHeader.cmake
270

Hm, indeed that text is bogus. Guess that was in draft mode still when I shuffled a lot with code snippets for the examples, but then I forgot to finish it up and missed the bits when skimming the doc text for completeness. Brushed over text coming in.

And going to give the whole documentation another calm concentrated read tonight myself again, seeing such things still in.

kossebau updated this revision to Diff 67554.Oct 9 2019, 1:54 PM
kossebau marked an inline comment as done.
  • update to David's findings in the docs
kossebau updated this revision to Diff 67584.Oct 9 2019, 11:43 PM

improve documentation, especially about the generated C++ macros

kossebau retitled this revision from Add ECMGenerateExportHeaders, for improved handling of deprecated API to Add ECMGenerateExportHeader, for improved handling of deprecated API.Oct 10 2019, 3:24 PM
kossebau updated this revision to Diff 67620.Oct 10 2019, 3:32 PM
kossebau marked an inline comment as done.

switch to do generation-time decision about deprecated(text) attribute usage

GCC/Clang are mostly ABI-compatible, so mixing and linking of artifacts from
both compilers can be expected, so headers should be compatible, too. Though
the compiler versions officially supported for KF (and thus ECM)* both
support attribute ((deprecated(text))), so it can be hardcoded into
the generated header without triggering incompatibilities.
On Windows mixing compilers seems not expected, so compiler detected at
generation time can also be expected at any further header usage time.

To keep the momentum, now going to merge, given there was no objection when I pointed out the plan to move forward both here and in the email to frameworks-devel.
I assume no-one has time enough to wrap their brain around this as well and/or too many other competing stuff which has more of their interest.

Thanks again @dfaure for your review feedback done here, as result this feature should be more useful to all of us.
Still hoping for more review and feedback post-merge, when people start to get in contact :) Will be eager to shape things some more where needed, especially before things get final with 5.64 release.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 10 2019, 8:50 PM
This revision was automatically updated to reflect the committed changes.