Page MenuHomeFreeBSD

[new port] games/barony: 3D roguelike game
ClosedPublic

Authored by devnexen_gmail.com on May 5 2018, 4:03 PM.

Details

Summary

New port games/barony in order to play Barony from Turningwheel

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

devnexen_gmail.com created this object with visibility "All Users".
AMDmi3 requested changes to this revision.May 7 2018, 5:35 PM
AMDmi3 added inline comments.
games/barony/Makefile
7

MASTER_SITES is where you fetch distfiles from. This is bogus in presence of USE_GITHUB. See not about WWW: in pkg-descr

25

USES=localbase preferred

29

Not needed.

32

Targets after all definitions please.

33

Better use SUB_FILES=barony.sh - this will automatically process placeholders from files/barony.sh.in and place resulting file under ${WRKDIR}/barony.sh. Then you'll just have to install it with ${INSTALL_SCRIPT}.

Also, instead of a huge effort to rename game binary, you can just move it into libexec/ here:

${MV} ${STAGEDIR}${PREFIX}/bin/${PORTNAME} ${STAGEDIR}${PREFIX}/libexec/${PORTNAME}

and call it from the wrapper script.

games/barony/files/barony.sh
5 ↗(On Diff #42172)

"$@" should always be quoted.

games/barony/files/patch-CMakeLists.txt
1 ↗(On Diff #42172)

This patch seem to exist for two purposes, each of this is bogus.

  • You don't need to rename the binary all over the build system, you could do in the install directive (install(... RENAME ...)) or in post-install target. Actually, you just need to intall it into libexec instead
  • We don't use games/ subdirectories under bin/ or share/. The default data installation directory is correct and doesn't need to be changed.
games/barony/pkg-descr
8

Lacks WWW:

This revision now requires changes to proceed.May 7 2018, 5:35 PM
jbeich requested changes to this revision.May 8 2018, 7:07 AM

Looks good in general but has minor issues (cosmetics can be ignored).

poudriere testport reports 2 missing dependencies, one of which can be stripped via LDFLAGS+=-Wl,--as-needed:

-- Checking for one of the modules 'SDL2_net'
CMake Error at /usr/local/share/cmake/Modules/FindPkgConfig.cmake:641 (message):
  None of the required 'SDL2_net' found
Call Stack (most recent call first):
  CMakeLists.txt:145 (PKG_SEARCH_MODULE)
====> Running Q/A tests (stage-qa)
Error: /usr/local/libexec/barony is linked to /usr/local/lib/libogg.so.0 from audio/libogg but it is not declared
as a dependency
Warning: you need LIB_DEPENDS+=libogg.so:audio/libogg

Requires LLD_UNSAFE=yes # bug 226980 as audio/openal-soft isn't compatible with -fuse-ld=lld by default:

/usr/bin/ld: error: cannot preempt symbol: alGetSourcei
>>> defined in /usr/local/lib/libopenal.so
>>> referenced by sound.cpp
>>>               CMakeFiles/barony-editor.dir/src/sound.cpp.o:(OPENAL_ThreadFunction(void*))

Contains only game engine but not documented in either COMMENT, pkg-descr and/or pkg-message:

[12-35-23] loading engine resources...
[12-35-23] error: failed to load image '/usr/local/share/barony/images/system/fancyWindow.png'

Minor style:

$ portlint -AC
FATAL: Makefile: [3]: use a tab (not space) after a variable name
FATAL: Makefile: [6]: use a tab (not space) after a variable name
FATAL: Makefile: [28]: use a tab (not space) after a variable name
3 fatal errors and 0 warnings found.
games/barony/Makefile
18

mixer2 (aka SDL2_mixer) is commented out in the source i.e., unused.

24

Convert CMAKE_ARGS=-D<VAR>=ON to CMAKE_ON=<VAR> per /usr/ports/CHANGES from 20171231.

25

Maybe qualify type e.g., -DEDITOR_EXE_NAME:STRING="barony-editor"

26

See above about CMAKE_ON.

33

share/barony is ${DATADIR_REL} or ${DATADIR} (absolute path). If you plan to use bin/${PORTNAME} maybe convert this one as well.

38

Not consistent with PLIST_FILES: libexec/${PORTNAME} vs. libexec/barony.

40

Not consistent with PLIST_FILES: bin/${PORTNAME} vs. bin/barony.

games/barony/files/barony.sh.in
5

Consider patching code to use ~/.barony in order to simplify the port e.g.,

--- src/game.cpp.orig	2018-04-17 02:51:23 UTC
+++ src/game.cpp
@@ -2158,6 +2158,8 @@ bool frameRateLimit( Uint32 maxFrameRate )
 
 -------------------------------------------------------------------------------*/
 
+#include <sys/stat.h>
+#include <err.h>
 #include <stdio.h>
 //#include <unistd.h>
 #include <stdlib.h>
@@ -2167,6 +2169,14 @@ bool frameRateLimit( Uint32 maxFrameRate )
 
 int main(int argc, char** argv)
 {
+
+	//System-wide install is read-only, so save under ~/
+	if (chdir(getenv("HOME")) != 0)
+		err(1, "cannot cd to $HOME");
+	if (mkdir(".barony", 0755) != 0 && errno != EEXIST)
+		err(1, "cannot mkdir $HOME/.barony");
+	if (chdir(".barony") != 0)
+		err(1, "cannot cd to $HOME/.barony");
 
 #ifdef LINUX
 	//Catch segfault stuff.
This revision now requires changes to proceed.May 8 2018, 7:07 AM

Can you switch differential visibility from "All Users" to "Public" via "Edit Revision" button? I tend to avoid non-public reviews.

Makes sense, last week I co-authored the OpenBSD package, I need to adjust to FreeBSD specificities :-) thanks for the feedback will bring update later on the day.

ping :)

ping

Not exactly sure what you are ping'ing about.
I had a quick look and the port looks ok.

Now, it looks like you are not a committer, so you cannot add this yourself. You should probably go and open a PR on our bugzilla so that the port gets into the pipeline and gets added to the repository.

tobik requested changes to this revision.May 25 2018, 7:40 AM

Not sure why I was added to this review. You seem to have ignored some of the raised review items without any comment.

Can you switch differential visibility from "All Users" to "Public" via "Edit Revision" button? I tend to avoid non-public reviews.

Same.

games/barony/Makefile
11

Add LICENSE_FILE

17

The indentation after the variables looks seriously off in the entire file (hard to tell in Phabricator). Indent with one tab or two if needed.

games/barony/files/barony.sh.in
5

What about @jbeich's suggestion? It was ignored without comment.

games/barony/pkg-descr
8

It's still missing WWW...

games/barony/pkg-message
1–2 ↗(On Diff #42286)

Remove the header.

8 ↗(On Diff #42286)

It doesn't work with unzip from the base system?

This revision now requires changes to proceed.May 25 2018, 7:40 AM
In D15310#328652, @mat wrote:

ping :)

ping

Not exactly sure what you are ping'ing about.
I had a quick look and the port looks ok.

Now, it looks like you are not a committer, so you cannot add this yourself. You should probably go and open a PR on our bugzilla so that the port gets into the pipeline and gets added to the repository.

Oh right sorry I thought it worked the same way as LLVM's Phabricator, ok I ll update the PR on bugzilla (the ticket already exists in fact).

Fair enough @tobik I ll do necessary changes later on. I m not ignoring comments just was waiting for more reviews.

games/barony/files/barony.sh.in
5

I got similar feedback from OpenBSD, thus there is an ongoing PR about this and will be added for the next GOG release version. So this shell script is just temporary but if you insist I ll apply the changes once I get back to my FreeBSD box.

devnexen_gmail.com changed the visibility from "All Users" to "Public (No Login Required)".May 25 2018, 7:47 AM
games/barony/pkg-message
8 ↗(On Diff #42286)

Good point, it should normally did not try.

games/barony/files/barony.sh.in
5

I'm not insisting. Your explanation makes sense. I was just wondering why you haven't done it :).

Maybe add a link to the PR to the wrapper script or Makefile so that we know it's supposed to be a temporary solution.

linimon retitled this revision from new games/barony to [new port] games/barony: 3D roguelike game.Aug 2 2018, 10:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 14 2018, 7:29 PM
Closed by commit rP477198: New port: games/barony (authored by tobik). · Explain Why
This revision was automatically updated to reflect the committed changes.