Bug 1052040 - Review Request: xtv - A file manager for the Linux console/xterm
Review Request: xtv - A file manager for the Linux console/xterm
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Petr Šabata
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-13 02:43 EST by Mohammed Isam
Modified: 2014-03-30 02:08 EDT (History)
5 users (show)

See Also:
Fixed In Version: xtv-1.0-2.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-03-27 23:15:10 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
psabata: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mohammed Isam 2014-01-13 02:43:54 EST
Spec URL: http://sites.google.com/site/mohammedisam2000/xtv.spec
SRPM URL: http://sites.google.com/site/mohammedisam2000/xtv-1.0-1.fc20.src.rpm
Description: The XTree View (XTV) is a file manager which is run under the Linux console/xterm. It provides an easy way to manage files/directories, and perform various operations on them by using menus and keyboard shortcuts.
Fedora Account System Username: mohammedisam
Comment 1 Mohammed Isam 2014-01-13 02:47:01 EST
This is my first package submission. I am working hard on this project so it would be very nice if I got sponsored. Many thanx beforehand.
Comment 2 Petr Šabata 2014-01-13 05:10:04 EST
Hello Mohammed,

I'd love to sponsor you and help you with your project, however first I'd like to check whether it's even possible.

I see you're Sudanese and assume your project is also developed in your home country.  As Fedora is a US project, it has to respect the US export laws.  The OFAC Sudan Sanctions Program mentios "goods, technology, or services of Sudanese origin" several times so let's first check with Fedora Legal.

