Bug 956304 - Review Request: fts - File Transfer Service V3
Review Request: fts - File Transfer Service V3
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dominik 'Rathann' Mierzejewski
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-24 11:49 EDT by michal.simon
Modified: 2014-11-26 12:02 EST (History)
4 users (show)

See Also:
Fixed In Version: fts-3.1.1-1.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-22 14:24:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dominik: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description michal.simon 2013-04-24 11:49:41 EDT
Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts.spec
  
SRPM URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-0.0.1-93.el6.src.rpm

Description: The File Transfer Service V3 - GRID middleware, which manages reliable file transfer, most importantly the distribution of data from the CERN Tier 0 to the Tier 1s. The project is currently being developed at CERN.

Fedora Account System Username: simonm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5297395

This is my first package and I need a sponsor!

I am one of the main developers of the FTS3 project.
Comment 1 Dominik 'Rathann' Mierzejewski 2013-04-28 04:56:04 EDT
Taking this review. I will sponsor Michał.
Comment 2 Dominik 'Rathann' Mierzejewski 2013-04-28 06:49:27 EDT
Initial comments:

0. Is this package EPEL-only? I see chkconfig and service command usage, but no corresponding SystemD commands.

1. Package doesn't build in EPEL-6 x86_64 mock:

cd /builddir/build/BUILD/fts-0.0.1/build/src/common && /usr/lib64/ccache/c++   -Dfts_common_EXPORTS -DWITH_IPV6 -DOPENSSL_THREADS -DWITH_OPE
NSSL -DNDEBUG -DSOCKET_CLOSE_ON_EXIT -D_REENTRANT -DBOOST_DATE_TIME_HAS_REENTRANT_STD_FUNCTIONS -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -
fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic   -findirect-inlining -ftree-switch-conversion -ftree-builtin-ca
ll-dce -fconserve-stack -Wno-long-long -Wconversion -Wuninitialized  -Wno-deprecated-declarations  -fno-strict-aliasing -Wextra -Wall -D_FOR
TIFY_SOURCE=2 -fstack-protector-all -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -O3 -DNDEBUG -fPIC -I/builddir/build/BUILD/fts-0.0.1/build/
src/common -I/builddir/build/BUILD/fts-0.0.1/src/common -I/builddir/build/BUILD/fts-0.0.1/src -I/builddir/build/BUILD/fts-0.0.1/build/src -I
/builddir/build/BUILD/fts-0.0.1/src/common/.   -o CMakeFiles/fts_common.dir/producer.cpp.o -c /builddir/build/BUILD/fts-0.0.1/src/common/pro
ducer.cpp
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp: In function 'void cancelTransfer()':
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:300: error: 'gfal2_cancel' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp: At global scope:
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:484: error: ISO C++ forbids declaration of 'gfalt_event_t' with no type
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:484: error: expected ',' or '...' before 'e'
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp: In function 'void event_logger(int)':
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:489: error: 'e' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:491: error: 'udata' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:502: error: 'GFAL_EVENT_TRANSFER_ENTER' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:505: error: 'GFAL_EVENT_TRANSFER_EXIT' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:510: error: 'GFAL_EVENT_CHECKSUM_ENTER' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:510: error: 'GFAL_EVENT_SOURCE' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:512: error: 'GFAL_EVENT_CHECKSUM_EXIT' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:515: error: 'GFAL_EVENT_DESTINATION' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:520: error: 'GFAL_EVENT_PREPARE_ENTER' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:522: error: 'GFAL_EVENT_PREPARE_EXIT' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:525: error: 'GFAL_EVENT_CLOSE_ENTER' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:527: error: 'GFAL_EVENT_CLOSE_EXIT' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp: At global scope:
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:484: warning: unused parameter 'gfalt_event_t'
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp: In function 'int main(int, char**)':
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:723: error: 'gfalt_set_event_callback' was not declared in this scope
/builddir/build/BUILD/fts-0.0.1/src/url-copy/main.cpp:1127: error: 'gfal2_release_file' was not declared in this scope
make[2]: *** [src/url-copy/CMakeFiles/fts_url_copy.dir/main.cpp.o] Error 1

and there are missing build dependencies if I try to build on EPEL-5:
Error: No Package found for activemq-cpp-library
Error: No Package found for libcurl-devel
Error: No Package found for pugixml-devel

I suggest sorting the BuildRequires and Requires entries alphabetically for easier maintenance.

2. Please explain the need to use these non-standard options for compilation:
-findirect-inlining -ftree-switch-conversion -ftree-builtin-call-dce -fconserve-stack -fno-strict-aliasing

3. The descriptions of the (sub)packages are insufficient in detail and should be more verbose.

