Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update (py)cryptominisat to version 5.11.21, remove no longer needed cryptominisat spkg #37669

Merged
merged 6 commits into from
Jun 9, 2024

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Mar 25, 2024

Fixes #34687
As pycryptosat is now a PyPI package, convert it to a pip package, and remove cryptominisat

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@dimpase
Copy link
Member Author

dimpase commented Mar 26, 2024

much more economic update - just convert pycryptosat into pip package, and drop cryptominisat as not needed as a standalone

Copy link

github-actions bot commented Mar 27, 2024

Documentation preview for this PR (built with commit 601631c; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Member

mkoeppe commented Mar 28, 2024

You converted it to a non-package.
Check that you committed all files.

@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2024

You converted it to a non-package. Check that you committed all files.

This removes cryptominisat, and only leaves pycryptominisat. We don't need cryptominisat by itself.

@dimpase dimpase changed the title update cryptominisat to version 5.11.21 update (py)cryptominisat to version 5.11.21, remove no longer needed cryptominisat Apr 17, 2024
@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2024

You converted it to a non-package. Check that you committed all files.

PS. As the reviewer is blocking me on GitHub, I don't get any corresponding notifications.
That's why I sat on this for so long.
If the reviewer wants to proceed in a timely manner, he better appoints a proxy to communicate here.
@saraedum

@dimpase dimpase changed the title update (py)cryptominisat to version 5.11.21, remove no longer needed cryptominisat update (py)cryptominisat to version 5.11.21, remove no longer needed cryptominisat spkg Apr 17, 2024
@dimpase
Copy link
Member Author

dimpase commented Apr 17, 2024

"failing" incremental CI are actually all good, only after the tests are run and all pass, some rather opaque Docker error pops up

[...]
--------------------
ERROR: failed to solve: process "/bin/sh -c make SAGE_SPKG=\"sage-spkg -y -o\" ${USE_MAKEFLAGS} ${TARGETS}" did not complete successfully: exit code: 2
DOCKER_BUILDKIT=1, so we cannot commit and tag the failed image
docker-debian-bullseye-standard-incremental: exit 1 (6748.90 seconds) /home/runner/work/sage/sage> bash -c 'for docker_target in with-targets; do BUILD_IMAGE_STEM=sage-$(echo docker-debian-bullseye-standard-incremental | sed "s/docker-//;s/-incremental//"); BUILD_IMAGE=$DOCKER_PUSH_REPOSITORY$BUILD_IMAGE_STEM-$docker_target; BUILD_TAG=96a1e14a13; TAG_ARGS=$(for tag in $BUILD_TAG 37669-merge; do echo --tag $BUILD_IMAGE:$tag; done); DOCKER_BUILDKIT=1; docker build . -f /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/Dockerfile --target $docker_target $TAG_ARGS --build-arg EXTRA_CONFIGURE_ARGS="--enable-experimental-packages --enable-download-from-upstream-url   --with-system-python3=yes  --enable-fat-binary" --build-arg BASE_IMAGE=ghcr.io/sagemath/sage/sage-$(echo docker-debian-bullseye-standard-incremental | sed -E "s/(docker-|-incremental|-sitepackages)//g")-with-targets:dev --build-arg BOOTSTRAP="./bootstrap" --build-arg TARGETS_PRE="$(if test -n "$TARGETS_PRE"; then echo $TARGETS_PRE; else echo reconfigure build doc-html ptest; fi)" --build-arg TARGETS="reconfigure build doc-html ptest" --build-arg TARGETS_OPTIONAL="build/make/Makefile" --build-arg NUMPROC=9 --build-arg USE_MAKEFLAGS="-k V=0 SAGE_NUM_THREADS=5"; status=$?; unset CONTAINER; if [ $status != 0 ]; then if [ $DOCKER_BUILDKIT = 0 ]; then BUILD_TAG="$BUILD_TAG-failed"; CONTAINER=$(docker ps -l -q); docker commit $CONTAINER $BUILD_IMAGE:$BUILD_TAG; else unset BUILD_TAG; echo "DOCKER_BUILDKIT=1, so we cannot commit and tag the failed image"; fi; fi; if [ -n "$BUILD_TAG" ]; then echo "Copying logs from the container to /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/sage/"; rm -f /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/sage/STATUS; docker run $BUILD_IMAGE:$BUILD_TAG bash -c " tar -c --ignore-failed-read -f - /sage/STATUS /sage/logs /sage/{prefix,venv}/var/lib/sage/installed ~/.sage/timings2.json /sage/pkgs/*/.tox/*/.sage/timings2.json /sage/pkgs/*/.tox/*/logs 2> /dev/null" | (cd /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental && tar xf -); if [ -f /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/sage/STATUS ]; then status=$(cat /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/sage/STATUS); fi; fi; unset PUSH_TAGS; if [ -n "$BUILD_TAG" ]; then if [ $status != 0 ]; then BUILD_TAG="${BUILD_TAG%-failed}-failed"; PUSH_TAGS=$BUILD_IMAGE:$BUILD_TAG; else PUSH_TAGS=$(echo $BUILD_IMAGE:$BUILD_TAG; for tag in 37669-merge; do echo "$BUILD_IMAGE:$tag"; done); fi; echo $BUILD_IMAGE:$BUILD_TAG >> /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/Dockertags; fi; if [ x"ghcr.io/sagemath/sage/" != x -a x"$PUSH_TAGS" != x ]; then echo Pushing $PUSH_TAGS; for tag in $PUSH_TAGS; do if docker push $tag; then echo $tag >> /home/runner/work/sage/sage/.tox/docker-debian-bullseye-standard-incremental/Dockertags.pushed; else echo "(ignoring errors)"; fi; done; fi; if [ $status != 0 ]; then exit $status; fi; done' pid=4212
  docker-debian-bullseye-standard-incremental: FAIL code 1 (6750.75=setup[0.87]+cmd[0.98,0.01,6748.90] seconds)
  evaluation failed :( (6750.87 seconds)
Error: Process completed with exit code 1.

@orlitzky
Copy link
Contributor

"failing" incremental CI are actually all good, only after the tests are run and all pass, some rather opaque Docker error pops up

this is #37786

@mkoeppe
Copy link
Member

mkoeppe commented Apr 17, 2024

You converted it to a non-package.
Check that you committed all files.

I'll explain it again: In this PR, the package pycryptosat, is a non-package because it has neither a checksums.ini (which would make it a normal or wheel package) nor a requirements.txt (which would make it a pip package) nor an spkg-install (which would make it a script package).

Actually try make pycryptosat with your branch. It gives you:

[pycryptosat-none] Error: pycryptosat is a dummy package and 
[pycryptosat-none] cannot be installed using the Sage distribution.

@dimpase
Copy link
Member Author

dimpase commented Apr 18, 2024

perhaps I forgot to check in requirements.txt

I'll fix it tomorrow

@mkoeppe
Copy link
Member

mkoeppe commented Apr 18, 2024

Thanks. Now pycryptosat has the needed requirements.txt, but it still has a version_requirements.txt (né install-requires.txt) file, which should be removed.

@@ -323,7 +323,7 @@ def SAT(solver=None, *args, **kwds):

- ``solver`` (string) -- select a solver. Admissible values are:

- ``"cryptominisat"`` -- note that the cryptominisat package must be
- ``"cryptominisat"`` -- note that the pycryptosat package must be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- ``"cryptominisat"`` -- note that the pycryptosat package must be
- ``"cryptominisat"`` -- note that the :ref:`pycryptosat <spkg_pycryptosat>` package must be

@saraedum
Copy link
Member

(dimpase, I took the liberty to fix a typo in the PR summary, hope you don't mind.)

@dimpase
Copy link
Member Author

dimpase commented Apr 19, 2024

that was a fallout of mass file renaming

No it wasn't.

I normally don't merge develop, I rebase over it, but this time the rebase was denied by GitHub, for a reason I didn't understand. That was the renamed file, I suppose.

And my foggy post covid brain overlooked it.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 20, 2024

No it wasn't. It was already broken when I reported it in #37669 (comment)

@dimpase
Copy link
Member Author

dimpase commented Apr 24, 2024

ping?

@mkoeppe
Copy link
Member

mkoeppe commented May 13, 2024

OK, let's merge it

@vbraun
Copy link
Member

vbraun commented Jun 3, 2024

merge conflict needs fixing

@mkoeppe mkoeppe added this to the sage-10.4 milestone Jun 6, 2024
@vbraun vbraun merged commit ae87c7f into sagemath:develop Jun 9, 2024
23 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade: cryptominisat 5.11
5 participants