Bug 895541 (ptbl) - Review Request: ptbl - Periodic Table
Summary: Review Request: ptbl - Periodic Table
Keywords:
Status: CLOSED NOTABUG
Alias: ptbl
Product: Fedora
Classification: Fedora
Component: zzuf
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-01-15 13:43 UTC by RudraB
Modified: 2014-01-16 09:21 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-11-03 17:18:48 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description RudraB 2013-01-15 13:43:40 UTC
Spec URL: http://web.warwick.ac.uk/~phslav/ptbl.spec
SRPM URL: http://web.warwick.ac.uk/~phslav/ptbl-1.0-1.fc18.src.rpm
Description: ptbl is a Periodic Table package written in C and GTK2
Fedora Account System Username: baptu

Comment 1 Antonio T. (sagitter) 2013-01-15 14:06:28 UTC
Hi RubraB.
Just few initial comments.

- Buildroot tag is necessary only if this package will be supported in EPEL5 and below. If it be so, "The BuildRoot value MUST be below %{_tmppath}/ and MUST contain at least %{name}, %{version} and %{release}. It may invoke mktemp since this is guaranteed to exist on every system. From there, packagers are expected to use a sane BuildRoot."
http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag

- NEWS and README files are empty. They can be erased.

- Changelog is empty. Why ?
http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

- You should consider macros' use. For instance:

%{_bindir}/%{name} instead %{_bindir}/ptbl

Comment 2 RudraB 2013-01-15 14:45:22 UTC
Antonio Trande,
 spec file is changed according to your suggestion and new spec and srpm uploaded in the same url

Comment 3 Antonio T. (sagitter) 2013-01-15 15:38:56 UTC
(In reply to comment #2)
> Antonio Trande,
>  spec file is changed according to your suggestion and new spec and srpm
> uploaded in the same url

Not quite right. ;)

At the comment #2 I add:

- Group tag is optional (http://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag), however 'Education' is not valid as you can see by using 'rpmlint' command:

$ rpmlint -I non-standard-group
non-standard-group:
The value of the Group tag in the package is not valid.  Valid groups are:
"Amusements/Games", "Amusements/Graphics", "Applications/Archiving",
"Applications/Communications", "Applications/Databases",
"Applications/Editors", "Applications/Emulators", "Applications/Engineering",
"Applications/File", "Applications/Internet", "Applications/Multimedia",
"Applications/Productivity", "Applications/Publishing", "Applications/System",
"Applications/Text", "Development/Debug", "Development/Debuggers",
"Development/Languages", "Development/Libraries", "Development/System",
"Development/Tools", "Documentation", "System Environment/Base", "System
Environment/Daemons", "System Environment/Kernel", "System
Environment/Libraries", "System Environment/Shells", "Unspecified", "User
Interface/Desktops", "User Interface/X", "User Interface/X Hardware Support".

- 'BuildRequires:  gnome-doc-utils >= 0.3.2'  is missing.

- "Your package should own all of the files that are installed as part of the %install process."
http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

In my opinion, you should do some changes. ;)

Comment 4 RudraB 2013-01-15 15:56:47 UTC
Hi Antonio,
Thanks for review. 
I  have done other changes, but confused about ownership point. I re-read the section you pointed, but still clueless.
what should I do?

Comment 5 RudraB 2013-01-15 16:04:06 UTC
$ rpmlint  ./ptbl.spec ../SRPMS/ptbl-1.0-1.fc18.src.rpm 
ptbl.src: W: no-version-in-last-changelog
ptbl.src: W: strange-permission ptbl-1.0.tar.gz 0600L
1 packages and 1 specfiles checked; 0 errors, 2 warnings.
 is the warning I am getting!