4. %{python_sitearch}/fts seems to unowned.

5. I see several files in %{_docdir}/fts3. That's not a standard location for documentation. Are these essential to the functioning of the software? Does something break if you remove them? If not, please list them as %doc.

6. only the server subpackage owns files in %{_sbindir}, so you could simply have %{_sbindir}/fts* instead of enumerating every single binary, but that's only a suggestion. Similar situation exists for the man pages.

7. Cosmetic issue: many lines have trailing whitespace.
Comment 3 Dominik 'Rathann' Mierzejewski 2013-04-28 06:50:27 EDT
Also, please start Release: numbering from 1.
Comment 4 michal.simon 2013-04-30 09:18:10 EDT
Thanks a lot for your feedback!

In response to the issues you pointed out:

0. Yes this package is meant only for EPEL-6

1. FTS package requires gfal2 version that is currently in testing so in order to build with mock you will need to enable testing repos or you can build it with koji.

As I wrote previously it is meant only for EPEL-6 (the pugixml package has not been released in EPEL-5)

BuildRequires and Requires entries have been sorted alphabetically.

2. The non standard compilation options have been removed.

3. I updated the description of the package and the subpackages

4. %{python_sitearch}/fts has an owner now

5. The files in question are now listed as %doc

6. I followed your suggestions regarding files in %{_sbindir} and %{_mandir}/man8/

7. I hope I removed all the trailing white-spaces

8. Release numbering starts now from 1
Comment 5 Dominik 'Rathann' Mierzejewski 2013-05-19 11:47:07 EDT
Package builds now. rpmlint output and my comments:
$ rpmlint /var/lib/mock/epel-6-x86_64/result
fts-devel.x86_64: E: description-line-too-long C This package contains development files (e.g. header files) for File Transfer Service V3.

Please wrap the line.

fts-devel.x86_64: W: no-manual-page-for-binary fts-set-priority
fts-devel.x86_64: W: no-manual-page-for-binary fts-transfer-list
fts-devel.x86_64: W: no-manual-page-for-binary fts-config-get
fts-devel.x86_64: W: no-manual-page-for-binary fts-config-del
fts-devel.x86_64: W: no-manual-page-for-binary fts-config-set
fts-devel.x86_64: W: no-manual-page-for-binary fts-transfer-submit
fts-devel.x86_64: W: no-manual-page-for-binary fts-transfer-cancel
fts-devel.x86_64: W: no-manual-page-for-binary fts-set-blacklist
fts-devel.x86_64: W: no-manual-page-for-binary fts-transfer-status
fts-devel.x86_64: W: no-manual-page-for-binary fts-set-debug

Can be ignored, but since you're the upstream, you could write those manpages.

fts-server.x86_64: W: spelling-error %description -l en_US bandwith -> bandwidth, band with, band-with

A real spelling error caught by rpmlint. Please fix.

fts-server.x86_64: W: non-standard-uid /var/lib/fts3/myosg.xml fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/status fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/logs fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/bdii_cache.xml fts3
fts-server.x86_64: W: non-standard-uid /var/log/fts3 fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/monitoring fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3 fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/stalled fts3

Can be ignored.

fts-server.x86_64: W: no-manual-page-for-binary fts_msg_bulk
fts-server.x86_64: W: no-manual-page-for-binary fts_url_copy
fts-server.x86_64: W: no-manual-page-for-binary fts_bringonline
fts-server.x86_64: W: no-manual-page-for-binary fts_bdii_cache_updater
fts-server.x86_64: W: no-manual-page-for-binary fts_db_cleaner
fts-server.x86_64: W: no-manual-page-for-binary fts_info_publisher
fts-server.x86_64: W: no-manual-page-for-binary fts_msg_cron
fts-server.x86_64: W: no-manual-page-for-binary fts_myosg_updater

As above.

fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-msg-bulk
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-myosg-updater
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-records-cleaner
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-msg-cron
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-info-publisher
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-bdii-cache-updater
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-server
fts-server.x86_64: W: service-default-enabled /etc/rc.d/init.d/fts-bringonline

Services should be disabled by default.

You have missing Requires for scriptlets:
Requires(post): chkconfig
Requires(preun): chkconfig
# This is for /sbin/service
Requires(postun): initscripts
Requires(preun): initscripts

fts-libs.x86_64: W: spelling-error %description -l en_US amongst -> among st, among-st, among
fts-libs.x86_64: W: spelling-error %description -l en_US jandling -> handling, candling, dandling

Please fix.

fts-libs.x86_64: W: non-standard-uid /usr/lib64/python2.6/site-packages/fts fts3

Can be ignored.

fts.src: W: spelling-error %description -l en_US importandly -> importantly, important
fts.src: W: invalid-url Source0: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/tar/fts-3.0.0.tar.gz HTTP Error 404: Not Found

