On 7 February 2015 at 00:59, hasufell <hasuf...@gentoo.org> 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
