Bug 1098480 - Review Request: fts-rest - The REST interface for FTS (File Transfer Service V3)
Summary: Review Request: fts-rest - The REST interface for FTS (File Transfer Service V3)
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Alejandro Alvarez
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-05-16 10:32 UTC by michal.simon
Modified: 2015-06-18 14:59 UTC (History)
3 users (show)

Fixed In Version: fts-rest-3.2.7-1.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-07-30 19:35:43 UTC
Type: ---
Embargoed:
a.alvarezayllon: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description michal.simon 2014-05-16 10:32:01 UTC
Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-rest.spec
SRPM URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-rest-3.2.5-1.src.rpm 
Description: The REST interface for FTS (File Transfer Service V3). The package includes the server site Web API and the corresponding CLI.
Fedora Account System Username: simonm

I would really appreciate a review so fts-rest can get into EPEL6.
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6855286

Comment 1 Adrien Devresse 2014-06-25 13:50:32 UTC
I take care of this Review.


I see that you have conditional for el5 in the spec file, do you target el5 too ?

Comment 2 Adrien Devresse 2014-06-25 14:28:34 UTC
First comments if EL5 is targeted :

- BuildRoot section is missing ( http://fedoraproject.org/wiki/EPEL:Packaging) 
- no clean section ( http://fedoraproject.org/wiki/EPEL:Packaging )
- You should not own the directory %{_libexecdir}/fts3 and %{_sysconfdir}/fts3 if other components that fts3-rest use them

rpmlint output :rpmlint ~/fts-rest-3.2.5-1.src.rpm 
fts-rest.src:134: E: files-attr-not-set
fts-rest.src:135: E: files-attr-not-set
fts-rest.src:136: E: files-attr-not-set
fts-rest.src:137: E: files-attr-not-set
fts-rest.src:138: E: files-attr-not-set
fts-rest.src:139: E: files-attr-not-set
fts-rest.src:140: E: files-attr-not-set
fts-rest.src:141: E: files-attr-not-set
fts-rest.src:142: E: files-attr-not-set
fts-rest.src:143: E: files-attr-not-set
fts-rest.src:146: E: files-attr-not-set
fts-rest.src:147: E: files-attr-not-set
fts-rest.src:148: E: files-attr-not-set
fts-rest.src:149: E: files-attr-not-set
fts-rest.src:154: E: files-attr-not-set
fts-rest.src:155: E: files-attr-not-set
fts-rest.src: W: no-cleaning-of-buildroot %install
fts-rest.src: W: no-cleaning-of-buildroot %clean
fts-rest.src: W: no-buildroot-tag
fts-rest.src: W: no-%clean-section
1 packages and 0 specfiles checked;

Comment 3 michal.simon 2014-06-25 14:45:01 UTC
Hi Adrien,
Thanks a lot for taking the review!

The package is only meant for EPEL6.

Comment 4 michal.simon 2014-06-30 09:49:54 UTC
The spec file has been updated, EPEL5 specific staff has been removed.

Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-rest.spec

Comment 5 Alejandro Alvarez 2014-07-14 14:19:55 UTC
Hi,

Please, can you check the rpmlint output from Adrien? Those errors are still appearing.

You are probably missing for each %files section

%defattr(-,root,root,-)

Also, from the guidelines
http://fedoraproject.org/wiki/Packaging:Python

python2-devel should be required

Also, the rpm assumes /etc/fts3 exists, which is own by fts-server, so that should be a Require

Since it packages Apache configuration files, maybe it would be a good idea to add a condrestart in the %post section

No manpages for the CLI tools. Not strictly required, but should eventually be provided.

Other than that, it looks OK to me.

Comment 6 michal.simon 2014-07-15 09:09:21 UTC
Thanks for the comments, here are new links:

Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-rest.spec

Source RPM: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-rest-3.2.6-1.el6.src.rpm

- added BuildRequires for python2-devel
- added Requires for fts3-server
- condrestart has been added
- there are man pages for the CLI

- the errors and warnings from rpmlint are probably due to an old rpmlint version, nevertheless I added %defattr (it won't do any harm), BuildRoot tag if only required for EPEL5 and below

Comment 7 Alejandro Alvarez 2014-07-15 09:54:10 UTC
rpmlint output
==============
fts-rest.spec: W: no-cleaning-of-buildroot %install
fts-rest.spec: W: no-cleaning-of-buildroot %clean
fts-rest.spec: W: no-buildroot-tag
fts-rest.spec: W: no-%clean-section
0 packages and 1 specfiles checked; 0 errors, 4 warnings.


MUST
====

[OK] The package must be named according to the Package Naming Guidelines.
[OK] Package does not use a name that already exist.
[OK] The spec file name must match the base package %{name}, in the format %{name}.spec
[OK] Spec file lacks Packager, Vendor, PreReq tags.
[OK] Changelog in prescribed format.
[OK] The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[OK] The License field in the package spec file must match the actual license.
[OK] The spec file must be written in American English.
[OK] The spec file for the package MUST be legible.
[--] If a rename, provides/obsoletes is specified.
[--] The spec file MUST handle locales properly.
[--] Every binary RPM package which stores shared library files in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.

[OK] If the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
[--] -debuginfo package or justification otherwise.
[--] Development files must be in a -devel package.
[--] Static libraries must be in a -static package.
[--] Devel packages must require the base package using a fully versioned dependency
[--] Large documentation files must go in a -doc subpackage.

[OK] The sources used to build the package must match the upstream source, as provided in the spec URL.

[OK] The package must contain code, or permissable content.

[OK] Package obeys FHS, except libexecdir and /usr/target.
[OK] Packages must NOT contain any .la libtool archives.
[OK] Packages must not own files or directories already owned by other packages
[OK] Packages containing GUI applications must include a %{name}.desktop file.
[OK] A Fedora package must not list a file more than once in the spec file's %files listings.
[OK] A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
[OK] Packages must NOT bundle copies of system libraries
[--] If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
[OK] All filenames in rpm packages must be valid UTF-8.
[OK] Permissions on files must be set properly.

[OK] Each package must consistently use macros.
[OK] No external kernel modules
[OK] No inclusion of pre-built binaries or libraries
[OK] No need for external bits
[OK] All build dependencies must be listed in BuildRequires.
[OK] If a package includes something as %doc, it must not affect the runtime of the application.
[--] %build honors applicable compiler flags or justifies otherwise.

	https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Compiler_flags

[OK] The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[--] If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
[OK] Package installs properly.

SHOULD
======
[--] All patches have an upstream bug link or comment
[OK] The source package does not include license text(s) as a separate file from upstream.
[OK] No PreReq
[OK] %makeinstall is not used
[OK] Timestamp is preserved
[OK] Parallel make
[OK] Subpackages other than devel should require the base package using a fully versioned dependency.
[--] If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[--] The placement of pkgconfig(.pc) files should be in a -devel pkg
[OK] The package builds in mock.
[OK] The package should compile and build into binary rpms on all supported architectures.
[OK] The package functions as described.
[OK] If scriptlets are used, those scriptlets must be sane.
[OK] The package should contain man pages for binaries/scripts

[--] The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.

Looks good to me.

Comment 8 michal.simon 2014-07-15 10:24:07 UTC
New Package SCM Request
=======================
Package Name: fts-rest
Short Description: This package provides the FTS3 REST interface
Upstream URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-rest-3.2.6-1.el6.src.rpm
Owners: simonm
Branches: el6
InitialCC:

Comment 9 Gwyn Ciesla 2014-07-15 12:36:08 UTC
Git done (by process-git-requests).

Comment 10 Fedora Update System 2014-07-15 14:51:18 UTC
fts-rest-3.2.7-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/fts-rest-3.2.7-1.el6

Comment 11 Fedora Update System 2014-07-16 03:51:29 UTC
fts-rest-3.2.7-1.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 12 Fedora Update System 2014-07-30 19:35:43 UTC
fts-rest-3.2.7-1.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 13 Alejandro Alvarez 2015-06-18 04:06:20 UTC
Package Change Request
======================
Package Name: fts-rest
New Branches: epel7
Owners: aalvarez

Comment 14 Gwyn Ciesla 2015-06-18 14:59:57 UTC
Git done (by process-git-requests).


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