Opened 13 months ago

Closed 11 months ago

Last modified 10 months ago

#31694 closed enhancement (fixed)

Upgrade pynac to support gcc-11

Reported by: mkoeppe Owned by:
Priority: blocker Milestone: sage-9.4
Component: packages: standard Keywords:
Cc: dimpase, gh-DaveWitteMorris, fbissey, arojas Merged in:
Authors: Dima Pasechnik, Matthias Koeppe Reviewers: Matthias Koeppe, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: c9c50e3 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

(from #31786)

pynac fails to compile with gcc-11 (headers porting) https://github.com/pynac/pynac/pull/375

... and ideally to consolidate the various patches added in #31679, #31645 etc.... as also requested in https://groups.google.com/g/sage-packaging/c/_vxPV7aKNjQ/m/b2-EdWP1BgAJ

When making a new release, also care should be taken to use a recent version of libtool that supports macOS Big Sur (see #31920).

Change History (71)

comment:1 Changed 13 months ago by gh-DaveWitteMorris

There are still several pynac bug tickets that may not be hard to fix, so I think I will be posting quite a few more pynac patches in the next few months, so let's wait on this. But it would be good to make an upgrade before the 9.4 release.

comment:2 Changed 13 months ago by mkoeppe

Aren't you preparing your patches using git? I'd recommend to create pull requests to the pynac repo to make it more transparent.

comment:3 Changed 12 months ago by mkoeppe

  • Cc fbissey added
  • Description modified (diff)
  • Priority changed from major to blocker
  • Summary changed from Upgrade pynac to Upgrade pynac to support gcc-11

comment:4 Changed 12 months ago by mkoeppe

  • Description modified (diff)

comment:5 Changed 12 months ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 11 months ago by mkoeppe

  • Cc arojas added
  • Description modified (diff)

comment:7 Changed 11 months ago by dimpase

I am going to cut a new release of pynac with all da patchez

comment:8 Changed 11 months ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/pynac/0.7.28
  • Commit set to e97c1b54c1a9fc8438a33803fa7b45b2a42643b0
  • Status changed from new to needs_review

New commits:

e97c1b5update pynac to 0.7.28

comment:9 Changed 11 months ago by fbissey

Thank you Dima! :)

comment:10 Changed 11 months ago by arojas

yah, thanks a lot!

comment:11 follow-up: Changed 11 months ago by dimpase

Dave, I've merged all your pynac patches into master, e.g.

https://github.com/pynac/pynac/commit/9d442a81aee9355c5526fff53246aa6912a18398

while the log is clear they are yours, I'm still the author. If you like you can do a PR on github to change the author of these commits.

comment:12 follow-up: Changed 11 months ago by mkoeppe

GH Actions run please

comment:13 in reply to: ↑ 12 ; follow-ups: Changed 11 months ago by dimpase

comment:14 in reply to: ↑ 11 ; follow-up: Changed 11 months ago by gh-DaveWitteMorris

Replying to dimpase:

Dave, I've merged all your pynac patches into master, e.g.

https://github.com/pynac/pynac/commit/9d442a81aee9355c5526fff53246aa6912a18398

while the log is clear they are yours, I'm still the author. If you like you can do a PR on github to change the author of these commits.

Thanks, Dima, that's great! I am not concerned about the "author", so nothing needs to be done, as far as I'm concerned.

Copying these patches to the pynac repository has been on my to-do list for a long time, but I never got around to it, so thanks for doing this! In the future, I'll try to save you (and the distro managers) some trouble by making the appropriate PRs.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 11 months ago by dimpase

Replying to gh-DaveWitteMorris:

Copying these patches to the pynac repository has been on my to-do list for a long time, but I never got around to it, so thanks for doing this! In the future, I'll try to save you (and the distro managers) some trouble by making the appropriate PRs.

I guess you actually might have a git repo with these patches applied (at least this is how I typically do package patching), so it could have been a breeze to convert these into PRs. Anyway, it's done now.

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

comment:16 Changed 11 months ago by slelievre

One can set the author name and email as different from the committer's:

$ git commit --author="Author Name <email@example.com>"

This can be used to amend the last commit done:

$ git commit --amend --author="Author Name <email@example.com>" --no-edit

Reference:

comment:17 in reply to: ↑ 13 Changed 11 months ago by gh-sheerluck

Replying to dimpase:

Replying to mkoeppe:

GH Actions run please

running at https://github.com/sagemath/sagetrac-mirror/actions/runs/972993967

gentoo failed bc system eclib wanted https://trac.sagemath.org/ticket/31443

checking whether we can link and run a program using eclib...
eclib version 20210503, using NTL bigints and NTL real and complex multiprecision floating point
yes; use eclib from the system

comment:18 in reply to: ↑ 13 Changed 11 months ago by dimpase

Replying to dimpase:

Replying to mkoeppe:

GH Actions run please

running at https://github.com/sagemath/sagetrac-mirror/actions/runs/972993967

looks like that everything (modulo the usual GH actions suspects, e.g. broken conda on some platforms) is working. Review?

comment:19 follow-up: Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

Broken on ubuntu-trusty

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

Also could you please have the GH Actions on pynac re-enabled? see https://github.com/pynac/pynac/actions/runs/971200691

comment:21 in reply to: ↑ 19 Changed 11 months ago by dimpase

Replying to mkoeppe:

Broken on ubuntu-trusty

I cannot imagine that

[pynac-0.7.28]   checking for flint/fmpq_poly.h... no
[pynac-0.7.28]   configure: error: This package needs flint headers

is a regression. The only difference between 0.7.28 and 0.2.72 that all patches for the latter are upstreamed, and two more PRs are merged, none of them have anything to do with flint.

Version 1, edited 11 months ago by dimpase (previous) (next) (diff)

comment:22 Changed 11 months ago by mkoeppe

ubuntu-trusty is OK on 9.4.beta3 (as you can see in https://github.com/sagemath/sage/actions/runs/958667352) and broken with this branch as your GH Actions run showed. Same with linuxmint-17.

comment:23 Changed 11 months ago by dimpase

I'm sure it's just crappy CI for museum age OSs - what could have caused flint headers to suddenly become unavailable?

comment:24 Changed 11 months ago by mkoeppe

No, my CI does not have such problems.

comment:25 follow-up: Changed 11 months ago by mkoeppe

As you know, you can test it locally using tox -e docker-ubuntu-trusty-standard.

comment:26 Changed 11 months ago by fbissey

Instead of arguing about the CI, could we have the config.log of pynac.

comment:27 Changed 11 months ago by mkoeppe

The logs are in the log artifacts of Dima's GH Actions run: https://github.com/sagemath/sagetrac-mirror/suites/3090181081/artifacts/70720481

comment:28 Changed 11 months ago by mkoeppe

OK actually the config.log is not there. So a local rerun is needed to produce them

comment:29 in reply to: ↑ 25 Changed 11 months ago by dimpase

Replying to mkoeppe:

As you know, you can test it locally using tox -e docker-ubuntu-trusty-standard.

My office machine doesn't have docker. OK, I can do an lxc install of ubuntu trusty, maybe...

comment:30 Changed 11 months ago by mkoeppe

That's #29283

comment:31 Changed 11 months ago by dimpase

linuxmint-17 reached EOL over 2 years ago, I don't know why we still keep it. https://forums.linuxmint.com/viewtopic.php?t=289281

ubuntu trusty is also beyond the end of unpaid support, EOL to happen in 10 months.

Let's drop them for 9.4.

comment:32 follow-up: Changed 11 months ago by mkoeppe

We can't have this discussion on every untested package upgrade ticket.

Last edited 11 months ago by mkoeppe (previous) (diff)

comment:33 Changed 11 months ago by mkoeppe

Here's the pynac config.log:

configure:17504: checking for gmp.h
configure:17504: gcc -std=gnu11 -c -O2 -g   conftest.c >&5
configure:17504: $? = 0
configure:17504: result: yes
configure:17514: checking for library containing __gmpz_get_str
configure:17544: gcc -std=gnu11 -o conftest -O2 -g   -Wl,-rpath-link,/sage/local/lib -L/sage/local/lib -Wl,-rpath,/sage/local/lib  conftest.c  >&5
/tmp/ccY7y07W.o: In function `main':
/sage/local/var/tmp/sage/build/pynac-0.7.28/src/conftest.c:34: undefined reference to `__gmpz_get_str'
collect2: error: ld returned 1 exit status
configure:17544: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "pynac"
| #define PACKAGE_TARNAME "pynac"
| #define PACKAGE_VERSION "0.7.28"
| #define PACKAGE_STRING "pynac 0.7.28"
| #define PACKAGE_BUGREPORT "<pynac-devel@googlegroups.com>"
| #define PACKAGE_URL ""
| #define PACKAGE "pynac"
| #define VERSION "0.7.28"
| #define ARCHIVE_VERSION 3
| #define ARCHIVE_AGE 0
| #define HAVE_STDIO_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_UNISTD_H 1
| #define STDC_HEADERS 1
| #define HAVE_DLFCN_H 1
| #define LT_OBJDIR ".libs/"
| #define HAVE_GMP_H 1
| /* end confdefs.h.  */
| 
| /* Override any GCC internal prototype to avoid an error.
|    Use char because int might match the return type of a GCC
|    builtin and then its argument prototype would still apply.  */
| char __gmpz_get_str ();
| int
| main (void)
| {
| return __gmpz_get_str ();
|   ;
|   return 0;
| }
configure:17544: gcc -std=gnu11 -o conftest -O2 -g   -Wl,-rpath-link,/sage/local/lib -L/sage/local/lib -Wl,-rpath,/sage/local/lib  conftest.c -lgmp   >&5
configure:17544: $? = 0
configure:17564: result: -lgmp
configure:17578: checking for flint/fmpq_poly.h
configure:17578: gcc -std=gnu11 -c -O2 -g   conftest.c >&5
In file included from /sage/local/include/flint/nmod_vec.h:29:0,
                 from /sage/local/include/flint/fmpz.h:31,
                 from /sage/local/include/flint/fmpq_poly.h:24,
                 from conftest.c:54:
/sage/local/include/flint/ulong_extras.h:119:1: error: unknown type name '_Thread_local'
 extern FLINT_TLS_PREFIX ulong * _flint_primes[FLINT_BITS];
 ^
/sage/local/include/flint/ulong_extras.h:119:31: error: expected '=', ',', ';', 'asm' or '__attribute__' before '*' token
 extern FLINT_TLS_PREFIX ulong * _flint_primes[FLINT_BITS];
                               ^
/sage/local/include/flint/ulong_extras.h:120:25: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'double'
 extern FLINT_TLS_PREFIX double * _flint_prime_inverses[FLINT_BITS];
                         ^
/sage/local/include/flint/ulong_extras.h:121:25: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'int'
 extern FLINT_TLS_PREFIX int _flint_primes_used;
                         ^
configure:17578: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "pynac"
| #define PACKAGE_TARNAME "pynac"
| #define PACKAGE_VERSION "0.7.28"
| #define PACKAGE_STRING "pynac 0.7.28"
| #define PACKAGE_BUGREPORT "<pynac-devel@googlegroups.com>"
| #define PACKAGE_URL ""
| #define PACKAGE "pynac"
| #define VERSION "0.7.28"
| #define ARCHIVE_VERSION 3
| #define ARCHIVE_AGE 0
| #define HAVE_STDIO_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_UNISTD_H 1
| #define STDC_HEADERS 1
| #define HAVE_DLFCN_H 1
| #define LT_OBJDIR ".libs/"
| #define HAVE_GMP_H 1
| /* end confdefs.h.  */
| #include <stddef.h>
| #ifdef HAVE_STDIO_H
| # include <stdio.h>
| #endif
| #ifdef HAVE_STDLIB_H
| # include <stdlib.h>
| #endif
| #ifdef HAVE_STRING_H
| # include <string.h>
| #endif
| #ifdef HAVE_INTTYPES_H
| # include <inttypes.h>
| #endif
| #ifdef HAVE_STDINT_H
| # include <stdint.h>
| #endif
| #ifdef HAVE_STRINGS_H
| # include <strings.h>
| #endif
| #ifdef HAVE_SYS_TYPES_H
| # include <sys/types.h>
| #endif
| #ifdef HAVE_SYS_STAT_H
| # include <sys/stat.h>
| #endif
| #ifdef HAVE_UNISTD_H
| # include <unistd.h>
| #endif
| #include <flint/fmpq_poly.h>
configure:17578: result: no
configure:17584: error: This package needs flint headers

comment:34 Changed 11 months ago by fbissey

So, we have flint headers but they are broken. I'd say because glibc is too old at first glance. Glad you could dig those because they are really useful in pinpointing issues.

comment:35 follow-up: Changed 11 months ago by mkoeppe

On this install, flint was installed from the SPKG.

comment:36 Changed 11 months ago by mkoeppe

And looking at the flint build log: it is using plain gcc, without -std=gnu11.

comment:37 in reply to: ↑ 35 Changed 11 months ago by fbissey

Replying to mkoeppe:

On this install, flint was installed from the SPKG.

I could tell from the headers tested. Either something is changed at install or those particular headers are not involved in building flint. Otherwise building flint would be a problem. It is worth trying without the gnu11 bit. Good spotting.

comment:38 follow-up: Changed 11 months ago by fbissey

I think the gnu11 is a setting bleeding from setting the C++ compiler to c++11. It probably shouldn't be there at all.

comment:39 in reply to: ↑ 38 Changed 11 months ago by fbissey

Replying to fbissey:

I think the gnu11 is a setting bleeding from setting the C++ compiler to c++11. It probably shouldn't be there at all.

Nope C++11 is supposed to be tested after. I have no idea where this one comes from. I also don't see it when I configure pynac locally (have to double check that) so I am not sure where it is introduced.

comment:40 Changed 11 months ago by mkoeppe

Right. There is no C code whatsoever in pynac. Everything is C++. Just the AC_LANG([C++]) is coming much too late!

comment:41 Changed 11 months ago by mkoeppe

Last edited 11 months ago by mkoeppe (previous) (diff)

comment:43 Changed 11 months ago by fbissey

Do I take it that it helped :)

comment:44 Changed 11 months ago by mkoeppe

Yes, this seems to do the trick.

Dima, can you please create a new release with this PR?

comment:45 in reply to: ↑ 20 Changed 11 months ago by dimpase

Replying to mkoeppe:

Also could you please have the GH Actions on pynac re-enabled? see https://github.com/pynac/pynac/actions/runs/971200691

no idea what "reenabled" means. They are enabled, I guess it's what we have in .github/ matters.

comment:46 follow-up: Changed 11 months ago by mkoeppe

Like the error message says: "Actions jobs for this repository have been disabled by GitHub staff. If you are the repository owner, you can contact support via https://github.com/contact for more information." I had to do this for one of my repositories too in the past.

comment:47 in reply to: ↑ 15 Changed 11 months ago by gh-DaveWitteMorris

Replying to dimpase:

Replying to gh-DaveWitteMorris:

Copying these patches to the pynac repository has been on my to-do list for a long time, but I never got around to it, so thanks for doing this! In the future, I'll try to save you (and the distro managers) some trouble by making the appropriate PRs.

I guess you actually might have a git repo with these patches applied (at least this is how I typically do package patching), so it could have been a breeze to convert these into PRs. Anyway, it's done now.

Yes, this should have been easy, but I know almost nothing about git, so my attempts failed when I tried a few weeks ago. (I didn't realize that I needed a fork, instead of a clone.) I think I succeeded in making a PR to the pynac repository this evening, and, now that I know how, I will try to make PRs of all of my pynac patches.

comment:48 in reply to: ↑ 46 ; follow-up: Changed 11 months ago by dimpase

Replying to mkoeppe:

Like the error message says: "Actions jobs for this repository have been disabled by GitHub staff.

I cannot see any error messages there. Maybe I am looking at the wrong places?

If you are the repository owner, you can contact support via https://github.com/contact for more information." I had to do this for one of my repositories too in the past.

comment:49 in reply to: ↑ 48 Changed 11 months ago by dimpase

Replying to dimpase:

Replying to mkoeppe:

Like the error message says: "Actions jobs for this repository have been disabled by GitHub staff.

I cannot see any error messages there. Maybe I am looking at the wrong places?

If you are the repository owner, you can contact support via https://github.com/contact for more information." I had to do this for one of my repositories too in the past.

OK, it's in "Annotations". Duh. OK, requested re-enabling https://support.github.com/ticket/personal/0/1212297

comment:50 Changed 11 months ago by git

  • Commit changed from e97c1b54c1a9fc8438a33803fa7b45b2a42643b0 to 3c131f7b368b4b929e8b7cbe97879b4708de11cf

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

3c131f7wip tarball

comment:51 Changed 11 months ago by git

  • Commit changed from 3c131f7b368b4b929e8b7cbe97879b4708de11cf to 11c80aa3a61496a976558b016c70ef8165218651

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

11c80aaupdated tarball with PRs 377 and 378

comment:52 Changed 11 months ago by dimpase

  • Status changed from needs_work to needs_review

comment:53 Changed 11 months ago by dimpase

tests are running on https://github.com/sagemath/sagetrac-mirror/actions/runs/978774922 (which is this branch with an extra pynac test from #31585)

comment:54 in reply to: ↑ 32 Changed 11 months ago by dimpase

Replying to mkoeppe:

We can't have this discussion on every untested package upgrade ticket.

I've opened #32074

comment:55 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work
Last edited 11 months ago by mkoeppe (previous) (diff)

comment:56 Changed 11 months ago by mkoeppe

Dima, could you please make a release without PR 378, so that we can get this blocker ticket done? Just focused on what this ticket says - add support for gcc 11, consolidate existing patches; no breaking/removing of other platform support.

comment:57 follow-up: Changed 11 months ago by mkoeppe

I volunteer to make the next pynac releases.

comment:58 in reply to: ↑ 57 Changed 11 months ago by dimpase

Replying to mkoeppe:

I volunteer to make the next pynac releases.

let's just drop gcc 4.x. It would minimise the disruption, and it's overdue anyway.

comment:59 Changed 11 months ago by gh-DaveWitteMorris

As I said on #32074, I am +1 on this proposal to drop support for gcc 4.x. That should solve the immediate problem.

comment:60 Changed 11 months ago by dimpase

Or even better/quicker, just add AC_CHECK_FUNC(__builtin_smull_overflow,...) to spkg-configure for gcc spkg.

comment:61 follow-up: Changed 11 months ago by mkoeppe

It's important to keep tickets focused. In particular for a blocker ticket like this one, which creates an urgency to get it in.

The urgent need to add support for gcc 11 is entirely unconnected to a decision to drop support for the gcc 4 versions used in some of our supported distributions.

It is poor practice to connect these two issues arbitrarily.

comment:62 Changed 11 months ago by dimpase

our resources are limited;

  • making a new pynac release without __builtin_smull_overflow takes time,
  • re-doing the branch with __builtin_smull_overflow takes time
  • (and God help you finding a workaround not using __builtin_smull_overflow, potentially another time waste to keep a museum compiler in...)

comment:63 in reply to: ↑ 61 Changed 11 months ago by mkoeppe

It's important to keep tickets focused -- that includes avoiding to give unsolicited advice to other developers regarding time management.

comment:64 Changed 11 months ago by mkoeppe

  • Branch changed from u/dimpase/packages/pynac/0.7.28 to u/mkoeppe/packages/pynac/0.7.28

comment:65 Changed 11 months ago by mkoeppe

  • Authors changed from Dima Pasechnik to Dima Pasechnik, Matthias Koeppe
  • Commit changed from 11c80aa3a61496a976558b016c70ef8165218651 to c9c50e3d1bb0a6d9ec17ede66a2f47da7cd89b01
  • Status changed from needs_work to needs_review

New commits:

c9c50e3build/pkgs/pynac: Upgrade to 0.7.29

comment:66 Changed 11 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe, ...

comment:68 Changed 11 months ago by dimpase

  • Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:69 Changed 11 months ago by mkoeppe

Thanks!

comment:70 Changed 11 months ago by vbraun

  • Branch changed from u/mkoeppe/packages/pynac/0.7.28 to c9c50e3d1bb0a6d9ec17ede66a2f47da7cd89b01
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:71 Changed 10 months ago by dimpase

  • Commit c9c50e3d1bb0a6d9ec17ede66a2f47da7cd89b01 deleted

I don't see this branch in the current develop branch, what's up?

Note: See TracTickets for help on using tickets.