-
Notifications
You must be signed in to change notification settings - Fork 175
Fix Windows/MinGW compilation issues #6077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix Windows/MinGW compilation issues #6077
Conversation
- Add Windows/MinGW platform detection in configure.ac - Fix io.h header conflict by reordering includes in streams.c - Add conditional compilation for termios.h in iostream.c and sysfiles.c - Provide fallbacks for getrusage on systems without it - Disable subprocess support on MinGW platforms - Make terminal raw mode functions conditional on termios availability Co-authored-by: tobiasdiez <[email protected]>
Co-authored-by: tobiasdiez <[email protected]>
Co-authored-by: tobiasdiez <[email protected]>
- Add AC_CHECK_FUNCS tests for realpath and readlink in configure.ac - Add conditional compilation in src/main.c for POSIX-specific functions - Provide fallback implementations when realpath/readlink are unavailable - Maintains full functionality on POSIX systems with graceful degradation on Windows - Fixes implicit function declaration errors on MinGW Co-authored-by: tobiasdiez <[email protected]>
… Windows Co-authored-by: tobiasdiez <[email protected]>
Co-authored-by: tobiasdiez <[email protected]>
- Add proper function availability checks for lstat and ttyname in configure.ac - Fix sigaction usage to be conditional with fallback to signal() - Fix mkdir to use single argument on MinGW/Windows - Fix lstat/S_ISLNK usage to be conditional, fallback to stat - Fix ttyname usage to be conditional with appropriate fallbacks - Reorder includes to put system headers before GAP headers to avoid conflicts - Add MinGW POSIX compatibility layer with explicit function declarations - Add access() constants (F_OK, R_OK, W_OK, X_OK) for MinGW This addresses all the compilation errors mentioned in the issue and enables GAP to compile on MinGW/Windows with proper graceful degradation of features. Co-authored-by: tobiasdiez <[email protected]>
…inGW) Co-authored-by: tobiasdiez <[email protected]>
…iron Co-authored-by: tobiasdiez <[email protected]>
Co-authored-by: tobiasdiez <[email protected]>
Co-authored-by: tobiasdiez <[email protected]>
…tps://github.com/tobiasdiez/gap into copilot/fix-19fc677e-dc3e-4169-93f2-346d1cf4b998
…t skipping Co-authored-by: tobiasdiez <[email protected]>
…mat issue Co-authored-by: tobiasdiez <[email protected]>
Co-authored-by: tobiasdiez <[email protected]>
|
Compilation is successful on my device, but I get runtime errors: These point to the line Adding the following debug outputs there: yield Not sure if those are a result of a wrong print command, or if they shine light on what's wrong here. |
|
Just to say I won't have time to look at this until next Wednesday as I'm on holiday, but from a quick glance I'd want to see it broken down into more pieces -- build system, fixes for different functions. I would guess the crash is because of the GC, and that is probably because it's not stack scanning properly. (It could also be a layout issue). Those are both harder to fix, but can be handled separately. |
fingolfin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Breaking this up in smaller bits is definitely the way to go. I left a couple quick comments
| #include <assert.h> | ||
| #include <stdint.h> | ||
|
|
||
| #include "config.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this is not an acceptable change, no header is allowed to include config.h, as it would break installed GAP (note that config.h is not installed, and is system dependant).
| src/ffdata.c | ||
| src/ffdata.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these files should not exist, if they do get created, something is wrong
| // Handle platform differences for setjmp/longjmp | ||
| #ifdef SYS_IS_MINGW | ||
| // On MinGW/Windows, _setjmp requires two arguments, so use regular setjmp | ||
| #define GAP_SETJMP(jmp_buf) setjmp(jmp_buf) | ||
| #define GAP_LONGJMP(jmp_buf, val) longjmp(jmp_buf, val) | ||
| #else | ||
| // On POSIX systems, use _setjmp/_longjmp for better performance | ||
| #define GAP_SETJMP(jmp_buf) _setjmp(jmp_buf) | ||
| #define GAP_LONGJMP(jmp_buf, val) _longjmp(jmp_buf, val) | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the wrong file for this. Perhaps common.h or better, a new header syssetjmp.h
| // inline symbols can define DEBUG_FORCE_EXTERN_INLINE before including | ||
| // this header to switch them to 'extern inline'. | ||
| # define EXPORT_INLINE extern inline | ||
| # elif defined(SYS_IS_MINGW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use __MINGW32__ resp. __MINGW64__ ?
| // libgap as a shared library | ||
| #define EXPORT_INLINE extern inline | ||
| // Request that common.h use 'extern inline' (see common.h for details) | ||
| #define DEBUG_FORCE_EXTERN_INLINE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removes salient points of the comment regarding lldb and gdb, that should be kept.
| #include "vars.h" | ||
| #include "vec8bit.h" | ||
| #include "vecffe.h" | ||
| #include "vecgf2.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Unless you have a strong reason these includes should not be added here.
| #else | ||
| // Fallback for systems without getrusage (e.g., Windows/MinGW) | ||
| // Use wall clock time instead of CPU time as an approximation | ||
| return SyNanosecondsSinceEpoch()/1000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if this was restricted to your system and otherwise runs into an error / warning so that future porters can find this place.
| tmp = buf.ru_stime.tv_sec * 1000 + buf.ru_stime.tv_usec / 1000; | ||
| ASS_LIST(res, 4, ObjInt_UInt(tmp)); | ||
| #else | ||
| // Fallback for systems without getrusage (e.g., Windows/MinGW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again I'd prefer if this was mingw specific
| #include "config.h" | ||
|
|
||
| // Include system headers first to avoid conflicts on Windows/MinGW | ||
| // where dirent.h includes system io.h which can conflict with GAP's io.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the order of the includes help with that?
| #endif | ||
|
|
||
| #ifndef environ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
| #endif | |
| #ifndef environ | |
| #elif !defined(environ) | |
|
One more thing:
This PR most definitely does not fix #4157. It is certainly some good first steps towards it (bravo), but "making it build" is not the same as "making it work" (indeed, otherwise you could just delete all the code, it would also build very well). |
|
Thanks for the comments! I will look at them later in detail, and will also extract more easily reviewable chunks. For now, could someone please run the CI here to make sure that it's not breaking anything on Linux. Thanks |
This PR resolves all compilation issues preventing GAP from building on Windows using MinGW. Fixes #4157.
It's a rough initial version and needs further cleanup. Will do this in a week or so. But I'm happy for any initial suggestions and feedback. The first question: do you prefer if the changes are split in smaller PRs?
Text for release notes
see title
Further details
For transparency: The changes were mostly created by GH Copilot, see tobiasdiez#1 for details (I mostly wanted to see how able the copilot feature is by now, and this seemed to be a good test case).