KDE/kdelibs/cmake/modules

[Alexander Neundorf]

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/FindLibDbus.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