Bug 229490

Summary: Review Request: pypar2 - graphical frontend to par2cmdline
Product: [Fedora] Fedora Reporter: Maxime Carron <maxime.carron>
Component: Package ReviewAssignee: Chitlesh GOORAH <chitlesh>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: chitlesh, laurent.rineau__fedora, lxtnow, maxime, mtasaka
Target Milestone: ---Flags: chitlesh: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-06-20 20:45:59 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:
Attachments:
Description Flags
spec file none

Description Maxime Carron 2007-02-21 15:13:40 UTC
Spec URL: http://carron.maxime.free.fr/fedora/rpms/pypar2.spec
SRPM URL: http://carron.maxime.free.fr/fedora/rpms/pypar2-1.2-1.fc6.src.rpm
Description: A graphical frontend to par2cmdline written in Python.

This is the first RPM I propose on Extras.
I've been helped by the french community (a small review have already been done : http://forums.fedora-fr.org/viewtopic.php?id=18581)

As it's the first time I contribute, I also need a sponsor.

Thanks,

Maxime

Comment 1 Xavier Lamien 2007-02-21 16:27:02 UTC
Hi Maxime,

I'll make a full review within a day, in the meantime you find out a sponsor.


Comment 2 Maxime Carron 2007-02-22 10:49:00 UTC
There is a colored version of my specfile for online reading at
http://carron.maxime.free.fr/fedora/rpms/pypar2.spec.html

Comment 3 Xavier Lamien 2007-02-23 17:47:41 UTC
Symlink should be relative.
The use of:
ln -s %{_datadir}/%{name}/src/main py %{buildroot}%{_bindir}/%{name}
doesn't look good.

rpmlint output from rpm:

W: symlink-should-be-relative


Comment 4 Xavier Lamien 2007-02-26 20:38:06 UTC
ping ?