This should be fixed as well.

6 packages and 0 specfiles checked; 1 errors, 40 warnings.
Comment 6 michal.simon 2013-05-24 10:18:49 EDT
Thanks again for your feedback!

In response to the issues you pointed out:

1. spelling has been fixed and lines have been wrapped
2. man pages have been added 
3. services are now disabled by default
4. the missing requirements for scriptlets have been added
5. the name of the tarball with sources has been updated

rpmlint is now silent expect 'non-standard-uid' warrning



Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts.spec
  
SRPM URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-3.0.1-1.el6.src.rpm
Comment 7 Dominik 'Rathann' Mierzejewski 2013-06-03 19:49:07 EDT
Some issues highlighted by fedora-review script:

Installation errors
-------------------
INFO: mock.py version 1.1.32 starting...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
Start: run
Mock Version: 1.1.32
INFO: Mock Version: 1.1.32
Start: lock buildroot
INFO: installing package(s): /home/rathann/build/SOURCES/fts/review-fts/results/fts-devel-3.0.1-1.el6.x86_64.rpm /home/rathann/build/SOURCES/fts/review-fts/results/fts-server-3.0.1-1.el6.x86_64.rpm /home/rathann/build/SOURCES/fts/review-fts/results/fts-libs-3.0.1-1.el6.x86_64.rpm /home/rathann/build/SOURCES/fts/review-fts/results/fts-client-3.0.1-1.el6.x86_64.rpm
ERROR: Command failed: 
 # ['/usr/bin/yum', '--installroot', '/var/lib/mock/epel-6-x86_64/root/', 'install', '/home/rathann/build/SOURCES/fts/review-fts/results/fts-devel-3.0.1-1.el6.x86_64.rpm', '/home/rathann/build/SOURCES/fts/review-fts/results/fts-server-3.0.1-1.el6.x86_64.rpm', '/home/rathann/build/SOURCES/fts/review-fts/results/fts-libs-3.0.1-1.el6.x86_64.rpm', '/home/rathann/build/SOURCES/fts/review-fts/results/fts-client-3.0.1-1.el6.x86_64.rpm', '--setopt=tsflags=nocontexts']
Error: Package: fts-server-3.0.1-1.el6.x86_64 (/fts-server-3.0.1-1.el6.x86_64)
           Requires: gfal2-plugin-http(x86-64) >= 2.1.0
 You could try using --skip-broken to work around the problem
 You could try running: rpm -Va --nofiles --nodigest

mock root did have epel-testing enabled.

Unfortunately, rpmlint is not completely silent (except for non-standard-uid):

Rpmlint
-------
Checking: fts-devel-3.0.1-1.el6.x86_64.rpm
          fts-server-3.0.1-1.el6.x86_64.rpm
          fts-libs-3.0.1-1.el6.x86_64.rpm
          fts-client-3.0.1-1.el6.x86_64.rpm
fts-server.x86_64: E: call-to-mktemp /usr/sbin/fts_url_copy
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/myosg.xml fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/status fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/logs fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/bdii_cache.xml fts3
fts-server.x86_64: W: non-standard-uid /var/log/fts3 fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/monitoring fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3 fts3
fts-server.x86_64: W: non-standard-uid /var/lib/fts3/stalled fts3
fts-libs.x86_64: E: call-to-mktemp /usr/lib64/libfts_common.so.0.0.1
fts-libs.x86_64: W: non-standard-uid /usr/lib64/python2.6/site-packages/fts fts3
4 packages and 0 specfiles checked; 2 errors, 9 warnings.

Source checksums are different:
----------------
https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/tar/fts-3.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : e42f0495c0cfbd6aadada260fb32ee17a3069ba975344292ca4d2e1a3efd0f45
  CHECKSUM(SHA256) upstream package : 8f16bbd35529c4880b069d61ffcd1fc00046fd18fa29318b7a7fa1062d7418fb

Additionally, in order to be sponsored, please find two (at least one non-trivial) pending package reviews and perform them (except for approving, which you can't yet - I'll do that). Please put chosen bug numbers in a comment in this review.
Comment 8 michal.simon 2013-06-14 06:34:47 EDT
The dependency on 'gfal2-plugin-http' has been removed.

The 'call-to-mktemp' error has been solved.

The checksum should be fine now.


URLs:

Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts.spec
  
SRPM URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-3.0.3-1.el6.src.rpm
Comment 9 Dominik 'Rathann' Mierzejewski 2013-06-16 21:28:04 EDT
Please record the changes you make between each update you post here in the %changelog section of the package.

I've also just noticed that the code is built with non-standard CFLAGS. -O3 overrides -O2. I think you should use

