Skip to content

Conversation

@tobiasdiez
Copy link

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).

Copilot AI and others added 30 commits August 27, 2025 09:46
- 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]>
- 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]>
- 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]>
@tobiasdiez
Copy link
Author

Compilation is successful on my device, but I get runtime errors:

[ "List Element: <list>[1] must have an assigned value" ]
while reading ./lib/type.g:351
Error before error-handling is initialized: 
[ "List Element: <list>[1] must have an assigned value" ]
while reading ./lib/type.g:353
Error before error-handling is initialized: 
[ "List Element: <list>[1] must have an assigned value" ]
while reading ./lib/type.g:355
....

These point to the line DeclareRepresentation( "IsNonAtomicComponentObjectRep", IsComponentObjectRep );.

Adding the following debug outputs there:

Print( "IS_OBJECT = ", IS_OBJECT, "\n" );
Print( "IsComponentObjectRep = ", IsComponentObjectRep, "\n" );

yield

IS_OBJECT = Error before error-handling is initialized: 
[ "Variable: <<unknown>> must have an assigned value" ]
IsComponentObjectRep = Error before error-handling is initialized: 
[ "Variable: <<unknown>> must have an assigned value" ]

Not sure if those are a result of a wrong print command, or if they shine light on what's wrong here.

@ChrisJefferson
Copy link
Contributor

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.

Copy link
Member

@fingolfin fingolfin left a 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"
Copy link
Member

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).

Comment on lines +27 to +28
src/ffdata.c
src/ffdata.h
Copy link
Member

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

Comment on lines +68 to +78
// 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

Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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"
Copy link
Member

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;
Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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?

Comment on lines +27 to +29
#endif

#ifndef environ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not

Suggested change
#endif
#ifndef environ
#elif !defined(environ)

@fingolfin
Copy link
Member

One more thing:

This PR resolves all compilation issues preventing GAP from building on Windows using MinGW. Fixes #4157.

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).

@tobiasdiez
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thoughts for a native (well, mingw) Window version of GAP

3 participants