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.
Taking this review. I will sponsor Michał.
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.
Also, please start Release: numbering from 1.
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
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.
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
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.
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
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.
- 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
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.
- 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
the first review I made: https://bugzilla.redhat.com/show_bug.cgi?id=975041
the second review I made: https://bugzilla.redhat.com/show_bug.cgi?id=982161
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.
Thanks a lot! My FAS user name is simonm
You are now sponsored. Congratulations and welcome to Fedora. You can request the package to be created in Fedora Git now.
New Package SCM Request ======================= Package Name: fts Short Description: GRID middleware, which manages reliable file transfer Owners: simonm aalvarez Branches: el6 InitialCC:
Git done (by process-git-requests).
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
fts-3.1.0-1.el6 has been pushed to the Fedora EPEL 6 testing repository.
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
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.
Package Change Request ====================== Package Name: fts New Branches: el5 Owners: aalvarez simonm InitialCC: Command line interface required in el5