Created apng2gif recipe #910

R167 · 2016-12-05T06:44:31Z

Related to gif2apng recipe.

fbrosson · 2016-12-05T06:49:00Z

Thanks!
I'm preparing a review right now, with some polishing ;-)

fbrosson

Looks OK, but a little polishing would be nice ;-)

fbrosson · 2016-12-05T06:50:33Z

media-gfx/apng2gif/apng2gif-1.7.recipe

+REQUIRES="
+	haiku$secondaryArchSuffix
+	lib:libz$secondaryArchSuffix
+	lib:libpng$secondaryArchSuffix


That's not required, by we like to sort alphabetically the lib:* lines in REQUIRES.

fbrosson · 2016-12-05T06:50:57Z

media-gfx/apng2gif/apng2gif-1.7.recipe

+BUILD_REQUIRES="
+	haiku${secondaryArchSuffix}_devel
+	devel:libz$secondaryArchSuffix
+	devel:libpng$secondaryArchSuffix


That's not required, by we like to sort alphabetically the devel:* lines in BUILD_REQUIRES.

fbrosson · 2016-12-05T06:53:26Z

media-gfx/apng2gif/apng2gif-1.7.recipe

+INSTALL()
+{
+	mkdir -p $prefix/bin
+	cp apng2gif $prefix/bin


Could you please use the install command, instead of mkdir and cp?
We have a recipe which you can get inspiration from:
app-misc/hexcompare/hexcompare-1.0.4.recipe

R167 · 2016-12-05T07:00:19Z

Sorry about that, just tacked the deps onto the end of the list cause I was rushing. Also, updated where I had previously done the cp in gif2apng. Would that change require a bump to revision?

fbrosson · 2016-12-05T07:08:51Z

LGTM.
I just need to build it and I'll merge this PR and validate your task.

Regarding the revision bump for cosmetic changes in other recipes, there is no need to bump if the resulting package is identical.
(But we usualy don't edit a single recipe with just cosmetic edits.)

If you wish, however, you might claim this task:
Get the Lint out of 5 Haikuports recipes (1)

fbrosson · 2016-12-05T07:10:50Z

media-gfx/apng2gif/apng2gif-1.7.recipe

+BUILD_PREREQUIRES="
+	cmd:gcc$secondaryArchSuffix
+	cmd:make
+	cmd:ld$secondaryArchSuffix


Oops, I missed this one ;-)
That's not required, by we like to sort alphabetically the cmd:* lines in BUILD_PREREQUIRES.

R167 · 2016-12-05T07:11:18Z

Sure thing. Only made that change because I had just copied the majority of that recipe over and thought it best to use a more standardized process than the mkdir && cp workflow. Will fix that latest one too. :-)

fbrosson · 2016-12-05T07:13:13Z

Hmm, that's a very good idea to fix apng2gif with gif2apng! Well done!

fbrosson · 2016-12-05T07:18:59Z

May I suggest an alternate commit message? What about this:

apng2gif: new recipe. gif2apng: a bit of polishing.

Hmm, you may need to add cmd:install to BUILD_PREREQUIRES. I need to figure this out.
If cmd:install is provided by the coreutils hpkg then it is not required. If not, then it should probably be added here.

R167 · 2016-12-05T07:26:57Z

just checked and everything went fine without cmd:install. Also, just squashed commits and updated primary commit message.

fbrosson · 2016-12-05T07:29:04Z

media-gfx/apng2gif/apng2gif-1.7.recipe

+else
+	commandSuffix=
+	commandBinDir=$prefix/bin
+fi


Hmm, let's drop this block (in both apng2gif and gif2apng).
There are several reasons for this:

We are not using commandSuffix.

We are only using commandBinDir in one place, INSTALL, and we can replace $commandBinDir by $prefix/bin for this recipe on all architectures, both primary and secondary.

This recipe does not provide any lib, so after we drop x86_gcc2 (some time in the future) we will be able to completely drop support for secondary architectures for this recipe.

