Opened 3 years ago
Closed 3 years ago
#21578 closed defect (fixed)
Problem with fflas.h on Cygwin since #17635
Description (last modified by )
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) Correction:
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).
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:
Change History (16)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
- Branch set to u/embray/ticket-21578
- Commit set to 36af775cb3a26fd0e149bf48579a2d63b07c5bc8
- Status changed from new to needs_review
comment:4 Changed 3 years ago by
Just
#undef B0 would work too.
In any case, this needs to be reported upstream.
comment:5 Changed 3 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:6 Changed 3 years ago by
- Cc cpernet added
comment:7 Changed 3 years ago by
- 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
comment:9 Changed 3 years ago by
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):
comment:10 Changed 3 years ago by
- 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
(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
- Commit changed from 36af775cb3a26fd0e149bf48579a2d63b07c5bc8 to 2b8c52c7f4a03011b29a3ba01b49e5cd7c796597
Branch pushed to git repo; I updated commit sha1. New commits:
comment:13 Changed 3 years ago by
- Status changed from needs_work to needs_review
Updated patch to mirror the upstream fix.
comment:14 Changed 3 years ago by
- 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
- Status changed from needs_review to positive_review
comment:16 Changed 3 years ago by
- Branch changed from u/embray/ticket-21578 to 2b8c52c7f4a03011b29a3ba01b49e5cd7c796597
- Resolution set to fixed
- Status changed from positive_review to closed
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: