Review Request 109157: Amarok buildsystem cleanup preview

Matěj Laitl matej at laitl.cz
Tue Feb 26 22:44:16 UTC 2013



> On Feb. 25, 2013, 9:48 p.m., Edward Hades Toroshchin wrote:
> > Why do you want the intermediate libs at all?
> 
> Matěj Laitl wrote:
>     In general, all points mentioned in http://www.cmake.org/Wiki/CMake/Tutorials/Object_Library#Motivation hold here, especially "This approach is easy to use and helps organize the project source tree.". (Note: approach used here doesn't use CMake "Object Libraries" at all)
> 
> Matěj Laitl wrote:
>     Also, one CMakeLists.txt per directory was agreed upon in Randa 2012 by all present Amarok devs, see http://community.kde.org/Amarok/Development/Randa2012/Architecture
> 
> Alexander Neundorf wrote:
>     If you want my opinion: if you really want those "convenience libraries", require at least cmake 2.8.9, and use cmake's OBJECT libraries, but really stay away from what you are doing in amarok_link_intermediate_libraries(). This is not portable at all and creates work the buildsystem should do (and can do if you use the features it provides for that).
>     
>     Once you require cmake 2.8.9, you can also start using cmake's built-in automoc feature.
>     
>

> If you want my opinion: if you really want those "convenience libraries", require at least cmake 2.8.9, and use cmake's OBJECT libraries, but really stay away from what you are doing in amarok_link_intermediate_libraries(). This is not portable at all and creates work the buildsystem should do (and can do if you use the features it provides for that).

I also considered using CMake object libraries, however, if I understand it correctly:
 a) CMake won't add -fPIC for me for object libraries that end up in shared libraries. [1] In 2.8.9 there's POSITION_INDEPENDENT_CODE which is much more portable, but this is not specific to object libraries at all and I'll still have to set it manually.
 b) CMake won't propagate -DBUILD_FOO_LIB from "parent" shared library to the "child" object library for me, I'll have to do it manually.

At this point I don't see what is more portable on using object libaries, as amarok_link_intermediate_libraries() does exactly a) and b) and I'll have to do it in some form too with object libraries.

[1] http://www.cmake.org/pipermail/cmake/2012-June/050941.html


- Matěj


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109157/#review28081
-----------------------------------------------------------


