KDE/kdelibs/cmake/modules

Sun Sep 6 15:17:24 CEST 2009

Diff updated. This one should be a good commit candidate, all concerns should 
have gone away.

In data domenica 06 settembre 2009 14:40:40, Alexander Neundorf ha scritto:
: > On Sunday 06 September 2009, Dario Freddi wrote:
> > Here it goes, new diff for findkde4internals and kde4macros
> 
> There is some inconsistency in the naming:
> KDE4_AUTH_INSTALL_ACTIONS()
> vs.
> KDE4_INSTALL_AUTH_HELPER_FILES()
> 
> How about KDE4_INSTALL_AUTH_ACTIONS() ?
> 
> From the documentation:
> 
> # This macro registers an action file for applications using KAuth.
> 
> Please explain this in more detail in the docs. It is not clear
> that "register" means to install the files at "make install" time to some
> location. Register with what ?
> It might also mention how the generated file will be named and where it
>  will be installed.
> 
> ...
> #   This macro adds the needed files for an helper for
> 
> I think it would be more clear if you use "helper executable" instead of
> just "helper" in all places where appropriate.
> 
> > (and the full polkitqt file :D) some comments below
> 
> We're almost there :-) Three issues left.
> 
> --------8<--------------8<-------
> #It really looks like find_path is not working, this one at least makes
>  things work
> set(POLKITQT_INCLUDE_DIR ${PC_POLKITQT_INCLUDEDIR}/PolicyKit/polkit-qt
>     ${PC_POLKITQT_INCLUDEDIR}/PolicyKit/)
> --------8<--------------8<-------
> 
> I mean, you knew I would have an issue with this, right ?
> So, in which directory is auth.h on your system ?
> Which value does PC_POLKITQT_INCLUDEDIR have on your system ?
> 
> I think the call should look like this:
> 
> find_path( POLKITQT_INCLUDE_DIR
>     NAMES polkit-qt/auth.h
>     PATH_SUFFIXES PolicyKit
>     HINTS ${PC_POLKITQT_INCLUDEDIR}
>     )
> 
> (the PATH_SUFFIXES appends the given subdir to each of the search dirs,
>  e.g. instead for /usr/include/plokit-qt/auth.h it will look
> in /usr/include/PolicyKit/plokit-qt/auth.h)
> 
> The second one:
> macro(dbus_add_activation_system_service _sources)
> 
> This should most probably go into FindDBus.cmake (which is right now in
> kdebase/workspace/cmake/modules/ )
> Or here's another one:
> http://www.opensync.org/browser/branches/3rd-party-cmake-modules/modules/Fi
> ndLibDbus.cmake?rev=3679 or here:
> http://anonsvn.wireshark.org/viewvc/trunk/cmake/modules/FindDBUS.cmake?view
> =markup&pathrev=19672
> 
> and please find out where that other dbus-related macro comes from which
>  you mentioned.
> 
> 3rd issue:
> please mark the result variables from the find_xxx() calls as ADVANCED (no
> blocker)
> 
> > In data sabato 05 settembre 2009 21:53:36, Alexander Neundorf ha scritto:
> > [snip]
> >
> > > Yes, please.
> > > This would add another macro which creates an executable, but without
> > > providing real benefit. I mean, every KDE developer should know
> > >
> > > kde4_add_executable()
> > > target_link_libraries()
> > > install(TARGETS)
> > >
> > > So there's nothing new here.
> > > Now to help with getting that stuff installed correctly we can provide
> > > a macro which helps with that. Which means there is 1 new thing to
> > > learn but the other code stays understandable for everybody else who
> > > don't know about KAuth.
> >
> > Done, but with a modification. The function (ah, tested and function
> > works nice, btw) also accepts the helper target and installs it. This for
> > a variety of reasons:
> >
> > 1. The macro has to know the target name since one of the configured
> > files needs it
> 
> This one is ok.
> 
> > 2. The helper has to be installed in libexec_install_dir, so we also make
> > sure that it gets installed into the correct location.
> 
> Also all other files have to be installed to the correct locations, that's
>  not different for this one.
> Please remove the install(TARGETS) from the function.
> We had long discussions about similar things in the past here on the list.
>  I don't want this to be hidden in a macro/function.
> 
> > The cmake code client-side would then be:
> >
> > kde4_add_executable(kcmremotewidgetshelper
> > private/remotewidgetshelper.cpp)
> > target_link_libraries(kcmremotewidgetshelper kdecore)
> >
> > kde4_install_auth_helper_files(kcmremotewidgetshelper
> > org.kde.kcontrol.kcmremotewidgets root)
> >
> > kde4_auth_install_actions(org.kde.kcontrol.kcmremotewidgets
> > kcm_remotewidgets.actions)
> >
> > I hope this makes up as a good compromise
> >
> > > Adding KDE4_ADD_AUTH_HELPER_EXECUTABLE() would also be 1 new thing to
> > > learn and it doesn't show that it will also install and link to
> > > kdecore. So the name would have to be something like
> > > KDE4_ADD_AUTH_HELPER_EXECUTABLE_LINK_TO_KDECORE_AND_INSTALL()
> > > I guess we don't want that ;-)
> >
> > Hehe, right :D
> >
> > > > > Macro KDE4_AUTH_REGISTER_ACTIONS:
> > > > >
> > > > > Maybe an "install" in the name of the macro would be a good idea,
> > > > > since it installs stuff.
> > > >
> > > > Uhm, you are right. KDE4_AUTH_INSTALL_ACTIONS? But it seems confusing
> > > > to me. REGISTER_AND_INSTALL is making it too long IMHO. Any other
> > > > suggestions?
> > >
> > > I don't have a problem with long names.
> > >
> > > Or just KDE4_INSTALL_AUTH_ACTIONS() ?
> >
> > Done this way :)
> 
> Sure ?
> It's still KDE4_AUTH_INSTALL_ACTIONS() in the patch.
> I would also suggest to turn it into a function() instead of a macro() (but
> that's no blocker).
> Do you know whether this macro also works correctly with DESTDIR set on
> Apple ? (also no blocker)
> 
> Alex
> 

-- 
-------------------

Dario Freddi
KDE Developer
GPG Key Signature: 511A9A3B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cmake_done_right_revenge_2.diff
Type: text/x-patch
Size: 8652 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20090906/bf7b0260/attachment-0001.diff 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-buildsystem/attachments/20090906/bf7b0260/attachment-0001.sig 