Bug 442714

SPEC URL: http://rpm.binaryhelix.org/SPECS/libs/sat-solver.spec
SRPM URL: http://rpm.binaryhelix.org/SRPMS/sat-solver-0.0.33-1.fc9.src.rpm

* Tue May 20 2008 Lorenzo Villani <lvillani> - 0.0.33-1
- Version bump

Building on x86_64 gives the following error during rpmbuild:

error: Installed (but unpackaged) file(s) found:
   /usr/lib/zypp/zypp-query-pool

Sorry, the above comment should have been on libzypp, not sat-solver.

SPEC URL: http://rpm.binaryhelix.org/SPECS/libs/sat-solver.spec
SRPM URL: http://rpm.binaryhelix.org/SRPMS/sat-solver-0.9.0-1.fc9.src.rpm

* Mon Jun 09 2008 Lorenzo Villani <lvillani> - 0.9.0-1
- Version bump

Good:
+ Local build works fine on x86_64
+ Rpmlint on source package is quite
+ Consistent macro usage
+ package use a OSS license
+ Tar archive in the package matches with upstream
  (md5sum: 52bc1b0309812eead630f8dfe795c923)
+ Rpmlint is quite on sat-solver package
+ File permissions seems ok.
+ Package seems not to have files own by other packages.
+ Local install and uninstall works fine.
+ Package build fine in mock (xu6_64, ppc64 and ppc, devel)


Bad:
- %{_includedir]/satsolver should be own by the package
- Package should not contains static libraries
- Package doesn't contains license text, but upstream package contains a
  verbatin copy of the license.
- Rpmlint complaints on devel package:
  $ rpmlint sat-solver-devel-0.9.0-1.fc9.x86_64.rpm
sat-solver-devel.x86_64: W: no-documentation
- Package seems not to use $RPM_OPT_FLAGS

Questions:
? Why you remove the testsuite. From my point of view this may be helpful to
ensure the QA of the package

Before uploading a new version of the package I need something to be explained:

