libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
8.71k stars 1.64k forks source link

sdl2-config cannot run on Solaris 10 #10092

Open joncox123 opened 1 week ago

joncox123 commented 1 week ago

The sdl2-config shell script cannot run properly on Solaris 10 because it requests /bin/sh as the shell on the first line. I find the script is actually incompatible with the sh shell that ships with Solaris 10 1/13. However, if I modify this to use /bin/bash, the script runs 100% correctly. I think GNU sh shell supports a superset of features of the traditional UNIX sh shell.

For Solaris systems, the first line of sdl2-config should be: #!/bin/bash

Moreover, is there a reason why sdl2-config should not use the bash shell by default on all systems? I think bash is pretty much the standard these days.

This requested change applies to all versions of SDL2, as far as I can tell.

madebr commented 1 week ago

Addressed in b9f005505

I only changed the interpreter on Solaris, and left it alone for other platforms.

It looks like package manager(s) (or at least Fedora does) change the shebang for their platform: Fedora 39's sdl2-devel package contains the #!/usr/bin/sh shebang instead of our #!/bin/sh.

sezero commented 1 week ago

What is it that sdl2-config actually wants to do but /bin/sh of solaris fails to comply? (I am guessing that it's $(...) syntax??) Should we not fix that instead of doing this?

slouken commented 1 week ago

What is it that sdl2-config actually wants to do but /bin/sh of solaris fails to comply? (I am guessing that it's $(...) syntax??) Should we not fix that instead of doing this?

Yes, that seems like a better idea than https://github.com/libsdl-org/SDL/commit/b9f005505b194f498494eec6631c38c3e219fa74

sezero commented 1 week ago

Yes, we should revert that. I think https://github.com/libsdl-org/SDL/commit/799d39a3022415f698cb92f44b872c8513d83249 (from PR/#5751) is responsible for the issue in this ticket.

smcv commented 1 week ago

For Solaris systems, the first line of sdl2-config should be: #!/bin/bash

This is, unfortunately, not portable either, even if we assume that everyone has bash installed. On "normal" Linux systems like Debian and Red Hat, /bin/bash exists, but on some of the weirder Linux distributions like NixOS, it does not. The mostly-portable way to have bash as interpreter (assuming it's installed at all) is #!/usr/bin/env bash.

I find the script is actually incompatible with the sh shell that ships with Solaris 10 1/13

Does Solaris /bin/sh not implement the POSIX-specified Shell Command Language? As far as I can find, POSIX mandates support for $(), cd -P and pwd -P, which are the new features introduced in 799d39a.

In practice the /bin/sh on Linux systems typically provides a superset of POSIX functionality - for example Debian mandates POSIX.1-2017 plus a few common extensions (echo -n, test -a, test -o, local, kill -9, trap -9), while Red Hat provides bash as /bin/sh and therefore also allows "bashisms" in /bin/sh scripts.

If /bin/sh only implements a subset of POSIX's Shell Command Language, then that's going to make portability of shell scripts reliant on folklore and unwritten rules, which seems very unfortunate: the whole point of POSIX is to give the developers of portable code something that they can rely on.

icculus commented 1 week ago

Counter-point: SDL2 is going into maintenance mode, sdl2-config is completely gone in SDL3, Solaris is a quirky niche platform, and we generate this script at build time anyhow...so @madebr's patch is fine.

Let's just take the W on this one and call it fixed. :)

sezero commented 1 week ago

which are the new features introduced in 799d39a.

And why did we need that patch, what did it really gain us?

madebr commented 1 week ago

You can use sdl2-config from a downloaded binary package and it will just work (on posix compliant systems). No need to run sed or an install script.

sezero commented 1 week ago

OK, all arguments are said, I guess. Wrap it up and close as @icculus says? (even though I don't actually like it..)

madebr commented 1 week ago

We're missing lots of information what exactly is not working.

@joncox123 please tell us

slouken commented 1 week ago

I'm inclined to fix this, once we understand what's wrong.

madebr commented 1 week ago

I reverted it in 8ae7841d9114fc6cad512d4f53ddbda3329beee5

sezero commented 1 week ago

We're missing lots of information what exactly is not working.

Does solaris not understand $() process substitution and do we need to use old-style ` ticks instead?

I think that's probably the major issue. See past 'config' commits, e.g.: http://git.savannah.gnu.org/gitweb/?p=config.git;a=commit;h=d70c4fa934de164178054c3a60aaa0024ed07c91 http://git.savannah.gnu.org/gitweb/?p=config.git;a=commit;h=0ee661a9f9cd6bf6da3fffcd85b59070f7467e94 .. and the discussions leading to them: https://lists.gnu.org/archive/html/config-patches/2021-05/index.html

Does it not understand the --?

Not sure

icculus commented 1 week ago

Does solaris not understand $() process substitution

Yep, that's it:

bash-4.3# /bin/sh ./sdl2-config
./sdl2-config: syntax error at line 4: `bindir=$' unexpected

The manpage doesn't have a more proper name for /bin/sh but claims to be mostly compatible with ksh, fwiw. It does understand backticks for running shell commands.

Does it not understand the --?

bash-4.3# /bin/sh
# dirname --
.

That...appears to be okay?

sezero commented 1 week ago

Does it not understand the --?

bash-4.3# /bin/sh
# dirname --
.

That...appears to be okay?

The real question should be support for -P I guess...