skipping the kdeinit magic on windows (Re:

[Ralf Habacker]

On Sunday 06 April 2008, Alexander Neundorf wrote:
 >On Sunday 06 April 2008, Ralf Habacker wrote:
 >> Ralf Habacker schrieb:
 >> > David Faure schrieb:
 >> >> On Thursday 03 April 2008, Ralf Habacker wrote:
 >> >>> David Faure schrieb:
 >> >>>> On Wednesday 02 April 2008, Ralf Habacker wrote:
 >> >>>>> SVN commit 793038 by habacker:
 >> >>>>>
 >> >>>>> kdeinit modules are not supported on win32
 >> >>>>>
 >> >>>>>  M  +10 -6     client/CMakeLists.txt   M  +11 -5
 >> >>>>> src/CMakeLists.txt
 >> >>>>>
 >> >>>>> --- trunk/KDE/kdebase/apps/konqueror/client/CMakeLists.txt
 >> >>>>> #793037:793038
 >> >>>>> @@ -10,13 +10,17 @@
 >> >>>>>
 >> >>>>>  kde4_add_app_icon(kfmclient_SRCS
 >> >>>>> "${KDE4_ICON_INSTALL_DIR}/oxygen/*/apps/system-file-manager.png")
 >> >>>>>
 >> >>>>> -kde4_add_kdeinit_executable( kfmclient NOGUI ${kfmclient_SRCS})
 >> >>>>> +if (WIN32)
 >> >>>>> +    add_definitions(-Dkdemain=main)
 >> >>>>> +    kde4_add_executable(kfmclient ${kfmclient_SRCS})
 >> >>>>> +    target_link_libraries(kfmclient  ${KDE4_KIO_LIBS} )
 >> >>>>> +else (WIN32)
 >> >>>>> +    kde4_add_kdeinit_executable( kfmclient NOGUI 
${kfmclient_SRCS})
 >> >>>>> +    target_link_libraries(kdeinit_kfmclient  ${KDE4_KIO_LIBS} )
 >> >>>>> +    install(TARGETS kdeinit_kfmclient  DESTINATION
 >> >>>>> ${INSTALL_TARGETS_DEFAULT_ARGS} )
 >> >>>>> +    target_link_libraries( kfmclient kdeinit_kfmclient )
 >> >>>>> +endif (WIN32)
 >> >>>>
 >> >>>> Ouch. But we define kdeinit modules everywhere in the source code.
 >> >>>> Maintaining CMakeLists.txt files like this one is going to be 
horrible.
 >> >>>>
 >> >>>> How about making kde4_add_kdeinit_executable do what's right for
 >> >>>> Windows, i.e. creating a static lib
 >> >>>> kdeinit_foo with -Dkdemain-main, and then the normal stuff from the
 >> >>>> CMakeLists.txt will work:
 >
 >Or just build everything directly into the executable and keep the 
library
 >just to keep it compatible.
 >
 >...
 >> After some private discussion with Alex and Christian I have prepared a
 >> patch which adds the following platform independent cmake macros to
 >> KDE4Macros.cmake
 >>
 >> kde4_kdeinit_add_executable(target options sources)
 >> kde4_kdeinit_link_libraries(target options libraries)
 >> kde4_kdeinit_install(target options)
 >>
 >>
 >> This api makes it possible to skip the kdeinit magic on windows without
 >> any further patching of package CMakeLists.txt.  (See appended testcase
 >> for konquerors kfmclient)
 >>
 >> The appended patch contains the win32 and non win32 implementations of
 >> the above mentioned macros taken from the macro
 >> kde_add_kdeinit_executable which will be obsolate now. I've tried hard
 >> to make sure it will work on unix abe before by inspecting the macro
 >> kde_add_kdeinit_executable.
 >>
 >> Any problems with this patch ?
 >
 >I don't like that it introduces two new macros.
 >The problem with the very first patch was that thetwo if()-branches 
contained
 >significantly different code, which would be very hard to remember.
 >I would prefer the following which still has an if(), but it's a 
simple one:
 >
 >kde4_add_kdeinit_executable( kfmclient NOGUI ${kfmclient_SRCS})
 >target_link_libraries(kdeinit_kfmclient  ${KDE4_KIO_LIBS} )
 >
 >install(TARGETS kfmclient ${INSTALL_TARGETS_DEFAULT_ARGS} )
 >if(NOT WIN32)
 >   install(TARGETS kdeinit_kfmclient ${INSTALL_TARGETS_DEFAULT_ARGS} )
 >endif(NOT WIN32)
 >
 >
 >This is only one if() and it's an if() with easy logic (exclude one 
lib from
 >installing), and if some linux developer forgets to put the install() 
into an
 >if()-block, also no big problem then: then you will get a small 
unnecessary
 >library installed on Windows (not nice, but it doesn't break anything, 
and it
 >should be really tiny).

 >I really think you should try to do something in your installer-tools 
so such
 >accidentally installed libs are excluded from the install package.


I do not understand some things: There is a complete platform 
independent solution which looks nice to the package developer, which 
removes the if(...) issues in the CMakeLists.txt for ever
and it is rejected against a low level platform specific solution

kde4_add_kdeinit_executable( kfmclient NOGUI ${kfmclient_SRCS})
target_link_libraries(kdeinit_kfmclient  ${KDE4_KIO_LIBS} )
install(TARGETS kfmclient ${INSTALL_TARGETS_DEFAULT_ARGS} )
if(NOT WIN32)
   install(TARGETS kdeinit_kfmclient ${INSTALL_TARGETS_DEFAULT_ARGS} )
endif(NOT WIN32)

which is
- only full understandable by inspecting KDE4Macros.cmake,
- forces unix developer to know windows install behavior to setup the 
the build system correctly,
- needs ongoing patch effort of kde windows developer in case linux devs 
forgot to think about windows install behavior and
- needs patching of an outside tool

only to avoid introducing two macros, which are designed to use the 
known call sequence(add_... _..link_libraries, __install) ????

Sorry, please take this not personally, but I do not think that this is 
the right way :-P - I believe a build system should make it as much as 
easy for package developers and to encapsulate platform issue as much as 
possible into some internal macros and if it is required to introduce 
new macros then it should be.

Anyway, appended is an updated patch - it needed some fixes and now does 
not requires an additional .cpp dummy file. Feel free to apply it. I 
will change the client code where required.

Bye
Ralf

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: kde4macros-kdeinit.patch
Url: http://mail.kde.org/pipermail/kde-buildsystem/attachments/20080407/4b6b69a8/attachment.ksh