Opened 3 years ago

Closed 3 years ago

#21578 closed defect (fixed)

Problem with fflas.h on Cygwin since #17635

Reported by: embray Owned by:
Priority: major Milestone: sage-7.4
Component: porting: Cygwin Keywords:
Cc: cpernet Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 2b8c52c (Commits) Commit: 2b8c52c7f4a03011b29a3ba01b49e5cd7c796597
Dependencies: Stopgaps:

Description (last modified by embray)

I first noticed this problem here

Compiling any Cython modules that include fflas.h either directly, on indirectly through ffpack, linbox, etc. fail to compile on Cygwin (and hypothetically other platforms).

This is due to the following lines in, of all places, pyport.h (which is included by Python.h):

#if defined(HAVE_OPENPTY) || defined(HAVE_FORKPTY)
#if !defined(HAVE_PTY_H) && !defined(HAVE_LIBUTIL_H) && !defined(HAVE_UTIL_H)
/* BSDI does not supply a prototype for the 'openpty' and 'forkpty'
   functions, even though they are included in libutil. */
#include <termios.h>
extern int openpty(int *, int *, char *, struct termios *, struct winsize *);
extern pid_t forkpty(int *, char *, struct termios *, struct winsize *);
#endif /* !defined(HAVE_PTY_H) && !defined(HAVE_LIBUTIL_H) */
#endif /* defined(HAVE_OPENPTY) || defined(HAVE_FORKPTY) */

Cygwin does not have (for whatever reason) HAVE_PTY_H or HAVE_LIBUTIL_H defined (even though pty.h does exist on Cygwin--this might be a bug in Python's configure.ac). Correction: HAVE_PTY_H is defined, so maybe termios.h is being included from somewhere else. Turns out the real culprit (still in pyport.h) is:

/* On QNX 6, struct termio must be declared by including sys/termio.h
   if TCGETA, TCSETA, TCSETAW, or TCSETAF are used.  sys/termio.h must
   be included before termios.h or it will generate an error. */
#if defined(HAVE_SYS_TERMIO_H) && !defined(__hpux)
#include <sys/termio.h>
#endif

My Linux system does not have sys/termio.h, but Cygwin does--apparently--maybe for some backward compatibility reason (it is simply an alias for sys/termios.h with an s).

This results in Python.h including termios.h which defines several macros for baud rates with names like B0, ..., B230400. This ends up breaking fflas-ffpack/fflas/fflas_igemm/igemm_kernels.inl, which is included (indirectly) from fflas.h, because it contains a variable named "B0" which is replaced by the macro in termios.h

Reported upstream at: https://github.com/linbox-team/fflas-ffpack/issues/57

Change History (16)

comment:1 Changed 3 years ago by embray

  • Description modified (diff)

comment:2 Changed 3 years ago by embray

  • Description modified (diff)

comment:3 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-21578
  • Commit set to 36af775cb3a26fd0e149bf48579a2d63b07c5bc8
  • Status changed from new to needs_review

First attempt at a workaround. For now I was going for a patch that changed the fewest lines, though a simpler change might just be to use different variable names...


New commits:

36af775Patch to work around https://trac.sagemath.org/ticket/21578

comment:4 Changed 3 years ago by jdemeyer

Just #undef B0 would work too.

In any case, this needs to be reported upstream.

comment:5 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:6 Changed 3 years ago by jdemeyer

  • Cc cpernet added

comment:7 Changed 3 years ago by embray

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

True, the #ifdef is superfluous I guess but for now it doesn't matter.

comment:8 Changed 3 years ago by embray

  • Description modified (diff)
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

comment:9 Changed 3 years ago by cpernet

What a pain to have B0 defined! I've fixed upstream fflas-ffpack, by renaming B0 into B_0 (and other variables in the same way, for consistency): https://github.com/linbox-team/fflas-ffpack/commit/346498a71b2759f5913ba8c4c2fe025bbf8b3faa

comment:10 Changed 3 years ago by embray

  • Status changed from positive_review to needs_work

Thanks! In that case I'll update the patch in this ticket to do the same.

comment:11 Changed 3 years ago by embray

(Though depending on the speed at which other Cygwin issues get resolved, this may become a moot point if we do another fflas-ffpack update before Cygwin is at 100% again :)

comment:12 Changed 3 years ago by git

  • Commit changed from 36af775cb3a26fd0e149bf48579a2d63b07c5bc8 to 2b8c52c7f4a03011b29a3ba01b49e5cd7c796597

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

2b8c52cReplace my patch with the patch from the upstream fix, which simply renames the offending variable (and its siblings).

comment:13 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Updated patch to mirror the upstream fix.

comment:14 Changed 3 years ago by jdemeyer

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

comment:15 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/embray/ticket-21578 to 2b8c52c7f4a03011b29a3ba01b49e5cd7c796597
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.