Opened 4 years ago

Closed 5 months ago

#25009 closed enhancement (fixed)

update and promote spkg primecount

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-9.5
Component: basic arithmetic Keywords:
Cc: mkoeppe, mmarco, isuruf Merged in:
Authors: Dima Pasechnik Reviewers: Matthias Koeppe
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: b9b4eaf (Commits, GitHub, GitLab) Commit: b9b4eaf334c6dd5261ccd229ca825b01b600e6a6
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

The prime_pi function is currently a symbolic function that can hold its argument as in

sage: n = SR.var('n')
sage: prime_pi(n)
prime_pi(n)

that also contains the Cython code for evaluating it

sage: prime_pi(13543)
1603

Unfortunately, it is buggy, see #24960 (fixed in #32894, using this ticket). We will promote the spkg primecount to standard and use it instead. This ticket does this part, and reverts deprecations from #32412. It also removes obsolete deprecation warning, and the corresponding no longer available parameter.

Upstream patch: https://github.com/kimwalisch/primesieve/pull/107 (rejected)


Below is the original part, no longer too relevant.


We move the evaluation part as a standalone function in arith/ and provide alternative implementations

  • using PARI/GP (only efficient in the range of tabulated primes)
  • using the different algorithms in the library primecount that is packaged in #24966

Change History (90)

comment:1 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:2 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 4 years ago by vdelecroix

  • Branch set to u/vdelecroix/25009-draft
  • Commit set to 3e2d53dcbf308b397af6b2aa3a3236821ee40244

New commits:

1297abapackage primecount
3e2d53ddraft for 25009

comment:4 Changed 11 months ago by mkoeppe

The package primecount has been provided with Sage since version 8.3, has been updated several times and is already outdated https://repology.org/project/primecount/versions

Is it time to start using it?

Because sage.libs.primecount uses an optional library, in the modularization of sagelib it would have to be split out to a separate package. But since src/sage/libs/primecount.pxd and src/sage/interfaces/primecount.pyx do not depend on Sage at all (except for a use of sage.misc.superseded), it would be best to make it a standalone Python library, like you did with pplpy.

comment:5 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-8.2 to sage-9.4
  • Summary changed from move prime_pi as a standalone function to move prime_pi as a standalone function, use library primecount

comment:6 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:7 Changed 6 months ago by dimpase

