Bug 783071

Some nitpicking :
1)
BuildRequires:  tex(tex), tex(latex)

would be better on 2 lines

2)
sed -i 's/1.46.1/1.46.0/' CMakeLists.txt

should be commented, since while I assume the goal is to make it compile with 1.46.0 boost version, that should be pushed upstream ( ie, treated like a patch ).


3) The doc pull the main package, and I am not sure if that's the way it should be ( ie, I can read the doc without the main framework, no, or may wish to propose it for download on a webserver ).

4) not all defattr have been dropped. So for consistency, I would either drop them all, or keep them all ( if you plan to submit for epel 5 )

5) Source0 do not have a url, and this could be added IMHO ( to ease various future checks ).

I will start a more formal review once this have been fixed :)

1. no problemo
2. diet do require Boost 1.46.1, but since the required fixes have been backported into Fedora's 1.46.0 Boost, there's no harm. It will be dropped as soon as F15 goes EOL
3. done
4. my bad, done.
5. upstream download url requires a password sent by mail after you fill in a form. I've been trying for a year to convince the project leader to drop this useless download process. This point should be fixed for the upcoming release.

Above spec and src.rpm have been updated.

1) the tarball is behind a form, and I am not keen into giving my personal detail without knowing where it goes ( no privacy policy, among others ). Can you maybe tell me more about it ? ( as I need to check the tarball )

2) the %{_datadir}/%{name}/FWorkflow.dtd create a unowned directory 

3) same for %{_docdir}/%{name}-doc/UsersFAQ.pdf

4) the rpm fail to build in mock, but build in koji, that's rather strange ( especially since everything look correct ) ( but if this build in koji, that should be enough I guess )

5) installing the rpm for doc do not install any license, that's against the guideline

1. Since the website is maintained by the research team, no data should be used for any commercial use. I'll forward your concern about the lack of privacy policy.

2. & 3. fixed

4. i guess, this is a mirror failure
http://koji.fedoraproject.org/koji/taskinfo?taskID=3906057

5. fixed

updated spec and src.rpm:
http://hguemar.fedorapeople.org/diet/diet.spec
http://hguemar.fedorapeople.org/diet/diet-2.8.0-2.fc16.src.rpm

I guess you should add a comment to explain how to get the tarball. I also guess the password can be placed as a comment ( especially since the form is asking lot of unrelated questions for a packager ).

I am planning to ask the question to the FPC, but do not let that be blocking ( since logservice seems to be behind the same form, so I guess that's not blocking ).

I just received the password by mail, so let's test and push the review forward.

Guidelines ask to enable the test. I see there is a directory Cmake/tests, and it does look like unit tests, have you tried to enable them ?

Also, the COPYING file is a BSD one ( and a old one, dating back to 2003 , I guess the software was licensed again ), while the license is listed as CeCILL in the spec. So I guess you should also ship the 2nd license too. 

And the source code do not really help on that, since they didn't fill the field "License" in the comments. The only file saying the code is under CeCILL is ./src/utils/batch/EucaLib/soapC.c. While I know that CeCILL is more or less the blessed licence for french research community ( since it was done for that ), I think you should warn upstream to clearly say it on the website, on documentation, etc ( as I didn't found anything regarding that ). 

In both case ( IE BSD or CeCILL ), that's free software so that should be ok.

1. they take more than half hour to execute on a high-end workstation so it's not an option.
2. i didn't included the right file (i'll check the BSD license file which might be irrelevant now)
3. i guess the tarball generation script has not been updated, i'll fix this for the upcoming release

http://koji.fedoraproject.org/koji/taskinfo?taskID=3908364

updated spec and src.rpm:
http://hguemar.fedorapeople.org/diet/diet.spec
http://hguemar.fedorapeople.org/diet/diet-2.8.0-3.fc16.src.rpm

So for the tests, it would be runnable on a smartphone in 3 years if we follow moore's law :)

Anyway, let me test that it install on f17 and that it start, and it should be good.

$ rpmlint diet-2.8.0-3.fc17.i686.rpm 
diet.i686: W: spelling-error %description -l en_US middleware -> middle ware, middle-ware, middleweight
diet.i686: E: shlib-with-non-pic-code /usr/lib/libDIET_admin.so.2.8.0
diet.i686: E: shlib-with-non-pic-code /usr/lib/libDIET_client.so.2.8.0
diet.i686: W: shared-lib-calls-exit /usr/lib/libDIET_client.so.2.8.0 exit
diet.i686: E: shlib-with-non-pic-code /usr/lib/libDIET_Dagda.so.2.8.0
diet.i686: W: shared-lib-calls-exit /usr/lib/libDIET_Dagda.so.2.8.0 exit
diet.i686: E: shlib-with-non-pic-code /usr/lib/libDIET_SeD.so.2.8.0
diet.i686: W: shared-lib-calls-exit /usr/lib/libDIET_SeD.so.2.8.0 exit
diet.i686: W: file-not-utf8 /usr/share/doc/diet-2.8.0/LICENSE.txt
diet.i686: W: no-manual-page-for-binary dietObjects
1 packages and 0 specfiles checked; 4 errors, 6 warnings.

rpmlint seems to be rather unhappy

The file-not-UTF8 is IMHO not that important, but nice to fix.

The shlib-with-non-pic-code , according to https://fedoraproject.org/wiki/Common_Rpmlint_issues#shlib-with-non-pic-code is a no-go, since this can cause issue with selinux.

The shlib-calls-exit is something to be brought upstream.
The lack of manpages for dietObjects too.

The package install fine, but I have no idea on how it work. It seems to need a configuration file, could you bundle a example in documentation ( but not blocking for the review )?


Also, the man pages of maDagAgent say this is GPLv3+, which is different from BSD and from CeCILL.

dietForwarder say GPLv3 ( without + ) ( and should IMHO have a --help option, but that's outside the scope of the review ). 

So the license tag should be IMHO clearer, cause I do not know if that's compatible.

Also, I think the service should be started by systemd, but IIRC, that's planned for a next iteration of the rpm ( again, not blocking for the review ) ?

1. the BSD license file has been removed (according INRIA, it is just a cruft)
2. the shlib-with-non-pic-code issue has been fixed in the master (I had to re-do the whole build system), since the patch is non trivial, a minor release will be done within next weeks. For the moment, i generated a post release tarball. Until the new download page is up, official tarball will be available on my personal page:
http://graal.ens-lyon.fr/~hguemar/diet/archives/
3. I pushed a tutorial on how testing DIET 
http://graal.ens-lyon.fr/~hguemar/diet/archives/diet-howto.rst
4. there's not much interest in having systemd services since each agent needs an unique name (and there is not necessarily one agent per machine). Probably, we need to have puppet/chef recipes for configuration in the long term.

new spec and new src.rpm:
http://hguemar.fedorapeople.org/diet/diet.spec
http://hguemar.fedorapeople.org/diet/diet-2.8.0-4.cd326f85f75cgit.fc16.src.rpm

And for the GPL v3 in some software ?

where ? All mentions to GPLv3 should have been removed by now.
The only remaining files licensed under GPLv2 are related to cloud support (not enabled since it's scheduled to be rewritten for next release) but there's no incompatibility with CeCILL

In the man pages 
 $ grep -r GPL .
./doc/man/dietForwarder.1:License: GPLv3
./doc/man/maDagAgent.1:License: GPLv3+
./doc/man/dietAgent.1:License: GPLv3+

But since that's IIRC compatible with cecill, I guess that's not a huge problem, so I will finish the review ( or restart, if I cannot find where I placed the file )

My bad, man pages were not regenerated (it is fixed in restructured text sources), i'll fix this later if it doesn't block the review

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[X]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[X]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[X]: MUST %build honors applicable compiler flags or justifies otherwise.
[X]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
     Note: The package did not built BR could therefore not be checked or the
     package failed to build because of missing BR
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only 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 is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
Package has no sources or they are generated by developer
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[x]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.


Generated by fedora-review 0.1.3
External plugins:

$ rpmlint *
diet.i686: W: spelling-error %description -l en_US middleware -> middle ware, middle-ware, middleweight
diet.i686: W: shared-lib-calls-exit /usr/lib/libDIET_client.so.2.8.0 exit
diet.i686: W: shared-lib-calls-exit /usr/lib/libDIET_Dagda.so.2.8.0 exit
diet.i686: W: shared-lib-calls-exit /usr/lib/libDIET_SeD.so.2.8.0 exit
diet.i686: W: no-manual-page-for-binary dietObjects
diet-devel.i686: W: no-documentation
diet-examples.i686: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 7 warnings.

the spelling-error is a false positive. The no-dcomentation is not a blocker, as is the lack of manpage.

The shared-lib-calls-exit is ugly, but that's outside of the scope of this review. 

So since all problems were solved, I think the package is good to go.

Thanks for reviewing this package !

New Package SCM Request
=======================
Package Name: diet
Short Description: A computational servers toolkit
Owners: hguemar
Branches: f15 f16 f17
InitialCC:

Git done (by process-git-requests).

diet-2.8.0-4.cd326f85f75cgit.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/diet-2.8.0-4.cd326f85f75cgit.fc17

diet-2.8.0-4.cd326f85f75cgit.fc17 has been pushed to the Fedora 17 testing repository.

diet-2.8.0-4.cd326f85f75cgit.fc17 has been pushed to the Fedora 17 stable repository.