Re: [PATCH 05/31] gnu: extra-cmake-modules: Update to 5.24.0.

[Andreas Enge]

On Mon, Aug 01, 2016 at 08:13:16PM +0200, David Craven wrote:
> From: Hartmut Goebel <address@hidden>
> * gnu/packages/kde-frameworks.scm: Update to 5.24.0.
> Co-authored-by: David Craven <address@hidden>

Now that you have taken over the project (and will probably end up modifying
the commits), and since you will do the final commits, I would do things the
other way round: Put yourself is the author, and add a "Co-authored-by" line
for Hartmut.

> -        (uri (string-append "http://download.kde.org/stable/frameworks/";
> +        (uri (string-append "mirror://kde/stable/frameworks/"
>                              (version-major+minor version) "/"
>                              name "-" version ".tar.xz"))

All modifications should either be done in separate patches, or at least be
mentioned in the commit log. Here the changing of the URL is not necessary
for the update, but probably does not warrant a separate commit (unless you
want to do it together with the introduction of the KDE mirrors, when you
could at the same time update the existing KDE packages). So you could add
a line:
[source]: Use mirror.

> +    (native-inputs
> +     `(("qtbase" ,qtbase)))    ; For tests (needs qmake)

This would also need to be mentioned in the commit message.

> +    (arguments `(#:tests? #f)) ; FIXME: All tests pass, but install fails 
> after
> +                               ; running tests.

And hopefully, there should be a line:
[arguments]: Enable tests.
!

For the record, here is the output of the install phase:
starting phase `install'
/gnu/store/sac1fmc84cvqnrh577flhpb6arkyr922-cmake-3.3.2/bin/cmake 
-H/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/extra-cmake-modules-5.24.0 
-B/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build --check-build-system 
CMakeFiles/Makefile.cmake 0
/gnu/store/sac1fmc84cvqnrh577flhpb6arkyr922-cmake-3.3.2/bin/cmake -E 
cmake_progress_start 
/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build/CMakeFiles 
/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build/CMakeFiles/progress.marks
make -f CMakeFiles/Makefile2 all
make[1]: Entering directory 
'/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build'
make -f tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/build.make 
tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/depend
make[2]: Entering directory 
'/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build'
tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/build.make:52: 
CMakeFiles/dummy.dir/depend.make: No such file or directory
tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/build.make:55: 
CMakeFiles/dummy.dir/progress.make: No such file or directory
tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/build.make:58: 
CMakeFiles/dummy.dir/flags.make: No such file or directory
make[2]: *** No rule to make target 'CMakeFiles/dummy.dir/flags.make'.  Stop.
make[2]: Leaving directory 
'/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build'
CMakeFiles/Makefile2:208: recipe for target 
'tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/all' failed
make[1]: *** [tests/ECMToolchainAndroidTest/CMakeFiles/dummy.dir/all] Error 2
make[1]: Leaving directory 
'/tmp/guix-build-extra-cmake-modules-5.24.0.drv-0/build'
Makefile:163: recipe for target 'all' failed
make: *** [all] Error 2

To me, this looks as if the tests created additional subdirectories, which
are now traversed during the install phase. One possible solution could be
to exchange the two phases.

I often take inspiration from Arch or Gentoo. In Arch:
   
https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/extra-cmake-modules
There is a "-DBUILD_TESTING=OFF" flag, so apparently they do not test.

In Gentoo, I found this:
   
https://gitweb.gentoo.org/proj/kde.git/commit/?id=f60141c0242b72efd3e80712e2c4cd024f1cffa8
The words "broken test" are suspicious.

In Linux From Scratch, for an older version of the package:
   http://www.linuxfromscratch.org/blfs/view/7.9/kde/extra-cmake-modules.html
"This package does not come with a test suite."

Andreas