search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 480 forks source link

Upgrade cryptominisat to 5.8.0, fix build on Cygwin #25374

Closed embray closed 2 years ago

embray commented 6 years ago

This is to upgrade to CryptoMiniSat 5.8.0, released 2020-07-06.

Tarball at: https://github.com/msoos/cryptominisat/archive/refs/tags/5.8.0.tar.gz

Release notes for the versions since 5.6.8 (current in Sage):

== 5.8.0 ==

Massive new release! Many-many improvements:

  • Gauss-Jordan elimination is enabled by default
  • Target Phases ("Stable Polarities") are used
  • CCAnr SLS solver is enabled and is set to work by default
  • Hybrid variable branching heuristics
  • Many-many speed improvements
  • Better DIMACS parsing to work for other systems
  • Made to work with new ApproxMC and UniGen

== 5.7.1 ==

  • Removing LSIDS, as it was interfering with performance.

== 5.7.0 ==

  • Improved parameters
  • Hybrid branching strategies
  • and much more

Previous upgrade:

Upstream: Fixed upstream, in a later stable release.

CC: @slel @tscrim @jm58660 @embray @jdemeyer @saraedum @sagetrac-tmonteil @videlec @orlitzky @novoselt @kiwifb @dimpase

Component: packages: optional

Keywords: upgrade, cryptominisat

Author: Erik Bray, Thierry Monteil, François Bissey, Matthias Koeppe

Branch: 69033db

Reviewer: Travis Scrimshaw, Julian Rüth, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/25374

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Changed dependencies from #25372 to none

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,6 @@
 This is to upgrade to CryptoMiniSat 5.8.0, released 2020-07-06.
+
+Tarball at: https://github.com/msoos/cryptominisat/archive/refs/tags/5.8.0.tar.gz

 Release notes for the versions since 5.6.8 (current in Sage):
edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Changed branch from public/25374-cryptominisat-5.8.0 to u/tmonteil/upgrade_cryptominisat_to_5_8_0

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

Changed author from Erik Bray to Erik Bray, Thierry Monteil

mkoeppe commented 3 years ago
comment:43

It would be good to address comment:39

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago
comment:44

Replying to @mkoeppe:

It would be good to address comment:39

I am not sure i understand the rationale in favor of such a move:

Or do you mean that we should make 2 spkg with different names (cryptominisat and pycryptosat) with the same upstream tarball, one with sdh_make and one with sdh_pip_install ?

mkoeppe commented 3 years ago
comment:45

Replying to @sagetrac-tmonteil:

Or do you mean that we should make 2 spkg with different names (cryptominisat and pycryptosat) with the same upstream tarball, one with sdh_make and one with sdh_pip_install ?