Bad things:
- %{_includedir}/satsolver: As far as I know, %{_includedir}/satsolver/* gives
ownership to directory-contained files
- static libraries: as far as i know, upstream build system (based on cmake)
only produces static library archives and not shared objects and it seems that
other packages depending on this library statically link the library in their
final executable
- verbatim copy of the license: I included that in both packages using %doc
- rpmlint warning with sat-solver-devel: the inclusion of the verbatim copy of
the license should fix this
- $RPM_OPT_FLAGS: This is due to usage of cmake, afaik, using
-DCMAKE_BUILD_TYPE=FEDORA is sufficient, I added that to the cmake line

And now the questions:
I removed the *huge* testsuite (nearly 50MiB of stuff -compressed-) just to save
bandwidth (yes, I'm lazy), If needed I can keep the testsuite inside the tarball
but this will make the review process much slower because i'll have to upload
~100MiB of compressed tarballs (~50MiB of .src.rpm and ~50MiB of packaged snapshot).

Side notes:
As far as I know both sat-solver, libzypp and zypper can be retrieved only from
upstream' subversion (or by 'extracting' source tarball from SuSE' .src.rpm) so
the source archive is just a local subversion checkout with SCM directories
stripped out (ie: tar'ed using --exclude-vcs)

A reviewer assigned this package to himself, then he didn't reply to my questions.

Releasing the package and restoring the fedora-review flag to empty value.

Reviewing a package is a volunteer process, so you cannot expect that there are
no delays in the communication. So removing a reviewer is at least unpolite.

Now some answers
- %{_includedir}/satsolver - this includes both the directory and the files in
the directory (and any deeper hierarchy), so use it
- $RPM_OPT_FLAGS - there should be a way how to pass own CFLAGS into the compile
process done with cmake, I think/hope :-)
- static libraries - then you should investigate, how to enable building of
shared libraries, using static libs is allowed only in very very special cases

If I remember correctly he assigned then de-assigned the bug to himself (or he
didn't assign the bug to himself at all!) but he left the fedora-review flag to
'?'. The reviewers guidelines state that the reviewer must set fedora-review to
'?' AND assign the bug to himself. This bug was not assigned to anybody but the
flag was set to '?', as a consequence I restored the default value for
fedora-review flag.

The "Show Bug Activity" link should tell you everything you need to know about
the history of this ticket.  As far as I can tell, this ticket has never been
assigned to anyone and Jochen isn't even CC'd on this ticket so he wouldn't see
any of the remarks.

Note that we do have some cmake information:
http://fedoraproject.org/wiki/Packaging/cmake

I would not approve this package without the test suite being included and run
at build time unless that's just not possible.

Yes, I should check the bug history and not blindly trust the comment #7. But I
think all my points are still valid. Our Wiki has a lot of answers for technical
questions. And I have a personal experience with Jochen as reviewer and I know
that it can take weeks before he returns to the review.

The tests could be perhaps omitted for package iterations that should clean the
general packaging issues but then they should be added back. Reduced size can
really help both sides when they are using e.g. ADSL. But I agree with Jason -
if they can be run, they must be included.

I can include them and remove that patch that disables the test suite at import-I can include them and remove that patch that disables the test suite at import-time, if needed

Uhm, sorry for the weird comment.. it seems that konqueror4 is not much usable.

SPEC URL: http://rpm.binaryhelix.org/SPECS/libs/sat-solver.spec
SRPM URL: http://rpm.binaryhelix.org/SRPMS/sat-solver-0.9.0-3.fc9.src.rpm

* Thu Jun 19 2008 Lorenzo Villani <lvillani> - 0.9.0-3
- Using %%cmake properly
- Proper handling of %%{_includedir}/satsolver

If you please can fix the static libs issue (ping upstream about the problem?),
I would do a proper review of this package (and rest of the zypper stack (which
seems in pretty good shape by the first brief look)).

I sent an email to upstream' development mailing list but I didn't get any
answer. What are we going to do if they don't respond?

Upstream says that they're doing such way because of the still unstable API.
Since they're stabilising the API they say that they will be providing shared
libraries in the near future.

SPEC URL: http://rpm.binaryhelix.org/SPECS/libs/sat-solver.spec
SRPM URL: http://rpm.binaryhelix.org/SRPMS/sat-solver-0.9.2-1.fc9.src.rpm

* Sat Jul  5 2008 Lorenzo Villani <lvillani> - 0.9.2-1
- Version bump (aligned to upstream stable version)

> Upstream says that they're doing such way because of the still unstable API.
> Since they're stabilising the API they say that they will be providing shared
> libraries in the near future.

If you must use static libs, then at least follow the guidelines:
 
 https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries

Add Provides for sat-solver-static and add 
Buildrequires: sat-solver-static to libzypp/zypper packages.

When package is included open a bug against the package regarding the
static libs issue.

SPEC URL: http://rpm.binaryhelix.org/SPECS/libs/sat-solver.spec
SRPM URL: http://rpm.binaryhelix.org/SRPMS/sat-solver-0.9.2-2.fc9.src.rpm

* Sat Jul  5 2008 Lorenzo Villani <lvillani> - 0.9.2-1
- provide sat-solver-static

NEW SPEC URL: http://www.binaryhelix.net/sat-solver.spec
NEW SRPM URL: http://www.binaryhelix.net/sat-solver-0.9.2-2.fc9.src.rpm

SPEC URL: http://omploader.org/vbm95/sat-solver.spec
SRPM URL: http://omploader.org/vbm96/sat-solver-0.9.4-2.fc9.src.rpm

* Thu Aug  7 2008 Lorenzo Villani <lvillani> - 0.9.4-2
- Removed link to source tarball, it is not needed. Upstream distributes
  sat-solver, libzypp and zypper using a branched subversion tree, the
  tarball which comes with the SRPM is just an exported subversion checkout

By URL in spec - http://en.opensuse.org/Libzypp/Devel I can't found any reference to sat-solver.

I'm glad to see this! Two quick notes:

miniSAT, another SAT solver that's used by several other programs, is here:
 https://bugzilla.redhat.com/show_bug.cgi?id=453701

"sat-solver" is a really generic name; it's quite likely that Fedora will end up with several competing SAT solvers.  Shouldn't this SAT solver's package name be something a little more specific, like "libzypp-sat-solver"?  That would leave "sat-solver" as a generic name (one that might, in the future, resolve to whatever default SAT solver the user desires, a la Java implementations).

In the "minisat2" package I added a small test case file, which _is_ satisfiable.  This would take a trivial amount of space, if you don't want to include a massive test suite.  It doesn't stress the solver, but it's a reasonable "smoke test" to see if the solver can run:


c
c start with comments
c
c 
p cnf 5 3
1 -5 4 0
-1 5 3 4 0
-3 -4 0

Do you really need to specify 'Requires:  expat rpm zlib'? RPM is able to pick dependencies on shared libraries automatically.

I'm not a native speaker, but I'm pretty sure the word is spelled 'satisfiability', not 'satisfyability' (Wikipedia and Googlefight confirm it).

@Pavel Alexeev
I'm sorry, the real URL is: http://en.opensuse.org/Libzypp/Sat_Solver

@David A. Wheeler
sat-solver is the upstream name. I just packaged it with it's original name. But I'm willing to change its name. Regarding the smoke test, I'll add that ASAP, it's more convenient than the huge test suite.

@Michal Schmidt
I'll remove that dependencies in 'Requires' and fix the spelling issues

I am on vacation, it'll require some time to get the issues fixed. Please be patient and thanks for the interest :-)

SRPM URL: http://omploader.org/vcDdi/sat-solver-0.9.4-3.fc9.src.rpm
SPEC URL: http://omploader.org/vcDdj/sat-solver.spec

* Sat Aug 23 2008 Lorenzo Villani <lvillani> - 0.9.4-3
- removed useless requires
- Added a rpm database corruption patch
- Use correct URI

If anyone wants to try the latest package with the included patch...
It *should* fix a bug where yum, when used after zypper (or vice-versa) corrupt the RPM database. In particular it seems to not initialize the db cursor correctly.
Please note that you should recompile libzypp and zypper against this version of the package. I'm testing it too, if no corruption happens in 2 days I'll assume that the issue has been fixed.

SRPM URL: http://omploader.org/vcDk3/sat-solver-0.9.4-4.fc9.src.rpm
SPEC URL: http://omploader.org/vcDk4/sat-solver.spec

Nothing exceptional, only fixed the mispelled word.

So is anyone interested in taking over this review?

Lorenzo, are you sponsored and part of the packager group? I could not locate you on this list: https://admin.fedoraproject.org/accounts/group/members/packager?search=*

(In reply to comment #31)
> Lorenzo, are you sponsored and part of the packager group? 

Lorenzo has "arbiter" FAS name is sponsored by spot.

I'm still willing to get the package in Fedora, the above links *should* still point to srpm and specfile, if not just tell me. Please report if there's anything wrong with the package. I got no responses for a long time but I don't want to give up ;-)

MUST Items: 

OK - rpmlint is clean
OK - follows Naming Guidelines
OK - spec file is named as %{name}.spec

xx - package does not meet Packaging Guidelines
    + The Source0 tag should have a valid URL pointing to the upstream release
      tarball. This is an important requirement. In case upstream does not
      provide any such tarball, the Spec should have a comment above the
      Source0 tag describing how the sources were obtained to create the
      package. See:
      https://fedoraproject.org/wiki/Packaging/SourceURL
    + Could you throw some light on why it is a problem to build the language
      bindings on Fedora? Is it because of the ruby-rpm breakage in Rawhide?
    + Please do not strip the test-suite, if it is not absolutely necessary.
      Laziness arising out of needing to upload the SRPM several times is not
      a valid reason. :-)
    + It is not really necessary to create %{_target_platform}. See:
      http://fedoraproject.org/wiki/Packaging/cmake#Specfile_Usage
    + To preserve timestamps you could consider using:
      make install INSTALL="%{__install} -p" DESTDIR=%{buildroot}
    + Please add a comment in the Spec to document the rationale for shipping
      static libraries.
    + You could consider shipping the other files in the doc/ sub-directory as
      %doc. However shipping doc/PLANNING and doc/README in both the main
      package and the -devel sub-package is redundant.

OK - license meets Licensing Guidelines
OK - License field meets actual license
OK - upstream license file included in %doc
OK - spec file uses American English
OK - spec file is legible

?? - sources might not match upstream sources
    + As noted earlier, please document how the sources were obtained. Place a
      comment above the Source0 tag for this.

xx - package does not build successfully
    + sat-solver-no-bindings.patch does not apply cleanly and causes a build
      failure in Rawhide, which uses '/usr/bin/patch -s -p0 --fuzz=0'. See:
      http://koji.fedoraproject.org/koji/getfile?taskID=882707&name=build.log

?? - ExcludeArch not needed

?? - missing build dependencies
    + Can not verify because package fails to build.

OK - no locales

xx - %post and %postun should not invoke ldconfig
    + Since shared libraries are not being shipped, invocation of
      /sbin/ldconfig is not needed and should be removed.

OK - package is not relocatable
OK - missing dependency on package that creates directory
OK - no duplicates in %file
OK - file permissions set properly
OK - %clean present
OK - macros used consistently
OK - contains code and permissable content
OK - -doc is not needed
OK - contents of %doc does not affect the runtime
OK - header files in -devel
OK - static libraries in -static package
OK - no pkgconfig files
OK - no shared library files

OK - -devel does not require base package
    + Only static libraries are provided as part of the -devel or -static
      package. Base package consists of only executables.

OK - no libtool archives
OK - %{name}.desktop file not needed
OK - does not own files or directories owned by other packages
OK - buildroot correctly prepped
OK - all file names valid UTF-8

SHOULD Items:

OK - upstream provides license text
xx - no translations for description and summary

xx - package does not build in mock successfully
    + As noted above, package fails to build in Rawhide.

?? - package builds on all supported architectures

?? - package functions as expected
    + Other components of the Zypper stack are needed to verify functionality.

xx - scriptlets are not sane
    + As noted above, scriptlets are not needed and should be removed.

OK - subpackages other than -devel are not needed
    + -devel provides a -static package.

OK - no pkgconfig files
OK - no file dependencies

SPEC URL: http://fedorapeople.org/gitweb?p=arbiter/public_git/rpm.git;a=blob_plain;f=libs/sat-solver/sat-solver.spec;hb=master
SRPM URL: http://fedorapeople.org/~arbiter/srpm/sat-solver-0.9.5-1.fc10.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=884098

* Thu Oct 16 2008 Lorenzo Villani <lvillani> - 0.9.5-1
- Comments on source0 origin
- Removed no-bindings patch
- Added perl and python bindings
- rpm 4.5.90 patch
- Test suite not stripped and enabled (recompressed tarball
  using bzip2 to save space and bandwidth)
- Preserve timestamp when installing
- Rationale about static libs
- Removed unneeded scriptlets
- Add other documentation to the main package
- Remove redundant documentation files from other packages

+ RPMLint is not clean on SRPM:

[rishi@freebook SOURCES]$ rpmlint ../SRPMS/sat-solver-0.9.5-1.fc10.src.rpm 
sat-solver.src: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 17)
sat-solver.src: W: strange-permission sat-solver.spec 0775
sat-solver.src: W: strange-permission sat-solver-db-corruption-fix.patch 0775
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
[rishi@freebook SOURCES]$

RPMLint is not clean on build sub-packages:

[rishi@freebook SPECS]$ rpmlint sat-solver-perl
sat-solver-perl.x86_64: E: description-line-too-long The sat-solver-perl package contains libraries and perl modules to be used inside
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
[rishi@freebook SPECS]$ rpmlint sat-solver-python
sat-solver-python.x86_64: E: description-line-too-long The sat-solver-python package contains libraries and python modules to be used inside
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
[rishi@freebook SPECS]$

Pretty simple actually. Just replace the tab with equivalent number of spaces on line 17 of sat-solver.spec, and make sure that the files in the SRPM have a permission of 0644. Lines in the description should not exceed 80 characters.

+ Is this package not meant for Fedora 9? The sat-solver-0.9.5-rpm4.5.90.patch patch broke the build on my Fedora 9 system. In case it is only meant for Fedora 10 and above, you could add a versioned dependency on rpm-devel (BuildRequires) and rpm-libs (Requires) to enforce this.

According to the Naming Guidelines, package names of Python modules should be prefixed with "python-" or "py". See: https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28python_modules.29

In this case you can use:
%package -n     python-%{name}
...
%description -n python-%{name}
...
%files -n python-%{name}

Similarly for the Perl sub-package consult: https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28perl_modules.29

Ping?

SPEC URL: http://fedorapeople.org/gitweb?p=arbiter/public_git/rpm.git;a=blob;f=libs/sat-solver/sat-solver.spec;hb=HEAD
SRPM URL: http://fedorapeople.org/~arbiter/srpm/sat-solver-0.9.6-1.fc10.src.rpm

* Sun Dec 28 2008 Lorenzo Villani <lvillani> - 0.9.6-1
- 0.9.6
- sat-solver-perl -> perl-sat-solver
- sat-solver-python -> python-sat-solver
- fix weird .spec file permissions
- fix mixed use of spaces and tabs
- depend on rpm 4.6.0 (ie: build on Fedora 10 only)

(In reply to comment #39)

> SPEC URL:
> http://fedorapeople.org/gitweb?p=arbiter/public_git/rpm.git;a=blob;f=libs/sat-solver/sat-solver.spec;hb=HEAD
> SRPM URL: http://fedorapeople.org/~arbiter/srpm/sat-solver-0.9.6-1.fc10.src.rpm

These URLs are not working.

Closing the request until upstream makes the necessary changes to libzypp to support RPM 4.6.

SPEC: http://gitorious.org/lvillani/specs/blobs/master/zypp/sat-solver/sat-solver.spec
SRPM: http://fedorapeople.org/~arbiter/sat-solver/


Notes: 
- reopening since the whole ZYpp stack seems now usable with rpm >= 4.6.0
- rpmlint:
[lvillani@enterprise SRPMS]$ rpmlint ../RPMS/x86_64/sat-solver{,-devel,-doc,-debuginfo}-0.14.11-1.fc12.x86_64.rpm ../RPMS/x86_64/{perl,python}-sat-solver-0.14.11-1.fc12.x86_64.rpm ./sat-solver-0.14.11-1.fc12.src.rpm ../specs/zypp/sat-solver/sat-solver.spec 
7 packages and 1 specfiles checked; 0 errors, 0 warnings.

Please have in mind that sat-solver is highly optimized for solving package dependencies. It is also a _static_ library which is being linked into libzypp and used nowhere else (yet).

(In reply to comment #43)
> Please have in mind that sat-solver is highly optimized for solving package
> dependencies. 

Are you suggesting me to use a different package name...

> It is also a _static_ library which is being linked into libzypp
> and used nowhere else (yet).

... and to build sat-solver along with libzypp source package? (I don't know if I can do that).

It fails to build on Fedora 13 because of the ld change

---

/usr/bin/ld: ../ext/libsatsolverext.a(repo_rpmdb.o): undefined reference to symbol 'pgpPrtPkts'
/usr/bin/ld: note: 'pgpPrtPkts' is defined in DSO /usr/lib/librpmio.so.1 so try adding it to the linker command line
/usr/lib/librpmio.so.1: could not read symbols: Invalid operation
collect2: ld returned 1 exit status

---

Details on how to fix is at 

http://lists.fedoraproject.org/pipermail/devel-announce/2010-February/000563.html

SPEC:
http://gitorious.org/lvillani/specs/blobs/raw/master/sat-solver/sat-solver.spec
SRPM: http://fedorapeople.org/~arbiter/repo/fedora/13/SRPM/sat-solver-0.15.0-1.fc13.src.rpm

Test (mock) builds for F13 here: http://fedorapeople.org/~arbiter/repo/fedora/13/

Patch submitted upstream.

zypper and lib-zypper has a BR on sat-solver-static but I don't find it in your mock build.

(In reply to comment #47)
> zypper and lib-zypper has a BR on sat-solver-static but I don't find it in your
> mock build.    

It is a "virtual" Provide from sat-solver-devel (http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries)

In response to comment #27:
>sat-solver is the upstream name. I just packaged it with it's original name.

Upstream isn't always the best :)  In openSUSE itself, the package is called libsatsolver (not libsat-solver nor sat-solver nor satsolver). Its subpackages are also called perl-satsolver (not perl-sat-solver). In that regard, sat-solver with a dash seems to be a relic name of the repository.

(In reply to comment #49)
> 
> Upstream isn't always the best :)  In openSUSE itself, the package is called
> libsatsolver (not libsat-solver nor sat-solver nor satsolver). 

It's called libsatsolver because they usually prefix library packages with "lib", IIRC.

> [...]
> In that regard, sat-solver with a dash seems to be a relic name of the
> repository.

The most correct name is, probably, "satsolver". It's a generic (and probably misleading) name. Maybe I can rename this package to "satsolver" now and, should some conflict arise, rename it again later.

Suggestions welcome (-:

>It's called libsatsolver because they usually prefix library packages
>with "lib", IIRC.

So, by that very logic, it should be "satsolver-libs" in Fedora.

(Though the Fedora naming scheme always felt strange. What will be done if you need two versions. libfoo1 and libfoo2 work out, but foo-libs and foo-libs - you see the clash.)

> "satsolver". It's a generic

Feel free to suggest; discussion was already started on similar hot topic http://lists.opensuse.org/opensuse-buildservice/2010-07/msg00179.html

(In reply to comment #51)
> 
> So, by that very logic, it should be "satsolver-libs" in Fedora.
> 

AFAIK, the "-libs" suffix is used when an application also happens to ship libraries usable by 3rd parties. So for pure libraries only "name" and "name-devel" are necessary, IIRC.

> Feel free to suggest; discussion was already started on similar hot topic
> http://lists.opensuse.org/opensuse-buildservice/2010-07/msg00179.html

Following the "if it ain't broken, don't fix it" motto, I'd like to push this package as "satsolver" (not "sat-solver") and rename it only if needed.

SPEC URL: http://github.com/lvillani/RPMs/raw/master/satsolver/satsolver.spec
SRPM URL: http://arbiter.fedorapeople.org/reviews/satsolver/satsolver-0.15.0-2.fc13.src.rpm

Changed package's name.
Also, I added some comments above PatchN and SourceN lines.

What's the status of this bug? There is another review at bug #729199.

Lorenzo - are you still there?

*** Bug 729199 has been marked as a duplicate of this bug. ***

Jussi, I am no longer involved in Fedora for the time being. I relinquished ownership of all my packages about one year ago. I thought I closed or reassigned all of my bugs but, apparently, I was wrong.

OK, closing.