Since you are a nice guy in infrastructure team, would you please using fedorapeople instead of foolbox for SPEC/SRPM storage?

(In reply to Christopher Meng from comment #1)
> Since you are a nice guy in infrastructure team, would you please using
> fedorapeople instead of foolbox for SPEC/SRPM storage?

ok. I'll try to use my feodrapeople space.

Here are the SPEC/SRPM files hosted on fedorapeople:

SPEC url: http://jam3s.fedorapeople.org/yarock.spec
SRPM url: http://jam3s.fedorapeople.org/yarock-0.9.64-1.fc20.src.rpm

Quick comments:

 o remove empty lines
 o change 
   A lightweight, beautiful music player written in C++/Qt 
     to
   A lightweight, beautiful music player
   
  and ditto:

    Yarock is a music player designed to provide a clean,  
    simple and beautiful music collection based on album coverart.
 
 o cmake -DCMAKE_INSTALL_PREFIX:PATH=/usr ..
   use %cmake here.

 o remove 
   rm -rf %{buildroot} 
   in %install

 o remove:
   %clean
   rm -rf %{buildroot}

 o remove
   %defattr(-,root,root,-)

 o be more specific here:
   %{_datadir}/icons/hicolor/*/*/*

 
please provide koji scratch build.

@Terje Røsten: I've applied all the changes you suggested except one. I had already tried %cmake and it didn't work for me. In fact using %cmake would lead to many errors during "make". I played with it a lot but it didn't work so I reverted to plain cmake. To make things even more complicated, the option -DCMAKE_INSTALL_PREFIX:PATH=/usr actually does NOT work here (but I still prefer to keep it just to remind the reviewers that I'm aware of it). In order to make the package to be installed in /usr/bin (instead of the default /usr/local/bin), I had to manually patch the cmakelists file (see the patch). It's a dirty trick but the only solution ( I had a discussion on cmake irc channel, made their head spin). Here is the updated spec: 
http://jam3s.fedorapeople.org/yarock.spec

Here is the link to koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6204882

james, can you document what some of those problems were when you tried using %cmake macro?

Some initial comments

1.  manual dependencies:

Requires:       qt 
Requires:       taglib
Requires:       qjson
Requires:       libechonest
Requires:       phonon
Requires:       sqlite

rpm should pick those up automatically (ie, if those libraries are linked into the executbable).


2.  icons
add icons scriptlets, 
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
and dependency on hicolor-icon-theme:
Requires: hicolor-icon-theme

fwiw, I disagree with a prior comment, I consider using
%{_datadir}/icons/hicolor/*/*/*
glob in %files fine.  In fact, it's simpler and more readable for simple package like this.  

(I'd welcome rationale/justification for alternative interpretations, or even references to packaging guidelines that recommend otherwise).

Use %{_prefix} for /usr.

Important: proper updating of changelog, describe every change you did, impossible to review without proper changelog.

rex: a compromise: %{_datadir}/icons/hicolor/*/apps/application-x-yarock.png :-)

Important: proper updating of changelog, describe every change you did, impossible to review without proper changelog.

Build is not verbose, not possible to verify build flags.

>  %{_datadir}/icons/hicolor/*/apps/application-x-yarock.png 

This is a new packager, please give them reasons/justifications to follow your advice.  While that's a little better, being one line, I still don't see any advantage to a full glob, 
%{_datadir}/icons/hicolor/*/*/*
For example, your suggestion will needlessly fail the build when/if upstream includes scalable/svg icons.

> For example, your suggestion will needlessly fail the build when/if upstream
> includes scalable/svg icons.

I want control over shipped files, changes should trigger a failed build.

Control is more important than convenience for the packager.

I've sponsored james to help co-maintain a infrastructure package. Removing the NEEDSPONSOR here.

> rpm should pick those up automatically (ie, if those libraries are linked into
> the executbable).

yes I know but better be safe than sorry ;)


> add icons scriptlets, 
> and dependency on hicolor-icon-theme:

Done. Although perhaps for those who are using KDE and other qt-based DE, this would be unnecessary.

> fwiw, I disagree with a prior comment

