Bug 481034 - Review Request: coccinelle - Semantic patching for Linux (spatch)
Review Request: coccinelle - Semantic patching for Linux (spatch)
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michal Schmidt
Fedora Extras Quality Assurance
Depends On: 436843
  Show dependency treegraph
Reported: 2009-01-21 15:45 EST by Richard W.M. Jones
Modified: 2009-03-18 15:10 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.1.5-3.fc10.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2009-03-18 05:51:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mschmidt: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)
use python_sitelib (1.27 KB, patch)
2009-01-29 10:52 EST, Michal Schmidt
no flags Details | Diff

  None (edit)
Description Richard W.M. Jones 2009-01-21 15:45:32 EST
Spec URL: http://www.annexia.org/tmp/ocaml/coccinelle.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/coccinelle-0.1.4-2.fc10.src.rpm
Coccinelle is a tool to utilize semantic patches for manipulating C
code.  It was originally designed to ease maintenance of device
drivers in the Linux kernel.
Comment 1 Richard W.M. Jones 2009-01-21 15:46:38 EST
Home page is http://www.emn.fr/x-info/coccinelle/
and there is a LWN article about the tool here:
Comment 2 Richard W.M. Jones 2009-01-21 17:19:26 EST
Small problem with Python 2.6 in Rawhide.  The updated package below
fixes this:

Spec URL: http://www.annexia.org/tmp/ocaml/coccinelle.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/coccinelle-0.1.4-3.fc11.src.rpm
* Wed Jan 21 2009 Richard W.M. Jones <rjones@redhat.com> - 0.1.4-3
- Patch for Python 2.6.

Koji scratch build:

rpmlint reports:

coccinelle.src:42: W: configure-without-libdir-spec

 - This is OK because this isn't an autoconf-based configure.

coccinelle.x86_64: W: devel-file-in-non-devel-package /usr/share/coccinelle/standard.h

 - Coccinelle uses this as a data file, it's not a normal include file.

coccinelle.x86_64: W: unstripped-binary-or-object /usr/bin/spatch

 - This file shouldn't be stripped, which will damage it.

coccinelle.x86_64: W: ocaml-mixed-executable /usr/bin/spatch

 - This is fine.  Prelink-defeating config script is implemented.

coccinelle.x86_64: W: ocaml-naming-policy-not-applied /usr/lib64/ocaml/stublibs/dll*.so

 - This is OK because it's not an OCaml library.
Comment 3 Michal Schmidt 2009-01-22 03:48:08 EST
- The license notes in the sources do not have the "...or (at your option) any later version" wording, so the license should be GPLv2, not GPLv2+.

- You rename the 'dllpycaml_stubs.so' file to 'dll*.so' by mistake.

- It would be nice to have the demos/ and docs/ directories in the package or in a separate -doc package.
Comment 4 Michal Schmidt 2009-01-22 03:51:35 EST
- Shouldn't coccilib go under %{python_sitelib}?

