Hey there, Thanks for the feedback.
On Tue, Oct 1, 2019, at 02:20, Edd Barrett wrote: > Hi, > > Thanks for looking at this. > > On Sun, Sep 29, 2019 at 01:16:13PM -0700, Travis Cole wrote: > > * A new port of libluv: > > - Neovim now depends on libluv > > - (See https://github.com/neovim/neovim/wiki/Following-HEAD) so I had to > > create a new port which I've attached. > > Let's start with this ^ > > Should the package name be "libluv" or "luv"? Upstream seems to call it "luv". You're probably right. I've changed it to just "luv". > > > > PKGVER = 1.30.1.1 > > Since that's only used once, you might aswell inline it. Or even better, > to the replacement using make's regex support: > > PKGNAME = libluv-${VER:S/-/./g} Good idea. Fixed. > > BUILD_DEPENDS = devel/cmake \ > > devel/libuv > > > > LIB_DEPENDS = devel/libuv > > If libuv is a LIB_DEPEND, I don't think you need it as a BUILD_DEPEND as > well. Makes sense. DRY. Fixed. > > CONFIGURE_ARGS = -DWITH_SHARED_LIBUV=ON \ > > -DWITH_LUA_ENGINE=Lua \ > > -DLUA_BUILD_TYPE=System \ > > -DBUILD_MODULE=OFF \ > > -DBUILD_SHARED_LIBS=ON \ > > Indent is odd here. I'd make all the `-D`s line up. Yeah, sorry fixed. > In Makefile and pkg/DESCR we need to use the right capitalisation for Lua and > LuaJIT: > - s/luajit/LuaJIT/g > - s/lua/Lua/g Those were a copy paste from the upstream README.md on github, but I agree about making it have the proper capitalization. Fixed. > `make port-lib-depends-check` says: > ---8<--- > Extra: pthread.26 > --->8--- > > So you probably want to kill that from your WANTLIB. A little surprised I didn't catch that. But fixed. > I notice that the source tarball bundles some dependencies in `deps/` and > uses them once during the build: > ---8<--- > ===> Building for libluv-1.30.1.1 > [1/3] /usr/local/pobj/libluv-1.30.1.1/bin/cc -DLUA_USE_DLOPEN -Dluv_EXPORTS > -I/usr/local/include -I/usr/local/include/lua-5.1 > -I/usr/local/pobj/libluv-1.30.1.1/luv-1.30.1-1/deps/lua-compat-5.3 -O2 -pipe > -g -DNDEBUG -fPIC -MD -MT CMakeFile > s/luv.dir/src/luv.c.o -MF CMakeFiles/luv.dir/src/luv.c.o.d -o > CMakeFiles/luv.dir/src/luv.c.o -c /usr/local/pobj/liblu > v-1.30.1.1/luv-1.30.1-1/src/luv.c > --->8--- > > So it's using `deps/lua-compat-5.3`. I notice we have a port > devel/lua-compat53 > already. It'd be best if you can make your port use it. Perhaps we should `rm > -rf` the `deps/` directory in `pre-build:` so as to avoid accidental usage of > bundled deps? This one I'm not totally sure what to do about. I tried your advice and it it breaks the build with: /usr/obj/ports/luv-1.30.1.1/luv-1.30.1-1/src/luv.c:20:10: fatal error: 'c-api/compat-5.3.h' file not found #include "c-api/compat-5.3.h" ^~~~~~~~~~~~~~~~~~~~ lua-compat-5.3 doesn't install any headers, and no other PLIST file in ports has it either. I'm not sure if it's best to keep the deps dir? or are we talking about modifying lua-compat53 to install those headers? Or is there a simpler option that I'm missing? > And finally: > ---8<--- > fremen$ /usr/ports/infrastructure/bin/portcheck > trailing whitespace in Makefile > --->8--- Ah, I should have caught this one to. Fixed. > > Other than these, this looks good. If you mail me a revised diff, I'll start > looking for an OK. I also added: SEPARATE_BUILD = YES New tar attached. Thanks!
luv.tar.gz
Description: application/gzip