Comment 6 Antonio T. (sagitter) 2013-01-15 16:16:35 UTC
(In reply to comment #4)
> Hi Antonio,
> Thanks for review. 
> I  have done other changes, but confused about ownership point. I re-read
> the section you pointed, but still clueless.
> what should I do?

All files/directories created by package must be owned of package; for instance:

$ rpm -q --list htop
/usr/bin/htop
/usr/share/doc/htop-1.0.2
/usr/share/doc/htop-1.0.2/AUTHORS
/usr/share/doc/htop-1.0.2/COPYING
/usr/share/doc/htop-1.0.2/ChangeLog
/usr/share/doc/htop-1.0.2/README
/usr/share/man/man1/htop.1.gz
/usr/share/pixmaps/htop.png

The '%file' section must contain all instructions to do that according to the Guidelines. Read also http://fedoraproject.org/wiki/Packaging:UnownedDirectories

Comment 7 RudraB 2013-01-15 16:28:32 UTC
Do you mean adding something like:
%defattr(644,root,root,755)
%attr(755,root,root) %{_bindir}/*

Comment 8 Rex Dieter 2013-01-15 18:09:40 UTC
One small suggestion (pet peave of mine), I'd suggest removing from Summary/%description the phrase:
"written on C and GTK for GNOME desktop"
end users generally don't care or need what language the app is written in, annd unless it *only* runs on gnome, don't mention that either.

(if you disagree, at least fix the grammar to say "in" instead of "on")

Comment 9 Antonio T. (sagitter) 2013-01-15 18:30:04 UTC
(In reply to comment #7)
> Do you mean adding something like:
> %defattr(644,root,root,755)
> %attr(755,root,root) %{_bindir}/*

No.

For instance:

Your package includes some sub-directories in the directory '/usr/share/gnome/help'; those sub-directories contain .xml files. Therefore on the '%file' section these must be indicated as owned of package.
'ptbl' binary is owned of package; ... and so on.

So:

%files
%doc COPYING
%{_bindir}/%{name}

%dir %{_datadir}/gnome/help/%{name}-manual
%{_datadir}/gnome/help/%{name}-manual/*/*.xml

...

> ptbl.src: W: strange-permission ptbl-1.0.tar.gz 0600L

Set the right permission for this file by editing .spec file
https://fedoraproject.org/wiki/Common_Rpmlint_issues?rd=PackageMaintainers/Common_Rpmlint_Issues#strange-permission

Now, you have everything necessary in order to complete your spec file. :)

Comment 10 Kalev Lember 2013-01-15 18:35:13 UTC
Some drive-by comments:

1) Should use the %{version} in the %Source URL, this way there's only one place where you need to update the version number afterwards.

2)
> BuildRequires:	atk
> BuildRequires:	gdk-pixbuf2
> BuildRequires:	pango

These should probably read 'atk-devel', 'gdk-pixbuf2', and 'pango-devel' instead. The -devel packages have the needed .so symlinks / pkgconfig files that are needed for linking against these libraries.

Note that it would probably be safe to just remove these 3 lines; gtk2-devel should drag them in by its requires. But you can list them explicitly too if you want to, either way works.

You can verify that all the needed BuildRequires are in place by doing koji scratch builds. Follow the instructions at the https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Install_the_client_tools_.28Koji.29_and_set_up_your_certificate page and then adjust the build requires until the scratch builds succeed. 'koji build --scratch ptbl-1.0-1.fc18.src.rpm'.

Comment 11 Kalev Lember 2013-01-15 18:40:06 UTC
> %changelog
> * Tue Jan 15 2013 Rudra Banerjee <rudra.banerjee.uk> 
>  -Initial Package
>  -spec file changed as per https://bugzilla.redhat.com/show_bug.cgi?id=895541#c1
>  -spec file updated:
>      -- install and file field edited as suggest by kalev and rdieter in #fedora-devel

A nitpick about the %changelog format. In Fedora, we use the following format:

%changelog
* Tue Jan 15 2013 Rudra Banerjee <rudra.banerjee.uk> - 1.0-3
- spec file updated: install and file field edited as suggest by kalev and rdieter in #fedora-devel

* Tue Jan 15 2013 Rudra Banerjee <rudra.banerjee.uk> - 1.0-2
- spec file changed as per https://bugzilla.redhat.com/show_bug.cgi?id=895541#c1