- coccilib/ contains two useless Makefiles.
Comment 5 Michal Schmidt 2009-01-22 04:07:59 EST
- I stripped /usr/bin/spatch and it ran just fine afterwards. Does the stripping exception really apply then? (https://fedoraproject.org/wiki/Packaging:OCaml#Stripping_binaries)
Comment 6 Richard W.M. Jones 2009-01-22 15:34:45 EST
Thanks, I'm going to try looking at this tomorrow.  But on the
subject of stripping binaries, firstly you can tell if the
binary was damaged by strip by looking at the end of the binary:

$ cp /usr/bin/spatch .
$ hexdump -C spatch | tail -3
00254d50  42 00 00 0c 66 43 52 43  53 00 00 10 58 00 00 00  |B...fCRCS...X...|
00254d60  05 43 61 6d 6c 31 39 39  39 58 30 30 38           |.Caml1999X008|

Notice the Caml1999... signature there.

$ strip spatch 
$ hexdump -C spatch | tail -3
00040940  ee 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00040950  01 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|

And notice that it's gone after stripping, and the binary
no longer works:

$ ./spatch 
No bytecode file specified.

Secondly, this method of attaching bytecode to binaries (a) sucks,
(b) is deprecated upstream, and (c) we should build a native code
(non-bytecode) version of spatch for Fedora which bypasses the
entire issue.

So the whole thing is a big bug, and I need to rebuild this
RPM, with any luck tomorrow.  Although the RPM above will work
for people who just want to try out coccinelle.
Comment 7 Michal Schmidt 2009-01-22 16:11:51 EST
I forgot I had /usr/local/bin/spatch from my previous experiments, so that's why spatch ran fine even after I stripped /usr/bin/spatch :-) Sorry for the confusion in #5.
Comment 8 Michal Schmidt 2009-01-23 16:11:50 EST

In the Debian review ticket for coccinelle (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=512658) they noticed that the sources include a slightly modified copy of pycaml and the library part of menhir.
I did not find any of these two packages currently in Fedora, so it would not cause a duplication, but still it would be nicer to have pycaml and menhir packaged separately if possible.
Comment 9 Richard W.M. Jones 2009-01-24 10:40:27 EST
I've been chatting with the Debian packagers about these
issues, and it looks like they are going to organize a
revitalized upstream for pycaml.  (Upstream is dead and
there are at least two major forks).  We don't have this
package in Fedora at the moment as you say.

I posted an intent to package Menhir a while back
(bug 436843) but haven't got around to a package yet.
Comment 10 Richard W.M. Jones 2009-01-26 10:53:15 EST
Phew, that was a bit more complicated than I anticipated ...

Here is an updated version which installs the optimized / native
code version of spatch by default, and leaves the bytecode version
for those poor platforms that have no code generator.  Every
Fedora platform should get the fast version in other words ...
The native code spatch can be, and is, stripped.

The Python 2.6 patch that I included earlier was a bit broken,
this includes a corrected version of that patch.

Menhir and pycaml and still built internally.

Spec URL: http://www.annexia.org/tmp/ocaml/coccinelle.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/coccinelle-0.1.4-6.fc11.src.rpm

* Mon Jan 26 2009 Richard W.M. Jones <rjones@redhat.com> - 0.1.4-6
- Install the shared library in _libdir.
- Install the native code version if we have the optimizing compiler.
Comment 11 Michal Schmidt 2009-01-27 09:03:26 EST
I don't think %{_libdir} is the correct path for the stub lib. Other ocaml-* packages put them in %{_libdir}/ocaml/stublibs/. And they have a *.so.owner file there too. I don't how that file is created.
Comment 12 Richard W.M. Jones 2009-01-28 15:28:58 EST
I tried putting the shared library into ${_libdir}/ocaml/stublibs
but that just means that spatch can't find the shared library and
so doesn't start up.  This doesn't appear to be an ordinary use
of the OCaml stublibs (which in ordinary OCaml programs are only
used for bytecode, and the bytecode interpreter does dlopen with
the correct library path).

The *.so.owner files are generated and used by ocamlfind, and are
not an issue here.

So I guess we have to leave the library in %{_libdir}.

In reply to comment 3:
I've corrected the license.  Docs and demos are now in subpackages.

In reply to comment 4:
I got rid of the useless Makefiles and moved the python library.
However I've little idea about the Python code, not even how to test
if it is working.

Spec URL: http://www.annexia.org/tmp/ocaml/coccinelle.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/coccinelle-0.1.4-8.fc11.src.rpm

* Wed Jan 28 2009 Richard W.M. Jones <rjones@redhat.com> - 0.1.4-8
- Remove useless Makefiles from python/coccilib.
- License is GPLv2 (not GPLv2+).
- Include documentation and demos in subpackages.
- Move python library to a more sensible path.
- Add a check section.
Comment 13 Michal Schmidt 2009-01-29 10:52:10 EST
Created attachment 330369 [details]
use python_sitelib

Please use the correct way to find Python's site-packages path described in http://fedoraproject.org/wiki/Packaging/Python

See the attached patch.
Comment 14 Martin Nagy 2009-03-11 17:13:01 EDT
Comment 15 Richard W.M. Jones 2009-03-12 09:10:10 EDT
On my list for today/tomorrow...
Comment 16 Richard W.M. Jones 2009-03-16 08:54:11 EDT
OK!  Better late than never ..

There is a new version upstream, and this uses the correct
method to find the python sitelib, using Michal's patch from
comment 13.

I believe, from reading back over the comments, that this should
address everything.

Spec URL: http://www.annexia.org/tmp/ocaml/coccinelle.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/coccinelle-0.1.5-1.fc11.src.rpm

* Mon Mar 16 2009 Richard W.M. Jones <rjones@redhat.com> - 0.1.5-1
- New upstream version 0.1.5.
- Use the correct method to get Python sitelib (Michal Schmidt).
Comment 17 Richard W.M. Jones 2009-03-16 08:57:03 EDT
rpmlint says:

coccinelle.src:77: W: configure-without-libdir-spec
coccinelle.x86_64: W: devel-file-in-non-devel-package /usr/share/coccinelle/standard.h

See comment 2 for both of these.

coccinelle.x86_64: W: no-soname /usr/lib64/dllpycaml_stubs.so

See comment 12.

coccinelle-examples.x86_64: W: spurious-executable-perm /usr/share/doc/coccinelle-examples-0.1.5/demos/launch.sh
coccinelle-examples.x86_64: W: doc-file-dependency /usr/share/doc/coccinelle-examples-0.1.5/demos/launch.sh R

I believe rpmlint is wrong here, and/or the packaging guidelines
aren't sufficiently specific about whether examples scripts can be
executable.  See:

Comment 18 Richard W.M. Jones 2009-03-16 09:00:18 EDT
Koji scratch build:

Comment 19 Richard W.M. Jones 2009-03-16 09:07:23 EDT
That didn't work, because of missing BR python-devel.
Attempt two:

Spec URL: http://www.annexia.org/tmp/ocaml/coccinelle.spec
SRPM URL: http://www.annexia.org/tmp/ocaml/coccinelle-0.1.5-2.fc11.src.rpm

* Mon Mar 16 2009 Richard W.M. Jones <rjones@redhat.com> - 0.1.5-2
- BR python-devel.

Koji scratch build:
Comment 20 Michal Schmidt 2009-03-17 12:44:26 EDT
OK: rpmlint was run and its output discussed.
OK: Package name meets the Package Naming Guidelines.
OK: The *.spec file name matches the package name.
OK: The package meets the Packaging Guidelines:
  OK: naming
  OK: version and release
  OK: licensing
  OK: no pre-built binaries
  OK: spec is legible
  OK: builds on all archs
  OK: FHS layout
  OK: changelogs
  OK: tags
  OK: BuildRoot tag
  OK: clean buildroot in %install and %clean
  OK: Requires
  OK: BuildRequires
  OK: summary and description
  OK: encoding
  OK: documentation
  N/A: compiler flags (program written in OCaml)
  N/A: debuginfo (program written in OCaml)
  N/A: devel packages
  OK: requiring base package
  N/A: shared libraries (dllpycaml_stubs.so is not a normal library)
  OK: no static libraries
  OK: no duplication of system libraries
  OK: rpath deleted
  N/A: config files
  N/A: initscripts
  N/A: desktop files
  OK: macros
  N/A: locale files
  OK: timestamps
  OK: parallel make not used for a reason
  N/A: scriptlets
  N/A: conditional deps
  OK: non-relocatable package
  OK: file and directory ownership
  N/A: users and groups
  N/A: web apps
  OK: no conflicts
  OK: no kernel modules
  OK: nothing in /srv
  OK: not bundling multiple projects
  !!! BAD !!!: Patch0 has no comment about its purpose and upstream status.
  OCaml-specific guidelines:
    OK: binary stripped, bytecode not stripped
    OK: compiled binary prefered to bytecode
    OK: tests for native compiler
OK: Meets Licensing Guidelines.
OK: License (GPLv2) tag matches the actual license.
OK: The license file included in %doc.
OK: Spec file written in American English.
OK: Source matches upstream. sha1sum: 3457c54a8e13e129a1c514debd6c9e7d41abf9d9  coccinelle-0.1.5.tgz
OK: Builds in Koji on all archs.
OK: No duplicates in %files.
OK: File permissions correct, defattr used properly.
OK: Code or permissable content.
OK: Large documentation in a separate subpackage.
OK: Program works even without %doc.
N/A: Headers in -devel.
N/A: pkconfig files.
N/A: Library with a suffix.
OK: Subpackages require the base package using full-versioned dependencies.
OK: No .la files.
OK: Does not own files or dirs already owned by other packages.
OK: Filenames are valid UTF-8.


Please add a comment for Patch0 explaining why the patch is needed and what its upstream status is.

The Packaging Guidelines recommend using *-doc as the documentation package name. Yours is named *-docs. Consider changing it. *-docs is not unusual, but *-doc is a bit more common:
yum list \*-doc | wc -l
yum list \*-docs | wc -l

None of these two issues are blockers and they are easy to fix before you import to CVS.

I approve the package.
Comment 21 Richard W.M. Jones 2009-03-17 13:09:17 EDT
Thanks Michal.  I'll make the changes you suggest.
Comment 22 Richard W.M. Jones 2009-03-17 13:13:06 EDT
New Package CVS Request
Package Name: coccinelle
Short Description: Semantic patching for Linux (spatch)
Owners: rjones
Branches: F-10
Comment 23 Kevin Fenzi 2009-03-17 23:30:08 EDT
cvs done.
Comment 24 Fedora Update System 2009-03-18 05:51:10 EDT
coccinelle-0.1.5-3.fc10.1 has been submitted as an update for Fedora 10.
Comment 25 Fedora Update System 2009-03-18 15:10:34 EDT
coccinelle-0.1.5-3.fc10.1 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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