I wanted to discuss it with Terje Røsten but he seems to have a point. But I've seen many packages' SPECS (e.g. KDE's Konversation) in fedora that use the full glob and apparently nobody criticize them. I went through the pain of spelling out all those files so doesn't matter to argue now.

> Use %{_prefix} for /usr.
Done. 

> Important: proper updating of changelog, describe every change you did,
> impossible to review without proper changelog.

You're right, totally forget about that. fixed now.

> Build is not verbose, not possible to verify build flags.

What do you mean 'not verbose'? rpmbuild by default outputs the result in verbose mode. If you are interested in the output here you go: http://paste.fedoraproject.org/56032/13851251

> can you document what some of those problems were when you tried using %cmake macro?

Well, I have to revert to previous SPECS to reproduce the errors again (I should have used git to keep track of file versions but I forgot). However I think you can reproduce it now with just replacing cmake line with %cmake .. and see what happens. Maybe it won't show those errors with the patch in place now. Believe me I messed with %cmake .. a lot and asked for a lot of help on irc channel. The use of cmake and that patch was the last resort.

Anyway, here is the updated spec, let me know of further comments.

The updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

Updated Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6213583

> I've sponsored james to help co-maintain a infrastructure package. Removing the
> NEEDSPONSOR here.

Thank you Kevin. much appreciated ;)

> yes I know but better be safe than sorry ;)

better to abide by the recommended guidelines:

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Explicit_Requires

"...  Packages must not contain unnecessary explicit Requires on libraries." that's a must, not a should, mind you. :)

(In reply to Rex Dieter from comment #16)
> > yes I know but better be safe than sorry ;)
> 
> better to abide by the recommended guidelines:
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Explicit_Requires
> 
> "...  Packages must not contain unnecessary explicit Requires on libraries."
> that's a must, not a should, mind you. :)

Let me explain why: 

if e.g qt gets renamed to e.g qt5 you are in trouble, while with no explicit deps you are fine.


>> Build is not verbose, not possible to verify build flags.
> What do you mean 'not verbose'? 

See e.g the line in logs:

Building CXX object src3party/qxt/CMakeFiles/qxt.dir/qxtglobal.cpp.o

There is no information here about build flags used. 

Change
make %{?_smp_mflags} -C %{_target_platform}
to
make %{?_smp_mflags} -C %{_target_platform} VERBOSE=1

to see the difference. You will then see an other problem, correct build flags
are not used. 

See:
 https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Re: verbosity

another consequence of not using %cmake macro which sets for you:
-DCMAKE_VERBOSE_MAKEFILE=ON

to see all the default options set, try:

rpm --eval "%cmake"

> better to abide by the recommended guidelines:

> if e.g qt gets renamed to e.g qt5 you are in trouble, while with no explicit
> deps you are fine.

Ok, See what you mean. fixed.


> make %{?_smp_mflags} -C %{_target_platform} VERBOSE=1

> to see the difference. You will then see an other problem, correct build flags
> are not used.

I see the difference but I have a hard time spotting any issues during the build time. The link you provided mention %{optflags}, shall I use that? What are the correct build flags? The original developer does not mention anything of that kind. I tried to use Rex's command to see what's going on in %{optflags}:

> to see all the default options set, try:
> rpm --eval "%cmake"

Are the flags indicated by (rpm --eval %{optflags} ) the right one?

updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

updated Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6214363

> Ok, See what you mean. fixed.

Thanks, great progress so far!

 
> I see the difference but I have a hard time spotting any issues during the
> build time. The link you provided mention %{optflags}, shall I use that?

Indeed, it's mandatory, build must use these flags. This is important for
security and creation for debuginfo package, abrt and retrace support etc.

 
> Are the flags indicated by (rpm --eval %{optflags} ) the right one?

Yes. 

Tip: if you get %cmake working all this will work out of box.

> Indeed, it's mandatory, build must use these flags. This is important for
> security and creation for debuginfo package, abrt and retrace support etc.

Done. I've added %{optflags} to CFLAGS and CPPFLAGS.

> Tip: if you get %cmake working all this will work out of box.

I wish it would work but It seems that some of the default options indicated in (rpm --eval %cmake) is giving "make" a hard time. It's been a long time that I've given up on %cmake.

Please check the results:

updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

updated koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6214507

I guess I'm just annoyed enough to figure out why %cmake fails. :)

Ah, pretty easy to spot (2 problems actually):

