Review Request 109157: Amarok buildsystem cleanup preview

Matěj Laitl matej at laitl.cz
Mon Mar 4 12:22:09 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.
>     
>
> 
> Matěj Laitl 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).
>     
>     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
> 
> Alexander Neundorf wrote:
>     Not portable: --whole-archive works with gcc on Linux, it does not work everywhere, at least not with the Microsoft compiler and with the Sun compilers, don't know about others.
>     You should at least test that those flags are available when using them.
>     For propagating flags etc., please ask on the cmake mailing list, Stephen has put a lof ot work into propagating such things, but I am not sure about OBJECT libraries.
>     
>     In doubt, I would just not use those intermediate libraries.
>     
>     Beside that, whether you agreed on something in Randa or not, it doesn't make sense to agree on something which is not supported by the tool you are using.
>

> Not portable: --whole-archive works with gcc on Linux, it does not work everywhere, at least not with the Microsoft compiler and with the Sun compilers, don't know about others.
You should at least test that those flags are available when using them.

You're right, I forgot about the --whole-archive thingie.

> Beside that, whether you agreed on something in Randa or not, it doesn't make sense to agree on something which is not supported by the tool you are using.

Well, I suppose you wouldn't disagree that CMake supports per-directory CMakeLists.txt even when building big shared libraries (amaroklib *is* big: composed of hundreds of object files). This intermediate library thing was just a first attempt to implement it and thanks to your review it turned out infeasible.

Anyways, I'll commit the first 2 non-controversial patches and try to think of a better way to do the per-dir CMakeLists.txt, exploring object libraries and how to propagate desired flags to them. Thanks for review and opinions to all participating.


- 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/amarok-devel/attachments/20130304/b9d35d55/attachment-0001.html>

More information about the Amarok-devel mailing list