Skip to content

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
Member

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
Member

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

@perazz
Copy link
Member

perazz commented Jan 11, 2025

With two approvals, I suggest to wait another couple of days, and then merge if there are no further comments.

@jalvesz
Copy link
Contributor

jalvesz commented Jan 17, 2025

I'll merge this PR as there have been no more comments! Thanks @chuckyvt

@jalvesz jalvesz merged commit 28250f3 into fortran-lang:master Jan 17, 2025
14 checks passed
@chuckyvt chuckyvt deleted the remove-mingw32 branch April 21, 2025 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants