conda-forge (linux): primecount fails to install

[mkoeppe]

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

[mkoeppe]

comment:1

An upgrade to primecount 7.2 does not fix the problem.

[mkoeppe]

comment:4

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.

[mkoeppe]

Upstream: Fixed upstream, but not in a stable release.

[sheerluck]

comment:6

wget https://github.com/kimwalisch/primecount/compare/v7.2...ab54277.patch 

v7.2...ab54277.patch applies to primecount-7.2.tar.gz just fine

[mkoeppe]

Dependencies: #33082

[mkoeppe]

Branch: u/mkoeppe/conda_forge__linux___primecount_fails_to_install

[mkoeppe]

Author: Matthias Koeppe

[mkoeppe]

comment:9

I have instead modified our build script

---

New commits:

6f1679d	build/pkgs/primecount/spkg-install.in: Use WITH_POPCNT=OFF unconditionally
71e3587	build/pkgs/primecount/spkg-install.in: Expand comment
afb575d	Merge #33082
a633c80	build/pkgs/primecount/spkg-install.in: Use std=gnu++...

[mkoeppe]

Commit: a633c80

[orlitzky]

comment:10

It might be better to backport the upstream patch for this one release. Not every compiler that supports std=c++ will support std=gnu++.

[mkoeppe]

comment:11

We are using this in other packages too. All compilers on all supported platforms support this.

[mkoeppe]

comment:12

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.

[mkoeppe]

Changed dependencies from #33082 to none

[orlitzky]

comment:14

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:

kimwalisch/primecount@b5a8286

The CMakeLists.txt says "POPCNT instruction is very important for performance", so using the upstream patch would be an improvement over the unconditional OFF.

[mkoeppe]

comment:15

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.

[mkoeppe]

comment:16

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:

kimwalisch/primecount@b5a8286

The CMakeLists.txt says "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.

[orlitzky]

Reviewer: Michael Orlitzky

[orlitzky]

comment:17

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.

[mkoeppe]

comment:18

Thanks!

[vbraun]

Changed branch from u/mkoeppe/conda_forge__linux___primecount_fails_to_install to a633c80