Package Review Request: lcm 0.7.1 SPEC: http://nmarques.fedorapeople.org/packages/lcm-0.7.1/lcm.spec SRPM: http://nmarques.fedorapeople.org/packages/lcm-0.7.1/lcm-0.7.1-1.el5.src.rpm Description: ------------ LCM is a library for message passing and data marshaling targeted at real-time systems where high-bandwidth and low latency are critical. It provides a publish/subscribe message passing model and an XDR-style message specification language with bindings for applications in C, Java, and Python. It was originally designed and used by the MIT DARPA Urban Challenge Team as its message passing system. Packager Notes: --------------- * Regarding RHEL5 builds I was unable to filter the python internal dependencies out. The macros available seem to work fine for RHEL6 and Fedora builds. This is the only known problem to me. * I'm available to maintain this package on Fedora since it builds perfectly from the same sources/spec.
Nelson, it looks like your Fedora account doesn't exist anymore (at least not under the username nmarques). Are you still interested in maintaining this package? If so please update the spec/SRPM URLs and I can take this review.
Dan I have suspended my account a few days ago because July will be a very demanding month for me at $dayjob; In August I will be away on holiday and for the first 2 weeks I will be completely out of contact from the world. I'll update the ticket and give a new location to the spec/srpm since I'm not re-activating my account before I return from holiday. There shouldnt be much work since I already have this packaged and tested on my $dayjob (it's one of the packages with a lot of relevance to us). NM
That's great Nelson. I will await the new URLs.
I have updated the .spec and provided a srpm for el6 at: * http://nmarques.fedorapeople.org/packages/lcm-0.7.1/lcm.spec * http://nmarques.fedorapeople.org/packages/lcm-0.7.1/lcm-0.7.1-1.el6.src.rpm I have a few doubts though... mainly regarding the Java stuff (not much proficient with Java); please be intensive on the java package review and requires/buildrequires. The main reason why I submitted this package to EPEL is because one of our departments has extensive use of this package; we believe that we could serve the community by maintaining it on EPEL. The version we use internally we have a more complete spec file which includes some parametrization done don %post, %preun etc. This is much to ensure that 'net.core.rmem_max', 'net.core.wmem_max' and others to have the correct parametrization. I'm not sure if this is something useful to Fedora, but I am allowed to share that information and use it on the package if it's useful. Let me know if you believe it might become useful. NM PS: I have enabled my Fedora account so use fedorapeople.
Updated .spec and src.rpm after some testing on RHEL-5; SRPM: http://nmarques.fedorapeople.org/packages/lcm-0.7.1/lcm-0.7.1-1.el5.src.rpm
(In reply to comment #4) > The main reason why I submitted this package to EPEL is because one of our > departments has extensive use of this package; we believe that we could > serve the community by maintaining it on EPEL. This is great to hear :-) > The version we use internally > we have a more complete spec file which includes some parametrization done > don %post, %preun etc. > > This is much to ensure that 'net.core.rmem_max', 'net.core.wmem_max' and > others to have the correct parametrization. I'm not sure if this is > something useful to Fedora, but I am allowed to share that information and > use it on the package if it's useful. Let me know if you believe it might > become useful. I'm not quite sure what kind of parametrization you mean here. Are you talking about a %post scriptlet to modify sysctl variables? Probably the best way to do that would be to have the package drop a file in /etc/sysctl.d/ with the necessary settings. You would need to add it as Source1 and then cp it into the buildroot during %build, and of course add it to %files with %config(noreplace). If you do that, you should probably leave the settings commented out unless the package *really* needs them to function. It's not nice to go setting sysctl variables on people unexpectedly.
A few issues I noticed after the first look: * Use %{version} in the %files list for the -java subpackage, instead of hardcoding it. * lcm-logplayer-gui and lcm-spy are actually Java apps and need lcm.jar. These should be moved into the -java subpackage. * Is it really necessary to split out liblcm from the main package? These packages are both very small, and they have same Requires. And the main package just requires liblcm anyway. Each subpackage incurs some overhead (in yum metadata, Koji, and other places) so it would better to merge them, unless there is a good reason not to. This would also make the spec a bit simpler. * No need to have %doc ChangeLog NEWS in the -devel subpackage, just move these to be with the rest of the %doc files. * The false _lcm.so Provides in the -python subpackage should be filtered out. I'm not sure if there is a nice way to do this on RHEL5 (I will look into this), but for the newer distros you should use the new %filter_provides_in to filter this out. http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering You can wrap it in a conditional like this: %if 0%{?fedora} || 0%{?rhel} >= 6 ... %endif * You can remove the BuildRequires on redhat-rpm-config. http://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 * The -java package should be noarch, except on RHEL5 where that doesn't work. Add BuildArch: noarch to the subpackage (wrapped in a conditional like above). * You will need to build and install Javadocs into a -javadoc subpackage. http://fedoraproject.org/wiki/Packaging:Java#Javadoc_installation Try something like this in %build: %{javadoc} -d %{buildroot}/%{_javadocdir}/%{name} -sourcepath lcm-java \ lcm.lcm lcm.logging lcm.spy lcm.util * Some mandatory Java-related R/BR are missing: http://fedoraproject.org/wiki/Packaging:Java#BuildRequires_and_Requires * A couple of other RHEL5-specific issues from: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#Distribution_specific_guidelines The -devel subpackage needs a Requires on pkgconfig (wrapped in a conditional). BuildRoot does not include %{release}. I'd suggest also wrapping that in a conditional. It makes it easier to strip out the RHEL5-specific later when you finally get to stop supporting RHEL5, in another few decades :-) * Make the Requires arch-specific (add %{?_isa}) for the -devel subpackage: http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package ... except for -java subpackage which is noarch, but only on RHEL >= 6. Use conditionals in the spec :-) Okay, we probably haven't found all the issue yet but this should be enough to work on for now.
(In reply to comment #7) > * The false _lcm.so Provides in the -python subpackage should be filtered > out. I'm not sure if there is a nice way to do this on RHEL5 (I will look > into this), It looks like no effort is made in RHEL5 base to filter out these Provides, so let's not worry about it. Please do go ahead and filter it out for RHEL >= 6 though. > but for the newer distros you should use the new > %filter_provides_in to filter this out. > > http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering > > You can wrap it in a conditional like this: > > %if 0%{?fedora} || 0%{?rhel} >= 6 > ... > %endif
Dan, I've updated the spec file to introduced the changes requested (hopefully, it's done in the right way); I've replaced the src.rpm for a el6 one. http://nmarques.fedorapeople.org/packages/lcm-0.7.1/
I just remembered that the provides filtering cannot be used in packages that install binaries or system libraries: http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Usage so you had better take it back out again, and we can ignore the false Provides: _lcm.so. Sorry about that. There are a couple more small issues I can see with your updated spec, but I think we are getting close :-) * -javadocs subpackage should be -javadoc * Typo here: Requires: %{name}-jave = %{version}-%{release} * Requires: java and BuildRequires: java-devel need an epoch of 1, as in BuildRequires: java-devel >= 1:1.6.0 Requires: java >= 1:1.6.0 * BuildRoot still lacks %{release} http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#BuildRoot_tag * For the -javadoc subpackage you can drop the Requires onto the -java subpackage, since the javadocs are just static HTML files. However you should add Requires: jpackage-utils because that package owns /usr/share/javadoc/. * BuildArch: noarch needs to be conditional on RHEL >= 6 since it breaks on RHEL5. * Typo in changelog: 0.7.1.2 should be 0.7.1-2
Uploaded new version with the suggested fixes. I would just take this opportunity to ask something: - Regarding the RHEL-5 'noarch' issue, is it really necessary to have a conditional for the 'noarch'? My experience with this kind of issues is that even without the conditional it should still build the package as long as the 'noarch' isn't declared on the main package. If it's declared on any sub-package, on RHEL-5 it's ignored and the arch from the main package is inherited. We sure we want those conditionals ?
(In reply to comment #11) > - Regarding the RHEL-5 'noarch' issue, is it really necessary to have a > conditional for the 'noarch'? My experience with this kind of issues is that > even without the conditional it should still build the package as long as > the 'noarch' isn't declared on the main package. If it's declared on any > sub-package, on RHEL-5 it's ignored and the arch from the main package is > inherited. > > We sure we want those conditionals ? I agree, the fewer conditionals cluttering the spec the better. But I think they are needed in this case. I noticed yesterday that without them the build fails for me on RHEL5 like this: checking target system type... Invalid configuration `noarch-redhat-linux-gnu': machine `noarch-redhat' not recognized configure: error: /bin/sh ./config.sub noarch-redhat-linux-gnu failed
Getting close, Nelson. :-) Two issues at the bottom. Package Review ============== Key: - = N/A x = Pass ! = Fail ? = Not evaluated ==== C/C++ ==== [x]: MUST Header files in -devel subpackage, if present. [x]: MUST ldconfig called in %post and %postun if required. [x]: MUST Package does not contain any libtool archives (.la) [x]: MUST Package does not contain kernel modules. [x]: MUST Package contains no static executables. [x]: MUST Rpath absent or only used for internal libs. [x]: MUST Package is not relocatable. [x]: MUST Development (unversioned) .so files in -devel subpackage, if present. ==== 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. [-]: MUST Buildroot is not present [x]: MUST Package contains no bundled libraries. [x]: MUST Changelog in prescribed format. [-]: MUST Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: MUST Sources contain only permissible code or content. [x]: MUST Each %files section contains %defattr if rpm < 4.4 [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 Spec file lacks Packager, Vendor, PreReq tags. [-]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [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 Package requires pkgconfig, if .pc files are present. (EPEL5) [x]: MUST Requires correct, justified where necessary. [!]: MUST Rpmlint output is silent. rpmlint lcm-python-0.7.1-3.el5.x86_64.rpm lcm-python.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint lcm-javadoc-0.7.1-3.el5.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint lcm-java-0.7.1-3.el5.x86_64.rpm lcm-java.x86_64: W: no-manual-page-for-binary lcm-spy lcm-java.x86_64: W: no-manual-page-for-binary lcm-logplayer-gui 1 packages and 0 specfiles checked; 0 errors, 2 warnings. rpmlint lcm-debuginfo-0.7.1-3.el5.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint lcm-devel-0.7.1-3.el5.x86_64.rpm lcm-devel.x86_64: W: no-documentation 1 packages and 0 specfiles checked; 0 errors, 1 warnings. rpmlint lcm-0.7.1-3.el5.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. rpmlint lcm-0.7.1-3.el5.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /home/dcallagh/work/fedora/reviews/lcm/lcm-0.7.1.tar.gz : MD5SUM this package : 9d1c389ba9d4f172f08a4d7f249b869e MD5SUM upstream package : 9d1c389ba9d4f172f08a4d7f249b869e [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. [-]: 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). [?]: SHOULD Package functions as described. Not evaluated as this is a new library. [!]: SHOULD Latest version is packaged. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD The placement of pkgconfig(.pc) files are correct. [x]: SHOULD Scriptlets must be sane, if used. [x]: SHOULD SourceX is a working URL. [-]: 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. ==== Java ==== [-]: MUST If source tarball includes bundled jar/class files these need to be removed prior to building [x]: MUST Packages have proper BuildRequires/Requires on jpackage-utils [x]: MUST Fully versioned dependency in subpackages, if present. [x]: MUST Javadoc documentation files are generated and included in -javadoc subpackage [x]: MUST Javadoc subpackages have Requires: jpackage-utils [x]: MUST Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink) [x]: SHOULD Package has BuildArch: noarch (if possible) [x]: SHOULD Package uses upstream build method (ant/maven/etc.) Issues: [!]: MUST Rpmlint output is silent. The no-documentation warning for lcm-python and lcm-devel can be ignored, since they both Require the base package. The man pages for lcm-spy and lcm-logplayer-gui should be moved from the base package into the -java subpackage, so that they are with their respective binaries. That will clear up the no-man-page warnings for lcm-java. [!]: SHOULD Latest version is packaged. I see on https://code.google.com/p/lcm/ that upstream is up to version 0.9.0. Is there a compelling reason why 0.7.1 has to be used instead of the latest version? Generated by fedora-review 0.1.3 External plugins:
Hi, I've updated the files (spec and .srpm): https://lcm.googlecode.com/files/lcm-0.9.0.tar.gz - man pages have been moved; several ways to do this, I've kept the glob and excluded the ones going into the java sub-package. - regarding the version, we have 0.7.1 fully tested; 0.8.0 has show regressions and a tiny bug related with the TTL's; We haven't tested 0.9.0 to know if the bug was fixed (one of my work colleagues did reported it upstream). So it's a bit of a wild card. I'm not sure if the bug reported which affected one of our platforms is the one mentioned on the 0.9.0 changelog. Will investigate! Nevertheless, I did updated the package. About the 'noarch' and the error you are getting, this might sound wrog but one potential work around might be to '--target=%{_arch}' which has been working for me in cases where needed, though in my $dayjob our packaging isn't as strict as in Fedora :)
Sorry, wrong link (had the source link on the clipboard), the correct link would be: http://nmarques.fedorapeople.org/packages/lcm-0.7.1/
Nice work Nelson! This package is APPROVED
Thanks Dan. I need now to find a sponsor; I'm not a member of the packagers group.
New Package SCM Request ======================= Package Name: lcm Short Description: Lightweight communications and marshaling Owners: nmarques mrunge Branches: el5 el6 InitialCC:
@Nelson, May I ask what happened tha the component, assignee flags and cc was changed ? * Component changed from "Package Review" to "fedora-review" * Flags fedora-review+ has been removed * Assignee changed from Dan to Stanislav. (In this three cases, the change is wrong) How did you create this package scm request ?
When I enabled the 'Fedora CVS' flag it got assigned to Stanislav, then I moved back to Dan. The Fedora Review was probably my bad, when I was trying to locate the 'Fedora CVS'. I've emailed Dan requesting to place the flag fedora-review+ again.
(In reply to comment #17) > Thanks Dan. I need now to find a sponsor; I'm not a member of the packagers > group. Ok, I've already sponsored Nelson.
Oops, I didn't realise Nelson needed a sponsor. I think in that case the review is supposed to be done by the sponsor. But since this one is done now, I will reset the fedora-review+ flag, unless Matthias has any objections?
Hi Dan, Thanks for a very pleasant and swift review; pretty nice when it goes like this. I believe the flag (or lack of such) is my fault, my apologies; I should've flagged it. I've asked for help on #fedora-devel and Matthias did a very warm welcome and helped me out. I do know now how a review is done and once I become a bit more familiar with the tools I will help back in other reviews. Dan, I don't mind taking help, feel free to jump in as co-maintainer; with time I can probably help co-maintaining other packages. Thanks both for the warm welcome and help. NM PS: Now we get this to testing ;)
No worries Nelson. You can put me down as a co-maintainer if you'd like, and I will help out as much as I can.
(In reply to comment #22) > Oops, I didn't realise Nelson needed a sponsor. I think in that case the > review is supposed to be done by the sponsor. But since this one is done > now, I will reset the fedora-review+ flag, unless Matthias has any > objections? Hi Dan, yes, Nelson was unaware, he needed a sponsor for EPEL, too. Since his other package review requests don't look bad, I've sponsored him. Package guidelines are saying, the first review must be done from a sponsor; in practice, this is often handled as in this case. Dan, you did a good and solid job in this review, so I don't see a requirement, to do that again for myself. Thank you! Nelson, just go ahead. If you want Dan as co-maintainer, too, you should insert a new Package SCM request. Only the last request will be handled. @Pingou Regarding re-setting all kinds of flags and assignments, I'm currently suspecting some kind of JavaScript blocker, but this is just a working thesis.
New Package SCM Request ======================= Package Name: lcm Short Description: Lightweight communications and marshaling Owners: nmarques mrunge dcallagh Branches: el5 el6 InitialCC: Add Dan Callaghan as co-maintainer please.
Git done (by process-git-requests).
Fixed up title of bug to be normal (some of our tools look for this).
New Package SCM Request ======================= Package Name: lcm Short Description: Lightweight communications and marshaling Owners: nmarques mrunge dcallagh Branches: f17 InitialCC: Requesting a Fedora 17 branch for this package please.
(In reply to comment #29) Nelson, you need to set the fedora-cvs? flag for this to be picked up. Also I think it is supposed to say "Package Change Request", I'm not sure if that will confuse their scripts...
Already exists.
Dan, I don't know what to do from here, and this what I did: - on pkgdb retired the package; - removed myself from all groups I was subscrived (packaging, CLA, etc); - filed a ticket with infra-structure to disable my account, if possible permanently; - closed as WONTFIX all requests I had pending. If any steps are needed to comply with a proper drop-out, let me know so I can proceed into them and try to exit gracefully. Thanks everyone for the time spent; you guys were awesome. NM
Package Change Request ====================== Package Name: lcm New Branches: f16 f17 el5 el6 Owners: dcallagh Nelson has deprecated this package, although really he meant to just orphan it because he is no longer contributing to Fedora. I will take ownership of it so that his hard work is not wasted. I'm also requesting two new branches, f16 and f17.
Git done (by process-git-requests). Unretired devel, el5 and el6, please take ownership. f16 and f17 created.
lcm-0.9.0-6.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/lcm-0.9.0-6.fc16
lcm-0.9.0-6.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/lcm-0.9.0-6.fc17
lcm-0.9.0-6.fc16 has been pushed to the Fedora 16 stable repository.
lcm-0.9.0-6.fc17 has been pushed to the Fedora 17 stable repository.
lcm is now available in EPEL5, EPEL6, F16, and F17.