Yes, this part at least would be a useful step, as the package will then work with the new separation of SAGE_LOCAL and SAGE_VENV (see for example #32442)

mkoeppe commented 3 years ago
comment:46

Replying to @sagetrac-tmonteil:

  • relying on pypi + pierrev (the anonymous maintainer of pycryptosat in pypi) adds 2 intermediates, for which the web of trust is not very clear to me

Yes, this is an unfortunate situation, perhaps we should contact the maintainers of the packages to help clarify it...

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago
comment:47

Replying to @mkoeppe:

Replying to @sagetrac-tmonteil:

Or do you mean that we should make 2 spkg with different names (cryptominisat and pycryptosat) with the same upstream tarball, one with sdh_make and one with sdh_pip_install ?

Yes, this part at least would be a useful step, as the package will then work with the new separation of SAGE_LOCAL and SAGE_VENV (see for example #32442)

OK. It seems nontrivial since the python/README.rst in the tarball states:

The pycryptosat python package compiles while compiling CryptoMiniSat. It
cannotbe compiled on its own, it must be compiled at the same time as
CryptoMiniSat.

So, perhaps the way is to keep a single spkg, compile the whole, and then install the various stuff according to the respective variables SAGE_LOCAL and SAGE_VENV ?

mkoeppe commented 3 years ago
comment:48

Replying to @sagetrac-tmonteil:

It seems nontrivial since the python/README.rst in the tarball states:

The pycryptosat python package compiles while compiling CryptoMiniSat. It
cannotbe compiled on its own, it must be compiled at the same time as
CryptoMiniSat.

So, perhaps the way is to keep a single spkg, compile the whole, and then install the various stuff according to the respective variables SAGE_LOCAL and SAGE_VENV ?

That wouldn't work so well because of the way how we keep installation records for packages.

I think we should try to split it into two packages and just build, but not install, the library a second time.

slel commented 3 years ago
comment:49

This fails for me on Cygwin. For details, see:

slel commented 3 years ago
comment:51

It seems pycryptosat might land on PyPI soon, see

slel commented 3 years ago
comment:52

In fact it has been there for a while:

but probably we are waiting on a new release?

slel commented 2 years ago
comment:54

See also

mkoeppe commented 2 years ago

Work Issues: see #33162

mkoeppe commented 2 years ago

Dependencies: #33162

mkoeppe commented 2 years ago

Changed dependencies from #33162 to #33162, #33183

dimpase commented 2 years ago
comment:58

Replying to @mkoeppe:

Replying to @sagetrac-tmonteil:

  • relying on pypi + pierrev (the anonymous maintainer of pycryptosat in pypi) adds 2 intermediates, for which the web of trust is not very clear to me

Yes, this is an unfortunate situation, perhaps we should contact the maintainers of the packages to help clarify it...

PyPI now lists msoos as the maintainer.

dimpase commented 2 years ago

Changed commit from 824a597 to ac42f69

dimpase commented 2 years ago
comment:59

needs testing on cygwin in partiular

New commits:

cdd19b0correctly install pycryptosat
e3d8e91spin off installation of cryptominisat's Python
c17897cchecksums as link, and added install-requires
cfcf33aretag the tests: cryptominisat->pycryptosat
37809fd#32545 : update cryptominisat version to 5.8.0.
c48c0ddremove verbosity of boolean polynomial doctest with side effects
ce292c5bump pycryptosat accordingly
ac42f69make pycryptosat spkg version a link
dimpase commented 2 years ago

Changed branch from u/tmonteil/upgrade_cryptominisat_to_5_8_0 to u/dimpase/packages/cryptominisat-5.8.0

dimpase commented 2 years ago

Changed reviewer from Julian Rüth to Julian Rüth, https://github.com/sagemath/sagetrac-mirror/actions/runs/1704383865

dimpase commented 2 years ago
comment:60

running tests on https://github.com/sagemath/sagetrac-mirror/actions/runs/1704383865

mkoeppe commented 2 years ago
comment:61

Note that you need to run the workflows tox-optional and cygwin-standard to test it...

dimpase commented 2 years ago
comment:62

Replying to @dimpase:

running tests on https://github.com/sagemath/sagetrac-mirror/actions/runs/1704383865

many (all?) runs failed with 

  [sagemath_doc_html-none]   make[3]: *** [Makefile:37: doc-inventory-reference] Error 2
  [sagemath_doc_html-none]   make[3]: Target 'doc-html' not remade because of errors.

(I cancelled after all docker ubuntu/debian runs failed)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c0c9f5cadd $(PYTHON_TOOLCHAIN) dependency
dbe186a#32545 : update cryptominisat version to 5.8.0.
0a4ebf2remove verbosity of boolean polynomial doctest with side effects
5079437bump pycryptosat accordingly
33abc1emake pycryptosat spkg version a link
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from ac42f69 to 33abc1e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6d4bf11add $(PYTHON_TOOLCHAIN) dependency
b621359#32545 : update cryptominisat version to 5.8.0.
85ed799remove verbosity of boolean polynomial doctest with side effects
7258790bump pycryptosat accordingly
7d5d58fmake pycryptosat spkg version a link
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 33abc1e to 7d5d58f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ea82e77spin off installation of cryptominisat's Python
be0b019checksums as link, and added install-requires
f834119retag the tests: cryptominisat->pycryptosat
a7bc1adadd $(PYTHON_TOOLCHAIN) dependency
e65de87revert curl change that sneaked in
76463a8correct SPKG.txt syntax
f16d57a#32545 : update cryptominisat version to 5.8.0.
aabd481remove verbosity of boolean polynomial doctest with side effects
17f9c59bump pycryptosat accordingly
ca6d123make pycryptosat spkg version a link
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 7d5d58f to ca6d123

edd8e884-f507-429a-b577-5d554626c0fe commented 2 years ago
comment:68

Is there something missing to get this ticket approved ?

mkoeppe commented 2 years ago
comment:69

It needs to be tested on Cygwin

edd8e884-f507-429a-b577-5d554626c0fe commented 2 years ago
comment:70

Replying to @mkoeppe:

It needs to be tested on Cygwin

Isn't it unmaintained ?

mkoeppe commented 2 years ago
comment:71

Fine with me to just declare that this optional package is not supported on Cygwin. But the ticket title promises something about Cygwin, so at least that should be changed.

dimpase commented 2 years ago
comment:72

Replying to @mkoeppe:

It needs to be tested on Cygwin

Can it be tested with GH Actions?

tscrim commented 2 years ago
comment:73

I can try to setup a Cygwin build and test on Monday when I get to my office since my computer there has Windows 11.

tscrim commented 2 years ago

Attachment: cryptominisat-5.8.0.log

tscrim commented 2 years ago
comment:74

I was not able to build it following the installation instructions for Cygwin, building Sage from source, and running sage -i cryptominisat. I have attached the log. Let me know what other data you might need.

kiwifb commented 2 years ago
comment:75

What version of cmake was used?

tscrim commented 2 years ago
comment:76

Replying to @kiwifb:

What version of cmake was used?

tscri@DESKTOP-I0BK7J0 ~/sage
$ cmake --version
cmake version 3.20.0

CMake suite maintained and supported by Kitware (kitware.com/cmake).
kiwifb commented 2 years ago
comment:77

Can you attach those two as well

See also "/home/tscri/sage/local/var/tmp/sage/build/cryptominisat-5.8.0/src/CMakeFiles/CMakeOutput.log".
See also "/home/tscri/sage/local/var/tmp/sage/build/cryptominisat-5.8.0/src/CMakeFiles/CMakeError.log".

I suspect a variable is empty when it shouldn't. It may just be a matter of an extra option, but it could be a windows special.

This is where cmake says it fails

string(REPLACE "\n" " " PY_LINKFORSHARED_CONFIG ${PY_LINKFORSHARED_CONFIG})

If shared linking doesn't make sense on cygwin, it is not looking good.

tscrim commented 2 years ago

Attachment: CMakeError.log

tscrim commented 2 years ago

Attachment: CMakeOutput.log

tscrim commented 2 years ago
comment:78

Done.

kiwifb commented 2 years ago
comment:79

Thanks. I always forgot how those two files are usually useless. I don't think we can easily build the python interface on cygwin. Mostly because to build the python module it wants dynamic flags - on linux for example

-- Found python interpreter, libs and header files
-- Building python interface
-- Python CFLAGS:  '-Wno-unused-result -Wsign-compare -DNDEBUG'
-- Python LDFLAGS: '-ldl  -lm'
-- Python LINKFORSHARED flags: '-Xlinker -export-dynamic'
-- Python module installation prefix: --prefix=/usr

If we can hand feed an appropriate -DPY_LINKFORSHARED_CONFIG=... to cmake on cygwin, we may be able to get something. The other option is to turn off python support on the platform :(

Anyone wanting to test flags can do so by running

EXTRA_OPTS="-DPY_LINKFORSHARED_CONFIG='$my_flags'" ./sage -i cryptominisat

courtesy of the cmake configure line in spkg-install.in

sdh_cmake -DUSE_GAUSS='ON' $EXTRA_OPTS
tscrim commented 2 years ago
comment:80

Replying to @kiwifb:

Anyone wanting to test flags can do so by running

EXTRA_OPTS="-DPY_LINKFORSHARED_CONFIG='$my_flags'" ./sage -i cryptominisat

I ran this and it also did not work for me. Maybe the key bit in the log is this:

[cryptominisat-5.8.0] -- Python CFLAGS:  '-Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall'
[cryptominisat-5.8.0] -- Python LDFLAGS: '-lcrypt -lintl -ldl'
[cryptominisat-5.8.0] -- Python LINKFORSHARED flags: ''

I can post updated logs if desired, but I don't think much has changed...

kiwifb commented 2 years ago
comment:81

Well you were supposed to replace $my_flags by what you think should be there if you have a clue. And yes the key bit is

[cryptominisat-5.8.0] -- Python LINKFORSHARED flags: ''

the cmake script cannot handle an empty string here.

tscrim commented 2 years ago
comment:82

That is what I was thinking, but I have no idea what to actually put there as I am ignorant/dumb wrt most of the build stuff. I also tried with what was in your post, but it didn't like the -export-dynamic. Trying it with -Xlinker also results in

[cryptominisat-5.8.0] -- Python LINKFORSHARED flags: ''
kiwifb commented 2 years ago
comment:83

I thought you could override the autodetection by providing it but may be you cannot. I'd have to run it interactively with ccmake to see what is possible.