[Bug 1409866] Review Request: perl-Astro-SunTime - Astro:: SunTime perl module

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1409866



--- Comment #6 from Andrew Bauer <zonexpertconsulting@xxxxxxxxxxx> ---
(In reply to Petr Pisar from comment #2)
> Next time, please link to raw spec file. Not an HTML document.

Understood.

> FIX: Do not undefine the %debug_package macro. debuginfo packages are not
> created for noarch packages by default.

Completed.

> 
> FIX: The summary text is poor. Instead of naming modules the package
> contains it should summaries purpose of the package. For example `Sun rise
> and set times'.

Summary text updated to state "Calculates sun rise/set times"
> 
> TODO: I think the `perl' in the description text should be spelled as `Perl'
> because it's a name of a programming language. `perl' denotes the Perl
> interpreter, a command for executing /usr/bin/perl.

Understood. Perl has been capitalized.

> 
> FIX: There is no license text or disclaimer in the sources. Where did you
> find that `GPL+ or Artistic' applies? In case there is no license defined in
> the sources, one has to contact author of the sources and ask him/her for
> clarification. If the author does not release a new version that will
> include the license, the author's clarification (e.g. an e-mail response
> with all e-mail headers) must be added to the package using %license macro
> in the %files section. If author does not provide any clarification
> acceptable by Fedora legal team, the package cannot be distributed withing
> Fedora distribution because copyright law denies redistributing copyrighted
> material by default.

Author responded favorably and has added a GPLv3 header to the source files in
his development branch on github. Specfile updated to reflect GPLv3. 
See below for further comment.

> 
> 
> The missing license is a fatal problem and I would stop the review now until
> resolving it. But because you are newbie, I will continue and show you what
> other deficiencies should be fixed in this package.


Thank you. This has been very helpful.

> TODO: You can simplify the find command in the %install section like this:
> 
>   find %{buildroot} -type f -name .packlist -delete
> 
> Otherwise you should build-require `coreutils' because you execute `rm'
> command provided by coreutils package.

Changed to using the "-delete" flag

> 
> TODO: Do not package `MANIFEST' file. It has no meaning in RPM package, it's
> used only by CPAN site.

Understood. MANIFEST has been removed.

> 
> TODO: Do not package `test.pl' file. We normally do not package tests as
> they are not needed at run time. We package them as a documentation if there
> is not any better documentation. This package does not have any
> documentation, but the test.pl actually does not show any usage of
> Astro::SunTime API, so it'd not good as as a documentation as well.

Thank you for the explanation. I had used perl-GD-Barcode.spec from the Fedora
repo as an example to follow, but based on your statement I will take that case
as the exception, rather than the norm. test.pl removed.

> 
> FIX: Build-require `perl' because you execute perl in
> perl-Astro-SunTime.spec at lines 17 and 26.
> 

Done.

> FIX: Build-require `findutils' (perl-Astro-SunTime.spec:32).
> FIX: Build-require `coreutils' (perl-Astro-SunTime.spec:32).
> 

Added findutils. Skipped coreutils since I previously changed the syntax of the
find statement.

> FIX: Build-require these Perl modules because they are used when executing
> tests:
> 
> `perl(POSIX)' (SunTime.pm:10)
> `perl(strict)' (SunTime.pm:12)
> `perl(Time::ParseDate)' (SunTime.pm:9)
> `perl(vars)' (SunTime.pm:2).
> 

These have been added

> You can use `tangerine' tool the scan the sources for used Perl modules. You
> can verify the modules are used by mangling their identifiers to
> non-existing modules in the sources and than executing `make test'.
> 

Ah, so this is the magic that determines this. Very good to know.

> All tests pass. Ok.
> 
> $ rpmlint perl-Astro-SunTime.spec
> ../SRPMS/perl-Astro-SunTime-0.01-4.fc26.src.rpm
> ../RPMS/noarch/perl-Astro-SunTime-0.01-4.fc26.noarch.rpm 
> perl-Astro-SunTime.spec:8: W: non-standard-group Applications/CPAN
> perl-Astro-SunTime.spec:48: W: macro-in-%changelog %check
> perl-Astro-SunTime.src: E: description-line-too-long C Astro::SunTime perl
> module provides a function interface to calculate sun rise/set times.
> perl-Astro-SunTime.src: W: non-standard-group Applications/CPAN
> perl-Astro-SunTime.src:48: W: macro-in-%changelog %check
> perl-Astro-SunTime.noarch: E: description-line-too-long C Astro::SunTime
> perl module provides a function interface to calculate sun rise/set times.
> perl-Astro-SunTime.noarch: W: non-standard-group Applications/CPAN
> perl-Astro-SunTime.noarch: W: incoherent-version-in-changelog 0.03-4
> ['0.01-4.fc26', '0.01-4']
> perl-Astro-SunTime.noarch: W: manifest-in-perl-module
> /usr/share/doc/perl-Astro-SunTime/MANIFEST
> 2 packages and 1 specfiles checked; 2 errors, 7 warnings.
> 
> FIX: Change the Group tag value to one enumerated in
> /usr/share/doc/rpm/GROUPS (`Development/Libraries' probably) or remove the
> tag because it's deprecated in Fedora.

Since my goal is to get this package in EPEL 6 & 7, in addition to Fedora, I
set the Group to Development/Libraries, rather than remove it.

> FIX: Escape `%' character in the %changelog by another per-cent character.
> FIX: Correct version number for the latest %changelog entry to match Version
> tag.
> 

Corrected.

> Please correct all `FIX' items, consider fixing `TODO' items and provide
> updated spec file.
> Resolution: Package NOT approved.

Regarding the recent addition of the license text by the author, this was done
in a development branch in his git repo, but has not yet part of a new release
not has it been pushed to CPAN:
https://github.com/fugina-hackery/perl-astro-suntime/commit/139224b051f3d3f7992dc3d9a2f5a4d35be9284f

While I could get the source files from git, rather than cpan, I don't believe
that is the correct thing to do in this case. Instead, I'll wait until a new
release appears in CPAN. I will update this bugzilla at that time. Let me know
if there are any acceptable alternatives.

specfile w/ no html formatting: 
https://raw.githubusercontent.com/knnniggett/specfiles/master/perl-Astro-SunTime.spec

srpm:
https://copr-be.cloud.fedoraproject.org/results/kni/zoneminder_deps/fedora-25-x86_64/00496154-perl-Astro-SunTime/perl-Astro-SunTime-0.01-5.fc25.src.rpm

No complaints from rpmlint:

$ rpmlint /home/abauer/rpmbuild/SRPMS/perl-Astro-SunTime-0.01-5.fc25.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint
/home/abauer/rpmbuild/RPMS/noarch/perl-Astro-SunTime-0.01-5.fc25.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]