Bug 735944

Summary: Review Request: comex-base - base component for comex project
Product: [Fedora] Fedora Reporter: Armando Basile <hmandevteam>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED INSUFFICIENT_DATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: hmandevteam, msuchy, mtasaka, package-review
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-08-21 09:22:06 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Armando Basile 2011-09-06 08:51:36 UTC
Spec URL: http://www.integrazioneweb.com/repository/specs/fedora/comex-base.spec
SRPM URL: http://www.integrazioneweb.com/repository/srpms/fedora/comex-base-0.1.7.0-1.fc15.src.rpm
Description: Is base component of a simple application that can be used to exchange
data with smartcards using PC/SC standard readers or smartmouse
phoenix serial reader.

Comment 1 Armando Basile 2011-09-07 21:55:24 UTC
Spec URL:
http://www.integrazioneweb.com/repository/specs/fedora/comex-base.spec
SRPM URL:
http://www.integrazioneweb.com/repository/srpms/fedora/comex-base-0.1.7.0-2.fc15.src.rpm

changes:
- added ExclusiveArch
- added pcsc-lite, pcsc-lite-devel, pcsc-lite-libs dependency
- added %{?_smp_mflags} to make command

Comment 2 Mamoru TASAKA 2011-09-08 20:00:03 UTC
Well, only did a very quick check

* License tag
  - GPLv2 is chosen for license tag in your spec, however
    I cannot see any files which specifies the version of
    GPL in the source. Note that just including GPLv2 license
    text does not actually specify the version of GPL
    used in the source.

    Currently License tag should be "GPL+"

* BuildRequires
  - Currently your srpm won't build on koji. Please make it
    sure that your srpm builds on koji (or with mockbuild)
    http://koji.fedoraproject.org/koji/taskinfo?taskID=3336036

    It seems mono-devel is needed for BR

* Explicit dependency
  - Note that with adding "BR: mono-devel", rpmbuild will find
    out mono-related runtime dependency (i.e. Requires)
    automatically and adds the detected dependencies to binary
    rpms. So writing "Requires: mono-core log4net" explicitly
    is not needed (after adding BR: mono-devel)

* Requires for main pacakge
  - Usually Requires for main package on subpackages should
    be (Epoch:)Version-Release specific and also isa specific
    
    https://fedoraproject.org/wiki/Packaging/Guidelines#Requiring_Base_Package

* pkgconfig dependency
  - Explicit "Requires: pkgconfig" is no longer needed on Fedora,
    it is added automatically by rpmbuild
    https://fedoraproject.org/wiki/Packaging/Guidelines#Pkgconfig_Files

* Deprecated usage
  Please remove lines which are no longer needed
  - %if 0%{?fedora} < 13 condition is useless. Fedora's version is at
    least 14 currently
  - The following is no longer needed on Fedora (EPEL may still need
    these)
    * BuildRoot: line
    * rm -fr %{buildroot} at the top of %install
    * %clean section

* Directory ownership issue
  - The directory %{_libdir}/mono/gac/ itself is owned by mono-core and
    this package should not own this directory itself.
  - The directory %{_datadir}/%{name}/ is not owned by any packages and
    this package should own it

* Requires: pcsc-lite-devel
  - Can't this be avoided by patching comex-base/comex-base.dll.config?

Note that I will be away till Sunday so it may be that replying to you
becomes late. By the way I will appreciate it if you would review my
review request (bug 678674)

Comment 3 Armando Basile 2011-09-11 20:56:27 UTC
SPEC: http://www.integrazioneweb.com/repository/specs/fedora/comex-base.spec
SRPMS: http://www.integrazioneweb.com/repository/srpms/fedora/comex-base-0.1.7.1-1.fc15.src.rpm

changes in sources http://code.google.com/p/comex-project/source/detail?r=73:
- removed dependence from pcsc-lite devel package
- changed alias in .config file (now also .Net see library)
- changed DllImport alias for Interop (now also .Net see library)
- added license information

changes in spec:
- in BuildRequires: changed mono-core to mono-devel 
- in Requires: removed pcsc-lite-devel
- in Requires: removed mono-core
- in Requires: removed log4net
- in Requires: removed pkgconfig
- build section: removed "if fedora < 13" condition
- removed BuildRoot
- install section: removed "rm -fr %{buildroot}"
- clean section: removed
- files section: changed ref from mono/gac/ to mono/gac/comex-base/
- files section: changed ref from %{name}/Languages/ to %{name}/


about your review request bug 678674, i don't know ruby so i don't know how could i help you.

Comment 4 Mamoru TASAKA 2011-09-12 16:09:03 UTC
Well, I have not checked your latest srpm, however just for this:

(In reply to comment #3)
> about your review request bug 678674, i don't know ruby so i don't know how
> could i help you.
Note that I don't know mono well, either :)

Comment 5 Armando Basile 2011-09-12 16:29:34 UTC
> Note that I don't know mono well, either :)
Wow, seems you are familiar with mono

Could try to review a mono package for first ?

Comment 6 Jason Tibbitts 2013-05-01 15:28:09 UTC
I am triaging old review tickets.  I can't promise a review if you reply, but by closing out the stale tickets we can devote extra attention to the ones which aren't stale.

spec and srpm links are invalid.

Comment 7 Armando Basile 2013-05-01 15:51:20 UTC
i updating specs and srmps, thanks for reply

Comment 9 Mario Blättermann 2013-06-22 10:06:19 UTC
Some initial comments before I give it a try on Koji:

Probably no debuginfo will be created, due to the nature of Mono software:
https://fedoraproject.org/wiki/Packaging:Debuginfo?rd=Packaging/Debuginfo#Useless_or_incomplete_debuginfo_packages_due_to_other_reasons
This is addressed by your comment in spec line 13. OK so far, but either you uncomment the line (if debuginfo will be usable in this certain case) or you have to escape the % signs with another one to make rpmlint happy. In some odd cases, even macros in comments could be expanded and lead to unexpected results.

Requires:	%{name} = %{version}
has to be
Requires: %{name}%{?_isa} = %{version}-%{release}
The isa tag needs to be added because your package is multiarch. The release tag makes sure that the -devel package will be updated with the base package.

%defattr(-,root,root,-)
is applicable for EPEL 5 only. As the second changelog entry says, you don't plan to package for EPEL 5, please remove that lines in the file lists.

In the file lists, %{_prefix}/lib has to replaced with %{_libdir}. Otherwise we would get a "hardcoded library path" warning from rpmlint. Or can you explain why you have to use /usr/lib explicitely instead of /usr/lib and /usr/lib64 depending on the package architecture?

%changelog
* Thu May 02 2013 Armando Basile <hmandevteam> 0.1.8.5-1.fc18
In the %changelog section, it is unneeded to mention the distribution release. A simple "0.1.8.5-1" is sufficient here. Doesn't matter which Fedora release is pointed by your spec, one could rebuild your package for f17 or f19, whatever, so that your "f18" tag becomes useless.

Requires: pcsc-lite
Requires: pcsc-lite-libs
The latter is not needed, it is required by pcsc-lite anyway.

You are using both tabs and whitespaces in your spec. It's rather cosmetic, but has to be fixed. I recommend spaces, that way the spec looks the same in any text editor, regardless of the configured tab width.

Comment 10 Mario Blättermann 2013-08-19 09:29:36 UTC
Any news here...?

Comment 11 Armando Basile 2013-09-15 18:26:01 UTC
Hi Mario, i will modify specs and resubmit, thanks

Comment 12 Miroslav Suchý 2015-08-21 09:22:06 UTC
No response for years. Closing. Feel free to reopen if you want to continue.