-D CMAKE_BUILD_TYPE=RelWithDebInfo instead of Release

There's no need to call /sbin/ldconfig in any %post/%postun scriptlets except for -libs subpackage.

%defattr(-,root,root,-) is only necessary for EPEL5 packages. It's best to add it in el5 branch only and keep the main spec file clutter-free.

The same goes for %clean section and rm -rf %{buildroot} at the beginning of %install.

Likewise, BuildRoot: tag is not necessary (only on EPEL5).

Similarly, per https://fedoraproject.org/wiki/Packaging:Python , defining %python_sitelib and %python_sitearch is necessary only on EPEL5. Moreover, you're not using %python_sitelib anywhere in the spec.

You can drop %attr(0755,root,root) and %attr(0644,root,root) from -server subpackage files unless the installed files have different access rights.

%files python
%defattr(-,root,root,-)
%dir %attr(0755,fts3,root) %{python_sitearch}/fts

Please explain the need for python-C glue libraries to be owned by non-root user.
Comment 10 michal.simon 2013-06-19 13:51:48 EDT
- changes are now recorded in changelog
- cmake build type has been changed to RelWithDebInfo
- the EPEL5 specific elements have been removed
- python bindings are no longer owned by fts3 user
Comment 11 Dominik 'Rathann' Mierzejewski 2013-06-19 18:19:52 EDT
Michał, you're supposed to provide a link to the latest specfile and src.rpm with each update, but I'll assume the URL for the specfile hasn't changed:
https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts.spec

%changelog entries must be sorted in reverse-chronological order (i.e. newest entries at the top) and you should drop %{?dist} from the version-release tags there.

Looking at the diff between current and previous spec, I still see some minor issues:

%files python
-%defattr(-,root,root,-)
-%dir %attr(0755,fts3,root) %{python_sitearch}/fts
+%dir %attr(0755,root,root) %{python_sitearch}/fts

The %attr part in the %dir line is now redundant. Also, is there any reason not to have simply:

%files python
%{python_sitearch}/fts

?

-python and -devel subpackages depend on the -libs subpackage, so including LICENSE and README as %doc in the two former packages is redundant.
Comment 12 michal.simon 2013-06-30 14:07:39 EDT
- changelog has been fixed
- the compact syntax you suggested is now used for -python subpackage
- the redundant %doc entries have been removed from -python and -devel subpackages


Spec URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts.spec
  
SRPM URL: https://grid-deployment.web.cern.ch/grid-deployment/dms/fts3/epel_release/fts-3.0.3-1.el6.src.rpm
Comment 13 michal.simon 2013-07-10 09:31:01 EDT
the first review I made:
https://bugzilla.redhat.com/show_bug.cgi?id=975041
Comment 14 michal.simon 2013-07-11 12:38:55 EDT
the second review I made:
https://bugzilla.redhat.com/show_bug.cgi?id=982161
Comment 15 Dominik 'Rathann' Mierzejewski 2013-07-13 17:30:13 EDT
This package is APPROVED. Thanks for your work, Michał. I'll take a look at your reviews now and once I find them acceptable, I'll sponsor you. Please provide your FAS user name.
Comment 16 michal.simon 2013-07-15 10:00:01 EDT
Thanks a lot! My FAS user name is simonm
Comment 17 Dominik 'Rathann' Mierzejewski 2013-07-21 16:49:10 EDT
You are now sponsored. Congratulations and welcome to Fedora. You can request the package to be created in Fedora Git now.
Comment 18 michal.simon 2013-07-22 09:07:01 EDT
New Package SCM Request
=======================
Package Name: fts
Short Description: GRID middleware, which manages reliable file transfer
Owners: simonm aalvarez
Branches: el6
InitialCC:
Comment 19 Jon Ciesla 2013-07-22 10:34:03 EDT
Git done (by process-git-requests).
Comment 20 Fedora Update System 2013-07-24 05:56:50 EDT
fts-3.1.0-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/fts-3.1.0-1.el6
Comment 21 Fedora Update System 2013-07-24 19:57:18 EDT
fts-3.1.0-1.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 22 Fedora Update System 2013-08-07 10:42:01 EDT
fts-3.1.1-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/fts-3.1.1-1.el6
Comment 23 Fedora Update System 2013-08-22 14:24:28 EDT
fts-3.1.1-1.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 24 Alejandro Alvarez 2014-11-26 11:35:05 EST
Package Change Request
======================
Package Name: fts
New Branches: el5
Owners: aalvarez simonm
InitialCC: 

Command line interface required in el5
Comment 25 Jon Ciesla 2014-11-26 12:02:51 EST
Git done (by process-git-requests).

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