-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
www-client/luakit: new package #5228
Conversation
Do you mind adding a live version as well? (9999) |
virtual/pkgconfig | ||
doc? ( app-doc/doxygen )" | ||
|
||
DOCS=() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to suppress default DOCS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm supressing default DOCS because the Makefile installs the documentation already during the install stage:
https://github.com/aidanholm/luakit/blob/develop/Makefile#L91-L96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother, tons of buildsystems do this, yet we dont mind. Stay as vanilla as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also FYI, COPYING.GPLv3
shouldn't be installed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I just noticed that. I guess I'll just rm it.
@Hummer12007 |
@SoapGentoo |
@lluixhi as a stopgap you can do that. If you care a lot about this package, the proper way of course is to Autotool/CMake/Meson-ise it and not fiddle with internal variables. |
www-client/luakit/luakit-9999.ebuild
Outdated
|
||
if [[ ${PV} == 9999 ]]; then | ||
inherit git-r3 | ||
EGIT_REPO_URI="git://github.com/aidanholm/luakit.git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aidanholm has transitioned to the luakit account as the new upstream (merged), iirc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, I'll change the links.
www-client/luakit/luakit-9999.ebuild
Outdated
inherit toolchain-funcs | ||
|
||
DESCRIPTION="A fast, light, simple to use micro-browser using WebKit and Lua." | ||
HOMEPAGE="https://aidanholm.github.io/luakit/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOMEPAGE should be luakit.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://luakit.org redirects to https://luakit.github.io/luakit, so I think I'll use that instead, I think Gentoo is trying to use more https links in homepages and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor nitpick. I'm going to try to build it, and if it passes tests, I'll fix it and commit.
|
||
inherit toolchain-funcs | ||
|
||
DESCRIPTION="A fast, light, simple to use micro-browser using WebKit and Lua." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a proper sentence so no .
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> Test phase: www-client/luakit-2017.07.26
make -j3 LUA_BIN_NAME=lua run-tests
x86_64-pc-linux-gnu-gcc-5.4.0 -fpic -march=k8-sse3 -mcx16 -msahf --param l1-cache-size=64 --param l1-cache-line-size=64 --param l2-cache-size=512 -O2 -pipe -frecord-gcc-switches -std=gnu99 -W -Wall -Wextra -pthread -I/usr/include/webkitgtk-4.0 -I/usr/include/gtk-3.0 -I/usr/include/at-spi2-atk/2.0 -I/usr/include/at-spi-2.0 -I/usr/include/dbus-1.0 -I/usr/lib64/dbus-1.0/include -I/usr/include/gtk-3.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libdrm -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/libsoup-2.4 -I/usr/include/libxml2 -I/usr/include/luajit-2.0 -I/usr/include/webkitgtk-4.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I./ -DVERSION=\"5bdc45fa\" -DDEVELOPMENT_PATHS -shared -Wl,-O1 -Wl,--as-needed -Wl,--hash-style=gnu -Wl,--export-dynamic -lgthread-2.0 -pthread -lwebkit2gtk-4.0 -lgtk-3 -lgdk-3 -lpangocairo-1.0 -lpango-1.0 -latk-1.0 -lcairo-gobject -lcairo -lgdk_pixbuf-2.0 -lsoup-2.4 -lgio-2.0 -lgobject-2.0 -lsqlite3 -lluajit-5.1 -ljavascriptcoregtk-4.0 -lglib-2.0 tests/util.c -o tests/util.so
lua: error loading module 'tests.util' from file './tests/util.so':
./tests/util.so: undefined symbol: g_dir_make_tmp
stack traceback:
[C]: ?
[C]: in function 'require'
tests/run_test.lua:15: in main chunk
[C]: ?
make: *** [Makefile:125: run-tests] Error 1
This indicates a wrong linker line. The source file should come before all the |
I'll write a patch for it and submit it upstream. |
@lluixhi, how's the patch going? If you need help, just lemme know. Otherwise, please leave upstream report/patch link. This is the kind of issue that Gentoo developers hit frequently, and that sometimes needs some convincing upstream. |
Finally made the patch. |
I also missed a couple of things that were required for the test phase: currently, it'll just skip the tests if built with luajit because I also fixed the installation paths because for some reason DOCDIR and XDGPREFIX aren't respecting DESTDIR. |
|
||
src_test() { | ||
# We're currently missing luacheck which is necessary to pass all tests | ||
nonfatal emake \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can specify RESTRICT="luajit? ( test )" (i.e. use-conditional)
Could you also package |
5e2ffad
to
4794e9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's almost good. Just let me know if you agree with adding :=
on lua since different versions are going to be incompatible, and i'll add that minor fix, test it and push. No need for you to update the ebuilds, that will only make more work for CI ;-).
|
||
RDEPEND=" | ||
dev-lua/luafilesystem[luajit=] | ||
!luajit? ( >=dev-lang/lua-5.1 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you really ought to use :=
for lua.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, I don't think luakit has been tested on lua 5.2 or 5.3, and I'm pretty sure the author makes no guarantees of it working with them. Woud <dev-lang/lua-5.2:=
be a better version dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes, I agree with adding :=
otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<
dependencies are a very bad thing™, because they prevent upgrades. Do not use them unless the thing actually does not work. Using :=
also guarantees that the package will be rebuilt on Lua upgrade, so if anything starts failing at build time, people will notice instantly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized we're commenting on luacheck. Whoops.
That makes sense though.
virtual/pkgconfig | ||
doc? ( app-doc/doxygen ) | ||
test? ( | ||
dev-lua/luassert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not strictly necessary but if you have some spare time, feel free to also submit a PR adding luajit flag here. However, I won't delay luakit because of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold up, I'm adding commits for luassert and say so I can add that.
9138ff7
to
e9966b1
Compare
instdir="$($(tc-getPKG_CONFIG) --variable INSTALL_LMOD $(usex luajit 'luajit' 'lua'))"/${PN} | ||
insinto "${instdir#${EPREFIX}}" | ||
doins src/init.lua | ||
einstalldocs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to match the original set of docs installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
instdir="$($(tc-getPKG_CONFIG) --variable INSTALL_LMOD $(usex luajit 'luajit' 'lua'))"/${PN} | ||
insinto "${instdir#${EPREFIX}}" | ||
doins -r src/* | ||
einstalldocs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also reverted.
|
||
newbin bin/luacheck.lua luacheck | ||
|
||
einstalldocs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also want to include CHANGELOG.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also added a doc use flag to build and install the HTML sphinx documentation.
@mgorny |
|
(and that's why people are supposed to use if;fi and not ugly one-liners) |
That's embarassing. I tested it with the doc flag enabled but I didn't test with it disabled and I didn't have the presence of mind to remember order of evaluation for and/or. Anyway, fixed. |
Bump to EAPI 6 Add Test phase. Gentoo-Bug: https://bugs.gentoo.org/628758
Bump to EAPI 6 Add Test phase Gentoo-Bug: https://bugs.gentoo.org/628758
Added test phases as per https://bugs.gentoo.org/628758 |
😞 The QA check for this pull request has found the following issues: Issues inherited from Gentoo (may be modified by PR): |
I'm afraid I don't have good news for you.
However, given your track record I'm just going to merge it and you can fix it in a followup. |
Not sure whether that's a compliment or a low blow, but I need to submit an issue upstream about it. While running tests I would occasionally run into those bugs, and it would also occasionally all pass -- something is flaky about them. on my end:
ohhh, part of it might be the name of the C compiler for the test phase. I'll change that in the next pull request. |
It was a compliment. Thanks for looking into it. |
luakit is an easily extensible WebKit-based browser written in lua.
This version uses Webkit 2 and GTK+ 3
This package was originally in the tree before but was abandoned because the maintainer quit in ~2012.
A new maintainer took it up and this is the first release.