prime_pi is slow, quite buggy (see #24960), and should be ditched in favour of calling primecount.

comment:8 Changed 6 months ago by dimpase

  • Branch changed from u/vdelecroix/25009-draft to u/dimpase/packages/primecount/update-to-standard
  • Commit changed from 3e2d53dcbf308b397af6b2aa3a3236821ee40244 to 1e24a4a159f79bd3c3e18b819951caa7282eb925
  • Dependencies #24966 deleted

bump to 7.1, and standard, remove deprecations


New commits:

1e24a4abump primecount to ver. 7.1, and standard

comment:9 Changed 6 months ago by dimpase

  • Cc mkoeppe added

comment:10 Changed 6 months ago by dimpase

this is a start for fixing #24960

comment:11 Changed 6 months ago by dimpase

  • Authors set to Dima Pasechnik
  • Cc mmarco added
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from move prime_pi as a standalone function, use library primecount to update and promote spkg primecount

I am not sure whether there is much value in the branch u/vdelecroix/25009-draft, as functions mentioned there are no longer in primecount.hpp, they are in primecount-internal.hpp, cf. https://github.com/kimwalisch/primecount

That is, they are not for public consumption. Let us use this ticket to update and promote the package to standard.

comment:12 Changed 6 months ago by git

  • Commit changed from 1e24a4a159f79bd3c3e18b819951caa7282eb925 to a660ee48e378dae2a21212c165709ae7d4d88304

Branch pushed to git repo; I updated commit sha1. New commits:

a660ee4removed deprecations

comment:13 follow-up: Changed 6 months ago by mkoeppe

Changes from optional to standard require a vote on sage-devel

comment:14 Changed 6 months ago by dimpase

It fixes a serious bug, I can't imagine any objections. Feel free to call the vote, though.

comment:15 Changed 6 months ago by mkoeppe

The new version also needs portability testing

comment:16 Changed 6 months ago by dimpase

for what it's worth, it works on a 32-bit Ubuntu 18.04

comment:17 Changed 6 months ago by git

  • Commit changed from a660ee48e378dae2a21212c165709ae7d4d88304 to e094a4a1221676e3b5f87ece361a38a4276b53d9

Branch pushed to git repo; I updated commit sha1. New commits:

e094a4aadd primecount to various config files

comment:18 follow-up: Changed 6 months ago by dimpase

do we want to handle conversion to an external package here, or on a follow-up?

comment:19 in reply to: ↑ 18 Changed 6 months ago by dimpase

Replying to dimpase:

do we want to handle conversion to an external package here, or on a follow-up?

I think it's not totally trivial, as this would need setup.py code for calling cmake to build the C++ library (yuck).

comment:20 follow-up: Changed 6 months ago by mkoeppe

No, the new Python package would not need to build the primecount library. Just like pplpy - which also does not build the ppl library.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 6 months ago by dimpase

Replying to mkoeppe:

No, the new Python package would not need to build the primecount library. Just like pplpy - which also does not build the ppl library.

reading https://gitlab.com/videlec/pplpy/-/blob/master/setup.py does not make me wiser, and anyway it's not a good example, as we're building it as a part of sagelib, anyway.

As primecount isn't always available as a system library, there should be some need for configuring paths, etc.

That all that can be done in 10 minutes is, hmm, a joke.

comment:22 in reply to: ↑ 21 Changed 6 months ago by mkoeppe

Replying to dimpase:

As primecount isn't always available as a system library, there should be some need for configuring paths

No, just assume that it's available and fail otherwise.

comment:23 Changed 6 months ago by dimpase

I've spent 180 minutes fighting idiotic Cython errors, in an attempt to make a stand-alone package. Something like

primecount/primecount.pyx:66:6: Function signature does not match previous declaration
warning: primecount/primecount.pyx:86:6: Function 'phi' previously declared as 'cpdef'

It's above my paygrade to create Cython setups, sorry. Put it on another ticket. The Cython docs are a joke.

comment:24 Changed 6 months ago by dimpase

You may try https://github.com/dimpase/primecount at ./sage --buildsh prompt - that's as far as I got

$ python3 setup.py build_ext --inplace
running build_ext
Compiling primecount/primecount.pyx because it changed.
[1/1] Cythonizing primecount/primecount.pyx
warning: primecount/primecount.pyx:66:6: Function 'nth_prime' previously declared as 'cpdef'

Error compiling Cython file:
------------------------------------------------------------
...
    sig_on()
    ans = primecount.pi(s)
    sig_off()
    return PyInt_FromString(ans, NULL, 10)

cpdef int64_t nth_prime(int64_t n) except -1:
     ^
------------------------------------------------------------

primecount/primecount.pyx:66:6: Function signature does not match previous declaration
warning: primecount/primecount.pyx:86:6: Function 'phi' previously declared as 'cpdef'

Error compiling Cython file:
------------------------------------------------------------
...
    if _do_sig(n): sig_on()
    ans = primecount.nth_prime(n)
    if _do_sig(n): sig_off()
    return ans

cpdef int64_t phi(int64_t x, int64_t a):
     ^
------------------------------------------------------------

primecount/primecount.pyx:86:6: Function signature does not match previous declaration
Traceback (most recent call last):
  File "/home/dimpase/work/primecount/setup.py", line 28, in <module>
    setup(
  File "/home/dimpase/work/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/setuptools/__init__.py", line 153, in setup
    return distutils.core.setup(**attrs)
  File "/home/dimpase/work/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/setuptools/_distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/home/dimpase/work/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/setuptools/_distutils/dist.py", line 967, in run_commands
    self.run_command(cmd)
  File "/home/dimpase/work/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/setuptools/_distutils/dist.py", line 986, in run_command
    cmd_obj.run()
  File "/home/dimpase/work/primecount/setup.py", line 9, in run
    self.distribution.ext_modules[:] = cythonize(
  File "/home/dimpase/work/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/Cython/Build/Dependencies.py", line 1102, in cythonize
    cythonize_one(*args)
  File "/home/dimpase/work/sage/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/Cython/Build/Dependencies.py", line 1225, in cythonize_one
    raise CompileError(None, pyx_file)
Cython.Compiler.Errors.CompileError: primecount/primecount.pyx
(sage-buildsh) dimpase@penguin:primecount$ 

comment:25 Changed 6 months ago by dimpase

I've made it build, but the module doesn't have importable functions, for some reason.

comment:26 Changed 6 months ago by dimpase

The followup in on #32894 - and needs Cython experts attention. It's far from trivial.

Let's meanwhile get this ticket in.

Last edited 6 months ago by dimpase (previous) (diff)

comment:27 Changed 6 months ago by dimpase

  • Status changed from needs_review to needs_work
  • Work issues set to spin out primesieve

OK, we actaully should spin off the library primesieve (https://repology.org/project/primesieve/versions) - which is a dependency here, and (by default primecount vendors it, but has an option not to).

Many distros package primesieve separately, so in particular for using system packages it makes perfect sense. (Perhaps primesieve has potentially independent uses in Sage).

comment:28 Changed 6 months ago by git

  • Commit changed from e094a4a1221676e3b5f87ece361a38a4276b53d9 to 784f89d9de3f0cc2f823b9895842dea35ee7c187

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

c99b691bump primecount to ver. 7.1, and standard
09c4647removed deprecations
e9cbcd1add primecount to various config files
784f89dsplit primesieve off primecount

comment:29 Changed 6 months ago by git

  • Commit changed from 784f89d9de3f0cc2f823b9895842dea35ee7c187 to 29447182575cb8d7adaa5d4a6f7858f98131c83e

Branch pushed to git repo; I updated commit sha1. New commits:

31c2c2eadd spkg-configs for primecount/sieve
2944718check version of primesieve in the config of primecount

comment:30 Changed 6 months ago by git

  • Commit changed from 29447182575cb8d7adaa5d4a6f7858f98131c83e to 5b30d1d7a12d82155d7c524fe7403affff36604f

Branch pushed to git repo; I updated commit sha1. New commits:

5b30d1ddistros info for primecount/sieve

comment:31 Changed 6 months ago by git

  • Commit changed from 5b30d1d7a12d82155d7c524fe7403affff36604f to e8dbc402fe944247f79ba32faca93f561f336d21

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

e8dbc40distros info for primecount/sieve, correct m4s

comment:32 in reply to: ↑ 13 Changed 6 months ago by dimpase

Replying to mkoeppe:

Changes from optional to standard require a vote on sage-devel

see https://groups.google.com/g/sage-devel/c/tl_fxCJEh1Y/m/y1R2O7RVBgAJ

comment:33 Changed 6 months ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

the patch build/pkgs/primecount/patches/sieve_version.patch is proposed to upstream, see https://github.com/kimwalisch/primecount/pull/47

comment:34 follow-up: Changed 6 months ago by dimpase

  • Status changed from needs_work to needs_review

Matthias, what is

# sage_setup: distribution = sagemath-primecount

in the first line of src/sage/interfaces/primecount.pyx, and why I have to remove it in order for this branch to work? (I'm testing on Fedora 34, where primecount/sieve come from the system, if it matters)

Last edited 6 months ago by dimpase (previous) (diff)

comment:35 Changed 6 months ago by dimpase

If it's there then the file in question is ignored by make build. Is it something for optional packages, and so should be removed?

comment:36 Changed 6 months ago by git

  • Commit changed from e8dbc402fe944247f79ba32faca93f561f336d21 to 9ca1f677371aa9834b2094fe56e73b8f4c7f93f2

Branch pushed to git repo; I updated commit sha1. New commits:

c655136remove sagemath-primecount declaration
9ca1f67misc config changes for primesieve

comment:37 Changed 6 months ago by mkoeppe

The changes to setup.cfg.m4 and pyproject.toml.m4 make no sense. These files are for declaring dependencies on Python packages

In the dependencies files, cmake should be an order-only dependency

comment:38 Changed 6 months ago by mkoeppe

In the dependencies of sagelib, it's probably not necessary to repeat primesieve

comment:39 follow-up: Changed 6 months ago by mkoeppe

sdh_cmake already sets CMAKE_INSTALL_PREFIX, no need to repeat it

comment:40 Changed 6 months ago by mkoeppe

  • Work issues spin out primesieve deleted

comment:41 in reply to: ↑ 34 Changed 6 months ago by mkoeppe

Replying to dimpase:

Matthias, what is

# sage_setup: distribution = sagemath-primecount

in the first line of src/sage/interfaces/primecount.pyx, and why I have to remove it in order for this branch to work?

The sagelib build system removes this file when primecount is not present. See https://github.com/sagemath/sage/blob/develop/pkgs/sagemath-standard/setup.py#L76

comment:42 Changed 6 months ago by mkoeppe

And this mechanism does not know anything about system packages.

As this ticket is making primecount a standard package, removing the sage_setup: distribution annotation is OK.

comment:43 in reply to: ↑ 39 Changed 6 months ago by dimpase

Replying to mkoeppe:

sdh_cmake already sets CMAKE_INSTALL_PREFIX, no need to repeat it

$ git grep CMAKE_INSTALL_PREFIX 
build/bin/sage-dist-helpers:#    GNUInstallDirs module) so that `CMAKE_INSTALL_PREFIX` and
build/bin/sage-dist-helpers:    cmake . -DCMAKE_INSTALL_PREFIX="${SAGE_INST_LOCAL}" \
build/pkgs/primecount/spkg-install.in:    -DCMAKE_INSTALL_PREFIX=$SAGE_LOCAL \
build/pkgs/primesieve/spkg-install.in:    -DCMAKE_INSTALL_PREFIX=$SAGE_LOCAL \
build/pkgs/scipoptsuite/patches/0003-use-INSTALL_NAME_DIR-fix-for-libscip.patch:+    INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}/lib"
build/pkgs/scipoptsuite/spkg-install.in:cmake .. -DCMAKE_INSTALL_PREFIX="${SAGE_LOCAL}"/ -DCMAKE_VERBOSE_MAKEFILE=ON -DGMP_DIR="${SAGE_LOCAL}" -DZLIB_ROOT="${SAGE_LOCAL}" -DReadline_ROOT_DIR="${SAGE_LOCAL}" -DBLISS_DIR="${SAGE_LOCAL}" -DIPOPT=FALSE
build/pkgs/symengine/spkg-install.in:cmake -DCMAKE_INSTALL_PREFIX="$SAGE_LOCAL" \
src/doc/en/developer/packaging.rst:   that ``CMAKE_INSTALL_PREFIX`` and ``CMAKE_INSTALL_LIBDIR`` are set

so few other packages do this too - symengine, scipoptsuite

comment:44 Changed 6 months ago by mkoeppe

That can be cleaned up on this ticket as well, I think

comment:45 Changed 6 months ago by git

  • Commit changed from 9ca1f677371aa9834b2094fe56e73b8f4c7f93f2 to d8e2cda4f1efb33cfc028e24e6f6869226408ba5

Branch pushed to git repo; I updated commit sha1. New commits:

d8e2cdaadjust deps,remove stuff from cfg and toml

comment:46 Changed 6 months ago by git

  • Commit changed from d8e2cda4f1efb33cfc028e24e6f6869226408ba5 to abce309cd4ae20219c220ebd00a0c954146915fe

Branch pushed to git repo; I updated commit sha1. New commits:

3f1e441no need for CMAKE_INSTALL_PREFIX (it's in sdh_cmake)
abce309flyby patch: remove CMAKE_INSTALL_PREFIX, as it's set in sdh_cmake

comment:47 Changed 6 months ago by dimpase

testing revealed a packaging bug in Debian - they don't install cmake configs for primesieve, and so primecount (to be built) can't find it. :-( Has reported this...

We can instead cook up something, perhaps patch CMakeList of primecount more, make it rely on pkg-config instead? I don't know (recognising that this is broken without actually running cmake is no fun...)

comment:48 Changed 6 months ago by mkoeppe

Perhaps unvendoring primesieve is not strictly necessary for this ticket?

comment:49 Changed 6 months ago by mkoeppe

... it does not seem to be a very important use case to cater to users on systems that have primesieve but not primecount...

comment:50 Changed 6 months ago by dimpase

well, I don't mind banning broken system primesieves, but I don't know a sane way to test this. E.g. cmake --find_package should do the job, but e.g. on Fedora

$ cmake --find-package -DNAME=primesieve -DCOMPILER_ID=GNU -DLANGUAGE=C -DMODE=link
CMake Error at /usr/share/cmake/Modules/FindThreads.cmake:66 (message):
  FindThreads only works if either C or CXX language is enabled
Call Stack (most recent call first):
  /usr/share/cmake/Modules/CMakeFindDependencyMacro.cmake:47 (find_package)
  /usr/lib64/cmake/primesieve/primesieveConfig.cmake:18 (find_dependency)
  /usr/share/cmake/Modules/CMakeFindPackageMode.cmake:183 (find_package)


primesieve not found.
CMake Error: Run 'cmake --help' for all supported options.
clpc171[/home/scratch2/dimpase/sage/sage-clang]$ cmake --find-package -DNAME=primesieve -DCOMPILER_ID=GNU -DLANGUAGE=C -DMODE=LINK
CMake Error at /usr/share/cmake/Modules/FindThreads.cmake:66 (message):
  FindThreads only works if either C or CXX language is enabled
Call Stack (most recent call first):
  /usr/share/cmake/Modules/CMakeFindDependencyMacro.cmake:47 (find_package)
  /usr/lib64/cmake/primesieve/primesieveConfig.cmake:18 (find_dependency)
  /usr/share/cmake/Modules/CMakeFindPackageMode.cmake:183 (find_package)


primesieve not found.
CMake Error: Run 'cmake --help' for all supported options.

comment:51 follow-up: Changed 6 months ago by dimpase

How about, in case of failure of cmake in spkg-install of primecount, just trying running cmake without -DBUILD_LIBPRIMESIEVE=OFF - then primesieve will be built ?

comment:52 Changed 6 months ago by dimpase

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

comment:53 Changed 6 months ago by dimpase

Debian fixed it in the latest unstable.

comment:54 Changed 6 months ago by dimpase

We actually have a catch-22 situation - we cannot reliably configure standard packages with cmake, as we don't guarantee its presence at the moment.

So cmake must be promoted to a hard pre-requisite.

I don't mind this, though...

comment:55 Changed 6 months ago by mkoeppe

When there's no system cmake, it makes no sense to look for system libraries' cmake information.

comment:56 Changed 6 months ago by dimpase

but you can't configure/build a standard package which relies on cmake if there is no cmake on the system.

While autotools can generate a configure script to be shipped, cmake only generated makefiles, not good enough for shipping.

comment:57 Changed 6 months ago by dimpase

we need to treat cmake spkg as we treat patch spkg.

I'd rather just get rid of patch as well, and make it a pre-req, too, though.

comment:58 follow-up: Changed 6 months ago by mkoeppe

Why do you think that we need cmake earlier than at the time of building packages?

comment:59 in reply to: ↑ 58 ; follow-up: Changed 6 months ago by dimpase

Replying to mkoeppe:

Why do you think that we need cmake earlier than at the time of building packages?

ah, right, I was actually using cmake in an spkg-configure.m4, but then decided it's too much trouble. But it's stuck in my mind that something fishy. Sorry for noise.

comment:60 in reply to: ↑ 59 Changed 6 months ago by mkoeppe

Replying to dimpase:

using cmake in an spkg-configure.m4

This was what I was referring to in comment:55. If there's no system cmake but finding a library has to depend on that library's installed cmake information, then the spkg-configure.m4 should just fail.

comment:61 in reply to: ↑ 51 Changed 6 months ago by dimpase

Replying to dimpase:

How about, in case of failure of cmake in spkg-install of primecount, just trying running cmake without -DBUILD_LIBPRIMESIEVE=OFF - then primesieve will be built ?

OK, here I need a shell programming advice. The problem I don't know how to solve is that cmake triggers an exit if it fails, so it teminates spkg-install script in case of failure. But I just want to know the result, and try something different if it fails.

I hoped something like this would work:

primc_config() {
echo "Configuring primecount: building primesieve $1"
sdh_cmake -DCMAKE_VERBOSE_MAKEFILE=ON \
          -DBUILD_STATIC_LIBS=OFF \
          -DBUILD_SHARED_LIBS=ON \
          -DBUILD_TESTS=ON \
          -DBUILD_LIBPRIMESIEVE=$1 \
          -DCMAKE_FIND_ROOT_PATH=$SAGE_LOCAL/lib/cmake \
          -DCMAKE_FIND_ROOT_PATH_MODE_LIBRARY=BOTH \
          -DCMAKE_INSTALL_PREFIX=$SAGE_LOCAL \
          ${EXTRA_OPTS} && echo OKOKOK && sdh_make_install
}

set -e
primc_config OFF || primc_config ON

but no, if the 1st call to primc_config fails in cmake, it just terminates the script.

Last edited 6 months ago by dimpase (previous) (diff)

comment:62 follow-up: Changed 6 months ago by mkoeppe

use a sub-shell: (exit 1) || echo survived

comment:63 Changed 6 months ago by git

  • Commit changed from abce309cd4ae20219c220ebd00a0c954146915fe to a6de7f427004aac442e177d58ad47a2ef2b637e8

Branch pushed to git repo; I updated commit sha1. New commits:

a6de7f4check external primesieve with cmake

comment:64 in reply to: ↑ 62 Changed 6 months ago by dimpase

Replying to mkoeppe:

use a sub-shell: (exit 1) || echo survived

OK, great, thanks. This helps.

Now ready for review.

comment:65 Changed 6 months ago by dimpase

  • Cc isuruf added

Time to ping more upstreams to package primecount and primesieve. Debian people are already busy with it.

comment:66 follow-up: Changed 6 months ago by mkoeppe

Needs portability testing

comment:67 in reply to: ↑ 66 Changed 6 months ago by dimpase

  • Reviewers set to https://github.com/sagemath/sagetrac-mirror/actions/runs/1500740315

Replying to mkoeppe:

Needs portability testing

running here: https://github.com/sagemath/sagetrac-mirror/actions/runs/1500740315

comment:68 Changed 6 months ago by dimpase

some tests are failing due to #32828

comment:69 Changed 6 months ago by mkoeppe

  • Status changed from needs_review to needs_work

On fedora-30-standard (https://github.com/sagemath/sagetrac-mirror/runs/4316021585?check_suite_focus=true): Error: Unable to find a match: primecount primecount-devel.

This means that you'll have to set IGNORE_MISSING_SYSTEM_PACKAGES=yes for fedora-30 in tox.ini.

comment:70 Changed 6 months ago by dimpase

well, accoding to https://fedoraproject.org/wiki/End_of_life already 31 and 32 are past EOL. How many past-EOL releases should we go? OK, one or two, but three...

comment:71 Changed 6 months ago by mkoeppe

We are supporting this version, and it is not within the scope of this ticket to break it.

comment:72 Changed 6 months ago by git

  • Commit changed from a6de7f427004aac442e177d58ad47a2ef2b637e8 to f754d069812b65253cda1c9d29da092b02105bf4

Branch pushed to git repo; I updated commit sha1. New commits:

f754d06fedora 30 doesn't have primecount pkg

comment:73 Changed 6 months ago by dimpase

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:74 Changed 6 months ago by mkoeppe

primesieve does not build on some of our still supported platforms. I have a patch (https://github.com/kimwalisch/primesieve/pull/107) for fixing most of them, but upstream does not want it. With the patch, there is one remaining failing platform, debian-jessie (https://github.com/kimwalisch/primesieve/issues/108), upstream indicates wontfix.

comment:75 follow-up: Changed 6 months ago by dimpase

time to drop gcc 4-based systems...

comment:76 Changed 6 months ago by dimpase

Please feel free to add patches.

comment:77 Changed 6 months ago by mkoeppe

  • Branch changed from u/dimpase/packages/primecount/update-to-standard to u/mkoeppe/packages/primecount/update-to-standard

comment:78 in reply to: ↑ 75 Changed 6 months ago by mkoeppe

  • Commit changed from f754d069812b65253cda1c9d29da092b02105bf4 to b9b4eaf334c6dd5261ccd229ca825b01b600e6a6

Replying to dimpase:

time to drop gcc 4-based systems...

Yes, soon, I'd say. It would just feel a bit excessive to do so just for a single function.

I'll see if the failure on debian-jessie is as easy to fix as the other failures were. If not, we can drop debian-jessie in 9.5 already, it seems to have a particularly messy compiler.


New commits:

b9b4eafbuild/pkgs/primesieve: Add patch to support ubuntu-trusty, linuxmint-17
Last edited 6 months ago by mkoeppe (previous) (diff)

comment:79 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:80 Changed 6 months ago by mkoeppe

  • Description modified (diff)

comment:81 Changed 6 months ago by dimpase

IMHO the patch for std:align should go into gcc spkg, not here. Test if it's there, if not, put it into std namespace, instead of patching around all the C++ code affected by it.

While strictly speaking putting new functions into namespace std is undefined behaviour, with gcc is just works (IMHO).

comment:82 Changed 6 months ago by mkoeppe

primesieve is the only the package that needs it.

And patching the system compiler sounds like a terrible idea.

comment:83 Changed 6 months ago by dimpase

the compiler is OK in this case, it's STL that it's not.

If I recall correctly, that's how we patched STLs for CGAL back in 1999:

All you need to to is to put -I foo/bar/ into CXXFLAGS, and put into foo/bar a file named memory, where one puts #include <memory> and an implementation of align() inside namespace std {...} block.

comment:84 Changed 6 months ago by mkoeppe

Sure, that's another way to do this, but I don't think it's better than what's already done and tested

comment:85 Changed 6 months ago by dimpase

OK, I've just rebased the branch of #32894 over the branch here, and will do a run on GH Actions there. It will test this ticket along the way, too.

comment:87 Changed 6 months ago by mkoeppe

  • Reviewers changed from https://github.com/sagemath/sagetrac-mirror/actions/runs/1500740315 to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:88 follow-up: Changed 6 months ago by isuruf

Can you add primecount to distros/conda.txt?

comment:89 in reply to: ↑ 88 Changed 6 months ago by dimpase

Replying to isuruf:

Can you add primecount to distros/conda.txt?

done on #32894 (to minimize rebasing :))

comment:90 Changed 5 months ago by vbraun

  • Branch changed from u/mkoeppe/packages/primecount/update-to-standard to b9b4eaf334c6dd5261ccd229ca825b01b600e6a6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.