1.  it bundles qtsingleapplication
2.  this bundled build fails when building with -DBUILD_SHARED_LIBS:BOOL=ON

prepping patch to fix this...

See http://rdieter.fedorapeople.org/rpms/yarock/

yarock.spec : updated spec file
spec.patch : patch to original .spec
Yarock_0.9.64_11_source-system_libs.patch : patch applied in yarock.spec allowing use of system qtsingleappliation, qxt libraries

>  See http://rdieter.fedorapeople.org/rpms/yarock/

There is a minor typo in your spec:
qtsingleappliation-devel => qtsingleapplciation-devel

Anyway, even after fixing that the build fails. The variable QTSINGLECOREAPPLICATION_LIBRARIES is set to NOTFOUND:

http://paste.fedoraproject.org/56249/38519578

Besides, does it really worth it to add 2 more build dependencies and another patch just to make %cmake work? Wouldn't be easier to go along with my innocent cmake approach?

> There is a minor typo in your spec:
> qtsingleappliation-devel => qtsingleapplciation-devel

I meant "qtsingleapplication-devel". ;)

Bundled code is never allowed in Fedora. Or we won't approve your package.

Reference:

 https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Or you can start here:

 https://fedoraproject.org/wiki/Category:Package_Maintainers#Packaging_Guidelines

I think I have found the solution. Rex forgot one of the dependencies. lets see...

> Bundled code is never allowed in Fedora. Or we won't approve your package.
> Reference:
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Thanks for the heads up. With Rex's solution we shouldn't worry about that ;)

Success! Actually Rex forgot to add "qtsinglecoreapplication-devel" in the build requirements. With that in place, the build is successful. 

updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

updated Koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6215874

Special thanks to Rex for his patches. I think this is ready for inclusion in fedora repos. Comments are welcome.

Still bad:

http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

> Still bad:
> http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

Done, please see the %check section:

http://jam3s.fedorapeople.org/yarock.spec

fwiw, I can supply a separate (upstreamable) patch to fix build problems using the bundled code when building with -DBUILD_SHARED_LIBS:BOOL=ON option too.

> fwiw, I can supply a separate (upstreamable) patch to fix build problems using
> the bundled code when building with -DBUILD_SHARED_LIBS:BOOL=ON option too.

You can contact the original developer 'Sebastien Amardeilh' regarding that. I've already informed him that we are packaging yarock for fedora. 

Anyway, Thanks for providing those fine patches. Is there any other issues that needs to be solved? Any other comments regarding the SPEC?

If all the reviewers are satisfied with this package, please APPROVE it or otherwise let me know of any suggestions.

In actual review process has not started yet, no reviewer is assigned.

See:

http://fedoraproject.org/wiki/Package_Review_Process#Review_Process

for details.

> In actual review process has not started yet, no reviewer is assigned.

Shouldn't the sponsor be also the reviewer? Kevin Fenzi sponsored me (for co-maintaining another package) into the fedora packager git commit group: 

> I've sponsored james to help co-maintain a infrastructure package. Removing the NEEDSPONSOR here.

Is he also responsible for reviewing this package too? I'll have to talk to him.

> Shouldn't the sponsor be also the reviewer?

It's somewhat a grey area, since you've been sponsored prior to your first package review request already (for becoming a co-maintainer). The following page,

  http://fedoraproject.org/wiki/Package_Review_Process#Reviewer

only says:

| The Reviewer can be any Fedora account holder, who is a member of
| the packager group. There is one exception: If it is the first package
| of a Contributor, the Reviewer must be in the Sponsor group and be
| willing to sponsor that Contributor.

That refers to review requests with the NEEDSPONSOR flag, where a sponsor is needed for real progress. 

But this is not your "first package", only your first package review request. Any reviewer could do the review, since you are sponsored already. For the initial guidance, it would be fair if Kevin did the first review, however.

I'd be happy to review this, but not sure when I will get time to do so. :( 

So, if anyone else would like to, please feel free... if not, I will try and do so as time permits.

> %{_datadir}/icons/hicolor/16x16/apps/application-x-yarock.png
> %{_datadir}/icons/hicolor/32x32/apps/application-x-yarock.png
> %{_datadir}/icons/hicolor/48x48/apps/application-x-yarock.png
> %{_datadir}/icons/hicolor/64x64/apps/application-x-yarock.png
> %{_datadir}/icons/hicolor/128x128/apps/application-x-yarock.png

