conda-forge (linux): primecount fails to install
mkoeppe opened this issue · 23 comments
Both with conda-forge-standard and conda-forge-minimal on Linux
https://github.com/sagemath/sage/runs/4497392130?check_suite_focus=true
In file included from /sage/local/var/tmp/sage/build/primecount-7.1/src/src/deleglise-rivat/S2_easy_libdivide.cpp:28:
/sage/local/var/tmp/sage/build/primecount-7.1/src/include/min.hpp: In instantiation of 'B {anonymous}::min(A, B) [with A = __int128 unsigned; B = long unsigned int]':
/sage/local/var/tmp/sage/build/primecount-7.1/src/src/deleglise-rivat/S2_easy_libdivide.cpp:107:29: required from 'T {anonymous}::S2_easy_128(T, uint64_t, uint64_t, uint64_t, uint64_t, const Primes&, const primecount::PiTable&) [with T = __int128 unsigned; Primes = std::vector<unsigned int, std::allocator<unsigned int> >; uint64_t = long unsigned int]'
/sage/local/var/tmp/sage/build/primecount-7.1/src/src/deleglise-rivat/S2_easy_libdivide.cpp:183:25: required from 'T {anonymous}::S2_easy_OpenMP(T, int64_t, int64_t, int64_t, const Primes&, int, bool) [with T = __int128 unsigned; Primes = std::vector<unsigned int, std::allocator<unsigned int> >; int64_t = long int]'
/sage/local/var/tmp/sage/build/primecount-7.1/src/src/deleglise-rivat/S2_easy_libdivide.cpp:244:75: required from here
/sage/local/var/tmp/sage/build/primecount-7.1/src/include/min.hpp:47:38: error: static assertion failed: min(A, B): Cannot compare types A and B
47 | static_assert(is_comparable<A, B>::value,
| ^~~~~
make[5]: *** [CMakeFiles/libprimecount.dir/build.make:625: CMakeFiles/libprimecount.dir/src/deleglise-rivat/S2_easy_libdivide.cpp.o] Error 1
CI for upstream primecount: kimwalisch/primecount#52
Upstream fix: kimwalisch/primecount#52 (comment)
Upstream: Fixed upstream, but not in a stable release.
CC: @dimpase @isuruf @orlitzky
Component: packages: standard
Author: Matthias Koeppe
Branch/Commit: a633c80
Reviewer: Michael Orlitzky
Issue created by migration from https://trac.sagemath.org/ticket/33054
An upgrade to primecount 7.2 does not fix the problem.
Description changed:
---
+++
@@ -12,3 +12,5 @@
make[5]: *** [CMakeFiles/libprimecount.dir/build.make:625: CMakeFiles/libprimecount.dir/src/deleglise-rivat/S2_easy_libdivide.cpp.o] Error 1
```
+CI for upstream primecount: https://github.com/kimwalisch/primecount/pull/52
+Description changed:
---
+++
@@ -1,3 +1,5 @@
+Both with `conda-forge-standard` and `conda-forge-minimal` on Linux
+
https://github.com/sagemath/sage/runs/4497392130?check_suite_focus=true
```With #32894, which adds the conda package information, at least the conda-forge-standard configuration can be built, so we can defer this issue to the Sage 9.6 series.
Upstream: Fixed upstream, but not in a stable release.
Description changed:
---
+++
@@ -16,3 +16,5 @@
CI for upstream primecount: https://github.com/kimwalisch/primecount/pull/52
+Upstream fix: https://github.com/kimwalisch/primecount/pull/52#issuecomment-1001395162
+wget https://github.com/kimwalisch/primecount/compare/v7.2...ab54277.patch
v7.2...ab54277.patch applies to primecount-7.2.tar.gz just fine
Author: Matthias Koeppe
It might be better to backport the upstream patch for this one release. Not every compiler that supports std=c++ will support std=gnu++.
We are using this in other packages too. All compilers on all supported platforms support this.
Note that the upstream fix just disables 128 bit support when "std=c++.." is in use. So this is not what we want.
Needs review.
Replying to @mkoeppe:
Note that the upstream fix just disables 128 bit support when "std=c++.." is in use. So this is not what we want.
Yes, but this branch already does,
sed "s/-std=c++/-std=gnu++/g"
and that could just as easily be
sed "s/-std=c++//g"
I was also thinking that this ticket was responsible for -DWITH_POPCNT=OFF, but it looks like that came from elsewhere. Nevertheless, there's an upstream fix for it that doesn't cause a performance degradation:
The CMakeLists.txt says "POPCNT instruction is very important for performance", so using the upstream patch would be an improvement over the unconditional OFF.
Replying to @orlitzky:
Replying to @mkoeppe:
Note that the upstream fix just disables 128 bit support when "std=c++.." is in use. So this is not what we want.
Yes, but this branch already does,
sed "s/-std=c++/-std=gnu++/g"and that could just as easily be
sed "s/-std=c++//g"
No, this doesn't work because the compiler without the -std flag may not be using a sufficiently new standard.
Replying to @orlitzky:
I was also thinking that this ticket was responsible for
-DWITH_POPCNT=OFF, but it looks like that came from elsewhere.
Yes, #33082.
Nevertheless, there's an upstream fix for it that doesn't cause a performance degradation:
The
CMakeLists.txtsays "POPCNT instruction is very important for performance", so using the upstream patch would be an improvement over the unconditional OFF.
#33082 comment:4 explains that our fix also does not lead to performance degradation.
Reviewer: Michael Orlitzky
Replying to @mkoeppe:
#33082 comment:4 explains that our fix also does not lead to performance degradation.
...when __GNUC__ is defined. Now that there's a proper upstream fix, that too is unnecessarily compiler-specific. Moreover if we used the upstream patches, we'd be forced to remove these hacks for the next version of primecount; as it is, they may linger for years.
But okay. This isn't a ticket worth arguing over all night.
Thanks!
Changed branch from u/mkoeppe/conda_forge__linux___primecount_fails_to_install to a633c80