> 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@kde.org > CCMAIL: Patrick von Reth <vonr...@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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel