Bug 711764 - Review Request: osc-source_validator - osc source validator
Review Request: osc-source_validator - osc source validator
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Haïkel Guémar
Fedora Extras Quality Assurance
:
Depends On: 711762
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 08:29 EDT by Jerome Soyer
Modified: 2011-09-19 22:33 EDT (History)
5 users (show)

See Also:
Fixed In Version: osc-source_validator-0.1-4.el5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-09-19 18:59:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
karlthered: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jerome Soyer 2011-06-08 08:29:03 EDT
Spec URL: http://fedorapeople.org/~saispo/osc-source_validator.spec
SRPM URL: http://fedorapeople.org/~saispo/osc-source_validator-0.1-1.fc15.src.rpm
Description: 
Hi ! I just finished packaging up yolk, and i would appreciate a review so
that i can get it into Fedora !

osc is checking valid sources on pre-checkin via this package.
Comment 1 Yanchuan Nian 2011-06-08 10:29:14 EDT
Few comments
1. you must use a full URL to identify where the pristine source code is.
see here:https://fedoraproject.org/wiki/Packaging/SourceURL

2.get_source.sh is not necessary here, for it is contained in osc-source_validator-0.1.tar.bz2.
You'd better remove Source1 from the spec file.

3.Fedora (as of F-10) does not require the presence of the BuildRoot tag in the spec.
see here:https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

4. The name of the rpm is just what the Source unpacks to, so -n %{name}-%{version} should be removed from %prep.
see here:https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_.25setup_command

5./usr/lib is for architecture-dependent data, while /usr/share is for architecture-independent data.
That way, systems with different CPUs can share /usr/share.

%install
mkdir -p %{buildroot}%{_prefix}/lib/osc/source_validators
cp -a [0-9]* helpers %{buildroot}%{_prefix}/lib/osc/source_validators
%files
%defattr(-,root,root,-)
%doc COPYING
%{_prefix}/lib/osc

should be changed to

%install
mkdir -p %{buildroot}%{_datarootdir}/osc/source_validators
cp -a [0-9]* helpers %{buildroot}%{_datarootdir}/osc/source_validators
%files
%defattr(-,root,root,-)
%doc COPYING
%{_datarootdir}/osc
see here:https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_and_Filesystem_Hierarchy_Standard_.28FHS.29
Comment 2 Jerome Soyer 2011-06-09 04:29:45 EDT
Hi,

I update my Spec and my SRPMS.

I prefer to keep buildroot for EPEL.

Spec URL: http://fedorapeople.org/~saispo/osc-source_validator.spec
SRPM URL:
http://fedorapeople.org/~saispo/osc-source_validator-0.1-2.fc15.src.rpm

Thanks for the review.
Comment 3 Haïkel Guémar 2011-06-09 04:53:01 EDT
@Yanchuan, thanks for your pertinent comments.
1. && 2. osc-source_validator does not provides source tarballs (jerome had the
same issue with osc #711762). I second Yanchuan on removing source1, i'd rather
advise that you put a comment explaining how generating the tarball from git (2
lines) and fix pristine sources either by patching them or using sed within your spec
3. since jerome plans to support EPEL, it shouldn't be a problem. 
4. second that
5. since osc is looking for helpers in /usr/lib/osc, that would break everything.
It should be possible to fix source_validators path in osc, for instance:  
sed -i 's#/usr/lib/osc/source_validators#/usr/share/osc/source_validators#' osc/conf.py # note that tests might need the same fix too
Could you contact upstream and ask them if they could fix that issue ?
Comment 4 Haïkel Guémar 2011-06-09 04:58:02 EDT
add a dependency to #711762 (osc)
Comment 5 Haïkel Guémar 2011-06-09 06:16:30 EDT
The helpers issue is not that clear, Matthieu Bridon pointed this:
"/usr/lib includes object files, libraries, and internal binaries that are not intended to be executed directly by users or shell scripts. [22]" 
http://www.pathname.com/fhs/pub/fhs-2.3.html#USRLIBLIBRARIESFORPROGRAMMINGANDPA

Some packages like systemd put helpers scripts in /usr/lib, some like dracut put them in /usr/share. Since FHS explicitely allows helpers scripts in /usr/lib but doesn't forbid putting them in /usr/share.
%{_libexecdir} is also a strong option according Fedora Packaging Guidelines for helpers scripts. Since it's not recommended by FHS, it's almost certain that it won't be upstream-able

I requested my fellow packagers opinions on that matter on devel mailing list.
Comment 6 Jerome Soyer 2011-06-14 08:15:39 EDT
Fixed upload is here, and as we discuss on IRC and devel ML, we prefer to keep them in /usr/lib at this time.

Spec URL: http://fedorapeople.org/~saispo/osc-source_validator.spec
SRPM URL:
http://fedorapeople.org/~saispo/osc-source_validator-0.1-2.fc15.src.rpm
Comment 7 Haïkel Guémar 2011-06-24 01:57:00 EDT
os-source_validator (python package)

MUST: rpmlint must be run on src.rpm and rpm. KO
$rpmlint -iv osc-source_validator-0.1-2.fc15.src.rpm 
osc-source_validator.src: I: checking
osc-source_validator.src: W: spelling-error Summary(en_US) validator -> invalidator, validation, validate
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: summary-not-capitalized C osc source validator
Summary doesn't begin with a capital letter.

osc-source_validator.src: W: spelling-error %description -l en_US pre -> per, ore, pee
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: spelling-error %description -l en_US checkin -> chicken, checking, check in
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: I: checking-url http://en.opensuse.org/openSUSE:Build_Service_Tools#OSC_source_validator (timeout 10 seconds)
osc-source_validator.src:20: E: hardcoded-library-path in %{_prefix}/lib/osc/source_validators
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

osc-source_validator.src:21: E: hardcoded-library-path in %{_prefix}/lib/osc/source_validators
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

osc-source_validator.src:26: E: hardcoded-library-path in %{_prefix}/lib/osc
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

osc-source_validator.src:30: W: macro-in-%changelog %{SOURCE1}
Macros are expanded in %changelog too, which can in unfortunate cases lead to
the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally odd
entries eg. in source rpms, which is rarely wanted.  Avoid use of macros in
%changelog altogether, or use two '%'s to escape them, like '%%foo'.

osc-source_validator.src: W: no-%build-section
The spec file does not contain a %build section.  Even if some packages don't
directly need it, section markers may be overridden in rpm's configuration to
provide additional "under the hood" functionality, such as injection of
automatic -debuginfo subpackages.  Add the section, even if empty.

osc-source_validator.src: W: invalid-url Source0: osc-source_validator-0.1.tar.bz2
The value should be a valid, public HTTP, HTTPS, or FTP URL.

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

$ rpmlint -iv osc-source_validator-0.1-2.fc15.noarch.rpm 
osc-source_validator.noarch: I: checking
osc-source_validator.noarch: W: spelling-error Summary(en_US) validator -> invalidator, validation, validate
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: summary-not-capitalized C osc source validator
Summary doesn't begin with a capital letter.

osc-source_validator.noarch: W: spelling-error %description -l en_US pre -> per, ore, pee
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: spelling-error %description -l en_US checkin -> chicken, checking, check in
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: I: checking-url http://en.opensuse.org/openSUSE:Build_Service_Tools#OSC_source_validator (timeout 10 seconds)
osc-source_validator.noarch: W: only-non-binary-in-usr-lib
There are only non binary files in /usr/lib so they should be in /usr/share.

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

==> you should fix the summary issue

==> since there's no upstream tarball, you should add a comment explaining how did you generate the provided tarball

==> missing requires: osc (obvious), other requirements were correctly detected by rpm

==> missing %build section

MUST: package named accordingly to package naming guidelines. OK
osc is mostly a command-line tool and does not provide a module usable by third-party.

MUST: spec file name match %{name}. OK

MUST: package meets packaging guidelines.

MUST: package must be licensed under a fedora-compliant license. OK (GPLv2+)

MUST: license field in package spec match actual license. OK

MUST: spec is in legible american english. OK

MUST sources provided match upstream's. KO
no upstream tarball, no explanation on how did you get the sources

MUST: package successfully compiles on at least one primary architecture (all of them). OK

MUST: all build dependencies are listed in BR (mock compliant). OK 

MUST: package must own all directories it creates. OK

MUST: package does not list a file more than once in %files section. OK

MUST: permissions are properly set. OK

MUST: package consistenly use macros. OK

MUST: package contains permissable content. OK

MUST: package does not own directories owned by other packages. OK

MUST: all filenames in package are valid UTF-8 (fixed by the reviewee in current spec).  OK

SHOULD: mock builds were done for fedora 14/15/devel on all primary architectures (x86/x86_64) OK

SHOULD: the module provided works) OK

SHOULD: man pages are provided (warnings about them are here irrelevant).


