Skip to content

Conversation

@tskaar
Copy link
Contributor

@tskaar tskaar commented Jul 31, 2025

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.h has been removed, and termios.h is 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

tskaar added 2 commits July 31, 2025 20:10
Rather than being dependent on OS-level defines,
refactor to use feature-level defines.
@dlmiles
Copy link
Collaborator

dlmiles commented Jul 31, 2025

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.

@RTimothyEdwards
Copy link
Owner

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

@tskaar
Copy link
Contributor Author

tskaar commented Jul 31, 2025

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

Using Cygwin, I tried compiling it on a Windows 10 VM, the textio code compiled correctly, however there are other compile errors (not applicable to this PR), so not sure how much point there is to check for compatibility...

For reference, these fails:

netlist.c: In function ‘NLNetName’:
netlist.c:378:22: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘asm’
  378 |     extern int etext asm("etext");
      |                      ^~~
netlist.c:378:22: warning: implicit declaration of function ‘asm’ [-Wimplicit-function-declaration]
netlist.c:390:27: error: ‘etext’ undeclared (first use in this function); did you mean ‘atexit’?
  390 |     if (net <= (NLNet *)(&etext))

runstats.c:39:14: error: conflicting types for ‘sbrk’; have ‘char *()’
   39 | extern char *sbrk();
      |              ^~~~
In file included from /usr/include/unistd.h:4,
                 from runstats.c:31:

Compiled using:

CFLAGS="-std=c17 -g -D_DEFAULT_SOURCE=1" ./configure
make

with environment:

gcc version 13.4.0 (GCC)
GNU Make 4.4.1

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.

Thanks!

@RTimothyEdwards RTimothyEdwards requested a review from dlmiles August 1, 2025 18:17
Copy link
Owner

@RTimothyEdwards RTimothyEdwards left a 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.

@RTimothyEdwards
Copy link
Owner

@dlmiles : I should have pinged you in the last comment.

@dlmiles
Copy link
Collaborator

dlmiles commented Aug 4, 2025

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
cc0c02c

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)
Tier2: FreeBSD, OpenBSD, Solaris, CYGWIN (all current releases)
Tier3: Linux bleeding edge (Arch)

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 -DSYSV inclusion for Linux build as part of ./configure this is looking to be replaced with -D_DEFAULT_SOURCE=1 by default but that is also one of the many simultaneous matters being improved. As we can see that is starting to show issues on FC42 as an example and will eventually also make it into LTS.

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.

@tskaar
Copy link
Contributor Author

tskaar commented Aug 5, 2025

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:

  1. Leave/Close this PR
  2. I add the referenced commit as a patch in Arch's build system (effectively cherry-picking it)
  3. Keep the patch until your PR is ready to be merged.

Then we avoid the effort of your (already cleaner) solution needed to be rebased.

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.

Yes, so keeping the patch downstream until your PR is merged is perfectly fine.

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.

Compilation: Mismatch between utils/magsgtty.h and textio/txInput.c on v8.3.534

3 participants