I agree with Rex Dieter (comment #11) there. Enumerating every single icon (and even every single size!) explicitly is totally pointless. It will just fail the build for no reason when upstream adds new icons.

I disagree with Terje's comment #12 here:

> I want control over shipped files, changes should trigger a failed build.
>
> Control is more important than convenience for the packager.

It really makes no sense to fail the build on new added icons. It is normal for applications to ship multiple icons: an application icon, a MIME type icon, custom actions not covered by the icon themes etc. There is no reason to require manually editing the specfile each time one is added.

The use of globs is at the packager's discretion. As long as the directory ownership is correct (i.e. you MUST NOT use something like /* which ends up owning /usr, of course!), there is no guideline that forbids using globs.

> I agree with Rex Dieter (comment #11) there. Enumerating every single icon (and
> even every single size!) explicitly is totally pointless. It will just fail the
> build for no reason when upstream adds new icons.

> I disagree with Terje's comment #12 here

Now the vote is 2 to 1 in favor of using globs. In that case, We revert to globs.

Since Kevin Fenzi is busy at the moment, I just hope that this package find a reviewer as soon as possible.  

Update SPEC:
http://jam3s.fedorapeople.org/yarock.spec

I'll look at reviewing this today... hopefully look for a full review later. ;)

1. /usr/share/locale/yarock doesn't seem to be owned. Not sure if this
is a find_lang bug or what. Perhaps the arguments are confusing it? 
You can manually run the find_lang.sh rpm uses and see if you can 
track it down. Or perhaps someone else has seen this?

2. Whats the status of upstreaming the patches? It's usually good 
practice to add comments next to any patches in the spec with links
to any upstream bug reports, or even just a 'sent to upstream on yyyy-mm-dd'

Otherwise things look good from here. Solve those and I can approve. ;) 

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 5 files have unknown
     license. Detailed output of licensecheck in /home/fedora/kevin/yarock
     /review-yarock/licensecheck.txt
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/locale/yarock
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/locale/yarock
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: gtk-update-icon-cache is invoked in %postun and %posttrans if package
     contains icons.
     Note: icons in yarock
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is known to not require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or desktop-
     file-validate if there is such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: yarock-0.9.64-2.fc21.x86_64.rpm
          yarock-0.9.64-2.fc21.src.rpm
yarock.x86_64: W: spelling-error %description -l en_US coverart -> cover art, cover-art, covert
yarock.x86_64: W: no-manual-page-for-binary yarock
yarock.x86_64: E: incorrect-locale-subdir /usr/share/locale/yarock/yarock_cs.qm
yarock.src: W: spelling-error %description -l en_US coverart -> cover art, cover-art, covert
2 packages and 0 specfiles checked; 1 errors, 3 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint yarock
yarock.x86_64: W: spelling-error %description -l en_US coverart -> cover art, cover-art, covert
yarock.x86_64: W: no-manual-page-for-binary yarock
yarock.x86_64: E: incorrect-locale-subdir /usr/share/locale/yarock/yarock_cs.qm
1 packages and 0 specfiles checked; 1 errors, 2 warnings.
# echo 'rpmlint-done:'



Requires
--------
yarock (rpmlib, GLIBC filtered):
    /bin/sh
    hicolor-icon-theme
    libQtCore.so.4()(64bit)
    libQtDBus.so.4()(64bit)
    libQtGui.so.4()(64bit)
    libQtNetwork.so.4()(64bit)
    libQtSolutions_SingleApplication-2.6.so.1()(64bit)
    libQtSolutions_SingleCoreApplication-2.6.so.1()(64bit)
    libQtSql.so.4()(64bit)
    libQtXml.so.4()(64bit)
    libQxtGui.so.0()(64bit)
    libX11.so.6()(64bit)
    libc.so.6()(64bit)
    libechonest.so.2.1()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libphonon.so.4()(64bit)
    libqjson.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.1)(64bit)
    libtag.so.1()(64bit)
    rtld(GNU_HASH)



Provides
--------
yarock:
    application()
    application(yarock.desktop)
    yarock
    yarock(x86-64)