On Feb. 25, 2013, 9:36 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109157/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2013, 9:36 p.m.)
> 
> 
> Review request for Amarok and Build System.
> 
> 
> Description
> -------
> 
> Hi fellow devs, this is a preview of how I'd like to clean up our (Amarok) buildsystem. The main idea is that every directory should has its own CMakeLists.txt, forming a small static library (build-time only), these small "intermediate" static libs get linked to bigger libraries like amaroklib.so etc. This preview does it on the shared/ directory which is turned into a separate shared library first. This review request are 3 commits squashed. Please speak up if you see some issues or something is not clear/too ugly for you.
> 
> 
> Biggie: introduce libamarokshared.so library to avoid building things in shared/ twice
> 
> Shared libraries exist to *cough* share built code, use them! This is first part
> of much larger buildsystem changes planned in Randa 2012.
> 
> This also brings some other changes that were needed, namely avoiding UTILITIES_BUILD,
> changing Meta::Tag::writeTags() slightly etc.
> 
> @all git builders: you may need to remove build/CMakeCache.txt as some cached variables
> may become invalid.
> 
> @Patrick: this needs at least build testing on Windows to ensure I've done the
> KDE_IMPORT/EXPORT macros right.
> 
> CCMAIL: amarok-devel at kde.org
> CCMAIL: Patrick von Reth <vonreth at kde.org>
> 
> 
> Split AMAROK_EXPORT and AMAROK_CORE_EXPORT code and move to appropriate locations
> 
> ...made possible by the previous commit.
> 
> 
> buildsystem: start transition to per-directory CMakeLists.txt; introduce AmarokIntermediateLib.cmake
> 
> This is an example how transitioning to per-directory CMakeLists.txt
> can be made. We introduce AmarokIntermediateLib.cmake file with
> convenience macros amarok_add_intermediate_library and
> amarok_link_intermediate_libraries.
> 
> We immediately use them to make shared/collectionscanner and
> shared/collectionscanner free-standing with their own CMakeLists.txt
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt d96cbc96de94fcc898b6d8af64c65634a28dd594 
>   cmake/modules/AmarokIntermediateLib.cmake PRE-CREATION 
>   shared/CMakeLists.txt PRE-CREATION 
>   shared/FileType.h 1b6facf66384bb70842c30e5a8ccd771c3009e1d 
>   shared/FileType.cpp f02605845778b6d99b74155fdd8974082bb915fd 
>   shared/FileTypeResolver.cpp f7c3eab8c9287f238c4eeec3a66def94aade3254 
>   shared/MetaReplayGain.h bf130cfe6f3a05b3852d6508447b3fc74e560e23 
>   shared/MetaTagLib.h cfa9910a7a2b0b24d8051c1c01866d239fdbe891 
>   shared/MetaTagLib.cpp 2e785e03b07193367d42491602f3e33cd862420c 
>   shared/README 3d84f0deca111240087c97fc2290f1c6748a3b51 
>   shared/TagsFromFileNameGuesser.h 7f4443ef7c662970931a1fc06471c00bf053ab5d 
>   shared/amarok_export.h cdab8033e35dcca817f275211b5a3bef4652a2c2 
>   shared/amarokshared_export.h PRE-CREATION 
>   shared/collectionscanner/Album.h 91c3501ce615c170d440564a0e57fd2a30840e5b 
>   shared/collectionscanner/Album.cpp 520452360e2b5ed4f1b52f6ff270721d08996bf3 
>   shared/collectionscanner/BatchFile.h 61192770366e17e85637729b051a2f7a353cfc48 
>   shared/collectionscanner/CMakeLists.txt PRE-CREATION 
>   shared/collectionscanner/Directory.h 03f07ce3f189825bdc2eeff6e7c79fea2ed0ad8c 
>   shared/collectionscanner/Directory.cpp a6b02f86d8473645fd56958f3c5a6df01b65c2e1 
>   shared/collectionscanner/Playlist.h 420bafc32bb80efe170c99b7e783163a9b4801b6 
>   shared/collectionscanner/ScanningState.h dedf04998b4c0cd501fb22c8814754ed622c99dc 
>   shared/collectionscanner/Track.h 96d243e898aa06319fa7347239098d6486d63adb 
>   shared/tag_helpers/APETagHelper.h 2da873434b536af62cbbb546790dd90d1f733496 
>   shared/tag_helpers/ASFTagHelper.h 99818949e8781847980a88ae59c7f7dd47ee955f 
>   shared/tag_helpers/ASFTagHelper.cpp 966c29838106e722ff8606cc32d35c417ddf565f 
>   shared/tag_helpers/CMakeLists.txt PRE-CREATION 
>   shared/tag_helpers/ID3v2TagHelper.h 5aec4ed82b7b6fc37d52b9809452ffdb3063af9c 
>   shared/tag_helpers/ID3v2TagHelper.cpp 7d50ca64888f856541cb6c56b3d6e720d17a4df8 
>   shared/tag_helpers/MP4TagHelper.h 8a5b1851939b8f20f450a8852db1d1d35e478b3c 
>   shared/tag_helpers/MP4TagHelper.cpp 37fcc1767a931f76a04fcd9fa72031594a0633e3 
>   shared/tag_helpers/StringHelper.h 6b31b684abf2ef5cb4eb3736f18b6f31a1729037 
>   shared/tag_helpers/TagHelper.h 898a76bb1d7e73afdf84a1d4e466cf314b53b807 
>   shared/tag_helpers/TagHelper.cpp 6909483a4df8d880143b5250991ad21daff129bb 
>   shared/tag_helpers/VorbisCommentTagHelper.h c5e87b3df344baecefb5d1d194f004a566d69804 
>   shared/tag_helpers/VorbisCommentTagHelper.cpp 85689fb5db0ad26333281454b7826688a4ad0503 
>   src/CMakeLists.txt 6f453e2097c861e085c0c9c3c4c1534a45d477f8 
>   src/EngineController.h f4835bc97e6deb84c9c556e9859cfede5bf48f04 
>   src/amarok_export.h PRE-CREATION 
>   src/browsers/playlistbrowser/UserPlaylistModel.h 186837bc07df982d31a394333f5974b06669abd2 
>   src/core-impl/capabilities/AlbumActionsCapability.h 789e6c7106d7db5dae6bf4a4baffd4c878a70ac0 
>   src/core-impl/capabilities/timecode/TimecodeBoundedPlaybackCapability.h 05749d35ecc5d34f4567f17f3c5f2e905abdaff2 
>   src/core-impl/capabilities/timecode/TimecodeEditCapability.h bfd50407acbfe1d5b1f02186032c927cd7b04b39 
>   src/core-impl/collections/db/sql/CMakeLists.txt d6ba891c93327dbe5fa21eb0499ce83ef6523151 
>   src/core-impl/collections/db/sql/SqlMeta.cpp 2032a2b03c40a66f91bbc6afb0cfc8f4244a410b 
>   src/core-impl/collections/ipodcollection/CMakeLists.txt 1dc5bce67c88d7207f76c3ca3590e66a876549b9 
>   src/core-impl/collections/playdarcollection/CMakeLists.txt afa9300be9e6791d8e753bbf2cf0529408d639da 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.h aeabcfe0aa4442ae7899c8b082a4ff52ec41e9c9 
>   src/core-impl/collections/support/jobs/WriteTagsJob.cpp 21d5c1b170b8a1809276ca84bf69bded3c511795 
>   src/core-impl/collections/umscollection/CMakeLists.txt ff3dd39116eefe0ea1cb6bffe9b4f14cfc6c22f8 
>   src/core-impl/collections/umscollection/UmsCollection.cpp 08f40b379a155ff1bed67d11acd01c46c4687448 
>   src/core-impl/meta/file/File.h 10132c6cda8444a3653c304d8d366239691b20e5 
>   src/core-impl/meta/file/File_p.h 652787010099e39acbac5baf56c1b234fbc1cbb6 
>   src/core-impl/meta/proxy/MetaProxy.h 7097a59a33938f8df8c926cea61ddc3fe22adc97 
>   src/core-impl/playlists/providers/user/UserPlaylistProvider.h 48d020735775e0112fc6548882f66fca8ebce05d 
>   src/core-impl/playlists/types/file/PlaylistFile.h 4322da992261e4fac564a7bcd5bc2d5ae1fec989 
>   src/core-impl/support/PersistentStatisticsStore.h 752c7044e7ef62b88dd1947b3cbbae6e868583eb 
>   src/core/CMakeLists.txt 2809dc3db1525d87d78fcbc276c7b4aa0f60b203 
>   src/core/amarokcore_export.h PRE-CREATION 
>   src/core/capabilities/ActionsCapability.h 6cf53d93d091bc6cdf02da5a16b8e23a841a3d59 
>   src/core/capabilities/BookmarkThisCapability.h b0072e09e2f2500f078e062f959107265c7497a1 
>   src/core/capabilities/Capability.h ba7dadccf72373954b1838ea6c10bb24e52d8da3 
>   src/core/capabilities/CollectionImportCapability.h ed4fdc64d98d3ce1c4a3c590a886601aacb1ba89 
>   src/core/capabilities/CollectionScanCapability.h 0b650c54e4e074797205db6ca44b334f00a8160d 
>   src/core/capabilities/EditCapability.h 9cc76be585d3ee2ce7ba0a10ca1cbc58b58113c2 
>   src/core/capabilities/FindInSourceCapability.h f720dde4e6e556233003e2cd0a1058979fafe717 
>   src/core/capabilities/OrganiseCapability.h 547e1b78ff9f2043eed2f6c6ec3d582ecc7d16da 
>   src/core/capabilities/ReadLabelCapability.h 5536d9967914c1e98589212448c85a87d11a6035 
>   src/core/capabilities/SourceInfoCapability.h 0c58878b7807b486b5d57127bff2588b17892d7a 
>   src/core/capabilities/StreamInfoCapability.h f53640229f28ea3c76798ee9132ae9393d73a9e9 
>   src/core/capabilities/TranscodeCapability.h be5f217d7cc19683e9fd25fb3081c1dbe276afa4 
>   src/core/capabilities/WriteLabelCapability.h b23700dce6d881bafd2235ab858106cfed720061 
>   src/core/collections/Collection.h 48f9a08d3b008028e9c88b77c170522122a68671 
>   src/core/collections/CollectionLocation.h 11a64d1be7272b38cbd6c816986c509cf75e0618 
>   src/core/collections/CollectionLocationDelegate.h 6f39c19792ba2e245a5cba74bc8b1a809c21d80c 
>   src/core/collections/QueryMaker.h a30b94fef31bcdea29e3bc8a60ed1fbee067879f 
>   src/core/collections/support/SqlStorage.h c2ce8adad6a8e9b15d5da30a4d4f7943757f2726 
>   src/core/collections/support/TrackForUrlWorker.h c41917605fb41a6fbd6610a22875cd4e887a15fa 
>   src/core/interfaces/Logger.h 346fbb623b2843c6424c491780a660feb017fca5 
>   src/core/interfaces/MetaCapability.h 14885b0a81d1c3b4585795a778ac146fac72b746 
>   src/core/meta/Meta.h d0fea2064bb2107478350d2464265323166f9707 
>   src/core/meta/Statistics.h 3007592b604bcc5d6731639413d445095be2dd79 
>   src/core/meta/support/MetaConstants.h 97881a04a5579203fb745c6c6d5b1c13c9126055 
>   src/core/meta/support/MetaKeys.h 3c97cc9a3da422059dac880a39b7cf9a58887e18 
>   src/core/meta/support/MetaUtility.h 66dce9203b883f8b31d3f54d2170390863c51d53 
>   src/core/playlists/Playlist.h b4e4f404a2c07d6dc66e0cb1c4f8e31f12bd10a3 
>   src/core/playlists/PlaylistFormat.h 21baac68ec359e4dd38a28af05cec8892f4603a9 
>   src/core/playlists/PlaylistProvider.h 365792de089a9263fb80591ecffbe695cfc56bed 
>   src/core/podcasts/PodcastMeta.h cacc9948709241b8b43df5c01e95f9756d37476b 
>   src/core/support/Amarok.h 9c6f37ac78796f60c57e1f393ace47f38ce68d41 
>   src/core/support/Components.h d8cdd3ff84fecb92882b60d47fcfb115b7896b04 
>   src/core/support/Debug.h 085d033a3d5a7f06b7cd64b82519de8ed9d927fe 
>   src/core/support/PluginFactory.h a16259aad84ed7da3fb396bc9fd6adf9b40c99cc 
>   src/core/support/SmartPointerList.h 37d0bab7a0d9c3afd65a10a6ba895cda07e752b5 
>   src/core/transcoding/TranscodingConfiguration.h d0738fb4ac01b77db252b2cc13a82b4e2c8bb9e7 
>   src/core/transcoding/TranscodingController.h 5e313e45cc303832bfdf34ef4517687ea4aab960 
>   src/core/transcoding/TranscodingFormat.h ce9ed198d8ea20c5f5e194285eab6513a970b163 
>   src/core/transcoding/TranscodingProperty.h 9cc10aa17a665ac9208baae0543936efca772be3 
>   src/covermanager/CoverFetchingActions.h a3d2112477a091c02ce0269a80afaca1865c4af5 
>   src/covermanager/CoverViewDialog.h 41478911d52ecfca0a6624f8fea40dd1523cd024 
>   src/dbus/mpris1/PlayerHandler.h 8f7a77fb2fb41fef21afb895be29b3d42b27b3b3 
>   src/dialogs/TrackOrganizer.h 8c4ab84d2d9bf0c35c4e000169c10c61d39a2dee 
>   src/playlist/PlaylistActions.h e8e541be8d60e87156f716ee6efd10ed948fd6cb 
>   src/playlist/PlaylistController.h 368b89e6707582c50d8a40c38a6dd53d85ea2977 
>   src/playlist/PlaylistModel.h bf2052c74cf5401c5cc0fff8951662be0fabc591 
>   src/scriptengine/MetaTypeExporter.h 334dc1e60bcbbe5301db47f2f0fbd57a83c05a79 
>   src/services/gpodder/CMakeLists.txt f2cca2e78d65580deb6cf5f3ce26ac2d05089fd5 
>   tests/core-impl/collections/db/sql/CMakeLists.txt bf387af80ae541d464ec1090e153e5b35ec8bc24 
>   tests/core-impl/collections/db/sql/TestSqlScanManager.cpp 26b6943d816cc6045eafc6a83f1d00f7e309a99e 
>   utilities/collectionscanner/CMakeLists.txt f3d9f40dd97a793cd184384b962d45e87539c7a8 
>   utilities/collectionscanner/CollectionScanner.h de9309ce7ead5553004996e320d73fbb5d92058a 
>   utilities/collectionscanner/CollectionScanner.cpp bac3e59ae6c460d08b51634d84570d36482c3d0c 
> 
> Diff: http://git.reviewboard.kde.org/r/109157/diff/
> 
> 
> Testing
> -------
> 
> Builds, works, tests pass
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20130226/cfb5c1ba/attachment-0001.html>


More information about the Kde-buildsystem mailing list