[gentoo-dev] Re: [gentoo-commits] gentoo-x86 commit in games-board/stockfish: stockfish-6.ebuild metadata.xml Manifest ChangeLog

[Ben de Groot]

On 7 February 2015 at 00:59, hasufell <hasufell@g.o> wrote:
> Ben de Groot (yngwin):
>> yngwin      15/02/05 20:09:33
>>
>>   Added:                stockfish-6.ebuild metadata.xml Manifest ChangeLog
>>   Log:
>>   Initial commit (bug #318337)
>>

First off: thank you for the review!

>>
>> EAPI=5
>> inherit toolchain-funcs
>>
>
> This breaks consistency. Now users cannot rely on games.eclass anymore.
> We should either abandon it completely or follow it consistently.

I thought we had abandoned it already?

Is there anything to be gained from using games.eclass here? It is a
chess engine that simply installs one file in /usr/bin/.

>
>> DESCRIPTION="The strongest chess engine in the world"
>
> This isn't very informative. I'd suggest
> DESCRIPTION="Free UCI chess engine derived from Glaurung 2.1"

I just took the description from upstream's website. But you are
right, it could be more informative. Will fix.

>> HOMEPAGE="http://stockfishchess.org/"
>
> Probably add the github link here too.

I will unify the live and release ebuilds.

>> SRC_URI="https://stockfish.s3.amazonaws.com/${P}-src.zip"
>>
>> LICENSE="GPL-3"
>> SLOT="0"
>> KEYWORDS="~amd64 ~x86"
>> IUSE="cpu_flags_x86_avx2 cpu_flags_x86_popcnt cpu_flags_x86_sse"
>>
>> DEPEND=""
>
> unzip is missing from DEPEND

I thought portage auto-depends on this. But I can add || ( unzip zip )
to be explicit.

>> RDEPEND=""
>>
>> S=${WORKDIR}/${P}-src/src
>>
>> src_prepare() {
>>       sed -e 's:-strip $(BINDIR)/$(EXE)::' -i Makefile
>> }
>>
>
> missing "|| die"... I'd also rather make this a patch, so it doesn't
> silently break on version bump

The die would not accomplish anything, unless the file wasn't there
anymore. I thought this was too simple for a patch, but see below.

>> src_compile() {
>>       local my_arch
>>       use x86 && my_arch=x86-32-old
>>       use cpu_flags_x86_sse && my_arch=x86-32
>>       use amd64 && my_arch=x86-64
>>       use cpu_flags_x86_popcnt && my_arch=x86-64-modern
>>       use cpu_flags_x86_avx2 && my_arch=x86-64-bmi2
>>
>>       emake build ARCH=${my_arch} CXX="$(tc-getCXX)" CXXFLAGS="${CXXFLAGS}"
>
> This currently breaks all cpu flags, because it overwrites anything the
> Makefile does to CXXFLAGS, including -msse and -DIS_64BIT and even other
> flags that are not CPU specific (e.g. -DNDEBUG).

Thanks for catching this! I guess we do need to patch the Makefile
then, to *also* honour user-set CXXFLAGS and LDFLAGS. Or could we get
away with just letting the Makefile do its thing?

> Fixing this should definitely be done in a revbump.

Obviously. Will do.

-- 
Cheers,

Ben | yngwin
Gentoo developer