On 21 February 2015 at 16:18, Michał Górny <mgo...@gentoo.org> wrote: > Hi, > > Don't you think it sucks to review a few ebuilds in one e-mail? :)
No. :) > neovim: > >> # Copyright 1999-2015 Gentoo Foundation >> # Distributed under the terms of the GNU General Public License v2 >> # $Header: $ >> >> EAPI=5 >> inherit cmake-utils flag-o-matic >> >> DESCRIPTION="Vim's rebirth for the 21st century" >> HOMEPAGE="https://github.com/neovim/neovim"; >> if [[ ${PV} == 9999 ]]; then >> inherit git-r3 >> EGIT_REPO_URI="git://github.com/neovim/neovim.git" >> KEYWORDS="" >> else >> inherit vcs-snapshot >> COMMIT="8efb3607a7f6cefce450953c7f8d5e3299347bae" >> SRC_URI="https://github.com/${PN}/${PN}/tarball/${COMMIT} -> >> ${P}.tar.gz" > > I don't think relying on stability of generated tarballs is a good > idea. The same applies to almost all other ebuilds. Do we often run into trouble with that? I know it's not perfect, but it should be temporary, until we get regular releases. >> KEYWORDS="~amd64 ~x86" >> fi >> >> LICENSE="Apache-2.0 vim" >> SLOT="0" >> IUSE="perl python" >> >> CDEPEND="dev-lang/luajit >> >=dev-libs/libtermkey-0.17 >> >=dev-libs/unibilium-1.1.1 >> >=dev-libs/libuv-1.2.0 >> >=dev-libs/msgpack-0.6.0_pre20150220 > > Accidentally found trailing whitespace here! Not present in the actual ebuild. >> dev-lua/LuaBitOp >> dev-lua/lpeg >> dev-lua/lua-MessagePack" >> DEPEND="${CDEPEND} >> virtual/libiconv >> virtual/libintl" >> RDEPEND="${CDEPEND} >> perl? ( dev-lang/perl ) >> python? ( dev-python/neovim-python-client )" >> >> src_configure() { >> append-cflags "-Wno-error" >> append-cppflags "-DNDEBUG -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1" >> local mycmakeargs=( -DCMAKE_BUILD_TYPE=Release ) > > That looks like a very bad idea. Doesn't that disable all the Gentoo > fancy overrides needed for sane CC/CXX etc.? Any other way to do that then? >> cmake-utils_src_configure >> } > > And in the end it fails to build with some linker errors like: > > CMakeFiles/nvim.dir/tui/tui.c.o: In function `tui_set_scroll_region': > tui.c:(.text+0xa2): undefined reference to `unibi_get_str' > CMakeFiles/nvim.dir/tui/tui.c.o: In function `unibi_set_if_empty': > tui.c:(.text+0x40c): undefined reference to `unibi_get_str' > > or for ncurses, if libtermkey was linked against ncurses. Long story > short, it's linking to -ltermkey statically without providing > -lunibilium before it... which (static linking) is a horrible thing > to do anyway. WFM, but yeah it's not ideal. I'll contact upstream about it. > libtermkey: > >> # Copyright 1999-2015 Gentoo Foundation >> # Distributed under the terms of the GNU General Public License v2 >> # $Header: $ >> >> EAPI=5 >> inherit eutils multilib >> >> DESCRIPTION="Library for easy processing of keyboard entry from >> terminal-based programs" >> HOMEPAGE="http://www.leonerd.org.uk/code/libtermkey/"; >> SRC_URI="http://www.leonerd.org.uk/code/${PN}/${P}.tar.gz"; >> >> LICENSE="MIT" >> SLOT="0" >> KEYWORDS="~amd64 ~x86" >> IUSE="demos" >> >> RDEPEND="|| ( dev-libs/unibilium >> sys-libs/ncurses[unicode] )" > > No, no, no, no, no and no. This dependency is meaningless, and broken. > > You're looking for either: > > # ignore ncurses since neovim will pull in unibilium anyway, > # and then libtermkey will prefer it > RDEPEND="dev-libs/unibilium:=" > > or a USE flag to toggle the two. No auto-magic || () is doing. Since neovim upstream now moved completely from ncurses to unibilium, I guess it makes sense to just do that here too. >> DEPEND="${RDEPEND} >> sys-devel/libtool > > Using system-wide libtool is horrendously broken. This is something for > upstream to fix (like they should start using a sane build system) > but if you really want to commit it like this, already open a bug alike > https://bugs.gentoo.org/show_bug.cgi?id=515554. I'll report upstream. Anything else we can do in the meantime? > The same applies to unibilium. > >> virtual/pkgconfig >> demos? ( dev-libs/glib:2 )" >> >> src_prepare() { >> if ! use demos; then >> sed -e 's|all: $(LIBRARY) $(DEMOS)|all: $(LIBRARY)|' -i >> Makefile || die > > sed -e '/^all:/s:$(DEMOS)::' ... > >> fi >> } >> >> src_compile() { >> emake PREFIX="${EPREFIX}/usr" all > > You need LIBDIR here too, otherwise the executable gets wrong rpath. ok > The same applies to unibilium. Also unibilium doesn't respect CC here. > >> } >> >> src_install() { >> emake PREFIX="${EPREFIX}/usr" LIBDIR="${EPREFIX}/usr/$(get_libdir)" \ >> DESTDIR="${D}" install >> prune_libtool_files >> } > > neovim-python-client: > >> # Copyright 1999-2015 Gentoo Foundation >> # Distributed under the terms of the GNU General Public License v2 >> # $Header: $ >> >> EAPI=5 >> PYTHON_COMPAT=( python{2_7,3_3,3_4} pypy ) >> inherit distutils-r1 >> >> DESCRIPTION="Python client to connect to Neovim thru its msgpack-rpc API" >> HOMEPAGE="https://github.com/neovim/python-client"; >> SRC_URI="https://github.com/neovim/python-client/archive/0.0.28.tar.gz -> >> ${P}.tar.gz" > > Hardcoded PV. And obviously github generated tarball :). Oops! That was an oversight. >> >> LICENSE="Apache-2.0" >> SLOT="0" >> KEYWORDS="~amd64 ~x86" >> IUSE="" >> >> DEPEND=">=dev-python/click-3.0 >> >=dev-python/msgpack-0.4.0 >> !python_targets_pypy? ( dev-python/greenlet ) >> !python_targets_python3_4? ( dev-python/trollius )" > > Whaaaaaaaaaaaaaaaaat?! What are those negative deps supposed to mean? We need pypy OR greenlet, and python-3.4 OR trollius. This was the simplest way I saw to express that. > I'm pretty sure you meant: > > $(python_gen_cond_dep 'python*' 'dev-python/greenlet[${PYTHON_USEDEP}]') > $(python_gen_cond_dep python{2_7,3_2,3_3} 'pypy*' > 'dev-python/trollius[${PYTHON_USEDEP}]') Hmm, black magic?! Tasty! After perusing the grimoire--I mean eclass--I get the idea. Thanks for the hint! >> RDEPEND="${DEPEND}" >> >> S=${WORKDIR}/${P/neovim-/} > > > -- > Best regards, > Michał Górny Thanks for reviewing! -- Cheers, Ben | yngwin Gentoo developer
