games-fps/yamagi-quake2: new package #6904
Conversation
|
Pull Request assignment Areas affected: ebuilds games-fps/yamagi-quake2: @gentoo/proxy-maint (new package) Bugs linked: 314751 In order to force reassignment and/or bug reference scan, please append |
gentoo-repo-qa-bot
added
new package
puleglot
force-pushed
the
yamagi-quake2
branch
from
63d2f5b
to
323fa32
Compare
|
@monsieurp I've added commit message. |
puleglot
force-pushed
the
yamagi-quake2
branch
2 times, most recently
from
57afc7f
to
cc574c0
Compare
|
ping |
| -CFLAGS := -O2 -fno-strict-aliasing -fomit-frame-pointer \ | ||
| - -Wall -pipe -g -fwrapv -arch i386 -arch x86_64 | ||
| +CFLAGS += -fno-strict-aliasing -fomit-frame-pointer \ | ||
| + -Wall -fwrapv -arch i386 -arch x86_64 |
There was a problem hiding this comment.
I don't think we do multi-arch builds on Darwin. Plus, you've removed that from LDFLAGS, so I suppose you broke Darwin for good ;-). Not that it matters.
There was a problem hiding this comment.
Hmm.. I don't quite get your comment about LDFLAGS.
There was a problem hiding this comment.
Sorry, I have mistaken the if branches. Please disregard that part.
| +LDFLAGS += -lm -ldl -rdynamic | ||
| else ifeq ($(YQ2_OSTYPE),FreeBSD) | ||
| -LDFLAGS := -L/usr/local/lib -lm | ||
| +LDFLAGS += -L/usr/local/lib -lm |
There was a problem hiding this comment.
Again, if you alter other variables, please make sure to remove stray -L flags there as well.
| @@ -0,0 +1,30 @@ | |||
| --- a/Makefile 2017-05-25 12:45:51.000000000 +0300 | |||
There was a problem hiding this comment.
The context between the two patches looks very similar. Maybe you'd be enable to reduce them to one patch e.g. by reducing -U context?
There was a problem hiding this comment.
eapply -l seems to work here
|
|
||
| RDEPEND="sys-libs/zlib:0= | ||
| client? ( | ||
| media-libs/libsdl2 |
There was a problem hiding this comment.
Deps on SDL usually require some USE flags like [video] or [sound] for the respective subsystems.
| use dedicated && targets+=" server" | ||
|
|
||
| local t | ||
| for t in config "${targets}"; do |
There was a problem hiding this comment.
This loop looks ugly-ish and is going to repeatedly make people wtf about quoting. Can't you just pass all targets to emake?
There was a problem hiding this comment.
"config" target just shows configuration variables. It's output is messed up when doing parallel build, eg:
make -j6 config game client ref_gl1 ref_gl3 server VERBOSE=1 DLOPEN_OPENAL=no WITH_CDA=no WITH_SYSTEMWIDE=yes WITH_SYSTEMDIR=/usr/share/games/quake2 WITH_ZIP=yes WITH_OGG=yes WITH_OPENAL=yes
Build configuration
============================
WITH_CDA = no
===> Building baseq2/game.so
mkdir -p release/baseq2
WITH_OPENAL = yes
make release/baseq2/game.so
WITH_SDL2 = yes
make[1]: Entering directory '/var/tmp/portage/games-fps/yamagi-quake2-7.10/work/quake2-7.10'
WITH_X11GAMMA = no
WITH_ZIP = yes
WITH_SYSTEMWIDE = yes
===> Building quake2
mkdir -p release
WITH_SYSTEMDIR = /usr/share/games/quake2
============================
There was a problem hiding this comment.
Oh. In that case I'd suggest a helper function that wraps emake with arguments, and call it like mymake config, then mymake everything.
| } | ||
|
|
||
| src_install() { | ||
| insinto /usr/$(get_libdir)/${PN} |
There was a problem hiding this comment.
Use /usr/lib, and don't use ${PN} in src_install as that impairs readability.
| # Yamagi Quake II expects all binaries to be in the same directory | ||
| # See stuff/packaging.md for more info | ||
| exeinto /usr/$(get_libdir)/${PN} | ||
| doins -r release/* |
There was a problem hiding this comment.
Use release/. to avoid unnecessary instant-glob.
|
|
||
| if use client; then | ||
| doexe release/quake2 | ||
| dosym /usr/$(get_libdir)/${PN}/quake2 /usr/bin/yquake2 |
There was a problem hiding this comment.
Do not ever use absolute symlinks, they're broken in some versions of Portage.
|
|
||
| einstalldocs | ||
| if use client; then | ||
| insinto /usr/share/doc/${PF}/examples |
puleglot
force-pushed
the
yamagi-quake2
branch
3 times, most recently
from
bc15e33
to
a26b3e9
Compare
| } | ||
|
|
||
| src_compile() { | ||
| local targets="game" |
There was a problem hiding this comment.
Could you convert this into an array? Appending with whitespace is kinda ugly.
| for addon in ctf rogue xatrix; do | ||
| use ${addon} || continue | ||
| pushd "${WORKDIR}"/quake2-${addon}-* >/dev/null || die | ||
| emake VERBOSE=1 |
There was a problem hiding this comment.
emake -C ... instead of pushd/popd.
Afterwards, you may leverage using three inline use ... && emake -C ... instead of the loop (your choice).
| dosym ../lib/yamagi-quake2/quake2 /usr/bin/yquake2 | ||
|
|
||
| newicon stuff/icon/Quake2.svg "yamagi-quake2.svg" | ||
| make_desktop_entry "yquake2" "Yamagi Quake II" |
There was a problem hiding this comment.
Don't you need to specify the icon somehow if it's not named after the executable?
There was a problem hiding this comment.
Icon is named after the package name (${PN}) by default.
| use ${addon} || continue | ||
| pushd "${WORKDIR}"/quake2-${addon}-* >/dev/null || die | ||
| insinto /usr/lib/yamagi-quake2/${addon} | ||
| doins release/game.so |
There was a problem hiding this comment.
Put the path here instead of pushd/popd?
| esac | ||
|
|
||
| make_wrapper "yquake2-${addon}" "yquake2 +set game ${addon}" | ||
| make_desktop_entry "yquake2-${addon}" "Yamagi Quake II: ${addon_name}" |
|
|
||
| einstalldocs | ||
| if use client; then | ||
| docinto examples |
There was a problem hiding this comment.
Good question. This script shows how one could rip a soundtrack from the game CD to use it with this engine.
This is the Yamagi Quake II Client, an enhanced version of id Software's Quake II with focus on offline and coop gameplay. Both the gameplay and the graphics are unchanged, but many bugs in the last official release were fixed and some nice to have features like widescreen support and a modern OpenGL 3.2 renderer were added. Unlike most other Quake II source ports Yamagi Quake II is fully 64 bit clean and is still actively maintained. Closes: https://bugs.gentoo.org/314751
|
Pull request CI report Report generated at: 2018-03-04 05:13 UTC Issues already there before the PR (double-check them): |
|
Thank you for review! :) |
|
…aaaand merged. By the way, you may look into establishing some relation to |
|
Yes, I'll do it later. Other quake2 engines in gentoo repo have |
|
Weird. I thought the purpose of separate data package is to deal with the that. Yeah, postinst is fine. |
Closes: https://bugs.gentoo.org/314751