Remarks:
description isn't very clear, you should properly explain that this is a source validation plugin to OSC (OpenSuse build Service Command-line)
Comment 8 Jerome Soyer 2011-06-24 08:38:24 EDT
New Package SCM Request
=======================
Package Name: osc-source_validator
Short Description: osc source validator
Owners: saispo
Branches: f14 f15 el5 el6
InitialCC: saispo
Comment 9 Gwyn Ciesla 2011-06-24 08:59:14 EDT
Git done (by process-git-requests).
Comment 10 Haïkel Guémar 2011-06-24 10:04:09 EDT
This package has NOT been approved yet.
Comment 12 Haïkel Guémar 2011-07-06 10:29:15 EDT
$ rpmlint -iv osc-source_validator-0.1-3.fc15.src.rpm                                                                                                                                            (0)
osc-source_validator.src: I: checking
osc-source_validator.src: W: spelling-error Summary(en_US) validator -> invalidator, validation, validate
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: spelling-error %description -l en_US validator -> invalidator, validation, validate
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: spelling-error %description -l en_US openSUSE -> opens Use, open SUSE, open-SUSE
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: spelling-error %description -l en_US pre -> per, ore, pee
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: W: spelling-error %description -l en_US checkin -> chicken, checking, check in
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.src: I: checking-url http://en.opensuse.org/openSUSE:Build_Service_Tools#OSC_source_validator (timeout 10 seconds)
osc-source_validator.src:22: E: hardcoded-library-path in %{_prefix}/lib/osc/source_validators
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

osc-source_validator.src:23: E: hardcoded-library-path in %{_prefix}/lib/osc/source_validators
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

osc-source_validator.src:28: E: hardcoded-library-path in %{_prefix}/lib/osc
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.

osc-source_validator.src:37: W: macro-in-%changelog %{SOURCE1}
Macros are expanded in %changelog too, which can in unfortunate cases lead to
the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally odd
entries eg. in source rpms, which is rarely wanted.  Avoid use of macros in
%changelog altogether, or use two '%'s to escape them, like '%%foo'.

osc-source_validator.src: W: no-%build-section
The spec file does not contain a %build section.  Even if some packages don't
directly need it, section markers may be overridden in rpm's configuration to
provide additional "under the hood" functionality, such as injection of
automatic -debuginfo subpackages.  Add the section, even if empty.

osc-source_validator.src: W: invalid-url Source0: osc-source_validator-0.1.tar.bz2
The value should be a valid, public HTTP, HTTPS, or FTP URL.

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

$ rpmlint -iv /var/lib/mock/fedora-15-x86_64/result/osc-source_validator-0.1-3.fc15.noarch.rpm                                                                                                                                        (64)
osc-source_validator.noarch: I: checking
osc-source_validator.noarch: W: spelling-error Summary(en_US) validator -> invalidator, validation, validate
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: spelling-error %description -l en_US validator -> invalidator, validation, validate
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: spelling-error %description -l en_US openSUSE -> opens Use, open SUSE, open-SUSE
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: spelling-error %description -l en_US pre -> per, ore, pee
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: W: spelling-error %description -l en_US checkin -> chicken, checking, check in
The value of this tag appears to be misspelled. Please double-check.

osc-source_validator.noarch: I: checking-url http://en.opensuse.org/openSUSE:Build_Service_Tools#OSC_source_validator (timeout 10 seconds)
osc-source_validator.noarch: W: only-non-binary-in-usr-lib
There are only non binary files in /usr/lib so they should be in /usr/share.

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


==> rpmlint is OK, but you should fix the macros issue accordingly (not mandatory but good practice)

==> you should add a comment explaining how you generated the tarball (as you did with osc) and then i'll approve this review.
Comment 14 Fedora Update System 2011-08-16 10:06:34 EDT
osc-source_validator-0.1-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/osc-source_validator-0.1-4.fc14
Comment 15 Fedora Update System 2011-08-16 10:07:22 EDT
osc-source_validator-0.1-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/osc-source_validator-0.1-4.fc15
Comment 16 Fedora Update System 2011-08-16 10:08:40 EDT
osc-source_validator-0.1-4.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/osc-source_validator-0.1-4.el5
Comment 17 Fedora Update System 2011-08-16 10:09:51 EDT
osc-source_validator-0.1-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/osc-source_validator-0.1-4.el6
Comment 18 Fedora Update System 2011-08-16 16:54:18 EDT
osc-source_validator-0.1-4.el5 has been pushed to the Fedora EPEL 5 testing repository.
Comment 19 Fedora Update System 2011-09-19 18:59:08 EDT
osc-source_validator-0.1-4.fc14 has been pushed to the Fedora 14 stable repository.
Comment 20 Fedora Update System 2011-09-19 19:01:46 EDT
osc-source_validator-0.1-4.fc15 has been pushed to the Fedora 15 stable repository.
Comment 21 Fedora Update System 2011-09-19 22:31:37 EDT
osc-source_validator-0.1-4.el6 has been pushed to the Fedora EPEL 6 stable repository.
Comment 22 Fedora Update System 2011-09-19 22:33:20 EDT
osc-source_validator-0.1-4.el5 has been pushed to the Fedora EPEL 5 stable repository.

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