Conversion to the cmake building system. #4

Merged
SlavekB merged 2 commits from feat/cmakeConv into master 3 years ago
Ghost commented 4 years ago

What is building so far (symbols visibility is off):

  • kbstateapplet
  • kmag
  • kmousetool
  • kmouth
  • ksayit
  • kttsd - weird header kcmkttsmgrwidget.ui.h
  • doc
  • IconThemes - seems ok, have a look at trinity.png
What is building so far (symbols visibility is off): - [x] kbstateapplet - [x] kmag - [x] kmousetool - [x] kmouth - [x] ksayit - [x] kttsd - weird header kcmkttsmgrwidget.ui.h - [x] doc - [x] IconThemes - seems ok, have a look at trinity.png
Ghost added the PR/wip label 4 years ago
Ghost commented 4 years ago
Poster

Rem: kttsd/compat directory, test over kspeech.h --> ksayIt

AC_TRY_COMPILE(
[#include <kspeech.h>],
[
if (4 == KSpeech::mtHtml);
],
have_latest_kspeech=yes,
Rem: kttsd/compat directory, test over kspeech.h --> ksayIt ``` AC_TRY_COMPILE( [#include <kspeech.h>], [ if (4 == KSpeech::mtHtml); ], have_latest_kspeech=yes, ```
Ghost commented 4 years ago
Poster

One KTTS audio plugin ought to be available amongst these: akode, aRts, alsa and gstreamer.
I suggest to make aRts the default and the others optionals.

One KTTS audio plugin ought to be available amongst these: akode, aRts, alsa and gstreamer. I suggest to make aRts the default and the others optionals.
Ghost commented 4 years ago
Poster

Rem install: kttsd_audioplugin.desktop Done!

Rem install: kttsd_audioplugin.desktop Done!
Ghost commented 4 years ago
Poster

Hardcoded path "txt2pho" hadifix/initialconfig.h

Hardcoded path "txt2pho" hadifix/initialconfig.h
Ghost commented 4 years ago
Poster

Note: On my system, automake builds 5 times kspeech.kidl and kspeechsink.kidl, their resulting files (kspeech_stub.cpp, kspeech_stub.h and kspeechsink_skel.cpp) 4 times.

Note: On my system, automake builds 5 times kspeech.kidl and kspeechsink.kidl, their resulting files (kspeech_stub.cpp, kspeech_stub.h and kspeechsink_skel.cpp) 4 times.
Ghost commented 4 years ago
Poster

Rem: ksayIt, at the moment, is built with the kttsd/compat directory.

Have to gather aRts libraries during linking time. --> done, but ksayIt needs aRts.
See if aRts is optionnal or a requirement.

Rem: ksayIt, at the moment, is built with the kttsd/compat directory. Have to gather aRts libraries during linking time. --> done, but ksayIt needs aRts. See if aRts is optionnal or a requirement.
Chris commented 4 years ago
Collaborator

These days I will start to create ebuilds, so I can start to test your work, @cethyel.

And it seems back to Gentoo days aRts was optional. At least it was the case in KDE 3.5.10. So aRts should be optional still today, because I don't think there was much work on that components since back then.

By the way: I like your way of comments. That reminds me on my todo list for TDE. 😸

EDIT: It seems I was too fast with my findings. In fact ksayit had some hard-depend on tdemultimedia-arts. But only because it seems it was buld with --enable-ksayit-audio-plugins hardcoded. Maybe that can be avoided?

Also, it seems akode was the default, if no backend was selected. But if aRts is needed anyway, I would like to see aRts selected as default too instead of akode or so. 👍

These days I will start to create ebuilds, so I can start to test your work, @cethyel. And it seems back to Gentoo days aRts was optional. At least it was the case in KDE 3.5.10. So aRts should be optional still today, because I don't think there was much work on that components since back then. By the way: I like your way of comments. That reminds me on my todo list for TDE. :smile_cat: EDIT: It seems I was too fast with my findings. In fact `ksayit` had some hard-depend on `tdemultimedia-arts`. But only because it seems it was buld with `--enable-ksayit-audio-plugins` hardcoded. Maybe that can be avoided? Also, it seems `akode` was the default, if no backend was selected. But if aRts is needed anyway, I would like to see aRts selected as default too instead of akode or so. :+1:
Ghost commented 4 years ago
Poster

These days I will start to create ebuilds, so I can start to test your work, @cethyel.

Any feedbacks are welcomed.

it seems it was buld with --enable-ksayit-audio-plugins hardcoded. Maybe that can be avoided?

Once It build "as a whole", I'll look for definitions in the sources, I'll have a look on the configure script (haven't done that yet 😅 ), we shall see then how everything can be shaped, stay tuned!

> These days I will start to create ebuilds, so I can start to test your work, @cethyel. Any feedbacks are welcomed. > it seems it was buld with --enable-ksayit-audio-plugins hardcoded. Maybe that can be avoided? Once It build "as a whole", I'll look for definitions in the sources, I'll have a look on the configure script (haven't done that yet :sweat_smile: ), we shall see then how everything can be shaped, stay tuned!
Ghost commented 4 years ago
Poster

Configure's options:

  • --enable-ksayit-audio-plugins build audio plugins for KSayIt [default=no]
  • --enable-kttsd-command build KTTSD Command Plugin [default=yes]
  • --enable-kttsd-epos build KTTSD Epos plugin [default=yes]
  • --enable-kttsd-festivalint build KTTSD Festival Interactive plugin [default=yes]
  • --enable-kttsd-flite build KTTSD Festival Lite (flite) [default=yes]
  • --enable-kttsd-freetts build KTTSD FreeTTS Plugin [default=yes]
  • --enable-kttsd-hadifix build KTTSD Hadifix Plugin [default=yes]
  • --without-arts build without aRts [default=no]
  • --with-gstreamer enable support for GStreamer [default=no]
  • --with-alsa enable support for ALSA [default=check]
  • --with-akode enable the aKode decoder [default=no]

see to add optional kate plugin. Done!

Configure's options: - [x] --enable-ksayit-audio-plugins build audio plugins for KSayIt [default=no] - [x] --enable-kttsd-command build KTTSD Command Plugin [default=yes] - [x] --enable-kttsd-epos build KTTSD Epos plugin [default=yes] - [x] --enable-kttsd-festivalint build KTTSD Festival Interactive plugin [default=yes] - [x] --enable-kttsd-flite build KTTSD Festival Lite (flite) [default=yes] - [x] --enable-kttsd-freetts build KTTSD FreeTTS Plugin [default=yes] - [x] --enable-kttsd-hadifix build KTTSD Hadifix Plugin [default=yes] - [x] --without-arts build without aRts [default=no] - [x] --with-gstreamer enable support for GStreamer [default=no] - [x] --with-alsa enable support for ALSA [default=check] - [x] --with-akode enable the aKode decoder [default=no] see to add optional kate plugin. Done!
Ghost commented 4 years ago
Poster

Rem:
kmag/icons --> adjust automake with new folder. Done!
In the doc folder, remove "man-*.1.docbook" file, add man pages instead
Rework man pages...

Rem: kmag/icons --> adjust automake with new folder. Done! In the doc folder, remove "man-*.1.docbook" file, add man pages instead Rework man pages...
Ghost force-pushed feat/cmakeConv from e7d7fa7069 to 953d25e3a1 3 years ago
Ghost force-pushed feat/cmakeConv from 953d25e3a1 to bbb5fc5bb8 3 years ago
Ghost force-pushed feat/cmakeConv from bbb5fc5bb8 to 155e7fa75a 3 years ago
Ghost force-pushed feat/cmakeConv from 155e7fa75a to 986225f81b 3 years ago
Ghost force-pushed feat/cmakeConv from 986225f81b to f570c4a3d6 3 years ago
Ghost force-pushed feat/cmakeConv from f570c4a3d6 to 721a8fe117 3 years ago
Ghost force-pushed feat/cmakeConv from 721a8fe117 to 46f1f489fb 3 years ago
Ghost force-pushed feat/cmakeConv from 46f1f489fb to 2e963dbd54 3 years ago
Ghost commented 3 years ago
Poster

Rem:

  • aRts optional Done!
  • refine aRts libraries Done!
  • add kayit man page Done!
Rem: - aRts optional Done! - refine aRts libraries Done! - add kayit man page Done!
Ghost force-pushed feat/cmakeConv from 2e963dbd54 to d91f0b2e72 3 years ago
Ghost force-pushed feat/cmakeConv from d91f0b2e72 to 3ca002b491 3 years ago
SlavekB reviewed 3 years ago
SlavekB left a comment
Owner

In the first round of revision, I only did a building test. It was successful except for a few little things. Here are some notes.

In the first round of revision, I only did a building test. It was successful except for a few little things. Here are some notes.
##### icons
tde_install_icons()
Owner

Originally, the icons were installed in an application-specific folder, and only the application icon as such was installed in the global folder.

Since there is no risk of a collision (all icons contain application names), then it is probably not a problem to install as you prepared – everything in the global folder.

I mention this only to consider which way to choose.

Originally, the icons were installed in an application-specific folder, and only the application icon as such was installed in the global folder. Since there is no risk of a collision (all icons contain application names), then it is probably not a problem to install as you prepared – everything in the global folder. I mention this only to consider which way to choose.
SlavekB marked this conversation as resolved
add_subdirectory( KTTSD_Lib )
add_subdirectory( src )
#add_subdirectory( Freeverb_plugin ) where's the <arts/artsmodules.h> header?
Owner

We now know that the header is in tdemultimedia, so builds can be enabled.
However, there should probably be an option like WITH_KSAYIT_FREEVERB and a corresponding check to see if everything needed is present.

We now know that the header is in tdemultimedia, so builds can be enabled. However, there should probably be an option like `WITH_KSAYIT_FREEVERB` and a corresponding check to see if everything needed is present.
SlavekB marked this conversation as resolved
##### Freeverb_plugin (module)
tde_add_library( Freeverb_plugin MODULE AUTOMOC
Owner

With automake it is built and installed as libFreeverb_plugin => here you will need to add lib prefix to the module name.

With automake it is built and installed as `libFreeverb_plugin` => here you will need to add `lib` prefix to the module name.
SlavekB marked this conversation as resolved
tde_create_translated_desktop(
SOURCE ksayit_libFreeverb_service.desktop
DESTINATION ${SERVICES_INSTALL_DIR}
Owner

The file should be installed in SERVICETYPES_INSTALL_DIR.

The file should be installed in `SERVICETYPES_INSTALL_DIR`.
SlavekB marked this conversation as resolved
##### libkcm_kttsd (kpart)
tde_add_kpart( libkcm_kttsd MODULE AUTOMOC
Owner

With automake it is built and installed as kcm_kttsd => here you will need to remove lib prefix from the module name.

With automake it is built and installed as `kcm_kttsd` => here you will need to remove `lib` prefix from the module name.
SlavekB marked this conversation as resolved
Ghost force-pushed feat/cmakeConv from 3ca002b491 to 47bf79bbfb 3 years ago
Ghost force-pushed feat/cmakeConv from 47bf79bbfb to bcc8094aa1 3 years ago
Owner

It looks good. I'll take a closer look, probably over the weekend. It seems to be nearing completion of the conversion – great job! Thank you @cethyel

It looks good. I'll take a closer look, probably over the weekend. It seems to be nearing completion of the conversion – great job! Thank you @cethyel
Ghost force-pushed feat/cmakeConv from bcc8094aa1 to 2b845ff305 3 years ago
Ghost removed the PR/wip label 3 years ago
Ghost changed title from WIP:Conversion to the cmake building system. to Conversion to the cmake building system. 3 years ago
SlavekB reviewed 3 years ago
##### testfilter (test)
tde_add_executable( testfilter AUTOMOC
Owner

Instead of a separate tde_add_executable and add_test you can use tde_add_check_executable – it will be the same as tde_add_executable, just add the TEST argument. As a result, the testfilter will only be built if tests are enabled.

Instead of a separate `tde_add_executable` and `add_test` you can use `tde_add_check_executable` – it will be the same as `tde_add_executable`, just add the `TEST` argument. As a result, the `testfilter` will only be built if tests are enabled.
SlavekB marked this conversation as resolved
SlavekB added 2 commits 3 years ago
16521f99e7
cmake: Use tde_add_check_executable instead of a combination
cd27387750
cmake: Simplify 'skel' and 'stub' generation for kspeech and kspeechlink
Owner

As you can see, I've simplified the generation of skel and stub for kspeech and kspeechsink, as well as their subsequent linking.

Because either combinations of kspeech.skel and kspeechsink.stub or kspeech.stub and kspeechsink.skel are always used there, there are kspeech_skel-static and kspeech_stub-static static libraries that correspond to these combinations. Thanks to this, there is no need for any separate generation of skel / stub or any explicit DEPENDENCIES.

Please try it on your system.

As you can see, I've simplified the generation of `skel` and `stub` for `kspeech` and `kspeechsink`, as well as their subsequent linking. Because either combinations of `kspeech.skel` and `kspeechsink.stub` or `kspeech.stub` and `kspeechsink.skel` are always used there, there are `kspeech_skel-static` and `kspeech_stub-static` static libraries that correspond to these combinations. Thanks to this, there is no need for any separate generation of `skel` / `stub` or any explicit `DEPENDENCIES`. Please try it on your system.
Ghost added 1 commit 3 years ago
4a6b3e10e7
Fix default options.
Ghost commented 3 years ago
Poster

As you can see, I've simplified the generation of skel and stub for kspeech and kspeechsink, as well as their subsequent linking.

Because either combinations of kspeech.skel and kspeechsink.stub or kspeech.stub and kspeechsink.skel are always used there, there are kspeech_skel-static and kspeech_stub-static static libraries that correspond to these combinations. Thanks to this, there is no need for any separate generation of skel / stub or any explicit DEPENDENCIES.

Please try it on your system.

It works, nice! 👍

> As you can see, I've simplified the generation of `skel` and `stub` for `kspeech` and `kspeechsink`, as well as their subsequent linking. > > Because either combinations of `kspeech.skel` and `kspeechsink.stub` or `kspeech.stub` and `kspeechsink.skel` are always used there, there are `kspeech_skel-static` and `kspeech_stub-static` static libraries that correspond to these combinations. Thanks to this, there is no need for any separate generation of `skel` / `stub` or any explicit `DEPENDENCIES`. > > Please try it on your system. It works, nice! :+1:
Ghost commented 3 years ago
Poster

My install (automake and here with cmake) does not include the png icons from IconThemes/mono/png/.
Should we include those?

My install (automake and here with cmake) does not include the png icons from *IconThemes/mono/png/*. Should we include those?
Ghost added 1 commit 3 years ago
90162d4269
Cleanup headers in ui files.
Owner

My install (automake and here with cmake) does not include the png icons from IconThemes/mono/png/.
Should we include those?

I was hesitant about what the icon was and whether it was actually used somewhere … and yes, it seems to be installed and used also in other themes, so it makes sense to install it.

Note: Do not hesitate to make squash commits into one – including my adjustments, which are only a small part of your great work.

> My install (automake and here with cmake) does not include the png icons from *IconThemes/mono/png/*. > Should we include those? I was hesitant about what the icon was and whether it was actually used somewhere … and yes, it seems to be installed and used also in other themes, so it makes sense to install it. Note: Do not hesitate to make squash commits into one – including my adjustments, which are only a small part of your great work.
Ghost changed title from Conversion to the cmake building system. to WIP:Conversion to the cmake building system. 3 years ago
Ghost added the PR/wip label 3 years ago
Owner

I tried to give away the definition of -UTQT_NO_ASCII_CAST and I did not notice any problem. Please, can you verify it on your system?

I tried to give away the definition of `-UTQT_NO_ASCII_CAST` and I did not notice any problem. Please, can you verify it on your system?
SlavekB reviewed 3 years ago
CMakeLists.txt Outdated
tde_conditional_add_subdirectory( BUILD_KSAYIT ksayit )
tde_conditional_add_subdirectory( BUILD_KTTSD kttsd )
tde_conditional_add_subdirectory( BUILD_DOC doc )
add_subdirectory( IconThemes )
Owner

Now I realize that there is no BUILD_ICONTHEMES option (or something like that). This can be useful for distributions that build individual parts separately.

Now I realize that there is no `BUILD_ICONTHEMES` option (or something like that). This can be useful for distributions that build individual parts separately.
SlavekB marked this conversation as resolved
SlavekB reviewed 3 years ago
pkg_search_module( ALSA alsa )
if( ALSA_FOUND )
check_include_file( "sys/time.h" HAVE_SYS_TIME )
Owner

It seems that after moving the test for header files inside the WITH_ALSA / ALSA_FOUND conditions, there is an incorrect indentation of the entire moved block.

It seems that after moving the test for header files inside the `WITH_ALSA` / `ALSA_FOUND` conditions, there is an incorrect indentation of the entire moved block.
SlavekB marked this conversation as resolved
SlavekB reviewed 3 years ago
)
install(
FILES kmouth.desktop
Owner

A desktop file that escaped attention should be processed using tde_create_translated_desktop.

A desktop file that escaped attention should be processed using `tde_create_translated_desktop`.
SlavekB marked this conversation as resolved
SlavekB reviewed 3 years ago
install(
FILES
de.desktop
Owner

Several other desktop files that escaped attention…

Several other desktop files that escaped attention…
SlavekB marked this conversation as resolved
SlavekB reviewed 3 years ago
)
install(
FILES ksayit.desktop
Owner

Another desktop file that escaped attention.

Another desktop file that escaped attention.
SlavekB marked this conversation as resolved
Owner

I apologize for a lot of individual comments, instead of one summary revision. At the beginning of the revision and I did not expect more than one comment, so I sent a single comment, in the end there were more comments… Anyway, everything is just small things.

I apologize for a lot of individual comments, instead of one summary revision. At the beginning of the revision and I did not expect more than one comment, so I sent a single comment, in the end there were more comments… Anyway, everything is just small things.
Ghost added 1 commit 3 years ago
a6d3862181
Fix few desktop files for translations.
Ghost force-pushed feat/cmakeConv from a6d3862181 to a22a5da2e5 3 years ago
Ghost force-pushed feat/cmakeConv from a22a5da2e5 to 78ef701861 3 years ago
Ghost changed title from WIP:Conversion to the cmake building system. to Conversion to the cmake building system. 3 years ago
Ghost removed the PR/wip label 3 years ago
SlavekB reviewed 3 years ago
SlavekB left a comment
Owner

Although I did not expect any further comments, I have some suggestions there – see below.

Although I did not expect any further comments, I have some suggestions there – see below.
check_include_file( "sys/time.h" HAVE_SYS_TIME )
check_include_file( "time.h" HAVE_TIME )
if( HAVE_SYS_TIME AND HAVE_TIME )
Owner

It looks like I will have one more question / comment. In other modules, we use tests as follows to verify the coexistence of time.h and sys/time.h:

check_include_file( "sys/time.h"         HAVE_SYS_TIME_H    )
check_include_files( "sys/time.h;time.h" TIME_WITH_SYS_TIME )

This method has the advantage that these tests are sufficient to verify, without the need for additional conditions, both the existence of sys/time.h and whether the use of time.h does not cause a collision with sys/time.h. You can see that this is exactly the combination I recently added to tork to solve the FTBFS on various distributions.

The current condition code, if time.h exists, sets HAVE_SYS_TIME_H even if it does not exist.

It looks like I will have one more question / comment. In other modules, we use tests as follows to verify the coexistence of `time.h` and `sys/time.h`: ``` check_include_file( "sys/time.h" HAVE_SYS_TIME_H ) check_include_files( "sys/time.h;time.h" TIME_WITH_SYS_TIME ) ``` This method has the advantage that these tests are sufficient to verify, without the need for additional conditions, both the existence of `sys/time.h` and whether the use of `time.h` does not cause a collision with `sys/time.h`. You can see that this is exactly the combination I recently added to tork to [solve the FTBFS](../tork/commit/bda2d7559f18b2e02050dc433c5ff70cec8e3362) on various distributions. The current condition code, if `time.h` exists, sets `HAVE_SYS_TIME_H` even if it does not exist.
SlavekB marked this conversation as resolved
install(
Owner

I suggest a small simplification:

while( _lang de en nl sv )

    install(
        FILES
            ${_lang}-courteousness.phrasebook
            ${_lang}-greetings.phrasebook
            ${_lang}-howareyou.phrasebook
            ${_lang}-personal.phrasebook

        DESTINATION ${DATA_INSTALL_DIR}/kmouth/books/${_lang}
    )

    tde_create_translated_desktop(
        SOURCE ${_lang}.desktop
        DESTINATION ${DATA_INSTALL_DIR}/kmouth/books/${_lang}
    )

endwhile( )
I suggest a small simplification: ``` while( _lang de en nl sv ) install( FILES ${_lang}-courteousness.phrasebook ${_lang}-greetings.phrasebook ${_lang}-howareyou.phrasebook ${_lang}-personal.phrasebook DESTINATION ${DATA_INSTALL_DIR}/kmouth/books/${_lang} ) tde_create_translated_desktop( SOURCE ${_lang}.desktop DESTINATION ${DATA_INSTALL_DIR}/kmouth/books/${_lang} ) endwhile( ) ```
Owner

Of course, the above should be

foreach( _lang de en nl sv )
...
endforeach()
Of course, the above should be ``` foreach( _lang de en nl sv ) ... endforeach() ```
SlavekB marked this conversation as resolved
Ghost added 1 commit 3 years ago
0dc6c2e443
An other bash of cleanup.
Ghost commented 3 years ago
Poster

@SlavekB when you've got time, take a look at the last push.
When there's no more comments, I'll squash/reorder once and for all.

@SlavekB when you've got time, take a look at the last push. When there's no more comments, I'll squash/reorder once and for all.
Ghost force-pushed feat/cmakeConv from 0dc6c2e443 to b34a77af51 3 years ago
Owner

@SlavekB when you've got time, take a look at the last push.
When there's no more comments, I'll squash/reorder once and for all.

Thank you, it looks good.

> @SlavekB when you've got time, take a look at the last push. > When there's no more comments, I'll squash/reorder once and for all. Thank you, it looks good.
Ghost force-pushed feat/cmakeConv from b34a77af51 to 91fc9555ab 3 years ago
SlavekB approved these changes 3 years ago
SlavekB left a comment
Owner

Everything looks good.
@cethyel great job, thank you!

Everything looks good. @cethyel great job, thank you!
SlavekB merged commit 91fc9555ab into master 3 years ago
SlavekB deleted branch feat/cmakeConv 3 years ago
SlavekB added this to the R14.0.10 release milestone 3 years ago

Reviewers

SlavekB approved these changes 3 years ago
The pull request has been merged as 91fc9555ab.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdeaccessibility#4
Loading…
There is no content yet.