New port games/barony in order to play Barony from Turningwheel
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| games/barony/Makefile | ||
|---|---|---|
| 7 ↗ | (On Diff #42172) | MASTER_SITES is where you fetch distfiles from. This is bogus in presence of USE_GITHUB. See not about WWW: in pkg-descr |
| 25 ↗ | (On Diff #42172) | USES=localbase preferred |
| 29 ↗ | (On Diff #42172) | Not needed. |
| 32 ↗ | (On Diff #42172) | Targets after all definitions please. |
| 33 ↗ | (On Diff #42172) | 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.
|
| games/barony/pkg-descr | ||
| 8 ↗ | (On Diff #42172) | Lacks WWW: |
Comment Actions
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 ↗ | (On Diff #42241) | mixer2 (aka SDL2_mixer) is commented out in the source i.e., unused. |
| 24 ↗ | (On Diff #42241) | Convert CMAKE_ARGS=-D<VAR>=ON to CMAKE_ON=<VAR> per /usr/ports/CHANGES from 20171231. |
| 25 ↗ | (On Diff #42241) | Maybe qualify type e.g., -DEDITOR_EXE_NAME:STRING="barony-editor" |
| 26 ↗ | (On Diff #42241) | See above about CMAKE_ON. |
| 33 ↗ | (On Diff #42241) | share/barony is ${DATADIR_REL} or ${DATADIR} (absolute path). If you plan to use bin/${PORTNAME} maybe convert this one as well. |
| 38 ↗ | (On Diff #42241) | Not consistent with PLIST_FILES: libexec/${PORTNAME} vs. libexec/barony. |
| 40 ↗ | (On Diff #42241) | Not consistent with PLIST_FILES: bin/${PORTNAME} vs. bin/barony. |
| games/barony/files/barony.sh.in | ||
| 5 ↗ | (On Diff #42241) | 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. |
Comment Actions
Can you switch differential visibility from "All Users" to "Public" via "Edit Revision" button? I tend to avoid non-public reviews.
Comment Actions
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.
Comment Actions
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.
Comment Actions
Not sure why I was added to this review. You seem to have ignored some of the raised review items without any comment.
Same.
| games/barony/Makefile | ||
|---|---|---|
| 11 ↗ | (On Diff #42286) | Add LICENSE_FILE |
| 17 ↗ | (On Diff #42286) | 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 ↗ | (On Diff #42241) | What about @jbeich's suggestion? It was ignored without comment. |
| games/barony/pkg-descr | ||
| 8 ↗ | (On Diff #42172) | 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? |
Comment Actions
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).
Comment Actions
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 ↗ | (On Diff #42241) | 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. |
| games/barony/pkg-message | ||
|---|---|---|
| 8 ↗ | (On Diff #42286) | Good point, it should normally did not try. |
| games/barony/files/barony.sh.in | ||
|---|---|---|
| 5 ↗ | (On Diff #42241) | 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. |