Source checksums
----------------
https://launchpad.net/yarock/trunk/0.9.64/+download/Yarock_0.9.64_11_source.tar.gz :
  CHECKSUM(SHA256) this package     : 619d4fb6dc2afb1c15ff0b7409cfc5df7fe4a3f5d2f84e60d62129038fc5832e
  CHECKSUM(SHA256) upstream package : 619d4fb6dc2afb1c15ff0b7409cfc5df7fe4a3f5d2f84e60d62129038fc5832e


Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -n yarock
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, SugarActivity, Perl, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

> 1. /usr/share/locale/yarock doesn't seem to be owned. Not sure if this
> is a find_lang bug or what. Perhaps the arguments are confusing it? 
> You can manually run the find_lang.sh rpm uses and see if you can 
> track it down. Or perhaps someone else has seen this?

This is the usual problem with Qt translation files (used by Qt-only stuff, KDE uses gettext instead). There is really no standard location where to put them. /usr/share/locale/%{name} is not that great a location for translations, "yarock" is not a locale. The gettext scheme is /usr/share/locale/%{locale}/%{name}.mo, but Qt doesn't support that scheme, it expects *_%{locale}.qm (or just %{locale}.qm, which some apps try to use and which is NOT supported by %find_lang) files in a common directory.

In any case, the directory containing the translations needs to be listed in %files with a %dir tag, e.g.:
%dir %{_datadir}/locale/%{name}/
The %find_lang script will only collect the files inside the directory, so the directory itself needs to be listed explicitly. And %dir means that we only want the directory and not its contents, because the contents are found and tagged with the correct %lang(*) tags by %find_lang.

> In any case, the directory containing the translations needs to be listed in
> %files with a %dir tag, e.g.:
> %dir %{_datadir}/locale/%{name}/

Thanks a lot for this solution. Saved me from a lot of headaches. I've added it to the new spec.

> 2. Whats the status of upstreaming the patches? It's usually good
> practice to add comments next to any patches in the spec with links
> to any upstream bug reports, or even just a 'sent to upstream on yyyy-mm-dd'

Done. I sent the patches to upstream and added comments to the spec file. I also fixed that little "coverart" typo in description.

updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

Great. Thanks for the pointer on the locale dir Kevin Koffler. 

Usually when adding comments for patches, thats done at the top where the patch is defined instead of in the prep where it's applied. You might move those comments up to the top, you can do that before you import it... 

I don't see any further blockers, so this package is APPROVED.

> Usually when adding comments for patches, thats done at the top where the patch
> is defined instead of in the prep where it's applied.

Well, I've seen both variants, but yes, doing it at the top is more common.

> Usually when adding comments for patches, thats done at the top where the patch
> is defined instead of in the prep where it's applied. You might move those
> comments up to the top, you can do that before you import it... 

Done. 

> I don't see any further blockers, so this package is APPROVED.

Thank you very much. It's kind of exciting to see my first package approved ;)

Special thanks to those who commented on and reviewed this packages: Kevin Fenzi, Rex Dieter, Kevin Kofler, Terje Røsten, and Christopher Meng.

I'll proceed with the instructions indicated in the following wiki:
https://fedoraproject.org/wiki/Package_SCM_admin_requests


updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

