Bug 529

please feel free to make any use of

http://www.ojuba.org/downloads/updates/2/SRPMS/mlt-0.3.1-0.2.svn1180.oj2.src.rpm
http://www.ojuba.org/downloads/updates/2/SRPMS/mlt++-0.3.0-1.oj2.src.rpm
http://www.ojuba.org/downloads/updates/2/SRPMS/kdenlive-0.7-1.oj2.src.rpm

I'm not suggesting that they are better, it's just those I packed for our Fedora 10 based distro

(In reply to comment #1)
> please feel free to make any use of
> 
> 
> I'm not suggesting that they are better, it's just those I packed for our
> Fedora 10 based distro


Your .spec files are based (inherited) from yandex  .spec files (also written for F10). I was using these (yandex's) .spec files for my builds, but I improved its with new versions of sources (MLT 0.36 and Kdenlive 0.7.2.1), resolved Rpaths in Kdenlive and added other  improvements.
But thanks for help!

bye,
Zarko

Hmm, when validate whestleypreview.desktop file, I'm getting this issues:

westleypreview.desktop: error: file contains key "CacheThumbnail" in group "Desktop Entry", but keys extending the format should start with "X-"
westleypreview.desktop: error: file contains key "IgnoreMaximumSize" in group "Desktop Entry", but keys extending the format should start with "X-"

westleypreview.desktop: error: key "MimeType" is present in group "Desktop Entry", but the type is "Service" while this key is only valid for type "Application"


But, if I ignore them then this previewer works OK in KDE...

I must validate .desktop files in packages, so how I can handle this type of issues? (maybe this isn't a issue, maybe it is a KDE specificum ??)

Zarko

will they work in KDE as expected if X- is added(X-CacheThumbnail, X-IgnoreMaximumSize or X-KDE-CacheThumbnail, X-KDE-IgnoreMaximumSize)

http://standards.freedesktop.org/desktop-entry-spec/latest/ar01s08.html

in the next link
http://standards.freedesktop.org/desktop-entry-spec/latest/apb.html
some extensions that do not have X-KDE prefix:

> For historical reasons KDE is using some KDE-specific extensions that are currently not prefixed by a X-KDE- prefix. 

I'd suggest just ignoring those complaints and skipping the validation, that .desktop file is installed into a KDE-specific directory, it is never used outside of KDE and the file works as is in KDE.

FWIW, KDE's own thumbnailers also have CacheThumbnail=, IgnoreMaximumSize= and MimeType= entries.

You might want to try replacing your cmake line with the %cmake macro. It includes the necessary cmake flags, and it will define environment variables for Fedora-specific build flags that make should use.

Actually, this is a KDE 4 application, so it should be using %cmake_kde4.

Please also use our _kde4_* macros in the file list where possible, in particular you should use %{_kde4_appsdir} instead of %{_datadir}/kde4/apps. The full list of our path macros can be found in the kde-filesystem specfile:
http://cvs.fedoraproject.org/viewvc/rpms/kde-filesystem/devel/kde-filesystem.spec?revision=1.38&view=markup

(In reply to comment #8)
> Actually, this is a KDE 4 application, so it should be using %cmake_kde4.
> 
> Please also use our _kde4_* macros in the file list where possible, in
> particular you should use %{_kde4_appsdir} instead of %{_datadir}/kde4/apps.


I just converted spec file to use kde4 macros.

http://wiki.open.hr/~zpintar/fedora10/RPMFusion/SPECS/kdenlive.spec
Source:
http://wiki.open.hr/~zpintar/fedora10/RPMFusion/SRPMS/kdenlive-0.7.2.1-2.fc10.src.rpm

- New Kdenlive version 0.7.3 
- Tested for upcoming Fedora 11
- Rpmlint - clean!

Spec:
http://wiki.open.hr/~zpintar/fedora-11/RPMFusion/SPECS/kdenlive.spec

Source:
http://wiki.open.hr/~zpintar/fedora-11/RPMFusion/SRPMS/kdenlive-0.7.3-1.fc11.src.rpm

 

small item, but you can drop
-DCMAKE_INSTALL_PREFIX=%{_prefix} -DCMAKE_SKIP_RPATH=true
now that you're using %{cmake_kde4} macro

(In reply to comment #11)
> small item, but you can drop
> -DCMAKE_INSTALL_PREFIX=%{_prefix} -DCMAKE_SKIP_RPATH=true
> now that you're using %{cmake_kde4} macro
> 

And RPATHs, too?

yep, try
rpm --eval "%{cmake_kde4}"
to see all that it does and includes

(In reply to comment #11)
> small item, but you can drop
> -DCMAKE_INSTALL_PREFIX=%{_prefix} -DCMAKE_SKIP_RPATH=true
> now that you're using %{cmake_kde4} macro
> 

Resolved...

http://wiki.open.hr/~zpintar/fedora-11/RPMFusion/SPECS/kdenlive.spec

Source:
http://wiki.open.hr/~zpintar/fedora-11/RPMFusion/SRPMS/kdenlive-0.7.3-2.fc11.src.rpm

Here are my notes for this package:

- rpmlint is silent

! Please make the description span 80 columns

! effects/README can go to %doc

* BuildRequires:  mlt++-devel must be removed

* Also these BR's seem unnecessary to me. Package builds without them:
   BuildRequires:  qt-devel
   BuildRequires:  kdemultimedia-devel
   BuildRequires:  kdebase-devel
   BuildRequires:  kdebindings-devel

* I don't think that we need this explicit Requires:
   Requires:       kdebase-runtime

* The file src/initeffects.cpp contains references to
    "/usr/lib/ladspa"
    "/usr/local/lib/ladspa"
These should be sed'ed or patched to work on 64bit systems

! I think the license should be just GPLv2+. The only LGPL bit is src/regiongrabber* and these get compiled into a final binary together with GPL stuff, and hence making the package GPL.

* Afaict, the package does not install anything in the dynamic linker's default paths. So you don't need to call /sbin/ldconfig in %post and %postun.

* Each package must consistently use macros, as described in the macros section of Packaging Guidelines. Please use the %{name} macro consistently.

* We prefer %defattr(-,root,root,-)

* Fedora specific optflags are not honored. Please fix this. Ref: http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
You can see the Fedora specific optflags via
   rpm -E %optflags

! desktop-file-validate should go to %check

* desktop entry has a MimeType key. So we must obey the scriptlets guideline:
   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database

Removing the "sponsorship request" blocker since Zarko is sponsored in Fedora.

(In reply to comment #15)
> 
> * The file src/initeffects.cpp contains references to
>     "/usr/lib/ladspa"
>     "/usr/local/lib/ladspa"
> These should be sed'ed or patched to work on 64bit systems
> 

I sent this issue to upstream, because next release of Kdenlive (0.7.4) which is compatible with new MLT 0.4.0 will be out about next week. So, I think that is wise to wait for this release.

Until that, I'll use trunk to resolve issues notified in your review.

(In reply to comment #15)
> 
> * Fedora specific optflags are not honored. Please fix this. Ref:
> http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
> You can see the Fedora specific optflags via
>    rpm -E %optflags
>

Is this mean that I must use this:

make %{optflags}

instead of this:

make %{?_smp_mflags}

(In reply to comment #18)
> 
> Is this mean that I must use this:
> 
> make %{optflags}
> 
> instead of this:
> 
> make %{?_smp_mflags}
> 

Ah, I found...

You mean this:

make %{?_smp_mflags} CFLAGS="%{optflags}"

BTW, I tried the latest subversion of kdenlive, and it's still do not work with MLT 0.4.0, so only what we can is waiting for release....

(In reply to comment #19)
> (In reply to comment #18)
> > 
> > Is this mean that I must use this:
> > 
> > make %{optflags}
> > 
> > instead of this:
> > 
> > make %{?_smp_mflags}
> > 
> 
> Ah, I found...
> 
> You mean this:
> 
> make %{?_smp_mflags} CFLAGS="%{optflags}"
> 

No, no... It is more complicated than just that.

When the C/C++/Fortran compiler is invoked, you have to make sure that the Fedora specific optflags are passed to the compiler. How this is going to happen depends on the build system (autoconf/automake, cmake, scons, waf, ...). 

In this case the package uses cmake. Fedora's %cmake or %cmake_kde4 macros define the Fedora specific optflags for you, and normally, you don't have to change anything. Just compare the outputs of
   rpm -E %optflags
   rpm -E %cmake_kde4
and you will see that %cmake_kde4 is actually enforcing the %optflags.

However, life is not perfect and sometimes the upstreams claim that they know the best flags that any system on the face of the planet should use.

I guess that the flags defined by your %cmake_kde4 macro are being overriden somewhere. You need to find out where this happens and prevent the overriding.

(In reply to comment #21)

> 
> I guess that the flags defined by your %cmake_kde4 macro are being overriden
> somewhere. You need to find out where this happens and prevent the overriding.
> 

Hmm, I found something, but I don't know is it that?

Please, look:

In the file called build.make:
=========================================================================
# Include the compile flags for this target's objects.
include src/cmake_bindir/CMakeFiles/kdenlive.dir/flags.make
-------------------------------------------------------------------------
	cd /home/zarko/src/kdenlive-0.7.4/src/cmake_bindir && /usr/lib/ccache/c++   $(CXX_DEFINES) $(CXX_FLAGS) -o CMakeFiles/kdenlive.dir/kdenlive_automoc.o -c /home/zarko/src/kdenlive-0.7.4/src/cmake_bindir/kdenlive_automoc.cpp
=========================================================================
Compiler, obviously takes flags from file flags.make:

=========================================================================
# compile CXX with /usr/lib/ccache/c++
CXX_FLAGS = -DMLT_PREFIX=\""/usr"\" -O2 -g -I/home/zarko/src/kdenlive-0.7.4/src/cmake_bindir -I/home/zarko/src/kdenlive-0.7.4/src -I/home/zarko/src/kdenlive-0.7.4 -I/home/zarko/src/kdenlive-0.7.4/src/widgets -I/usr/include/kde4 -I/usr/include/kde4/KDE -I/usr/include/phonon -I/usr/include/QtXmlPatterns -I/usr/include/QtWebKit -I/usr/include/QtHelp -I/usr/include/QtAssistant -I/usr/include/QtDBus -I/usr/include/QtTest -I/usr/include/QtUiTools -I/usr/include/QtScript -I/usr/include/QtSvg -I/usr/include/QtXml -I/usr/include/QtSql -I/usr/include/QtOpenGL -I/usr/include/QtNetwork -I/usr/include/QtDesigner -I/usr/include/Qt3Support -I/usr/include/QtGui -I/usr/include/QtCore -I/usr/include/Qt -I/usr/lib/qt4/mkspecs/default -I/usr/include/mlt   -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
=========================================================================

If it's that, then I must probably override these flags with optflags, by the patch? 

> > 
> > I guess that the flags defined by your %cmake_kde4 macro are being overriden
> > somewhere. You need to find out where this happens and prevent the overriding.
> > 
> 
And better, I found this (file generated by Cmake) CmakeCache.txt
Stay this:

==================================================================
# This is the CMakeCache file.
# For build in directory: /home/zarko/src/kdenlive-0.7.4
# It was generated by CMake: /usr/bin/cmake
# You can edit this file to change values found and used by cmake.
# If you do not want to change any of the values, simply exit the editor.
# If you do want to change a value, simply edit, save, and exit the editor.
# The syntax for the file is as follows:
# KEY:TYPE=VALUE
# KEY is the name of a variable in the cache.
# TYPE is a hint to GUI's for the type of VALUE, DO NOT EDIT TYPE!.
# VALUE is the current value for the KEY.
==================================================================

and this is interesting for flags:

==================================================================
//Flags used by the compiler during all build types.
CMAKE_CXX_FLAGS:STRING=
==================================================================
So, I concluded, if we define that string with optflags, than make routine will be override with these parameters.

Or, adding this line in CmakeLIst.txt
SET(CMAKE_CXX_FLAGS " flags goes here")

What are you thinking about that?

> In the file called build.make:
> Compiler, obviously takes flags from file flags.make:

These are generated files. Please look in the CMakeLists.txt files instead.

> So, I concluded, if we define that string with optflags, than make routine will
> be override with these parameters.

The program must actually be overriding CMAKE_CXX_FLAGS somewhere in their CMakeLists.txt, you just need to remove that. CMake normally picks up the CXXFLAGS from the environment (which are set by %cmake_kde4) automatically.

The offender is FindLIBMLT.cmake, it contains:
SET(CMAKE_CXX_FLAGS -DMLT_PREFIX=\\\"\"${MLT_PATH}\"\\\")
That should be:
ADD_DEFINITIONS(-DMLT_PREFIX=\\\"\"${MLT_PATH}\"\\\")
(and the quoting could be simplified with current CMake versions by setting the CMake policy which fixes quoting to NEW, but that's not important).

(In reply to comment #25)
> The offender is FindLIBMLT.cmake, it contains:
> SET(CMAKE_CXX_FLAGS -DMLT_PREFIX=\\\"\"${MLT_PATH}\"\\\")
> That should be:
> ADD_DEFINITIONS(-DMLT_PREFIX=\\\"\"${MLT_PATH}\"\\\")
> (and the quoting could be simplified with current CMake versions by setting the
> CMake policy which fixes quoting to NEW, but that's not important).
> 

Thanks!

I'll report this to upstream (to avoid patching ;) )

Created attachment 190 [details]
spec file for the upcoming 0.7.4 releese (based on svn rev 3426)

(In reply to comment #26)
> (In reply to comment #25)
> > The offender is FindLIBMLT.cmake, it contains:
> > SET(CMAKE_CXX_FLAGS -DMLT_PREFIX=\\\"\"${MLT_PATH}\"\\\")
> > That should be:
> > ADD_DEFINITIONS(-DMLT_PREFIX=\\\"\"${MLT_PATH}\"\\\")
> > (and the quoting could be simplified with current CMake versions by setting the
> > CMake policy which fixes quoting to NEW, but that's not important).
> > 
> 
> Thanks!
> 
> I'll report this to upstream (to avoid patching ;) )
> 

i'm sure you have it already at your hands, but for the uncertain case that it's not so, i'll send you my spec file for the soon to be released version 0.7.4 of kdenlive. since the afore mentioned fixes are already upstream (at least in svn rev 3426, though this rev contains some other serious bugs) i tried it - and guess what it worked (or at least most of the time)

used your 0.7.3-2 spec as base, just added the "buildrequire mlt-devel >= 0.4.0-3" stuff, since from my point of view, you won't have much fun with building against older versions of mlt.
out of the same idea i added the (at least after my opinion) missing "require mlt >= 0.4.03" part, since installing the package without having mlt installed won't make much fun either.
and at last a minor correction, had to change the %{_kde4_datadir}/applications/kde/%{name}.desktop parts to %{_kde4_datadir}/applications/kde4/%{name}.desktop

mfg,
martin 

(In reply to comment #28)

> 
> i'm sure you have it already at your hands, but for the uncertain case that
> it's not so, i'll send you my spec file for the soon to be released version
> 0.7.4 of kdenlive. since the afore mentioned fixes are already upstream (at
> least in svn rev 3426, though this rev contains some other serious bugs) i
> tried it - and guess what it worked (or at least most of the time)


Thanks, but I'm just working on new spec file, and it is done. (With some other staff added)

(In reply to comment #29)
> (In reply to comment #28)
> 
> > 
> > i'm sure you have it already at your hands, but for the uncertain case that
> > it's not so, i'll send you my spec file for the soon to be released version
> > 0.7.4 of kdenlive. since the afore mentioned fixes are already upstream (at
> > least in svn rev 3426, though this rev contains some other serious bugs) i
> > tried it - and guess what it worked (or at least most of the time)
> 
> 
> Thanks, but I'm just working on new spec file, and it is done. (With some other
> staff added)
> 

if you refer to http://wiki.open.hr/~zpintar/fedora-11/SPECS/kdenlive.spec which i just this moment tried to rebuild i have to tell you that it doesn't work (for me)

i get the following complaint:

+ desktop-file-validate /home/seldon/rpmbuild/BUILDROOT/kdenlive-0.7.4-1.fc11.i386//usr/share/applications/kde/kdenlive.desktop
/home/seldon/rpmbuild/BUILDROOT/kdenlive-0.7.4-1.fc11.i386//usr/share/applications/kde/kdenlive.desktop: file does not exist

i already have addressed this in my spec file (change .../kde/... to .../kde4/...)

mfg,
martin

(In reply to comment #30)
> 
> if you refer to http://wiki.open.hr/~zpintar/fedora-11/SPECS/kdenlive.spec
> which i just this moment tried to rebuild i have to tell you that it doesn't
> work (for me)

No, I thought on my spec file at local machine which I which I have not put here, yet because I'm waiting for stable release of kdenlive 0.7.4.

BTW, yours spec file is based on my old one, which has not been updated by the last review notes (my local it is! :) )

(In reply to comment #28)
> 
> used your 0.7.3-2 spec as base, just added the "buildrequire mlt-devel >=
> 0.4.0-3" stuff, since from my point of view, you won't have much fun with
> building against older versions of mlt.
> out of the same idea i added the (at least after my opinion) missing "require
> mlt >= 0.4.03" part, since installing the package without having mlt installed
> won't make much fun either.

I don't think this is necessary. The lowest mlt that is published in Fedora is 0.4.0-3. There is no easy way of building an RPM against a lower version.

> and at last a minor correction, had to change the
> %{_kde4_datadir}/applications/kde/%{name}.desktop parts to
> %{_kde4_datadir}/applications/kde4/%{name}.desktop
> 

Good catch. Again, I wonder why the package builds fine on my computer.

(In reply to comment #32)
> (In reply to comment #28)
> 
> Good catch. Again, I wonder why the package builds fine on my computer.
> 

BTW, Orcan do you want that I publish my new spec, or you want (like me) waiting for release (few days)

(In reply to comment #33)
> 
> BTW, Orcan do you want that I publish my new spec, or you want (like me)
> waiting for release (few days)
> 

However you wish. But I can't promise to look at it until the weekend.

(In reply to comment #34)
> 
> However you wish. But I can't promise to look at it until the weekend.
> 

In that case, lets wait! :)

I just finished kdenlive package of current stable version 0.7.4.

Also, I accepted all the latest review's suggestions, so please take a look:

http://wiki.open.hr/~zpintar/fedora-11/RPMFusion/SPECS/kdenlive.spec
http://wiki.open.hr/~zpintar/fedora-11/RPMFusion/SRPMS/kdenlive-0.7.4-2.fc10.src.rpm

Mock passed and rpmlint is clean... 

Thanks! All the issues are addressed. The one thing that needs to be done is: 
    %find_lang %{name}
should go to the end of the %install section. Currently it is in the %check section, which is a wrong place to put it.

(In reply to comment #37)
> Thanks! All the issues are addressed. The one thing that needs to be done is: 
>     %find_lang %{name}
> should go to the end of the %install section. Currently it is in the %check
> section, which is a wrong place to put it.
> 

Done.

Spec and Source links are the same...

Thanks!

----------------------------------------------------------------------
This package (kdenlive) is finally APPROVED by oget for rpmfusion-free
----------------------------------------------------------------------

Wow, it took some time. (9 months?)

(In reply to comment #39)

> 
> Wow, it took some time. (9 months?)
> 


Hmm, I took this package at 2009-04-08, what happened before, I do not know :)

Thanks for review!

Package CVS request
======================
Package Name: kdenlive
Short Description: Non-linear video editor
Owners: grof
Branches: F10 F11
InitialCC: grof
----------------------
License tag: free

(In reply to comment #41)
> Package CVS request

Done

> Branches: F10 F11

Reminder: Build either for devel/ or for F-11, not both, otherwise the package will make it into the release repos and the updates repos (see recent mail to the list for details)

I'd suggest you build it only for the F-11 branch now (and hence it gets added to the updates repos) and for devel/ later once rawhide opens again

(In reply to comment #42)
> 
> I'd suggest you build it only for the F-11 branch now (and hence it gets added
> to the updates repos) and for devel/ later once rawhide opens again
> 

I try, and import procedure going fine until the end, than I got this error message:

Log message unchanged or not specified
a)bort, c)ontinue, e)dit, !)reuse this message unchanged for remaining dirs
Action: (continue)
? F-11/import.log
**** Access denied: grof is not in ACL for rpms/kdenlive/F-11
cvs commit: Pre-commit check failed
cvs [commit aborted]: correct above errors first!


Why? What is wrong? 

(In reply to comment #43)
> Why? What is wrong? 

Wrong ACLs on the server side. fixed

Can we have the review closed ?

(In reply to comment #45)
> Can we have the review closed ?
> 

Actually, yes.