Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 180034
Summary: | Review Request: perl-Font-TTF (part of the dejavu-fonts toolchain) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nicolas Mailhot <nicolas.mailhot> | ||||||||||||
Component: | Package Review | Assignee: | Jason Tibbitts <j> | ||||||||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||||||||||
Severity: | medium | Docs Contact: | |||||||||||||
Priority: | medium | ||||||||||||||
Version: | rawhide | CC: | perl-devel, tremble | ||||||||||||
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
||||||||||||
Target Release: | --- | ||||||||||||||
Hardware: | noarch | ||||||||||||||
OS: | Linux | ||||||||||||||
Whiteboard: | |||||||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||||||
Doc Text: | Story Points: | --- | |||||||||||||
Clone Of: | Environment: | ||||||||||||||
Last Closed: | 2006-02-05 18:53:06 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: | |||||||||||||||
Bug Depends On: | |||||||||||||||
Bug Blocks: | 163779 | ||||||||||||||
Attachments: |
|
Description
Nicolas Mailhot
2006-02-04 22:21:39 UTC
Created attachment 124187 [details]
perl-Font-TTF spec file
Very simple spec file
My rpmlint has nothing to say about it
Generally you should supply a src.rpm; it makes the review process a bit easier and some of the review items require having it. My rpmlint complains: W: perl-Font-TTF wrong-file-end-of-line-encoding /usr/share/doc/perl-Font-TTF-0.37/README.TXT and indeed README.TXT has CRLF endings. However, there are several packages which include documentation files with DOS-style line endings so I don't believe this is a blocker. Fix it up if you like. Other issues: I can't review spec file naming. I can't review the source used to build the SRPM as none was provided. BuildRequires: perl is not permitted. The license does not seem to be GPL. I see only: The Perl TTF module is licensed under the Perl Artistic License. I don't understand this comment in the %files section: # For arch-specific packages: vendorarch I can't install the resulting RPM: error: Failed dependencies: perl(Win32) is needed by perl-Font-TTF-0.37-1.noarch perl(Win32::Registry) is needed by perl-Font-TTF-0.37-1.noarch I believe this is the result of lib/Font/TTF/Win32.pm. I'm not sure what would be best to do here. You can fix it with a quick rm %{buildroot}/%{perl_vendorlib}/Font/TTF/Win32.pm in the %install section but I'm not completely sure if that's acceptable. Another solution would be to postprocess the output of the dependency generator, but that's rather unpalatable as well (and more complicated than just deleting the file). Nicolas, would you please provide an SRPM? (In reply to comment #2) > Generally you should supply a src.rpm; it makes the review process a bit easier > and some of the review items require having it. I know, it's just that my isp changed it's upload rules and I couldn't locate the new ones yesterday > My rpmlint complains: > W: perl-Font-TTF wrong-file-end-of-line-encoding > /usr/share/doc/perl-Font-TTF-0.37/README.TXT > > and indeed README.TXT has CRLF endings. However, there are several packages > which include documentation files with DOS-style line endings so I don't believe > this is a blocker. Fix it up if you like. It's as you want, the rpmlint in rawhide does not care > Other issues: > I can't review spec file naming. > I can't review the source used to build the SRPM as none was provided. > BuildRequires: perl is not permitted. This line is straight from the fedora-rpmdevtools-1.4-2.fc5 perl template. Is the template wrong ? > The license does not seem to be GPL. I see only: > The Perl TTF module is licensed under the Perl Artistic License. Ok, my mistake, copied the licensing in dries rpm without checking > I don't understand this comment in the %files section: > # For arch-specific packages: vendorarch This is a bit of fedora-rpmdevtools-1.4-2.fc5 perl template I should have snipped > I can't install the resulting RPM: > > error: Failed dependencies: > perl(Win32) is needed by perl-Font-TTF-0.37-1.noarch > perl(Win32::Registry) is needed by perl-Font-TTF-0.37-1.noarch > > I believe this is the result of lib/Font/TTF/Win32.pm. I'm not sure what would > be best to do here. You can fix it with a quick > > rm %{buildroot}/%{perl_vendorlib}/Font/TTF/Win32.pm > > in the %install section but I'm not completely sure if that's acceptable. I agree on this, will do > Another solution would be to postprocess the output of the dependency generator, > but that's rather unpalatable as well (and more complicated than just deleting > the file). (In reply to comment #3) > Nicolas, would you please provide an SRPM? Since the SRPM is only 148 kiB I'll attach it too Created attachment 124197 [details]
proposed srpm
Created attachment 124198 [details]
Cleaned-up spec
(In reply to comment #4) > (In reply to comment #2) > > BuildRequires: perl is not permitted. > This line is straight from the fedora-rpmdevtools-1.4-2.fc5 perl template. Is > the template wrong ? Mileages vary. IMO not and rather the MUST in the guidelines is a bit over the top in this case. See bug 179426. I agree about the BuildRequires: perl thing, and indeed in another review I said it was not a blocker (which was my mistake) but then I noticed the MUST. I'm not sure what to do here; I think the MUST is unnecessary and conflicts with language in the the packaging guidelines: "There is no need to include the following packages or their dependencies as BuildRequires because they would occur too often." which doesn't sound very MUST like. We can't just ignore the guidelines, so I suggest removing the BuildRequires: while this gets worked out on the mailing list. Also, could you comment the %exclude you added, so it's obvious why this is required. I'll finish off the review in a few minutes. (In reply to comment #9) > We can't just ignore the guidelines, so I suggest removing the BuildRequires: > while this gets worked out on the mailing list. I strongly suspect most FE packages follow the official templates, so leaving it might be the more conservative option IMHO. But I'll follow the reviewer position ;) > Also, could you comment the %exclude you added, so it's obvious why this is > required. > > I'll finish off the review in a few minutes. Ok, new spec/srpm in 30s Created attachment 124215 [details]
spec file take #3
Created attachment 124216 [details]
srpm take #3
Cool. OK, everything's looking good: No rpmlint blockers, just the end-of-line warning. Package meets naming and packaging guidelines. License is acceptable and matches License: tag. Specfile is properly named, legible, well-written, well-commented and uses macros consistently. Source file matches upstream. Package builds and installs on FC3 and FC4. BuildRequires: is proper. Approved. Package Change Request ====================== Package Name: perl-Font-TTF New Branches: EL-5 EL-6 Owners: tremble nim listed on https://fedoraproject.org/wiki/EPEL/ContributorStatusNo Git done (by process-git-requests). |