(In reply to Kevin Kofler from comment #44)
> > 1. /usr/share/locale/yarock doesn't seem to be owned. Not sure if this
> > is a find_lang bug or what.
 
/usr/share/locale/yarock is a bug. Package simply MUST not install i18n files there in.

The are supposed to install them into /usr/share/locale/<language>

> This is the usual problem with Qt translation files (used by Qt-only stuff,
> KDE uses gettext instead). There is really no standard location where to put
> them. /usr/share/locale/%{name} is not that great a location for
> translations, "yarock" is not a locale.
Exactly - This package is broken and should to be fixed.

> /usr/share/locale/yarock is a bug. Package simply MUST not install i18n files there in.

reference?

I thought Kevin Kofler said:

> The gettext scheme is /usr/share/locale/%{locale}/%{name}.mo, but Qt doesn't 
> support that scheme, it expects *_%{locale}.qm (or just %{locale}.qm, which 
> some apps try to use and which is NOT supported by %find_lang) files in a
> common directory.
-----

> This package is broken and should to be fixed.

and what is your solution exactly?

I thought this was a a bug with qt.

(In reply to James Abtahi from comment #50)
> > /usr/share/locale/yarock is a bug. Package simply MUST not install i18n files there in.
> 
> reference?
This is common sense ever since i18n exists and is hard-wired into many tools.

> I thought Kevin Kofler said:
> 
> > The gettext scheme is /usr/share/locale/%{locale}/%{name}.mo, but Qt doesn't 
> > support that scheme, it expects *_%{locale}.qm (or just %{locale}.qm, which 
> > some apps try to use and which is NOT supported by %find_lang) files in a
> > common directory.
> -----
> 
> > This package is broken and should to be fixed.
> 
> and what is your solution exactly?
I haven't had a deepper look into this package yet.

I only stumbled over the 
%dir %{datadir}/locale/%{name}
inside of your *spec. A line which is multiply questionable.

> I thought this was a a bug with qt.
I can't tell without having had a deeper look into your package. My guess would be, this is an bug inside of this package.

Where is your latest src.rpm?

http://jam3s.fedorapeople.org/yarock.spec
does not match the src.rpm under http://jam3s.fedorapeople.org/

One solution that Qt programs often use is to install the *.qm files under %{_datadir}/%{name}/translations. It's also not really optimal, but at least it doesn't abuse %{_datadir}/locale.

The ideal solution would of course be to just use gettext, but unfortunately, Qt suffers from NIH syndrome there. :-( (And KDE actually ignores that and uses gettext, but non-KDE Qt stuff normally uses the Qt system.)

> Where is your latest src.rpm?
> does not match the src.rpm under http://jam3s.fedorapeople.org/

updated now:
http://jam3s.fedorapeople.org/yarock-0.9.64-2.fc20.src.rpm

> One solution that Qt programs often use is to install the *.qm files under %
> {_datadir}/%{name}/translations. It's also not really optimal, but at least it
> doesn't abuse %{_datadir}/locale.

sounds like a good solution. @Ralf Corsepius: Would this solution be alright with you?

hum... something is terribly wrong. according to this source:

http://www.cmake.org/Wiki/CMake:How_To_Build_Qt4_Software#Translations

.ts  file are just the xml source translations while the .qm files are the binary translations which are executed. but the .ts files are installed directly in the /usr/share/locale/yarock after installation. No .qm file is there just .ts files.

I think the original author made some mistakes in the src/CMakeLists and the patch (.installationpath) is really building upon that bad design. Specifically It doesn't seem right to hard-code /usr/share/locale there. One probably should use something like this:

install(FILES ${QM_FILES} DESTINATION ${CMAKE_INSTALL_PREFIX}/translations)

but I'm no expert with these translation stuff. However, I've got a feeling that the source of the problem is in src/CMakeLists.  any ideas?

nah, something stupid happened there. nothing related to the package. the .qt files are installed in the /usr/share/locale/yarock. Ignore my previous comment. sorry about it.

I've applied Kevin Kofler's solution and now the translation files are installed in %{_datadir}/%{name}/translations (see the installationpath patch). I've also noticed that "qt" package translation files are installed in a similar fashion. So this should be fine. Let me know what you think. 

updated SPEC and SRPM:
http://jam3s.fedorapeople.org/yarock.spec
http://jam3s.fedorapeople.org/yarock-0.9.64-3.fc20.src.rpm

Please add:
%dir %{_datadir}/%{name}
which is otherwise unowned. Other than that, it looks fine.

> Please add:
> %dir %{_datadir}/%{name}
> which is otherwise unowned. Other than that, it looks fine.

Done. I'm going to put a request for creation of a repository in the next few hours. In the meantime please let me know of any final thoughts.

updated SPEC:
http://jam3s.fedorapeople.org/yarock.spec

New Package SCM Request
=======================
Package Name: yarock
Short Description: A lightweight, beautiful music player
Owners: jam3s
Branches: f19 f20
InitialCC:

Git done (by process-git-requests).

yarock-0.9.64-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/yarock-0.9.64-3.fc19

yarock-0.9.64-3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/yarock-0.9.64-3.fc20

yarock-0.9.64-3.fc20 has been pushed to the Fedora 20 testing repository.

yarock-0.9.64-3.fc19 has been pushed to the Fedora 19 stable repository.

yarock-0.9.64-3.fc20 has been pushed to the Fedora 20 stable repository.