Comment 5 Maxime Carron 2007-02-27 16:54:33 UTC
pong!
(I'm busy at school. Just one more test!)

W: symlink-should-be-relative
I can't do anything else.

If the link is relative, it links to the building environment
(/var/tmp/pypar2-1.2-1.fc6.maxca-root-builder/usr/share/pypar2/src/main.py).

And actually, due to macro, it is relative (I think).


PS : I still need a sponsor.

Comment 6 Mamoru TASAKA 2007-02-27 17:06:45 UTC
(In reply to comment #5)
> pong!
> (I'm busy at school. Just one more test!)
> 
> W: symlink-should-be-relative
> I can't do anything else.

Hint:
------------------------------------------------
rm %{buildroot}%{_bindir}/%{name}
ln -s %{_datadir}/%{name}/src/main.py %{buildroot}%{_bindir}/%{name}
------------------------------------------------
This symlink uses absolute path for main.py

One of the easiest way to create relative symlink is:
-------------------------------------------------
rm %{buildroot}%{_bindir}/%{name}
pushd %{buildroot}%{_bindir}
ln -sf `find .. -name main.py` %{name}
popd
--------------------------------------------------
This creates a symlink as
pypar2 -> ../share/pypar2/src/main.py

Comment 7 Xavier Lamien 2007-02-27 17:24:10 UTC
I tried this way:
ls -s ../..%{_datadir}/%{name}/src/main.py %{buildroot}%{_bindir}/%{name}

it's work but symlink point like that :

/usr/bin/pypar2 -> ../../usr/share/pypar2/src/main.py



Comment 8 Xavier Lamien 2007-02-27 17:26:57 UTC
typo: it's "ln -s"  not "ls -s"

Comment 9 Paul Howarth 2007-03-01 15:06:58 UTC
(In reply to comment #7)
> I tried this way:
> ls -s ../..%{_datadir}/%{name}/src/main.py %{buildroot}%{_bindir}/%{name}
> 
> it's work but symlink point like that :
> 
> /usr/bin/pypar2 -> ../../usr/share/pypar2/src/main.py

This method assumes that %{_bindir} is two directory levels down from the root
directory, which is OK for Fedora but might cause issues for people wanting to
use the package elsewhere (e.g. with %{_bindir} = /usr/site/bin etc.)

A simple way to avoid the problem is to create the symlink as absolute within
the buildroot and then use the symlinks utility to convert it to an optimized
relative link:

ln -s %{buildroot}%{_datadir}/%{name}/src/main.py %{buildroot}%{_bindir}/%{name}
symlinks -cs %{buildroot}%{_bindir}

Result on Fedora:
/usr/bin/pypar2 -> ../share/pypar2/src/main.py

If you take this approach, you'll need to add symlinks as a buildrequire.


Comment 10 Mamoru TASAKA 2007-03-01 15:29:43 UTC
INFO: 1.3 is out.

Comment 11 Maxime Carron 2007-03-03 14:24:53 UTC
> INFO: 1.3 is out.
yep!!

Spec URL: http://carron.maxime.free.fr/fedora/rpms/pypar2.spec
Read it online at http://carron.maxime.free.fr/fedora/rpms/pypar2.spec.html
SRPM URL: http://carron.maxime.free.fr/fedora/rpms/pypar2-1.3-1.fc6.src.rpm

* rpmlint is now quiet
   (In reply to comment #9)
   Thanks Paul, I followed your advices.
* I tried fedora-qa script
   i added gettext as BR


Comment 12 Xavier Lamien 2007-03-03 15:35:06 UTC
OK, sound goog...

Just a comment: why set "Requires: to complete."
Are you thinking about missing Requires ?

Comment 13 Maxime Carron 2007-03-03 16:01:44 UTC
yes.

I thought rpm was able to find all requires by itsefl, but it doesn't seem so.
I need to make some few more tests.

http://www.fedoraproject.org/wiki/Packaging/Guidelines#head-0711805dd733fe3b31741e9d5236d72941a79d94
Actually, only librairies are found by rpm (if I understand right).

Comment 15 Xavier Lamien 2007-03-06 19:29:42 UTC
Well,

I'll check them out.

Comment 16 Xavier Lamien 2007-03-17 15:19:23 UTC
OK - Mock Build on FC-6 FC-Devel (i386)
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License is GPL
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
4343ff1521dfd8c524428f707e7c3068  pypar2-1.3.tar.gz
OK - Package has correct buildroot.
OK - BuildRequires isn't redundant.
OK - patch0 is applied correctly and work.
OK - %build and %install stages is correct and work.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Changelog section is correct.
OK - rpmlint output are clean on RPM and SRPM files.

OK - Should function as described.
OK - Should package latest version

--------------------------
Installed package
--------------------------

OK - .desktop file is correct and work.
OK - GTK front-end start with no error.
OK - All buttom from front-end work properly.
Ok - Files associations is proper.

--------------------------



Comment 17 Chitlesh GOORAH 2007-03-18 17:12:49 UTC
Ok, I'm sponsoring Maxime Carron.

Comment 18 Maxime Carron 2007-03-18 18:07:04 UTC
Chitlesh, here is the build.log :
http://carron.maxime.free.fr/fedora/rpms/pypar2-1.3-2.build.log

Thanks for your sponsoring.

Comment 19 Laurent Rineau 2007-03-23 12:58:50 UTC
As the packager of par2cmdline, I always wanted a GUI for par2cmdline in 
Extra. gpar2 (from parchive.sf.net) was too badly written to be accepted in 
Extras (see deferred bug #190991 and bug #190992). PyPar2 will hopfully be a 
solution.

I will be quite busy in my job, until summer. I offered Maxime to co-maintain 
par2cmdline. Seems that he accepted (actually par2cmdline required no 
intervention in the past year but the FC6 mass-rebuild). I will try to find 
out in the Wiki what is the procedure to make somebody co-maintain a 
package...

Congratulations, Maxime!


Comment 20 Chitlesh GOORAH 2007-03-26 17:28:24 UTC
#001: is there any reason why the package isn't called as PyPar2 instead of 
pypar ?

#002: you can drop the following from the package
 - /usr/share/doc/pypar2-1.3/install.txt
The user doesn't need it to install PyPar2, but yum
 - /usr/share/doc/pypar2-1.3/pypar2.1
This is a duplicate of /usr/share/man/man1/pypar2.1.gz

#003: Do you have 
%dist  .fc6 
on your ~/.rpmmacros ?

If you do please remove it, if not you will have tag setting problems on 
fedora's CVS.

Comment 21 Chitlesh GOORAH 2007-03-26 17:36:15 UTC
Though I won't consider the following as a blocker, I would like you to use 
the following instead of the patch.

desktop-file-install --vendor ""  \
    --add-category Utilities      \
    --remove-category GTK         \
    --remove-category Application \
[...]

Comment 22 Chitlesh GOORAH 2007-03-26 18:17:03 UTC
comment #21 will also fix:
+desktop-file-install --vendor= --dir=/var/tmp/pypar2-1.3-2-root-chitlesh/usr/share/applications /var/tmp/pypar2-1.3-2-root-chitlesh//usr/share/applications/pypar2.desktop
/var/tmp/pypar2-1.3-2-root-chitlesh//usr/share/applications/pypar2.desktop: 
key "Categories" string list not semicolon-terminated, fixing

just to point out:
%{buildroot}/%{_datadir}/applications/%{name}.desktop
>> /var/tmp/pypar2-1.3-2-root-chitlesh//usr/share/applications/pypar2.desktop

but
%{buildroot}%{_datadir}/applications/%{name}.desktop
>> /var/tmp/pypar2-1.3-2-root-chitlesh/usr/share/applications/pypar2.desktop

%{_datadir} already possesses '/' as its first character

Comment 23 Maxime Carron 2007-03-26 19:14:08 UTC
#001 : euh... because that's how its autor called it. Pypar2 is a frontend to
the par2 utility.

#002 : done

#003 : yes i've "%dist  .fc6" in my ~/.rpmmacros
What do i have to do?
Do i need to comment this line in the ~/.rpmmacros and rebuild it?

Comment 24 Chitlesh GOORAH 2007-03-27 20:25:10 UTC
(In reply to comment #23)
> #001 : euh... because that's how its autor called it. Pypar2 is a frontend 
to
> the par2 utility.
> 

On http://pypar2.silent-blade.org/, I see
_PyPar2_ is a graphical frontend for the Linux par2 utility

I suggest renaming your %{name} macro and your spec file.
Thus you can update:
%setup -q -n PyPar2-%{version}
to
%setup -q

> #003 : yes i've "%dist  .fc6" in my ~/.rpmmacros
> What do i have to do?
> Do i need to comment this line in the ~/.rpmmacros and rebuild it?

You should simply remove it from your ~/.rpmmacros. If possible contact the 
writer of the tutorial you read, because it isn't stated in the actual fedora 
guidelines.



Comment 25 Maxime Carron 2007-03-28 22:35:21 UTC
DONE :
======
In reply to comment #21
* I have removed the patch and fix the command as you suggested.

In reply to comment #22
* I also fix the problem of the '/'

TO DO :
=======
In reply to comment #22
* About changing the name is the specfile pypar2 by PyPar2 : I won't be able to
use the macro name during the spec, because all files or directories are named
"pypar2" and not "PyPar2"

What do you suggest?

(Thanks a lot for your help)

Comment 27 Maxime Carron 2007-04-01 09:42:37 UTC
Here is the srpm without any tag (%vendor, %dist, ...) in the .rpmmacros
SRPM : http://carron.maxime.free.fr/fedora/rpms/pypar2-1.3-3.src.rpm


Comment 28 Maxime Carron 2007-04-07 23:52:59 UTC
up!

What do i do now?
What about the question in comment #25.

Comment 29 Chitlesh GOORAH 2007-04-11 10:45:31 UTC
(In reply to comment #21)
> desktop-file-install --vendor ""  \
>     --add-category Utilities      \
> [...]

It should be "Utility" instead of "Utilities".

If not, during the build you will have:

/var/tmp/pypar2-1.3-3.fc7-root-chitlesh/usr/share/applications/pypar2.desktop:
warning: Categories values must be one of "AudioVideo", "Audio", "Video",
"Development", "Education", "Game", "Graphics", "Network", "Office", "Settings",
"System", "Utility", "Building", "Debugger", "IDE", "GUIDesigner", "Profiling",
"RevisionControl", "Translation", "Calendar", "ContactManagement", "Database",
"Dictionary", "Chart", "Email", "Finance", "FlowChart", "PDA",
"ProjectManagement", "Presentation", "Spreadsheet", "WordProcessor",
"2DGraphics", "VectorGraphics", "RasterGraphics", "3DGraphics", "Scanning",
"OCR", "Photography", "Viewer", "DesktopSettings", "HardwareSettings",
"PackageManager", "Dialup", "InstantMessaging", "IRCClient", "FileTransfer",
"HamRadio", "News", "P2P", "RemoteAccess", "Telephony", "WebBrowser",
"WebDevelopment", "Midi", "Mixer", "Sequencer", "Tuner", "TV",
"AudioVideoEditing", "Player", "Recorder", "DiscBurning", "ActionGame",
"AdventureGame", "ArcadeGame", "BoardGame", "BlocksGame", "CardGame",
"KidsGame", "LogicGame", "RolePlaying", "Simulation", "SportsGame",
"StrategyGame", "Art", "Construction", "Music", "Languages", "Science",
"Astronomy", "Biology", "Chemistry", "Geology", "Math", "MedicalSoftware",
"Physics", "Amusement", "Archiving", "Electronics", "Emulator", "Engineering",
"FileManager", "TerminalEmulator", "Filesystem", "Monitor", "Security",
"Accessibility", "Calculator", "Clock", "TextEditor", "Core", "KDE", "GNOME",
"GTK", "Qt", "Motif", "Java", "ConsoleOnly", "Screensaver", "TrayIcon",
"Applet", "Shell" (found "Utilities")

Comment 30 Maxime Carron 2007-04-18 21:21:07 UTC
How did you get this message?

Comment 31 Chitlesh GOORAH 2007-04-18 21:38:36 UTC
during rpmbuild -ba pypar2.spec on F7-devel

Comment 32 Mamoru TASAKA 2007-04-19 09:47:24 UTC
FYI pypar2 1.4 is out.

Comment 33 Maxime Carron 2007-04-19 11:17:57 UTC
Yep, i'm working on.

I still have a problem.
In reply to comment #29, i've changed Utilities in Utility, but there is
something I don't understand.

Shouldn't 
desktop-file-install --vendor ""  \
     --add-category Utility       \
     [...]
modify the desktop file?

Because since i don't use the patch (for the desktop file) ther is no entry in
thefedora menu for pypar2.

Do I need to use both patch and desktop-file-install options (--add-category and
--remove-category)?

Comment 34 Mamoru TASAKA 2007-04-19 12:24:16 UTC
(In reply to comment #33)
> Yep, i'm working on.
>
> Do I need to use both patch and desktop-file-install options 
> (--add-category and
> --remove-category)?

I just saw 1.4 desktop file and both
--add-category Utility --remove-category Application
seems needed


Comment 35 Chitlesh GOORAH 2007-04-23 15:04:58 UTC
Maxime , Can you to 1.4 please so that we can continue with the review ?

Comment 36 Maxime Carron 2007-04-24 11:21:28 UTC
Yes, i'm on it.

But i still have my problem :
If i don't use patch, there isn't any menu entry for pypar2.

I'll post my spec and srpm for version 1.4 with the problem tonight.

Comment 37 Chitlesh GOORAH 2007-04-24 18:00:47 UTC
Created attachment 153377 [details]
spec file

Maxime, here is the spec file with my modifications. Have a look and bump a
release for approval.

Comment 38 Maxime Carron 2007-04-24 18:57:55 UTC
OK!

All is right now.

SPEC : http://carron.maxime.free.fr/fedora/rpms/pypar2.spec
Read it online : http://carron.maxime.free.fr/fedora/rpms/pypar2-1.4-1.spec.html
SRPM : http://carron.maxime.free.fr/fedora/rpms/pypar2-1.4-1.src.rpm

Thanks a lot for your fixes

Comment 39 Chitlesh GOORAH 2007-04-26 08:33:41 UTC
MUST Items:

- MUST: rpmlint's output is clean
- MUST: The package is named according to the Package Naming Guidelines.
- MUST: The spec file name matches the base package %{name}
- MUST: The package meets the Packaging Guidelines.
- MUST: The package is licensed (GPL) with an open-source compatible license and
meet other legal requirements as defined in the legal section of Packaging
Guidelines.
- MUST: The License field in the package spec file matches the actual license.
- MUST: 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.
- MUST: The spec file must be written in American English.
- MUST: The spec file for the package is be legible. 
- MUST: The sources used to build the package must matches the upstream source,
as provided in the spec URL.
- MUST: The package successfully compiles and builds into binary rpms on at
least i386.
- MUST: All build dependencies is listed in BuildRequires.
- MUST: The spec file handles locales properly.
- MUST: If the package does not contain shared library files located in the
dynamic linker's default paths
- MUST: the package is not designed to be relocatable
- MUST: the package owns all directories that it creates.
- MUST: the package does not contain any duplicate files in the %files listing.
- MUST: Permissions on files are set properly.
- MUST: The package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
- MUST: The package consistently uses macros, as described in the macros section
of Packaging Guidelines.
- MUST: The package contains code, or permissable content. This is described in
detail in the code vs. content section of Packaging Guidelines.
- MUST: There are no Large documentation files
- MUST: %doc does not affect the runtime of the application. To summarize: If it
is in %doc, the program must run properly if it is not present.
- MUST: There are no Header files or static libraries 
- MUST: The package does not contain library files with a suffix 
- MUST: Package does NOT contain any .la libtool archives
- MUST: Package containing GUI applications includes a %{name}.desktop file, and
that file must be properly installed with desktop-file-install in the %install
section.
- MUST: Package does not own files or directories already owned by other packages. 

SHOULD Items:

 - SHOULD: The source package does include license text(s) as gpl.txt
 - SHOULD: mock builds succcessfully in i386.
 - SHOULD: The reviewer tested that the package functions as described. A
package should not segfault instead of running, for example.
 - SHOULD: No scriptlets were used, those scriptlets must be sane. 
 - SHOULD: No subpackages present.

APPROVED

Comment 40 Chitlesh GOORAH 2007-04-26 08:38:30 UTC
Follow http://fedoraproject.org/wiki/CVSAdminProcedure for the CVS Request
procedure.

As we agreed, add me as your co-maintainer.

Comment 41 Maxime Carron 2007-04-27 08:39:19 UTC
New Package CVS Request
=======================
Package Name: pypar2
Short Description: A GTK frontend for par2cmdline
Owners: maxime.carron, cgoorah.au
Branches: FC-5 FC-6


Comment 42 Laurent Rineau 2007-04-27 09:31:22 UTC
(In reply to comment #41)
> New Package CVS Request [...]

You need to set the flag fedora-cvs, for such requests.


Comment 43 Chitlesh GOORAH 2007-04-27 10:17:14 UTC
I'm doing it for him :)

Comment 44 Maxime Carron 2007-04-27 11:05:29 UTC
Thanks chitlesh.

Strangely i got "You tried to request fedora-cvs. Only an authorized user can
make this change." when i tried.

Comment 45 Mamoru TASAKA 2007-04-27 11:12:15 UTC
(In reply to comment #44)
> Strangely i got "You tried to request fedora-cvs. Only an authorized user can
> make this change." when i tried.

I just noticed from your comment:
(In reply to comment #0)
> This is the first RPM I propose on Extras.
Laurent, this means that you have to get sponsored?

Comment 46 Mamoru TASAKA 2007-04-27 11:13:02 UTC
(In reply to comment #45)
> Laurent, this means that you have to get sponsored?

Sorry, this comment of mine was for Maxime.

Comment 47 Maxime Carron 2007-04-27 12:10:27 UTC
Chitlesh is already my sponsor, no?

For the account system :
> Group Name	Username	Role Type	Role Status	Sponsor
> cvsextras 	mxcarron 		user 	approved 	chitlesh

Comment 48 Laurent Rineau 2007-04-27 12:36:03 UTC
(In reply to comment #47)
> Chitlesh is already my sponsor, no?
> 
> For the account system :
> > Group Name    Username        Role Type       Role Status     Sponsor
> > cvsextras     mxcarron                user    approved        chitlesh

Yes.

As for the "fedora-cvs flag" of the current bug, what you need is to be member 
of  the "fedorabugs" group.


Comment 49 Maxime Carron 2007-04-27 12:54:05 UTC
I'm also member of fedorabugs.

The problem is : how my account on the bugzilla is linked to my fedora account?
I think they aren't linked. That's why i'm not recognized.

Comment 50 Laurent Rineau 2007-05-27 15:41:56 UTC
pypar2 is approved since comment #39.

Maxime, what blocks you from importing it into Fedora?


Comment 51 Maxime Carron 2007-05-28 17:03:17 UTC
My email address.

I created my bugzilla account with the @fp.org email address.
This mail doesn't correspond with the email address in my account settings.
Hence I can't upload files.

In addition, i'm looking for a new mail service.
Then i'll reset my account, ...

Sorry for the delay.

Comment 52 Laurent Rineau 2007-06-12 08:08:25 UTC
(In reply to comment #51)
> I created my bugzilla account with the @fp.org email address.
> This mail doesn't correspond with the email address in my account settings.
> Hence I can't upload files.

Where cannot you upload file?

As far as I understand, you cannot require for a CVS branch, because your 
email address in not in fedorabugs.

Please change quickly your Bugzilla email adresse, here:
  https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
and commit you package to at least Rawhide.

In my opinion, you does no longer seem to be interrested in contributing to 
Fedora. If so, please tell us, so that somebody can take over that pypar2.


Comment 54 Maxime CARRON 2007-06-12 09:02:21 UTC
Here I am, with my new account!!

See bug #243826

I added all of you in CC field.


Comment 55 Chitlesh GOORAH 2007-06-12 09:38:36 UTC
*** Bug 243826 has been marked as a duplicate of this bug. ***

Comment 56 Chitlesh GOORAH 2007-06-12 09:45:44 UTC
Package Change Request
=======================
Owners: maxime, cgoorah.au

Comment 57 Maxime CARRON 2007-06-12 10:30:27 UTC
Package Change Request
======================
Package Name: pypar2
New Branches: F-7



Comment 58 Kevin Fenzi 2007-06-12 23:34:59 UTC
cvs done.