freeimage: fix Big-endian build

[barracuda156]

Fixes: https://trac.macports.org/ticket/69232

Description

Fix errors in the code. Looks like no-one ever tried to actually build it in upstream.

OpenEXR fix is borrowed from OpenEXR upstream code. Other two fix errors which are merely a result of poor design and lack of trying to compile the code.

Type(s)

• bugfix
• enhancement
• security fix

Tested on

macOS 10.6
Xcode 3.2

Verification

Have you

• followed our Commit Message Guidelines?
• squashed and minimized your commits?
• checked that there aren't other open pull requests for the same change?
• referenced existing tickets on Trac with full URL?
• checked your Portfile with port lint --nitpick?
• tried existing tests with sudo port test?
• tried a full install with sudo port -vst install?
• tested basic functionality of all binary files?
• checked that the Portfile's most important variants haven't been broken?

[ryandesign]

That's not upstream. Please check https://trac.macports.org/ticket/67489 where I posted the upstream bug URLs. There were multiple proposals for resolving this bug submitted in addition to a different one that was committed to their repository. I asked in the upstream bug which fix is correct and there was no reply so you might at least want to document which of the three solutions (or a fourth one?) you chose any why that's the correct one.

[barracuda156]

@ryandesign Thank you, and sorry that I did not check for existing tickets (and forgot there was mine). My patch looks identical to the one here: https://sourceforge.net/p/freeimage/patches/153
Assuming that original code from upstream was meaningful, this looks correct.

In addition, however, patching OpenEXR part is required, but that should be non-controversial, I literally borrowed it from OpenEXR master.

[barracuda156]

Take a look here: https://github.com/WinMerge/freeimage/blob/0e3616264c13c713e99e4723acdafa12cadf605b/Source/FreeImage/PluginDDS.cpp#L131-L167 (just for the sake of convenience, we know it is a clone).
And this: https://github.com/WinMerge/freeimage/blob/0e3616264c13c713e99e4723acdafa12cadf605b/Source/FreeImage/PluginDDS.cpp#L254-L255
Those two combined with the error you get trying to build it for BE arch explain my fix here: https://github.com/WinMerge/freeimage/blob/0e3616264c13c713e99e4723acdafa12cadf605b/Source/FreeImage/PluginDDS.cpp#L345-L373 (and yeah, turned out it was suggested much earlier and I should have just checked that ticket).
I think upstream “ported the code to BE” on paper and never bothered to compile it.

[pmetzger]

If you guys could let me know when this is ready, I'd appreciate it.

[barracuda156]

@pmetzger I guess let’s Ryan decide; upstream does not seem to bother for years, pretty unlikely they gonna respond now.

I do not think it is preferable to have the build broken indefinitely though. The only point which we are not sure about is a necessity of byte-swapping for Big-endian platforms in one of the plugins. It the worst case, assuming for the sake of argument that the fix is incorrect (notice, at least two other people suggested it earlier, independently), that plugin won’t work as expected on PowerPC. Something like colors gonna be wrong. But this was and likely is still broken for years in far more important software.

(Of course, if there is a better fix for this, it should be used instead.)

[pmetzger]

@barracuda156 @ryandesign How do we move this forward?

[barracuda156]

@pmetzger Well, this at least fixes the build and anyway is relevant only for Big-endian archs. If Ryan or someone else proposes a better solution, we can go with that. However that can also be done at any point later on. I do not think there is any reason to have it pending until upstream replies (which likely means it never gets fixed).

[pmetzger]

If the patches only applied on PPC, I could at least know that I wasn't going to break anything on x86 or ARM by merging.

[barracuda156]

If the patches only applied on PPC, I could at least know that I wasn't going to break anything on x86 or ARM by merging.

OpenEXR patch is verifiably just a copy paste from OpenEXR upstream (and it should actually fix i386 too), other patches apply to the chunks of code which are Big-endian (should be visible from macros in the source).

Generally speaking, it is preferable to have patches unconditional for the sake of sane rebasing. But Big-endian patches are fine to be made conditional: they will have the same effect regardless. OpenEXR patch is rather for 32-bit, but I leave it to you to decide: I don’t care that much about fixing 32-bit Intel, surely not if there is any opposition to that. In the latter case it can also go inside platform powerpc.

[ryandesign]

I don't have time to re-visit every PR or ticket I comment on and I don't need to be the one to decide everything.

I already pointed out that the ticket URL added in a portfile comment is not upstream, by which I meant to suggest that that URL should be replaced with the upstream bug report URLs.

I already pointed out one upstream solution that was committed and two alternates that were submitted in tickets, by which I meant to suggest that the URL of whichever solution was employed should be listed at the top of the patchfile like we customarily do.

If one of your patches comes from an upstream OpenEXR commit, put that commit URL at the top of that patchfile so we know where it came from.

[barracuda156]

@pmetzger @ryandesign I have updated links in portfile and commit body.