Skip to content
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

Remove mingw32 #914

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

chuckyvt
Copy link
Contributor

@chuckyvt chuckyvt commented Jan 1, 2025

This PR removes MINGW32 from the Windows CI checks. This version of MINGW receives limited support, and I don't believe it makes sense to use for validation of future development. This change was originally suggested in this PR.

@perazz
Copy link
Contributor

perazz commented Jan 3, 2025

Thank you @chuckyvt for this PR. I'm generally in favor of dropping support for ancient platforms. However, I find that:

So, maybe this can be deferred until issues related to 32-bit Windows may arise?

@chuckyvt
Copy link
Contributor Author

chuckyvt commented Jan 6, 2025

Will try to address your comments/concerns.

Have issues been seen with MINGW32: Yes, there have been. Work on PR #843 was paused due to CI failures with MINGW32. The failure traced back to a memory access error when reallocating an unlimited polymorphic variable. The failure is only seen with Mingw32. I tried every code refactor I could think of, but nothing seemed to address the issue,

Value of Win32 support: The company I work for is highly dependent on Win32 IFORT due to the amount of legacy binary libraries in use and will continue to use it for years to come. I suspect we aren't the only ones stuck in that boat. However, this legacy support is specific to Intel IFORT. (Note the Sleep issue mentioned is specific to IFORT). I doubt there are many, if any, uses of Stdlib in a Mingw32 project. As a side note, a future PR I would like to work on is to include Intel IFX in the Windows CI, and as part of that work could consider including 32-bit IFORT. However, with IFORT no longer being actively devloped, I'm not sure if that is right thing to do or not.

Value in cross platform validation: I think the work you're doing on subprocess type implementation is great. However, at first glance, I don't see anything there that is specific to Win32. The _WIN32 variable is common to all versions of Windows as best I know, so the existing 64-bit Windows GCC check should capture it. (Windows Intel IFX needs to be included in Windows CI checks as well as mentioned above and is future work)

Copy link
Contributor

@perazz perazz left a comment

Choose a reason for hiding this comment

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

LGTM given the motivations.

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.

3 participants