more fixes

[mirabilos]

• supercedes the Debian patches from #9/#10
  • moves the .so to .so.NUMBER* change to variables
  • does that for GNU (Linux, kFreeBSD, Hurd) only
  • adds support for .so.MAJOR.MINOR scheme some BSDs use
  • note which BSDs use it, and that others might need more patches
  • uses install -D only on GNU
  • leaves all other OSes alone
  • handles Debian’s multiarch libdir transparently for modern-style library installs on GNU distros
  • allows overriding LIBDIR for legacy GNU/Linux distros like ROSA Linux, https://github.com/raboof/sfArkLib/issues/11
• adds the patch from #12 bumping the ABI major with it

this should be compatible (i.e. merging both without conflicts) with #16

[mirabilos]

Arnout Engelen dixit:

why 1.0.0? we don't really guarantee any compatibility, so it'd probably be more semver-like to keep it 0.x.y?

Debian already has 0.0.0 here, and this has nothing to do with semver but with shared library versioning which has its own rules. Given that this is installed as shared library in Debian the disto guarantees compatibility (which it must), and this is just feeding the Debian patches upstream.

The change of ULONG from unsigned long to unsigned int (below) constitutes an ABI bump, so when adding it, I changed the major version. This makes more sense if you look at the individual commit diffs instead of the whole-PR diff.

-typedef unsigned long ULONG; +typedef unsigned int ULONG;

This doesn't make sense to me ;)

But it does ;-)

The original sfArk code was written for a platform where int was 16 bits and you had to use long to get a 32 bit integer.

On LP64 platforms, long is 64 bits now, which causes bugs on them, see https://github.com/raboof/sfArkLib/issues/12 for the source of this change.

Basically this brings the code into alignment with the original assumptions. See also: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/32862b84-f6e6-40f9-85ca-c4faf985b822 Here, ULONG is specified as 32-bit integer, “unsigned long” on Windows.

Now, Windows chose a different path when becoming 64-bitty than Unices did.

Before, you had ILP32 platforms (32-bit Windows, 32-bit Unix):

• int = 32 bit
• long = 32 bit
• pointer = 32 bit

With Unix, you get LP64 platforms because POSIX requires this:

• int = 32 bit
• long = 64 bit
• pointer = 64 bit

On Windows “x64” you get an LLP64 platform though:

• int = 32 bit
• long = 32 bit
• long long = 64 bit
• pointer = 64 bit

So the nomenclature here can be confusing to people who know only one of the platforms’ rules.

bye, //mirabilos -- 22:20⎜ The crazy that persists in his craziness becomes a master 22:21⎜ And the distance between the craziness and geniality is only measured by the success 18:35⎜ "Psychotics are consistently inconsistent. The essence of sanity is to be inconsistently inconsistent

[raboof]

-typedef unsigned long ULONG; +typedef unsigned int ULONG;

This doesn't make sense to me ;)

But it does ;-) So the nomenclature here can be confusing to people who know only one of the platforms’ rules

Sorry for being terse in my previous review, I was in a rush but wanted to give an answer ;).

I was quite aware of the fact that different platforms have differently-sized primitives. Changing from 'long' to 'int' because of the common interpretation on modern hardware also seems fine. What didn't make sense to me was keeping the name ULONG for an unsigned int. I didn't realize that this is some Microsoft(?) standard(?).

Given that background I think this change is a good improvement, though I'd personally prefer moving to a more explicit name such as uint32_t. A link to the convention in a comment would have been helpful but not essential.

It'd be great to have a testcase for the fixed decompression.

I guess this change also means the library is now broken for 32-bit systems. Given how common 64-bit systems are nowadays I think that is fine, though I'd welcome a patch making it work for both ;)

[mirabilos]

Hi Arnout,

Sorry for being terse in my previous review, I was in a rush but wanted to give an answer ;).

no problem, this way I was able to explain more quickly as well.

I was quite aware of the fact that different platforms have differently-sized primitives. Changing from 'long' to 'int' because of the common interpretation on modern hardware also seems fine. What didn't make sense to me was keeping the name ULONG for an unsigned int. I didn't realize that this is some Microsoft(?) standard(?).

I vaguely recalled it was also a Microsoft thing when writing the answer to you. It’s not limited to that. On 16-bit systems (such as DOS, CP/M-86, and probably other OSes from that time) you got 16 bit from int and 32 bit from long, because 32 bit had to be emulated and used up two CPU registers, so for something originating from such sy‐ stems this was natural.

Given that background I think this change is a good improvement, though I'd personally prefer moving to a more explicit name such as uint32_t.

For modern code, definitely. The question here is whether there’s a spec for sfArk somewhere that specifies this or something, and which one would (might) wish to stay close to. On the other hand, uint32_t is better as it won’t change even when we get to 128-bit systems…

A link to the convention in a comment would have been helpful but not essential.

Yeah, sorry, I was also a bit in a hurry, I was tracking this down in the hope of fixing sfArk conversion in Polyphone (though it turned out that it, too, suffers from issues wrt. endianness, but in Polyphone they’re hardcoded, which is worse).

It'd be great to have a testcase for the fixed decompression.

I’ll ask, I used testcases from the Polyphone author and will have to enquire about the licence etc.

I guess this change also means the library is now broken for 32-bit systems.

No, of course NOT! How would you even come to think that?

Given how common 64-bit systems are nowadays I think that is fine,

It’d be absolutely NOT fine! 32-bit systems are the majority of target platforms in Debian and absolutely MUST be supported.

In fact, #16 specifically fixes i386 (32-bit x86) and probably m68k (32-bit Motorola 68k) and no other architecture (that I know of).

bye, //mirabilos -- 21:12⎜ sogar bei opensolaris haben die von der community so ziemlich jeden mist eingebaut │ man sollte unices nich so machen das desktopuser zuviel intresse kriegen │ das macht die code base kaputt 21:13⎜<Vutral:#MirBSD> linux war früher auch mal besser :D

[raboof]

The question here is whether there’s a spec for sfArk somewhere that specifies this or something, and which one would (might) wish to stay close to

Unfortunately as far as I'm aware there is none.

I guess this change also means the library is now broken for 32-bit systems.

No, of course NOT! How would you even come to think that?

Sorry, I was confused - I meant systems on which the 'unsigned int' is 16 bits wide.

[mirabilos]

Arnout Engelen dixit:

I guess this change also means the library is now broken for 32-bit systems.

No, of course NOT! How would you even come to think that?

Sorry, I was confused - I meant systems on which the 'unsigned int' is 16 bits wide.

OK, those.

True. But then, if there’s really someone who wants to target DOS or 16-bit Windows (pre-95), they can ask and we’d probably be able to cobble together patches.

[raboof]

if there’s really someone who wants to target DOS or 16-bit Windows (pre-95), they can ask and we’d probably be able to cobble together patches.

Jup, agree