* Tue Jan 15 2013 Rudra Banerjee <rudra.banerjee.uk> - 1.0-1
- Initial Package

Comment 12 RudraB 2013-01-15 20:15:24 UTC
The scratch build are here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4871951

Comment 13 Mario Blättermann 2013-01-15 20:22:13 UTC
You are not sponsored yet in the packagers group, that's why I add FE-NEEDSPONSOR. Follow the guidelines to find a sponsor:
http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Comment 14 Michael Schwendt 2013-01-17 21:19:26 UTC
> $ rpmlint  ./ptbl.spec ../SRPMS/ptbl-1.0-1.fc18.src.rpm 
> ptbl.src: W: no-version-in-last-changelog
> ptbl.src: W: strange-permission ptbl-1.0.tar.gz 0600L
> 1 packages and 1 specfiles checked; 0 errors, 2 warnings.
>  is the warning I am getting!

Run "rpmlint -i" on _all_ rpms, the src.rpm _and_ all built rpms. The -i option will help you. Else ask.


I've downloaded the src.rpm from the koji scratch build, then used rpmdev-extract to examine its contents:

* I agree with comment 8.


> BuildRequires:  rarian-compat   

https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Scrollkeeper

I wonder why it uses scrollkeeper and runs no-op commands like scrollkeeper-update?


> %description
> Periodic Table application 

A few more words would be good. Not to explain what a periodic table is, but what the application can do. I mean, currently the %summary is even longer than the %descripton, and the description on your web page is better, too. ;)

https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description


> /usr/bin/ptbl

https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files


> %{_datadir}/images

That's an odd location for an application's images. Shouldn't it create its very own subdirectory as /usr/share/ptbl/images instead? The FHS suggests that.

$ repoquery --whatprovides /usr/share/images
linphone-mediastreamer-0:3.5.2-5.fc18.x86_64
linphone-0:3.5.2-4.fc18.x86_64
linphone-mediastreamer-0:3.5.2-5.fc18.i686
linphone-0:3.5.2-4.fc18.i686
 -> bug 896735

Comment 15 RudraB 2013-01-17 22:43:16 UTC
Michael Schwendt,
Thanks for your detailed review.

>$ rpmlint -i ./ptbl-1.0-1.fc18.src.rpm 
>ptbl.src: W: strange-permission ptbl-1.0.tar.gz 0600L
>A file that you listed to include in your package has strange permissions.
>Usually, a file should have 0644 permissions.

This issue is discussed in detail in #fedora-devel and it is suggested that this will be automatically corrected in final update in fedora. So, probably I can ignore this for the time being. Please comment.

>ptbl.src:14: W: mixed-use-of-spaces-and-tabs (spaces: line 14, tab: line 1)
>The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
>annoyance.  Use either spaces or tabs for indentation, not both.
Its probably more to do with style, but I will surely make it coherent in the next update(I have to write the man page, see below).

$ rpmlint -i ./ptbl-1.0-1.fc18.x86_64.rpm 
>ptbl.x86_64: W: incoherent-version-in-changelog 1.0-6 ['1.0-1.fc18', '1.0-1']
>The latest entry in %changelog contains a version identifier that is not
>coherent with the epoch:version-release tuple of the package.
I will change that accordingly

>ptbl.x86_64: E: zero-length /usr/share/doc/ptbl-1.0/AUTHORS
Will remove it
>ptbl.x86_64: W: no-manual-page-for-binary ptbl
>Each executable in standard binary directories should have a man page.
Will try to write the man page today and tomorrow. Please allow me some time.

>1 packages and 0 specfiles checked; 1 errors, 2 warnings.

I have already edited the image file path. 
After the man page is done and an svg as icon (I already have a .desktop file), I will resubmit the spec and srpm. 
Or please let me know.

Comment 16 Michael Schwendt 2013-01-18 10:53:20 UTC
> This issue is discussed in detail in #fedora-devel and it
> is suggested that this will be automatically corrected in
> final update in fedora.