Hope this turns out well.
Comment 3 Petr Šabata 2014-01-13 05:15:31 EST
I've mailed Fedora Legal.
Comment 4 Christopher Meng 2014-01-13 05:28:58 EST
Also, this package doesn't support x86_64, I don't think it's useful.
Comment 5 Petr Šabata 2014-01-13 06:22:16 EST
(In reply to Christopher Meng from comment #4)
> Also, this package doesn't support x86_64, I don't think it's useful.

Interesting, it works for me.
Comment 6 Rex Dieter 2014-01-13 07:54:10 EST
This is why:
ExclusiveArch:	i686
Comment 7 Rex Dieter 2014-01-13 07:55:11 EST
though, I would recommend adding a short .spec comment near the ExcluseArch explaining why it is needed.
Comment 8 Rex Dieter 2014-01-13 07:55:59 EST
while we're at it...

Since this uses,

%build
%cmake .

it currently lacks,
BuildRequires: cmake
Comment 9 Petr Šabata 2014-01-13 07:58:00 EST
(In reply to Rex Dieter from comment #6)
> This is why:
> ExclusiveArch:	i686

Ah, right.  I just built it manually.  I don't think that line is needed.
Comment 10 Pavol Rusnak 2014-01-13 11:24:47 EST
(In reply to Petr Šabata from comment #2)

I hope you thoroughly check nationality and residency of all other contributors as well ...
Comment 11 Mohammed Isam 2014-01-13 22:40:01 EST
(In reply to Petr Šabata from comment #2)
> Hello Mohammed,
> 
> I'd love to sponsor you and help you with your project, however first I'd
> like to check whether it's even possible.
> 
> I see you're Sudanese and assume your project is also developed in your home
> country.  As Fedora is a US project, it has to respect the US export laws. 
> The OFAC Sudan Sanctions Program mentios "goods, technology, or services of
> Sudanese origin" several times so let's first check with Fedora Legal.
> 
> Hope this turns out well.

Thanks a great deal, I really appreciate your support. In fact I am residing and working outside of Sudan, from Doha, Qatar, I don't know if this makes any difference??..
Comment 12 Mohammed Isam 2014-01-13 22:44:24 EST
(In reply to Rex Dieter from comment #8)
> while we're at it...
> 
> Since this uses,
> 
> %build
> %cmake .
> 
> it currently lacks,
> BuildRequires: cmake

I don't want to sound dumb, but this is my first package you know.. so what can I change in the spec file to start with?
Comment 13 Mohammed Isam 2014-01-13 22:45:38 EST
not to mention of course your feedback about the package itself, uselful or not.. ease of use and what-not..
Comment 14 Mohammed Isam 2014-01-13 23:20:44 EST
(In reply to Pavol Rusnak from comment #10)
> (In reply to Petr Šabata from comment #2)
> 
> I hope you thoroughly check nationality and residency of all other
> contributors as well ...

It's a solo project.. no other contributors so far.. Although all is welcomed!!
Comment 15 Petr Šabata 2014-01-14 03:10:31 EST
(In reply to Mohammed Isam from comment #12)
> I don't want to sound dumb, but this is my first package you know.. so what
> can I change in the spec file to start with?

Please, go through the Fedora Packaging Guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines

It's a fairly long document with many specific rules for various SIGs (Special Interest Groups).  It will take a few packages until you get comfortable with all that.

What you need to change in your package now is:

1. Don't use ExclusiveArch or BuildArch unless there's a reason for it.  Your code seems to run on non-i686 too and if it breaks on some arches, you should fix the code rather than excluding those.

2. There's a list of packages that are always present in the buildroot and hence don't need to be explicitly listed in your BuildRequires even if you use them:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Exceptions_2
You see coreutils is there (plus your SPEC file doesn't use it anyway).  Drop that build-time dependency.

3. Your package doesn't appear to use coreutils at runtime either, therefore the Requires: coreutils is also unneeded.  Drop it.

4. On the other hand, you use cmake to build your application.  cmake is not a part of the base buildroot and needs to be BuildRequire'd.

5. Consider writing a manpage.
Comment 16 Mohammed Isam 2014-01-14 08:49:22 EST
(In reply to Petr Šabata from comment #15)
> (In reply to Mohammed Isam from comment #12)
> > I don't want to sound dumb, but this is my first package you know.. so what
> > can I change in the spec file to start with?
> 
> Please, go through the Fedora Packaging Guidelines:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines
> 
> It's a fairly long document with many specific rules for various SIGs
> (Special Interest Groups).  It will take a few packages until you get
> comfortable with all that.
> 
> What you need to change in your package now is:
> 
> 1. Don't use ExclusiveArch or BuildArch unless there's a reason for it. 
> Your code seems to run on non-i686 too and if it breaks on some arches, you
> should fix the code rather than excluding those.
> 
> 2. There's a list of packages that are always present in the buildroot and
> hence don't need to be explicitly listed in your BuildRequires even if you
> use them:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Exceptions_2
> You see coreutils is there (plus your SPEC file doesn't use it anyway). 
> Drop that build-time dependency.
> 
> 3. Your package doesn't appear to use coreutils at runtime either, therefore
> the Requires: coreutils is also unneeded.  Drop it.
> 
> 4. On the other hand, you use cmake to build your application.  cmake is not
> a part of the base buildroot and needs to be BuildRequire'd.
> 
> 5. Consider writing a manpage.

all noted.. I changed the spec file as in 1-4, wrote a man page and added it to the package, recompiled the package and uploaded it (replaced the old spec and rpm -- same URL).. please see and advice..
Comment 17 Petr Šabata 2014-01-14 09:28:34 EST
The standalone SPEC and the one bundled inside the SRPM differ.

Also, don't bundle the gziped manpage, just install the uncompressed one and list it as %{_mandir}/man1/xtv.1*.  rpmbuild should take care of that.

Your project tarball also contains two slightly different README files.  The one in src/ seems a bit outdated (it also states the project is licensed under GPL, unlike the rest of the files).  I suggest removing it.
Comment 18 Mohammed Isam 2014-01-15 23:34:15 EST
removed the gz manpage as above.. updated the links.. the README file is used in the xtv under the help menu, and it reads the file from the '.' directory.. so I changed the code to read README from the doc directory and removed the README from inside src/..
Comment 19 Tom "spot" Callaway 2014-02-26 15:14:36 EST
After review with Red Hat, there is no issue with this contributor or this package. Lifting FE-Legal.
Comment 20 Petr Šabata 2014-03-17 06:09:30 EDT
I've sponsored Mohammed, removing the blocker.
Comment 21 Mohammed Isam 2014-03-17 15:34:03 EDT
New Package SCM Request
=======================
Package Name: xtv
Short Description: A file manager for the Linux console/xterm
Owners: mohammedisam
Branches: f20
InitialCC:
Comment 22 Jon Ciesla 2014-03-17 16:09:55 EDT
fedora-review flag not set
Comment 23 Petr Šabata 2014-03-18 03:59:49 EDT
Okay, I've reviewed the changes and it seems good enough now.
Approving.

Perhaps you could install the manpage and the infopage with cmake in the future.
Comment 24 Mohammed Isam 2014-03-18 10:09:08 EDT
Okay.. I will fix the man/info pages into cmake and update..
Comment 25 Jon Ciesla 2014-03-18 10:18:35 EDT
Git done (by process-git-requests).
Comment 26 Fedora Update System 2014-03-19 02:19:28 EDT
xtv-1.0-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/xtv-1.0-1.fc20
Comment 27 Fedora Update System 2014-03-19 09:13:46 EDT
xtv-1.0-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/xtv-1.0-2.fc20
Comment 28 Fedora Update System 2014-03-19 23:06:34 EDT
xtv-1.0-1.fc20 has been pushed to the Fedora 20 testing repository.
Comment 29 Fedora Update System 2014-03-27 23:15:10 EDT
xtv-1.0-1.fc20 has been pushed to the Fedora 20 stable repository.
Comment 30 Fedora Update System 2014-03-30 02:08:27 EDT
xtv-1.0-2.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.