-
Notifications
You must be signed in to change notification settings - Fork 134
Refactor: use functionality based checking instead of OS-level checking #435
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?
Refactor: use functionality based checking instead of OS-level checking #435
Conversation
Rather than being dependent on OS-level defines, refactor to use feature-level defines.
|
Do we have a CYGWIN recipe to hand ? Ideally want to confirm CYGWIN builds ok (CYGWIN is supposed to be POSIX on windows, so should not need any specific ifdef in the area of termios AFAIK). I shall try the commit with FreeBSD, OpenBSD and Solaris, which are the most affected, this is doing to take a week at least to come back on. I am also looking for a legacy version of each platform for this kind of commit, ones where the configure looks different to the current stable version. |
|
I stopped using Cygwin the day that Microsoft released WSL2. Does anybody still use it? But okay, I'm sure it's still more viable than Solaris. There is a lot of old cruft in the Magic build files that are better off being removed than attempting to support. In spite of decades of my attempts to keep backwards compatible support of magic from the 1980s. . . |
Using Cygwin, I tried compiling it on a Windows 10 VM, the For reference, these fails: Compiled using: with environment:
Thanks! |
RTimothyEdwards
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.
This looks good to me. The replacement of system-based checks with specific checks on TERMIOS vs. TERMIO makes a lot of sense and the code has been made cleaner overall. I would like Darryl to take a quick look over it, though.
|
@dlmiles : I should have pinged you in the last comment. |
|
I did see the comment soon after it was made. There is no update at this time, as per my previous comment I shall update this issue within a week (~3 days left) concerning how it works out with other platforms. The magstty.h being changed has recently had changes to cleanup that area (in the Makefile/build rework) the version is one that passed CIs to release, but other items exist in this same area that will probably address the bleeding edge Linux change, the current version of most of the change exists as commit While I have no interest in supporting legacy systems from 1980s, I am trying to not make things worse for the surviving lineage that is available as Open Source today. I do have working builders for FreeBSD, OpenBSD, Solaris/SunOS and now CYGWIN (since the past few days) in GitHub CI systems. https://github.com/dlmiles/magic/actions Which are used to validate changes that have a high-risk of cross-platform breakage. Supporting bleeding edge linux for me is like Tier-3 (lower priority) as it can turn into a game of whack-a-mole over changes that never make it into an LTS due to being removed/not-adopted. Glibc 2.42 is from 08-Jul-2025 and you'd be surprised how many months/years it takes to get it into the first LTS. That said I've no doubt this change will affect LTS Linux eventually. Arch users come to accept the world of hurt they like to subject themselves too over this so it is usual to patch things within an Arch build recipe. Tier1: Linux LTS (EL/Debian/OpenSuSE), MacOS (current releases per CPU) What is meant by this is a commit to address a lower tier should ideally not be breaking something in a higher tier. Most of the show stopper issues with the Tier2 were fixed in the recent Makefile/build change-set. The rest of the changes in this area seek to make it more robust/easier/automatic. Re the comment on the So in a few days I can at least report this commit does not introduce any regression on the other platforms, then provide the ok to merge. I can rebase the commit I link in this comment above on top of that merge if needed. |
|
I tested the commit you referenced (cc0c02c) and that compiles correctly on my machine. Considering that the commit effectively supersedes this pull-request, I am perfectly content with the following:
Then we avoid the effort of your (already cleaner) solution needed to be rebased.
Yes, so keeping the patch downstream until your PR is merged is perfectly fine. |
What?
This refactors some of the code to use functionality based checking rather than OS-level checking for header includes
Motivation
For bleeding edge systems that are using GLIBC v2.42,
termio.hhas been removed, andtermios.his the replacement.Due to recent refactors + removal of these headers one is not able to compile this when using GLIBC v2.24.
Note
Only tested the compilation on my machine (Arch Linux x86_64),
hopefully this doesn't break for other systems (BSD etc.)!
Closes: #434