Gif2apng

[R167]

Added in gif2apng recipe. Does not support gcc2.

[fbrosson]

There are a couple of things to change. I'm writing a review.

[fbrosson]

Changes to allow installation to bin/ instead of bin/x86/.

BTW, you added a patch but you did not reference it. I guess it was for x86_gcc2 only.
I'll try to see if we can make it build on x86_gcc2 primary arch with your patch.

[fbrosson]

Then we drop the $secondaryArchSuffix from cmd:gif2apng$secondaryArchSuffix to let haikuporter know that it should not expect anything in $prefix/bin/x86/, since we're installing to $prefix/bin/.
You should therefore have:

	cmd:gif2apng = $portVersion

[fbrosson]

Should be:

        mkdir -p $commandBinDir
        cp gif2apng $commandBinDir

The reason why we prefer using cp instead of mv is that mv would break haikuporter -F.

[fbrosson]

If a recipe does not build on x86_gcc2 primary arch then we usually install to $prefix/bin instead of $binDir (which is $prefix/bin/x86 on x86 secondary arch).
For that purpose we define commandBinDir with these lines between SECONDARY_ARCHITECTURES and PROVIDES:

if [ "$targetArchitecture" != x86_gcc2 ]; then
	commandBinDir=$binDir
else
	commandBinDir=$prefix/bin
fi

(You can copy that block from haikuports/media-video/dvdauthor/dvdauthor-0.7.1.recipe)

[R167]

forgot to add the patch to the recipe file. Fixing.

[mmuman]

Doesn't it have a make install ?
I'll let @fbrosson list the rest.

[R167]

@mmuman no, just generates the binary in the main directory. no install task defined in the makefile

[mmuman]

Ok then.

[fbrosson]

Cosmetic changes for DESCRIPTION and SOURCE_URI.

[fbrosson]

We can probably remove the "CLI version is OS-independent."

[fbrosson]

I forgot to suggest a slightly better SOURCE_URI:

SOURCE_URI="https://downloads.sourceforge.net/gif2apng/gif2apng-$portVersion-src.zip"

[R167]

just pushed updates @fbrosson. Thoughts?

[fbrosson]

Sorry for the delay. I was trying to build on x86_gcc2 primary arch with a variant of your patch.
The idea was to replace -lstdc++ by $(LIB_STDCPP) in the Makefile with the patch, and then define LIB_STDCPP with:

if [ "$effectiveTargetArchitecture" = x86_gcc2 ]; then
	PATCHES="gif2apng-$portVersion-lstdc++.patchset"
	LIB_STDCPP=-lstdc++.r4
else
	LIB_STDCPP=-lstdc++
fi

in order to be able to have this in BUILD:

	LIB_STDCPP=$LIB_STDCPP make

Unfortunately it does not work: I get undefined reference to __builtin_clz:

obj/zopfli/util.o: In function `ZopfliGetDistExtraBits':
util.c:(.text+0x21): undefined reference to `__builtin_clz'
obj/zopfli/util.o: In function `ZopfliGetDistExtraBitsValue':
util.c:(.text+0x5c): undefined reference to `__builtin_clz'
obj/zopfli/util.o: In function `ZopfliGetDistSymbol':
util.c:(.text+0xac): undefined reference to `__builtin_clz'

So I guess the best option is to drop x86_gcc2 as primary arch. (That's what you did ;-)
I don't know, however, if we should have:

ARCHITECTURES="!x86_gcc2 x86 x86_64"
SECONDARY_ARCHITECTURES="x86"

or

ARCHITECTURES="x86 x86_64"
SECONDARY_ARCHITECTURES="x86"

(I think we should have the !x86_gcc2 but I'm not sure.)

[R167]

@fbrosson yeah, that was the error I kept running into with GCC2. the issue is that zopfli requires later than GCC4. might just not include the x86_gcc2 for that reason, but up to you.

[fbrosson]

It would be better without the echo $commandBinDir ;-)

Everything else looks OK. I'll build it on each of my architectures and then merge.

[R167]

WHOOPS! completely forgot about that. doh!

[fbrosson]

While at it, can you also change ARCHITECTURES to:

ARCHITECTURES="!x86_gcc2 x86 x86_64"

Thanks!

[R167]

okay, fixed that now.

[R167]

okay, now it's fixed.

[fbrosson]

Maybe we should simplify INSTALL to have only:

INSTALL()
{
	mkdir -p $prefix/bin
	cp gif2apng $prefix/bin
}

I had suggested defining commandBinDir because we often do that, but since we don't even use it in BUILD (for this recipe) it does not really make sense to define commandBinDir just for INSTALL. Moreover, we just use it twice. (So sorry for the bad suggestion...)

[R167]

it's all good.

[fbrosson]

Great. Thanks! I'll validate your task asap.
Then I'll build the recipe on x86 and x86_64 before I merge.

[fbrosson]

It builds fine without cmd:automake so it would be nice to remove that dependency. Thanks!

[R167]

good now?

[fbrosson]

Yes, it's perfect ;-)

To HaikuPorts members: If you merge, please don't forget to schrink the commit message.
But don't worry. I'll merge it myself as soon as I have built it on all architectures.

[pulkomandy]

On Wed, Nov 30, 2016 at 05:41:01PM -0800, fbrosson wrote: (I _think_ we should have the **`!x86_gcc2`** but I'm not sure.)

Yes, it means "tested on x86_gcc2, does not work".

[fbrosson]

OK, thanks.

[fbrosson]

Now that this PR has been merged, the branch it is based on is no longer needed.
You should therefore click "Delete branch" for this PR to get rid of an unneeded branch in your repo. Thanks!
GitHub help: Deleting unused branches