No, please fix it during review. In particular since you will need to build an updated src.rpm anyway. You need to understand that warning, and it is very easy to fix this rather harmless one with a "chmod 0644 ptbl-1.0.tar.gz" prior to building the src.rpm. It is nothing that will be corrected "automatically", because you need to give files inside the src.rpm (and later in Fedora dist git, where this is much more important) proper default ownership and permissions.


> Will try to write the man page today and tomorrow.

Do notice that rpmlint says "SHOULD" and not "MUST. The review guidelines say:

| SHOULD: your package should contain man pages for binaries/scripts.
| If it doesn't, work with upstream to add them where they make sense.

"Where they make sense" may be an important part here. Unless you plan to add lots of command-line options to ptbl, its man page probably won't explain more than that ptbl is a graphical app and repeating a brief description and license information. That won't be very helpful. Especially not on an RPM based platform, where one can examine files also via their packages.

Comment 17 RudraB 2013-01-18 11:48:00 UTC
New spec and srpm is uploaded in 
http://web.warwick.ac.uk/~phslav/PTBL/ptbl-1.0-7.fc18.src.rpm
http://web.warwick.ac.uk/~phslav/PTBL/ptbl.spec

this is for quick update, and still have problems with man page and .desktop files. That will be changed soon(working on that).

Comment 18 RudraB 2013-01-18 17:03:12 UTC
current status:
x86_64]$ rpmlint -i ./ptbl-1.0-7.fc18.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

SRPMS]$ rpmlint -i ./ptbl-1.0-7.fc18.src.rpm 
ptbl.src: W: strange-permission ptbl.spec 0600L
A file that you listed to include in your package has strange permissions.
Usually, a file should have 0644 permissions.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.

koji build --scratch f19 ./ptbl-1.0-8.fc18.src.rpm 
Uploading srpm: ./ptbl-1.0-8.fc18.src.rpm
[====================================] 100% 00:00:01 157.78 KiB  84.78 KiB/sec
Created task: 4881763
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=4881763
.....
0 free  0 open  3 done  0 failed

4881763 build (f19, ptbl-1.0-8.fc18.src.rpm) completed successfully


Please check the updated spec and srpm at:
http://web.warwick.ac.uk/~phslav/PTBL/ptbl.spec
http://web.warwick.ac.uk/~phslav/PTBL/ptbl-1.0-7.fc18.src.rpm

