Greetings,

You should use %license for LICENSE instead of %doc.

There is no man file. You should probably need to work with upstream to write it. It could be written in markdown and you could always convert it to man while packaging, e.g.

%build
pandoc -s -f markdown_github -t man -V title=%{name} -V section=1 -V date="$(LANG=C date -d @$(stat -c'%Z' README.md) +'%B %d, %Y')" README.md -o %{name}.1

Remove any comments from SPEC that you don't need.

Replace spaces to tabs (line 25).

You might want your SPEC to produce hardened binaries (http://fedoraproject.org/wiki/Packaging:Guidelines#PIE).

Please note that this is informal review.

Ah, I also need to mention that description must not be over 80 columns per line so you should split it into multiple lines.

(In reply to amigo.elite from comment #1)

> You should use %license for LICENSE instead of %doc.

Fixed

> There is no man file. You should probably need to work with upstream to
> write it. It could be written in markdown and you could always convert it to
> man while packaging, e.g.
> 
> %build
> pandoc -s -f markdown_github -t man -V title=%{name} -V section=1 -V
> date="$(LANG=C date -d @$(stat -c'%Z' README.md) +'%B %d, %Y')" README.md -o
> %{name}.1

It is not CLI application and has no any CLI option.
Seems man is not required.

> Remove any comments from SPEC that you don't need.

Fixed.

> Replace spaces to tabs (line 25).

Fixed.
(Note: qterminal ^c-^v bag :-)

> You might want your SPEC to produce hardened binaries
> (http://fedoraproject.org/wiki/Packaging:Guidelines#PIE).

Done.

> Ah, I also need to mention that description must not be over 80 columns per line so you should split it into multiple lines.

Fixed (removed "multi-platform open source").

> Please note that this is informal review.

Ok

(In reply to amigo.elite from comment #2)
> Ah, I also need to mention that description must not be over 80 columns per
> line so you should split it into multiple lines.

Oops, forgot results:

Spec: https://tieugene.fedorapeople.org/rpms/wcm/wcm.spec
SRPM: https://tieugene.fedorapeople.org/rpms/wcm/wcm-0.18.0-1.fc20.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=8388372

FYI, 0.18.1 released today.

Hi, 
in the process of becoming a packager i would like to do first some informal reviews, and i would like to start with this package.
I will put in copy my sponsor that will do the formal review then.
Just one thing.. i have seen that there are new versions available on github of this package. Still interested to push version 0.18.0 or you would like to try with more recent versions?
thanks
Andrea

Hi 

Do you want to continue with this review?

(In reply to William Moreno from comment #7)
> Hi 
> 
> Do you want to continue with this review?

Yes, of course.
Waiting for reviewer.

Fresh build:
SPEC: https://tieugene.fedorapeople.org/rpms/wcm/wcm.spec
SRPM: https://tieugene.fedorapeople.org/rpms/wcm/wcm-0.20.0-1.fc21.src.rpm
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=10670315

You must add a appdata.xml file to your package, this way users will be hable to find your package in Gnome Software, please see:
https://fedoraproject.org/wiki/Packaging:AppData

Also add:
BuildRequires:  libappstream-glib

You can find a example here:
https://github.com/williamjmorenor/fedora-specs/blob/master/retext.appdata.xml

I will recomend than you include the icon in the icons directory,  this can work:

BuildRequires:  libpng-devel
BuildRequires:  ImageMagick
Requires:       hicolor-icon-theme

%install
[...]
mkdir -p %{buildroot}/%{_datadir}/icons/hicolor/{16x16,22x22,24x24,32x32,48x48,64x64,72x72,96x96,128x128,scalable}/apps
for s in 16x16 22x22 24x24 32x32 48x48 64x64 72x72 96x96 128x128
do
    convert ./wcm.png -resize $s %{buildroot}/%{_datadir}/icons/hicolor/$s/apps/wcm.png;
done

This spec can be a example:
https://github.com/williamjmorenor/fedora-specs/blob/master/retext.spec

Please update the spec and I will run the review

(In reply to William Moreno from comment #9)
> Requires:       hicolor-icon-theme

Not mandatory

SPEC: https://tieugene.fedorapeople.org/rpms/wcm/wcm.spec
SRPM: https://tieugene.fedorapeople.org/rpms/wcm/wcm-0.20.0-2.fc21.src.rpm
Koji builds:
EL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=10678490
F22: https://koji.fedoraproject.org/koji/taskinfo?taskID=10678561

Looks like there are some bundled libraries upstream

utf8proc
https://github.com/corporateshark/WCMCommander/tree/master/src/utf8proc

This looks like a bundled of:
https://admin.fedoraproject.org/pkgdb/package/utf8proc/

urlparser
https://github.com/corporateshark/WCMCommander/tree/master/src/urlparser

Looks like a bundled or (not packaged):
https://github.com/corporateshark/LUrlParser

The urlparser looks like a easy packaging library, Fedora Packaging Guidelines do not allow bundled libraries without the OK of FPC, try to package urlparser to test is wmc work with this library also please try to add Requires: utf8proc and remove the bundled lib to test if the package work with the system library.

(In reply to William Moreno from comment #11)
> Looks like there are some bundled libraries upstream
> 
> utf8proc

Fixed.

> urlparser
> https://github.com/corporateshark/WCMCommander/tree/master/src/urlparser
> 
> Looks like a bundled or (not packaged):
> https://github.com/corporateshark/LUrlParser

1. They are not identical and seems have different API.
2. git version has no any release yet.
So, I deside live bundled version as part of wcm sources.

SPEC: https://tieugene.fedorapeople.org/rpms/wcm/wcm.spec
SRPM: https://tieugene.fedorapeople.org/rpms/wcm/wcm-0.20.0-3.fc21.src.rpm
Koji:
EL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=10688357
F21: https://koji.fedoraproject.org/koji/taskinfo?taskID=10688371
F22: https://koji.fedoraproject.org/koji/taskinfo?taskID=10688391
F23: https://koji.fedoraproject.org/koji/taskinfo?taskID=10688450

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

Need Work:
==========

Fail: Package requires other packages for directories it uses.
Fail: Package must own all directories that it creates.
      Orphan dir: /usr/share/icons/hicolor/*/apps
      Add Requires: hicolors-icon-theme

Fail: gtk-update-icon-cache is invoked in %postun and %posttrans.
      See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

Fail: Sources can be downloaded from URI in Source: tag
Fail: SourceX tarball generation or download is documented.
      W: invalid-url Source0: 
      https://github.com/corporateshark/WalCommander/archive/WCMCommander-release-0.20.0.tar.gz 
      HTTP Error 404: Not Found
      The link: https://github.com/corporateshark/WCMCommander/archive/release-0.20.0.tar.gz 
      Work for me, please check if work for you and update it in the spec

Fail: Package contains BR: python2-devel or python3-devel
      There is some Python code in the tarball, please verify I will recomend include
      BuildRequires: python3-devel

Fail: Permissions on files are set properly.
      E: non-executable-script /usr/share/wcm/styles/solarize.sh 644 /bin/bash
      E: non-executable-script /usr/share/wcm/styles/solarized-gen.py 644 /usr/bin/python
      This files do not look to need execusion permissions, try removing the shebang or set then to 755

Fail: Patches link to upstream bugs/comments/lists or are otherwise
      justified.
      If you can propose some patches upstream please do it, 
      if patches are Fedora specific please add a comment about it in the spec.

Note: W: name-repeated-in-summary C WCM
      Drop the mane of the package in summary

Note: W: no-manual-page-for-binary wcm
      This is a easy fix, yua can use the package manedit to create a Unix manapage
      and include it, even a minimal man page can improve the user experience
      please propose the man page upstrea,

Note: W: file-not-utf8 /usr/share/doc/wcm/CHANGELOG.txt
      See: https://fedoraproject.org/wiki/Packaging_tricks#Convert_encoding_to_UTF-8

Please fix these issues to check:

?:    Package complies to the Packaging Guidelines

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

Generic:
Pass: Package is licensed with an open-source compatible license and meets
      other legal requirements as defined in the legal section of Packaging
      Guidelines.
Pass: License field in the package spec file matches the actual license.
Pass: %build honors applicable compiler flags or justifies otherwise.
Pass: Package contains no bundled libraries without FPC exception.
Pass: Changelog in prescribed format.
Pass: Sources contain only permissible code or content.
NA  : Development files must be in a -devel package
Pass: Package uses nothing in %doc for runtime.
Pass: Package consistently uses macros (instead of hard-coded directory
      names).
Pass: Package is named according to the Package Naming Guidelines.
Pass: Package does not generate any conflict.
Pass: Package obeys FHS, except libexecdir and /usr/target.
NA  : If the package is a rename of another package, proper Obsoletes and
      Provides are present.
Pass: Requires correct, justified where necessary.
Pass: Spec file is legible and written in American English.
NA  : Package contains systemd file(s) if in need.
Pass: Useful -debuginfo package or justification otherwise.
Pass: Package is not known to require an ExcludeArch tag.
      ARM: http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=3122269
      s390: http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=1936707
      ppc: http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=2705219
Pass: Large documentation must go in a -doc subpackage. 
Pass: Package successfully compiles and builds into binary rpms on at least
      one supported primary architecture.
Pass: Package installs properly.
Pass: Rpmlint is run on all rpms the build produces.
Pass: 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 %license.
Pass: Package does not own files or directories owned by other packages.
Pass: All build dependencies are listed in BuildRequires, except for any
      that are listed in the exceptions section of Packaging Guidelines.
Pass: Package uses either %{buildroot} or $RPM_BUILD_ROOT
Pass:  Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
      beginning of %install.
Pass: Macros in Summary, %description expandable at SRPM build time.
Pass: Package contains desktop file if it is a GUI application.
Pass: Package installs a %{name}.desktop using desktop-file-install or
      desktop-file-validate if there is such a file.
Pass: Dist tag is present.
Pass: Package does not contain duplicates in %files.
Pass: Package use %makeinstall only when make install DESTDIR=... doesn't
      work.
Pass: Package is named using only allowed ASCII characters.
Pass: Package does not use a name that already exists.
Pass: Package is not relocatable.
Pass: Sources used to build the package match the upstream source, as
      provided in the spec URL.
      83fc7d79f3a17623674adb7e0d7c9fb9  release-0.20.0.tar.gz
      83fc7d79f3a17623674adb7e0d7c9fb9  WCMCommander-release-0.20.0.tar.gz
Pass: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
Pass: File names are valid UTF-8.
Pass: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====
Generic:
NA:   If the source package does not include license text(s) as a separate
      file from upstream, the packager SHOULD query upstream to include it.
Pass: Final provides and requires are sane (see attachments).
Pass: Package functions as described.
Pass: Latest version is packaged.
Pass: Package does not include license text files separate from upstream.
NA:   Description and summary sections in the package spec file contains
      translations for supported Non-English languages, if available.
Pass: Package should compile and build into binary rpms on all supported
      architectures.
NA:   %check is present and all tests pass.
Pass: Packages should try to preserve timestamps of original installed
     files.
Pass: Packager, Vendor, PreReq, Copyright tags should not be in spec file
Pass: Reviewer should test that the package builds in mock.
Pass: Buildroot is not present
Pass: Package has no %clean section with rm -rf %{buildroot} (or
      $RPM_BUILD_ROOT)
Pass: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
Pass: Uses parallel make %{?_smp_mflags} macro.
Pass: SourceX is a working URL.
Pass: Spec use %global instead of %define unless justified.

===== EXTRA items =====
Generic:
Pass: Rpmlint is run on debuginfo package(s).
Pass: Rpmlint is run on all installed packages.
Pass: Large data in /usr/share should live in a noarch subpackage if package
      is arched.
Pass: Spec file according to URL is the same as in SRPM.

Rpmlint
-------
Checking: wcm-0.20.0-3.fc21.x86_64.rpm
          wcm-0.20.0-3.fc21.src.rpm
wcm.x86_64: W: name-repeated-in-summary C WCM
wcm.x86_64: W: file-not-utf8 /usr/share/doc/wcm/CHANGELOG.txt
wcm.x86_64: E: non-executable-script /usr/share/wcm/styles/solarize.sh 644 /bin/bash
wcm.x86_64: E: non-executable-script /usr/share/wcm/styles/solarized-gen.py 644 /usr/bin/python
wcm.x86_64: W: no-manual-page-for-binary wcm
wcm.src: W: name-repeated-in-summary C WCM
wcm.src: W: invalid-url Source0: https://github.com/corporateshark/WalCommander/archive/WCMCommander-release-0.20.0.tar.gz HTTP Error 404: Not Found
2 packages and 0 specfiles checked; 2 errors, 5 warnings.

Rpmlint (debuginfo)
-------------------
Checking: wcm-debuginfo-0.20.0-3.fc21.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Rpmlint (installed packages)
----------------------------
wcm.x86_64: W: name-repeated-in-summary C WCM
wcm.x86_64: W: file-not-utf8 /usr/share/doc/wcm/CHANGELOG.txt
wcm.x86_64: E: non-executable-script /usr/share/wcm/styles/solarize.sh 644 /bin/bash
wcm.x86_64: E: non-executable-script /usr/share/wcm/styles/solarized-gen.py 644 /usr/bin/python
wcm.x86_64: W: no-manual-page-for-binary wcm
2 packages and 0 specfiles checked; 2 errors, 3 warnings.

Requires
--------
wcm (rpmlib, GLIBC filtered):
    /bin/sh
    libX11.so.6()(64bit)
    libc.so.6()(64bit)
    libfreetype.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libsmbclient.so.0()(64bit)
    libsmbclient.so.0(SMBCLIENT_0.1.0)(64bit)
    libssh2.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.5)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libutf8proc.so.0.1()(64bit)
    rtld(GNU_HASH)

Provides
--------
wcm:
    appdata()
    appdata(wcm.appdata.xml)
    application()
    application(wcm.desktop)
    wcm
    wcm(x86-64)

(In reply to William Moreno from comment #13)
>       Orphan dir: /usr/share/icons/hicolor/*/apps
>       Add Requires: hicolors-icon-theme

Fixed

> Fail: gtk-update-icon-cache is invoked in %postun and %posttrans.

Fixed

> Fail: Sources can be downloaded from URI in Source: tag

Fixed

> Fail: Package contains BR: python2-devel or python3-devel

Fixed (python scripts removed from installation - these are devel tools)

> Fail: Permissions on files are set properly.
>       E: non-executable-script /usr/share/wcm/styles/solarize.sh 644
> /bin/bash
>       E: non-executable-script /usr/share/wcm/styles/solarized-gen.py 644
> /usr/bin/python

Fixed (devel tools)

> Fail: Patches link to upstream bugs/comments/lists

Fixed

> Note: W: name-repeated-in-summary C WCM
>       Drop the mane of the package in summary

?
"WCM Commander" is original application name.
But developers recomended to use "wcm" as short name.

> Note: W: no-manual-page-for-binary wcm

Fixed

> Note: W: file-not-utf8 /usr/share/doc/wcm/CHANGELOG.txt

Fixed

So:

SPEC: https://tieugene.fedorapeople.org/rpms/wcm/wcm.spec
SRPM: https://tieugene.fedorapeople.org/rpms/wcm/wcm-0.20.0-4.fc21.src.rpm
Koji:
EL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=10731456
F23: https://koji.fedoraproject.org/koji/taskinfo?taskID=10731472

Just a minor comment:

Do not use a compresed manpage
https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages

I will check the spec, do not update the spec now, I will not block the review for the compresed manpage so it is a trivial fix.

PACKAGE APROVED
===============

Remember to use the man page uncompresed

New Package SCM Request
=======================
Package Name: wcm
Short Description: WCM Commander
Upstream URL: https://github.com/corporateshark/WCMCommanderOwners: tieugene
Branches: f21 f22 f23 epel7
InitialCC:

New Package SCM Request
=======================
Package Name: wcm
Short Description: WCM Commander
Upstream URL: https://github.com/corporateshark/WCMCommander
Owners: tieugene
Branches: f21 f22 f23 epel7
InitialCC:

Git done (by process-git-requests).

drive-by note: lurlparser now has a release - https://github.com/corporateshark/LUrlParser/releases - and wcm is now using the exact same code - https://github.com/corporateshark/WCMCommander/commit/a5fd6281083398e31b857df5aff99caacf0837e5 . according to policy it should therefore now be unbundled.

https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#When_a_Bundled_Library_is_Discovered_Post-Review

(In reply to awilliam from comment #20)
> drive-by note: lurlparser now has a release -
> https://github.com/corporateshark/LUrlParser/releases - and wcm is now using
> the exact same code -
> https://github.com/corporateshark/WCMCommander/commit/
> a5fd6281083398e31b857df5aff99caacf0837e5 . according to policy it should
> therefore now be unbundled.
> 
> https://fedoraproject.org/wiki/Packaging:
> No_Bundled_Libraries#When_a_Bundled_Library_is_Discovered_Post-Review

lurlparser and wcm are from the same upstream.

BUT, this package should be named as wcmcommander.

they're from the same upstream vendor, but not the same upstream repository. strictly they ought to be packaged separately I believe. lurlparser is clearly intended to potentially be of use outside of wcmcommander, or it wouldn't be its own repository with its own release.

(In reply to awilliam from comment #22)
> they're from the same upstream vendor, but not the same upstream repository.
> strictly they ought to be packaged separately I believe. lurlparser is
> clearly intended to potentially be of use outside of wcmcommander, or it
> wouldn't be its own repository with its own release.

IMO No, another similar case is redis, it contains linenoise, which is written by redis upstream and blindly favored/stared by lots of people. I personally think it's pretty general and not good, unlike how redis has been favored. But upstream still uses it.

Some upstreams intend to seperate everything they have, in order to show the distinct usage of various subprojects, however then bundle subprojects into another subprojects to make them work together. I'd observe this, most of upstreams know their code better than downstream packagers, if upstream can't even handle problems correct, downstream can't handle them as well.

What I'm going to do is to open a PR and use git submodule functionality to force upstream to keep the code up to date, and motivate them to check the compatibilities before each new release of wcm.

Everyone's entitled to an opinion, but the wording of the policy and precedent here is pretty clear. I don't see any way FPC would *not* consider this a bundled library, based on literally hundreds of previous evaluations; you can look at the FPC ticket history if you like.

If the library it is now packaged the maintainer should always prefer the use of the system library, even if they come for the same upstream, so it is very posible than the bundled library it is the same, do not look like a big deal to use a linked library it is the same upstream for booth.

(In reply to William Moreno from comment #25)
> If the library it is now packaged the maintainer should always prefer the
> use of the system library, even if they come for the same upstream, so it is
> very posible than the bundled library it is the same, do not look like a big
> deal to use a linked library it is the same upstream for booth.

Now lurlparser is not packaged in Fedora.
And as about upstream - it is not library in common sense. It is simply 2 files - *.c and *.h. Without makefile etc.