Bug 1024993 - Review Request: lin_guider - Astronomical autoguiding program
Review Request: lin_guider - Astronomical autoguiding program
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2013-10-30 13:36 EDT by Lukash James
Modified: 2015-11-26 13:53 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Lukash James 2013-10-30 13:36:52 EDT
Spec URL: http://krypt3r.net63.net/packages/fedora/lin_guider.spec
SRPM URL: http://krypt3r.net63.net/packages/fedora/lin_guider-2.9.0-1.fc17.src.rpm
Description: Lin-guider is an astronomical autoguiding program for Linux.
It supports Philips, Logitech, uvc webcams, QHY5, QHY5-II-M, QHY5-II, 
QHY6, DSI2PRO astrocams for video and FTDI chip-based, parallel 
port-based (LPT) and QHY5, QHY6 astrocam devices for pulse guiding.
Fedora Account System Username: krypt3r
Blocks: FE-NEEDSPONSOR

It is my first package, so I need a sponsor.
Depends on the package 'firmware-ccd' (will be soon).
Comment 1 Lukash James 2013-10-30 13:51:06 EDT
SRPM URL changed to http://krypt3r.net63.net/packages/fedora/lin_guider-2.9.0-1.fc17.src.rpm.bz2
Comment 2 Antonio Trande 2013-10-30 14:07:20 EDT
Hi James.

(In reply to Lukash James from comment #1)
> SRPM URL changed to
> http://krypt3r.net63.net/packages/fedora/lin_guider-2.9.0-1.fc17.src.rpm.bz2

- Why ?

For an handy review, it would be better to have two direct links, one for the .spec file and one for the .srpm .

- Why a manual installation ?
Does '%make DESTDIR="" install' not work ?
Comment 3 Lukash James 2013-10-30 14:22:58 EDT
(In reply to Antonio Trande from comment #2)
> Hi James.
> 
> (In reply to Lukash James from comment #1)
> > SRPM URL changed to
> > http://krypt3r.net63.net/packages/fedora/lin_guider-2.9.0-1.fc17.src.rpm.bz2
> 
> - Why ?

Hello. URL changed because I can't get RPM file from my hosting (only bzipped). May be I can edit the description?

> For an handy review, it would be better to have two direct links, one for
> the .spec file and one for the .srpm .
> 
> - Why a manual installation ?
> Does '%make DESTDIR="" install' not work ?
After building lin_guider binary started from build directory. Makefile haven't 'install' target.

PS. I am not an upstream developer.
Comment 4 Lukash James 2013-10-30 14:57:02 EDT
(In reply to Lukash James from comment #3)
> URL changed because I can't get RPM file from my hosting (only
> bzipped). May be I can edit the description?
Bug fixed with .htaccess. SRPM URL in 'Description' now valid.
Comment 5 Antonio Trande 2013-10-30 15:23:27 EDT
(In reply to Lukash James from comment #3)
> > - Why a manual installation ?
> > Does '%make DESTDIR="" install' not work ?
> After building lin_guider binary started from build directory. Makefile
> haven't 'install' target.
> 
> PS. I am not an upstream developer.

Consider to leave a comment in the .spec file to describe the reasons of your choices, above all if they fall outside of packaging guidelines.

- You don't need of '%defattr(-, root, root)' line
  http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

- Please remove the lines

#ln -s /opt/gm_software/lin_guider/lg-wrapper.sh $RPM_BUILD_ROOT%{_bindir}/lin_guider
#ln -s /opt/gm_software/lin_guider/man/man1/lin_guider.1.gz $RPM_BUILD_ROOT%{_mandir}/man1/lin_guider.1.gz
#ln -s /opt/gm_software/lin_guider/man/ru/man1/lin_guider.1.gz $RPM_BUILD_ROOT%{_mandir}/ru/man1/lin_guider.1.gz
#install contrib/{mc.sh,mc.csh} $RPM_BUILD_ROOT%{_sysconfdir}/profile.d

if they are not necessary.

- Your package provides a .desktop file; validate it in a %check section.
   http://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

- Consider that starting from f20, every package docdir subdirectory will be unversioned. See https://fedoraproject.org/wiki/Changes/UnversionedDocdirs
Comment 6 Lukash James 2013-10-31 11:47:19 EDT
(In reply to Antonio Trande from comment #5)
> Consider to leave a comment in the .spec file to describe the reasons of
> your choices, above all if they fall outside of packaging guidelines.
> 
> - You don't need of '%defattr(-, root, root)' line
> - Please remove the lines
> - Your package provides a .desktop file; validate it in a %check section.
> - Consider that starting from f20, every package docdir subdirectory will be
> unversioned.
Thank you for your comments, Antonio. I fixed all of them (I think) and updated files in 'Description' section.
I have a question. Should I add new entries in %changelog if I fixed anything?
Comment 7 Antonio Trande 2013-10-31 12:10:22 EDT
(In reply to Lukash James from comment #6)
> I have a question. Should I add new entries in %changelog if I fixed
> anything?

- You may update and replace the existing data line since your package is not built yet.
See http://fedoraproject.org/wiki/Packaging:Guidelines#Multiple_Changelog_Entries_per_Release

- Move the %check section after %install
  Was 'desktop-file-install' line already in your first .spec file ? If yes, desktop-file-validate is useless at this point. :P

'desktop-file-install' already validates the .desktop files automatically. 

- Move the line
  
%{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}

at the top.
Comment 8 Lukash James 2013-10-31 12:26:53 EDT
(In reply to Antonio Trande from comment #7)
>   Was 'desktop-file-install' line already in your first .spec file ? If yes,
> desktop-file-validate is useless at this point. :P
No, 'install ...' was there :)

> 'desktop-file-install' already validates the .desktop files automatically. 
Oh, I see. I shall fix it.

> - Move the line
>   
> %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
> 
> at the top.
Before 'Name' and 'Version'? But will it work? 'Name' and 'Version' not defined in the first line.
Comment 9 Antonio Trande 2013-10-31 15:05:07 EDT
(In reply to Lukash James from comment #8)
> (In reply to Antonio Trande from comment #7)
> >   Was 'desktop-file-install' line already in your first .spec file ? If yes,
> > desktop-file-validate is useless at this point. :P
> No, 'install ...' was there :)

So I'm sorry.

> > - Move the line
> >   
> > %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
> > 
> > at the top.
> Before 'Name' and 'Version'? But will it work? 'Name' and 'Version' not
> defined in the first line.

Good question. 
Yes, it works; I've used it myself with another one package. I think because 'rpm' itself defines numerous macros.
If you add "%dump" to the beginning of your spec file, process with rpm, you can  examine them.
Comment 10 Lukash James 2013-10-31 17:13:27 EDT
(In reply to Antonio Trande from comment #9)
> Good question. 
> Yes, it works; I've used it myself with another one package. I think because
> 'rpm' itself defines numerous macros.
> If you add "%dump" to the beginning of your spec file, process with rpm, you
> can  examine them.

Thanks. Spec & SRPM updated.
Comment 11 Antonio Trande 2013-11-01 07:44:20 EDT
> It is my first package, so I need a sponsor.
> Depends on the package 'firmware-ccd' (will be soon).

I'm not a sponsor so I can't do an official review of your first packages (you need to be sponsored first of all).
However, I can to do an initial review to relieve your (prospective) sponsor of some of his work, as soon as even the 'firmware-ccd' package will be ready. 

For everything else it depends by yourself and by your sponsor (see http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group). :)

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