Comment 19 Antonio T. (sagitter) 2013-01-18 17:08:09 UTC
(In reply to comment #18)
> Please check the updated spec and srpm at:
> http://web.warwick.ac.uk/~phslav/PTBL/ptbl.spec
> http://web.warwick.ac.uk/~phslav/PTBL/ptbl-1.0-7.fc18.src.rpm

spec file is not reachable for me.

Comment 21 RudraB 2013-01-18 17:14:54 UTC
Antonio Trande,
Sorry for the inconvenience.
Please checke comment no. 20

Comment 22 RudraB 2013-01-18 17:18:12 UTC
IMHO,
at present the only problem is the desktop icon, every thing else looks ok.

Comment 23 Antonio T. (sagitter) 2013-01-18 17:32:59 UTC
(In reply to comment #20)
> Since spec file is not accessible,
> http://web.warwick.ac.uk/~phslav/ptbl.spec
> http://web.warwick.ac.uk/~phslav/ptbl-1.0-8.fc18.src.rpm

Both links must be accessible and .spec file must be updated like that one in the src.rpm

Please read https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage.

%{_bindir}/%name is wrong. %{_bindir}/%{name}

'%file' section seems uncomplete and must be separated from '%changelog' section.

Comment 24 RudraB 2013-01-18 17:53:42 UTC
is any file still not accessible?

Comment 25 Michael Schwendt 2013-01-18 19:19:10 UTC
Both downloads are accessible, but why do you refuse to fix the permissions of the files inside the src.rpm? This time it's the spec file:

  $ rpmlint ptbl-1.0-8.fc18.src.rpm 
  ptbl.src: W: strange-permission ptbl.spec 0600L
  1 packages and 0 specfiles checked; 0 errors, 1 warnings.

That could be due to a systematic error at your side, such as a wrong umask value.

[...]

> install -p -D -m 0644 %{SOURCE2} %{buildroot}%{_datadir}/applications/ptbl.desktop

Indeed, re-reading the Desktop Files guidelines is recommended:
https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

  $ cat ptbl.desktop 
  [Desktop Entry]
  Type=Application
  Exec=ptbl
  Name=ptbl
  Categories=Utility;GTK;
  Icon=ptbl.png
  X-Desktop-File-Install-Version=0.21

The icon is not found so far.
https://fedoraproject.org/wiki/Packaging:Guidelines#Icon_tag_in_Desktop_Files

Name=Periodic Table
would be much better.


> Summary:       Periodic_Table

A dubious '_' in there. ;-)


> Source0:        http://download-mirror.savannah.gnu.org/releases/%{name}/%{name}-%{version}.tar.gz

This doesn't pass the guidelines yet:

| MUST: The sources used to build the package must match
| the upstream source, as provided in the spec URL. [...]
|
| https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

$ md5sum ptbl-1.0.tar.gz 
cbf3cedbe8a5a7909e7e2b033add7031  ptbl-1.0.tar.gz
$ rm -f ptbl-1.0.tar.gz 
$ spectool -g ptbl.spec 
...
$ md5sum ptbl-1.0.tar.gz 
8fcddcbbdfc2e5e07d1cd454b86703f3  ptbl-1.0.tar.gz


>  %description
> -Periodic Table application 
> +Periodic Table application. Each element shows a list of \
> +physical/chemical properties, mainly focusing at the need \
> +of Condensed Matter Physicist and Material Scientist. 

Backslashes at the end are unusual in %description and are NOT necessary.
https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description

Comment 26 Michael Schwendt 2013-01-18 19:22:40 UTC
It seems you've re-released a modified ptbl-1.0.tar.gz several times - that should not have happened. Better not get used to it.

Comment 27 RudraB 2013-01-18 19:57:23 UTC
(In reply to comment #26)
> It seems you've re-released a modified ptbl-1.0.tar.gz several times - that
> should not have happened. Better not get used to it.

This is done purely because of the man file confusion.
I will try not to do that in future.

Comment 28 RudraB 2013-01-18 20:35:23 UTC
(In reply to comment #25)
> Both downloads are accessible, but why do you refuse to fix the permissions
> of the files inside the src.rpm? This time it's the spec file:
> 
>   $ rpmlint ptbl-1.0-8.fc18.src.rpm 
>   ptbl.src: W: strange-permission ptbl.spec 0600L
>   1 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> That could be due to a systematic error at your side, such as a wrong umask
> value.
Actually I tried a lot, but dont know what's causing the problem. In my sourcedir, I do have 0644. Not sure how its chaning.
I even set it manually in SOURCE, which does not refelct in build

Comment 29 Michael Schwendt 2013-01-18 20:58:49 UTC
$ rpmls -p ptbl-1.0-8.fc18.src.rpm
-rw-rw-r--  ptbl-1.0.tar.gz
-rw-rw-r--  ptbl.1
-rw-rw-r--  ptbl.desktop
-rw-------  ptbl.spec

How do you build your src.rpm from the individual files?

$ umask
0022
$ rpm -i ptbl-1.0-8.fc18.src.rpm 
warning: user rudra does not exist - using root
warning: group rudra does not exist - using root
warning: user rudra does not exist - using root
warning: group rudra does not exist - using root
warning: user rudra does not exist - using root
warning: group rudra does not exist - using root
warning: user rudra does not exist - using root
warning: group rudra does not exist - using root
$ cd ~/…/SOURCES/ptbl-1.0
$ chmod 0644 *
$ rpmbuild -bs ptbl.spec 
Wrote: ~/…/SRPMS/ptbl-1.0-8.fc18.src.rpm
$ rpmls -p ~/…/SRPMS/ptbl-1.0-8.fc18.src.rpm
-rw-r--r--  ptbl-1.0.tar.gz
-rw-r--r--  ptbl.1
-rw-r--r--  ptbl.desktop
-rw-r--r--  ptbl.spec

Voila! How do you do it?

Comment 30 RudraB 2013-01-18 21:13:17 UTC
(In reply to comment #29)
> Voila! How do you do it?
Not Sure!
I have started with the spec and tar-ball, and ended up here!
This error was obviously shown to me, as both user and group exists :(

Comment 31 RudraB 2013-01-18 21:15:12 UTC
 Michael Schwendt,
are  you available in #f-devel? can you provide me some time?

Comment 32 Michael Schwendt 2013-01-19 10:19:21 UTC
No, sorry. I only use IRC when I "must" and when the real-time communication makes a little bit of sense at least. For Packaging related guidance, it is inefficient and a waste of time.

Comment 33 RudraB 2013-01-19 10:32:31 UTC
I have modified the current spec file, known to have some error.
The spec and "rpmbuild -ba" s log is at:
http://web.warwick.ac.uk/~phslav/ptbl.spec.error
http://web.warwick.ac.uk/~phslav/ptbl_rpmbuild.log

The error is:
error: Installed (but unpackaged) file(s) found:
   /usr/share/images/ptbl.png
    Installed (but unpackaged) file(s) found:
   /usr/share/images/ptbl.png

Any guidance please?

Comment 34 Michael Schwendt 2013-01-19 11:08:03 UTC
Well, that's one of the most basic packaging mistakes:

Your %files section must list the actual files that are installed into the %buildroot. Ptbl installs /usr/share/images/ptbl.png but you've changed the %files entry to %{_datadir}/%{name}/images/ptbl.png

Comment 35 RudraB 2013-01-19 11:29:04 UTC
according to your comment https://bugzilla.redhat.com/show_bug.cgi?id=895541#c14, and as policy, I should install the image at %{_datadir}/%{name}/images/ptbl.png

So, can you kindly tell me what should I change in %install, i tried to install that using:
install -p -m 0755 images/* %{buildroot}%{_datadir}/%{name}/images/%{name}.png
But it is not working.

Comment 36 Michael Schwendt 2013-01-19 12:11:31 UTC
Hmmm, treat it a bit more like homework. ;-)  You currently do this:

| %install
| make install DESTDIR=%{buildroot}
| %find_lang ptbl-manual --with-gnome
| find %{buildroot} -type f

You've added the "find" command there probably because you want to examine the %buildroot contents. Examine the rpmbuild output to see what files get installed into there by the "make install …" invocation". Adjust the %files section accordingly. Repeat that step for every file/dir you install manually in the %install section.

If you like to learn about an advanced packaging technique, note that you can save yourself the rebuilding/compiling of the program (which can be helpful for much larger programs). You can run "rpmbuild --short-circuit -bi ptbl.spec" to process only the %install section. Repeatedly and till the %files section will be correct and rpmbuild will be happy.

> install -p -m 0755 images/* %{buildroot}%{_datadir}/%{name}/images/%{name}.png
> But it is not working.

Sure it is! You neglect the fact that ptbl's "make install …" installs the image file to a different location. That's the place rpmbuild complains about, because there is no matching entry in %files anymore.

Comment 37 RudraB 2013-01-19 13:10:29 UTC
Corrected spec and srpm uploaded:
http://web.warwick.ac.uk/~phslav/ptbl-1.0-10.fc18.src.rpm
http://web.warwick.ac.uk/~phslav/ptbl.spec

$ rpmlint -i ./ptbl.spec ../SRPMS/ptbl-1.0-10.fc18.src.rpm ../RPMS/x86_64/ptbl-1.0-10.fc18.x86_64.rpm 
./ptbl.spec:37: W: mixed-use-of-spaces-and-tabs (spaces: line 37, tab: line 1)
The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
annoyance.  Use either spaces or tabs for indentation, not both.

ptbl.src:37: W: mixed-use-of-spaces-and-tabs (spaces: line 37, tab: line 1)
The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
annoyance.  Use either spaces or tabs for indentation, not both.

2 packages and 1 specfiles checked; 0 errors, 2 warnings.

Comment 38 Michael S. 2013-01-19 16:24:37 UTC
Discusing with rudraB on irc, i am ok to sponsor him. Rudra, can you contact me once you have a few moment ( either by mail or on irc ), so we can discuss a bit ( while i figure what I should do as a sponsor also )

Comment 39 RudraB 2013-01-19 16:53:35 UTC
(In reply to comment #38)
> Discusing with rudraB on irc, i am ok to sponsor him. Rudra, can you contact
> me once you have a few moment ( either by mail or on irc ), so we can
> discuss a bit ( while i figure what I should do as a sponsor also )

Michael,
Thanks a lot!

Comment 40 Michael Schwendt 2013-01-19 17:14:45 UTC
> [Desktop Entry]
> Icon=ptbl

Does that work?
https://fedoraproject.org/wiki/Packaging:Guidelines#Icon_tag_in_Desktop_Files


Here's an interesting albeit old thread about "Where should icons for desktop files be stored?": http://lists.fedoraproject.org/pipermail/packaging/2008-July/004817.html

It goes back and forth a bit, but then points at the freedesktop specs, such as: http://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html#install_icons

Comment 41 RudraB 2013-01-19 22:00:32 UTC
(In reply to comment #40)
> > [Desktop Entry]
> > Icon=ptbl
> 
> Does that work?
yes....its working..as the short name convention.

Comment 42 RudraB 2013-01-21 12:43:13 UTC
An updated spec and srpm is uploaded at 
http://web.warwick.ac.uk/~phslav/ptbl-1.0-11.fc18.src.rpm
http://web.warwick.ac.uk/~phslav/ptbl.spec

%changelog
* Mon Jan 21 2013 Rudra Banerjee <rudra.banerjee.uk> - 1.0-11
-Desktop icon and help pictures corrected.


rpmlint -i ./ptbl.spec ../SRPMS/ptbl-1.0-11.fc18.src.rpm ../RPMS/x86_64/ptbl-1.0-11.fc18.x86_64.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

I request the reviewers to have a look.

Comment 43 Susi Lehtola 2013-01-29 09:13:19 UTC
As you are upstream, why are the man page and the desktop file not part of the standard distribution tarball?

You need to document the sources of these files as per
https://fedoraproject.org/wiki/Packaging:SourceURL

**

The %install section looks a bit messed up. Please remove empty statements such as
 find %{buildroot} -type f 
and the line continuations from
 desktop-file-install --dir=%{buildroot}%{_datadir}/applications %{SOURCE2}
as you are using longer lines than that just below.

As you are using desktop-file-install, there's no need to run
 desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop


Also the statement
 rm -vf %{buildroot}%{_datadir}/images/ptbl.png
 install -p -D -m 0644 images/* %{buildroot}%{_datadir}/gnome/help/ptbl-manual/C/images/%{name}.png
 install -p -D -m 0644 images/* %{buildroot}%{_datadir}/pixmaps/%{name}.png

is rather odd. First of all, the use of the * wildcard would suggest that there are many files to install, but from the syntax it seems that it's just a single file. Why not type out the full filename (which probably is just pbtl.png)?

Note that the -v and -f flags to rm are somewhat unnecessary, as the rm command is still shown in the rpmbuild output.

**

You are mixing ptbl and %{name}, which is bad style. Please choose a single form and stick with it.

**

In the %files section
 %{_datadir}/gnome/help/ptbl-manual/C/images/%{name}.png
makes the RPM package own just that file, but the RPM also creates the directories
 %{_datadir}/gnome/help/ptbl-manual/
 %{_datadir}/gnome/help/ptbl-manual/C/
 %{_datadir}/gnome/help/ptbl-manual/C/images/
which end up unowned. Changing the statement to 
 %{_datadir}/gnome/help/ptbl-manual
or preferably
  %{_datadir}/gnome/help/ptbl-manual/
for the sake of clarity will fix this issue.

**

Lastly, the %changelog is a wall of text. Please separate the entries with a single blank line.

Comment 44 RudraB 2013-01-29 09:50:36 UTC
(In reply to comment #43)
> As you are upstream, why are the man page and the desktop file not part of
> the standard distribution tarball?

Since this is my first project(both as a packager and upstreamer), initially there is no man/desktop file(I had help as the style of gnome). I add them later to meet the requirement of fedora. Now, once the evolution started, changeing the content of source is discouraged. That is why it is what it is. Once the review is over (and if it is selected, ahem), I will create a update soon, with those changes.

> 
> The %install section looks a bit messed up. Please remove empty statements
> such as
>  find %{buildroot} -type f 
> and the line continuations from
>  desktop-file-install --dir=%{buildroot}%{_datadir}/applications %{SOURCE2}
> as you are using longer lines than that just below.
> 
> As you are using desktop-file-install, there's no need to run
>  desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop
> 
> 
> Also the statement
>  rm -vf %{buildroot}%{_datadir}/images/ptbl.png
>  install -p -D -m 0644 images/*
> %{buildroot}%{_datadir}/gnome/help/ptbl-manual/C/images/%{name}.png
>  install -p -D -m 0644 images/* %{buildroot}%{_datadir}/pixmaps/%{name}.png
> 
> is rather odd. First of all, the use of the * wildcard would suggest that
> there are many files to install, but from the syntax it seems that it's just
> a single file. Why not type out the full filename (which probably is just
> pbtl.png)?
> 
> Note that the -v and -f flags to rm are somewhat unnecessary, as the rm
> command is still shown in the rpmbuild output.
> 
> **
> 
> You are mixing ptbl and %{name}, which is bad style. Please choose a single
> form and stick with it.
> 
> **
> 
> In the %files section
>  %{_datadir}/gnome/help/ptbl-manual/C/images/%{name}.png
> makes the RPM package own just that file, but the RPM also creates the
> directories
>  %{_datadir}/gnome/help/ptbl-manual/
>  %{_datadir}/gnome/help/ptbl-manual/C/
>  %{_datadir}/gnome/help/ptbl-manual/C/images/
> which end up unowned. Changing the statement to 
>  %{_datadir}/gnome/help/ptbl-manual
> or preferably
>   %{_datadir}/gnome/help/ptbl-manual/
> for the sake of clarity will fix this issue.
> 
> **
> 
> Lastly, the %changelog is a wall of text. Please separate the entries with a
> single blank line.

Noted. I have changed spec as suggested. Looks prettier.

Comment 45 Michael Schwendt 2013-01-29 10:08:12 UTC
> changeing the content of source is discouraged. 

Of course, but when you modified the 1.0 source tarball, you could have released it as 1.0.1, saving the extra work for the added files in the spec file.

;-)

Comment 46 Antonio T. (sagitter) 2013-11-02 11:06:24 UTC
Hi baptu.

ptbl 2.0 is out since June 2013.
Are you still interested to package this application ?

Comment 47 RudraB 2013-11-03 16:31:55 UTC
Hi Antonio,
Sorry for the delayed reply. Since this is my first package, my sponsor asked me to have mastered the procedure, which was tricky for me. so, I will probably remain upstream, and if some one find this interesting, is welcome to package it.

Comment 48 Antonio T. (sagitter) 2013-11-03 16:53:39 UTC
(In reply to RudraB from comment #47)
> Hi Antonio,
> Sorry for the delayed reply. Since this is my first package, my sponsor
> asked me to have mastered the procedure, which was tricky for me. so, I will
> probably remain upstream, and if some one find this interesting, is welcome
> to package it.

If you're not interested anymore, please close this review request, so someone else can candidate himself to package this software. ;)


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