Sure thing. Was initially going that route, but wasn't sure if you were going to end up having me add it.

fbrosson · 2016-12-05T07:31:42Z

OK, since you have successfully tested without cmd:install, I guess we don't need it in BUILD_PREREQUIRES.

fbrosson · 2016-12-05T07:35:04Z

Looks OK. May I ask you a final touch?
It would be nice to drop the detailed commit message, I mean, only keep a one-line commit title.

fbrosson · 2016-12-05T07:42:59Z

Cool! Thanks for the update of the commit message.

This recipe was really easy, wasn't it?
BTW, if you claim all the easy ones there won't be any left for the other students!
Would you mind taking a few hard tasks, from time to time?
We've got a few that might be hard (but might turn out to be not very hard): libdmtx, libusbmuxd, iec16022. Thanks!

R167 · 2016-12-05T07:47:14Z

Sure thing. I just did this one cause I just worked on gif2apng recipe and knew about its weird quirks (Source_dir needs to be an empty string and all the fun oddities for make), so I just figured I would do it. Definitely want to look more into the library ones if only to better understand how they work for linking, etc. Will do! :)

fbrosson · 2016-12-05T07:47:45Z

Oops, it would be better, after all, to bump REVISION (for gif2apng only, of course) because now the package includes the readme.txt file in $docDir.
(Sorry, I missed this, or maybe your question came before I had mentionned using cmd:install.)

R167 · 2016-12-05T07:48:45Z

done.

fbrosson · 2016-12-05T08:03:04Z

Thanks!
I'm having a look at the Makefile and it seems that the reason why it does not build on x86_gcc2 primary arch out of the box is that it links against libstdc++, which we have on x86 and x86_64, but which is missing on x86_gcc2 (to be precise, it is available but with a slightly different name: libstdc++.r4.)

I did workaround this issue myself on another package (not yet in the repo) which has the same dependency, but did never submit it because it still needs some polishing.

Would you mind if I ask @korli or @pulkomandy if it would be desirable to try to make apng2gif build on x86_gcc2 primary arch?

R167 · 2016-12-05T08:11:09Z

I don't mind. We could set it up so that it uses the correct one depending on target architecture, but that sounds like excessive complexity if the intention is to EOL gcc2 soon (not like it was already EOL everywhere else :)). I remember when I found that before, but ended up not using it because of a C99 requirement or something like that. IDK: I am tired and rambly right now. Judgement call is up to you.

Also, where can I find docs for adding libs in haikuports? Didn't see anything specific in the wiki, and I am looking at tackling the libp11 task next. If not any docs, any particular recipe/example to look at? (Biggest thing I really don't know how to do is going to be how to test if I did everything right). Thanks again, @fbrosson!

fbrosson · 2016-12-05T08:11:55Z

This is what we might try:

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

And in the patch we would replace -lstdc++ by $(LIB_STDCPP).
And in BUILD we would replace make by

LIB_STDCPP=$LIB_STDCPP make

Reminder: I don't know if this would desirable, so you might wish to wait for @korli's or @pulkomandy's or @mmuman's opinion about this.

fbrosson · 2016-12-05T08:16:19Z

Hmm, I wonder if this would work, without any patch:

if [ "$effectiveTargetArchitecture" = x86_gcc2 ]; then
	LIBS="-lstdc++.r4 -lpng -lz"
else
	LIBS="-lstdc++ -lpng -lz"
fi

pulkomandy · 2016-12-05T08:20:43Z

The proper way would be to use g++ as a linker, instead of calling ld or gcc directly. g++ knows which compiler flags to add so there is no need for an explicit -lstdc++.
I don't know if it is possible with the buildsystem here, if it is not, using the effective target arch seems the only way.

fbrosson · 2016-12-05T08:35:58Z

Since the Makefile defines CC=gcc, I thought it would be possible, with CC=g++ make or with make CC=g++, but none of these work:

/sources/apng2gif.cpp: In function `int load_apng(char *, vector<APNGFrame,allocator<APNGFrame> > &, unsigned int *)':
/sources/apng2gif.cpp:349: warning: comparison between signed and unsigned
/packages/gcc-2.95.3_2014_10_14-3/.self/develop/tools/i586-pc-haiku/bin/ld: cannot find -lstdc++
collect2: ld returned 1 exit status
make: *** [Makefile:8: all] Error 1

I tried, however, to use this (on x86_gcc2 primary arch):

make LIBS="-lstdc++.r4 -lpng -lz"

and it works perfectly!
Shall we drop SECONDARY_ARCHITECTURES and $secondaryArchSuffix and, instead, use the little conditional setting for LIBS?

fbrosson · 2016-12-05T08:38:50Z

BTW, I forgot to thank you for your reply!!!
(I was too excited with testing the LIBS= workaround ;-)

fbrosson · 2016-12-05T08:47:27Z

media-gfx/apng2gif/apng2gif-1.7.recipe

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


Shall use the LIBS= workaround?
If I'm not mistaken, this is what we need to do:

replace:

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

by

ARCHITECTURES="x86_gcc2 x86 x86_64" if [ "$effectiveTargetArchitecture" = x86_gcc2 ]; then LIBS="-lstdc++.r4 -lpng -lz" else LIBS="-lstdc++ -lpng -lz" fi

drop all occurences off $secondaryArchSuffix;

replace $prefix/bin by $binDir (in INSTALL)

(But let's see what our other contibutors thing about this.)

Oops, I forgot to mention we also need to change BUILD to:

make LIBS="$LIBS"

@korli, @pulkomandy, @mmuman, do we have green lights for this tiny workaround that allows to build apng2gif (and also gif2apng) on x86_gcc2 primary arch? I guess we have, but I'd like to be sure ;-) Thanks!
(I'm asking because I think @R167 is waiting for out green lights to do the change, as I suggested we should wait for your thoughts about it.)

yes, looks fine.

Cool, thanks @pulkomandy!

@R167, can you please update this PR with the changes ~~(for both recipes)~~?

EDIT: Although apng2gif builds fine on x86_gcc2 primary arch, gif2apng, on the other side, still does not (due to the zopfli stuff). So for gif2apng the only thing we can do is the polishing.

For the commit title I suggest ~~keeping~~ chaging it to:

apng2gif: new recipe. gif2apng: polishing.

got it. will do so tomorrow.

OK. fine.
Let me update the GCI task with a "Request More Work", otherwise we might forget and Google will think Haiku is not responding quickly enough ;-)

Oops, the commit title I suggested earlier is not very nice, after all.
So when you update this PR could you please replace

apng2gif: new recipe. gif2apng: a bit of polishing.

by

apng2gif: new recipe. gif2apng: polishing."

(My "a bit of" was a bit silly, wasn't it ;-)
Thanks!

LIBS works great with apng2gif, but gif2apng errors out on undefined references (i did make sure that it was using the right version). See error below. (added in patches for that, but it did NOT have the PATCHES var set. changes didn't succeed in fixing it anyways)

R167 · 2016-12-05T20:34:17Z

these changes work for apng2gif, but the libs override doesn't work with gif2apng, erroring out on:

gcc -Wall -pedantic -s -o gif2apng obj/./gif2apng.o obj/7z/WindowIn.o obj/7z/LZMA.o obj/7z/CRC.o obj/7z/DeflateDecoder.o obj/7z/LZMAEncoder.o obj/7z/LSBFDecoder.o obj/7z/IInOutStreams.o obj/7z/7zdeflate.o obj/7z/HuffmanEncoder.o obj/7z/LZMADecoder.o obj/7z/7zlzma.o obj/7z/AriBitCoder.o obj/7z/LSBFEncoder.o obj/7z/LiteralCoder.o obj/7z/LenCoder.o obj/7z/WindowOut.o obj/7z/InByte.o obj/7z/DeflateEncoder.o obj/7z/OutByte.o obj/zopfli/zlib_container.o obj/zopfli/cache.o obj/zopfli/util.o obj/zopfli/hash.o obj/zopfli/katajainen.o obj/zopfli/tree.o obj/zopfli/gzip_container.o obj/zopfli/lz77.o obj/zopfli/squeeze.o obj/zopfli/zopfli_lib.o obj/zopfli/blocksplitter.o obj/zopfli/deflate.o -lstdc++.r4 -lm -lz
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'
collect2: ld returned 1 exit status
Makefile:20: recipe for target 'gif2apng' failed

Latest commit rollsback changes to gif2apng (since they were previously bundled).
Thoughts, @fbrosson?

fbrosson · 2016-12-05T22:01:37Z

Everything is OK.
Although I can squash your commits "on the fly before the merge", if you squash your commits yourself before the merge it will look much nicer.
I guess you were waiting for the signal to squash, well, you got it ;-)
I've just validated your task on GCI, but don't forget to squash your commits here ;-)
Thanks!

R167 · 2016-12-05T22:22:31Z

Sure thing. Will squash for you now. Was just waiting for your signal in case I needed to revert anything. :)

fbrosson · 2016-12-05T22:34:32Z

That's perfect! Thanks!

fbrosson · 2016-12-19T11:59:17Z

@R167, I wrote this 2 weeks ago because you were by far the most active student at that time.

Would you mind taking a few hard tasks, from time to time?

But it was not fair (from my side) to suggest that you took harder tasks, even only from time to time.
So please ignore that old suggestion and, instead, do feel free to claim any task you wish!
BTW, I'd love to see more recipes claimed ;-)

fbrosson reviewed Dec 5, 2016

View reviewed changes

R167 force-pushed the apng2gif branch from 15fb35d to b199720 Compare December 5, 2016 07:12

R167 force-pushed the apng2gif branch from d27dc93 to 9ca69d7 Compare December 5, 2016 07:25

fbrosson reviewed Dec 5, 2016

View reviewed changes

R167 force-pushed the apng2gif branch from 9ca69d7 to 2907bcf Compare December 5, 2016 07:32

R167 force-pushed the apng2gif branch from 2907bcf to 7f7e585 Compare December 5, 2016 07:36

R167 force-pushed the apng2gif branch from 7f7e585 to 7f30109 Compare December 5, 2016 07:48

fbrosson reviewed Dec 5, 2016

View reviewed changes

R167 force-pushed the apng2gif branch from 7f30109 to b6e7996 Compare December 5, 2016 20:16

R167 force-pushed the apng2gif branch from 3de3aa2 to 7498c63 Compare December 5, 2016 20:44

apng2gif: new recipe. gif2apng: polishing.

b0e49d0

R167 force-pushed the apng2gif branch from 04cbd3f to b0e49d0 Compare December 5, 2016 22:24

fbrosson merged commit 39f3ed7 into haikuports:master Dec 5, 2016

R167 deleted the apng2gif branch December 9, 2016 05:37

Created apng2gif recipe #910

Created apng2gif recipe #910

Conversation

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016

fbrosson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016

Choose a reason for hiding this comment

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016

fbrosson commented Dec 5, 2016

R167 commented Dec 5, 2016

fbrosson Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrosson commented Dec 5, 2016

fbrosson commented Dec 5, 2016

fbrosson commented Dec 5, 2016

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016 • edited

fbrosson commented Dec 5, 2016

pulkomandy commented Dec 5, 2016

fbrosson commented Dec 5, 2016

fbrosson commented Dec 5, 2016

fbrosson Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbrosson Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R167 commented Dec 5, 2016 • edited

fbrosson commented Dec 5, 2016 • edited

R167 commented Dec 5, 2016

fbrosson commented Dec 5, 2016

fbrosson commented Dec 19, 2016

fbrosson Dec 5, 2016 •

edited

fbrosson commented Dec 5, 2016 •

edited

fbrosson Dec 5, 2016 •

edited

fbrosson Dec 5, 2016 •

edited

R167 commented Dec 5, 2016 •

edited

fbrosson commented Dec 5, 2016 •

edited