Created attachment 152921 [details] v1 - Convert to option helpers - Drop implicit USE_SDL=sdl (see _REQUIRES_xxx in bsd.sdl.mk) - Drop unused libGLU dependency (except for Cube1) - Track direct dependency on libX11 (for XFetchBytes) - CLIENT uses openal/vorbisfile *instead* of SDL_mixer
Maintainer CC'd
Logs and diffs for similar ports are in: https://reviews.freebsd.org/D1831
Hello, Jan Beich. Thanks for proposed changes. Honestly to say, I wanted to review the port in case of new version. I will comment about your patch after some checks.
Comment on attachment 152921 [details] v1 Most of the changes to options helpers are fine. The installation changes also work. There is just a need to MASTER_PLIST_FILES also: MASTER_PLIST_FILES= bin/${PORTNAME}_master libexec/${PORTNAME}_master to make "SUB_FILES= ${PLIST_FILES:Mbin/*:T}" thing work. Because of this, the attachment 152921 [details] is rejected. But overall, the changes are fine. To reject this patch, the Bugzilla maintainer needed, because I don't have sufficient user privileges to do this. I will propose my changes in the next comment.
Created attachment 152929 [details] Proposed patch (since 374303 revision) The proposed patch contains most of the changes from attachment 152921 [details]. I did some stylistic changes. Added MASTER_PLIST_FILES and PLIST_DIRS for empty directory. Wrapped cd related commands to parentheses. Also I did PORTREVISION bump, because of dependency and package changes. The silent creation of DATADIR and DOCSDIR removed, because COPYTREE_SHARE will take care about this. Added patch for source/src/bot/bot_waypoint.cpp file. How these changes might look in the log, in addition to yours: - PORTREVISION bump, because of dependency and package changes - Use gettext-runtime instead of gettext - Use PLIST_DIRS for empty directory - Remove silent creation of DATADIR and DOCSDIR, because not necessary in case of using COPYTREE_SHARE - Wrap cd related commands to parentheses - Add patch for source/src/bot/bot_waypoint.cpp file to fix "invalid source encoding" warning
Created attachment 152930 [details] The poudriere testport log (FreeBSD 10 amd64)
Created attachment 152933 [details] Proposed patch (since 374303 revision) After looking at review D1831 comments, I think, that Dmitry Marakasov (amdmi3@) is right about explicit USE_SDL=sdl. The proposed patch changed.
Created attachment 152934 [details] The poudriere testport log (FreeBSD 10 amd64)
> - Add patch for source/src/bot/bot_waypoint.cpp file to fix "invalid source encoding" warning This piece can probably go in separately but there're more warnings with clang 3.6 or gcc 4.9 + -O3. At least one of them proved fatal for games/cube (see bug 197604). $ make -C games/assaultcube entities.cpp:69:18: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(!&mmi) continue; ~ ^~~ entities.cpp:227:21: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(&mmi && mmi.h) ^~~ ~~ entities.cpp:296:9: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(&is) ~~ ^~ entities.cpp:631:21: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(&mmi && mmi.h) clipents++; ^~~ ~~ 4 warnings generated. physics.cpp:134:18: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(!&mmi || !mmi.h) continue; ~ ^~~ 1 warning generated. server.cpp:2183:139: warning: address of array 'v->action->desc' will always evaluate to 'true' [-Wpointer-bool-conversion] ...clients[v->owner]->name, v->action && v->action->desc ? v->action->desc : "[... ~~ ~~~~~~~~~~~^~~~ server.cpp:2190:152: warning: address of array 'v->action->desc' will always evaluate to 'true' [-Wpointer-bool-conversion] ...clients[v->owner]->name, v->action && v->action->desc ? v->action->desc : "[... ~~ ~~~~~~~~~~~^~~~ server.cpp:3460:56: warning: address of array 'vi->text' will always evaluate to 'true' [-Wpointer-bool-conversion] char *vmap = newstring(vi->text ? behindpath(vi-... ~~~~^~~~ ~ 3 warnings generated. weapon.cpp:338:10: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(!&mmi || !mmi.h) return false; ~ ^~~ 1 warning generated. zip.cpp:557:8: warning: variable 'target' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized] if(s->open(a, f) && (target = fopen(fname, "wb"))) ^~~~~~~~~~~~~ zip.cpp:565:8: note: uninitialized use occurs here if(target) fclose(target); ^~~~~~ zip.cpp:557:8: note: remove the '&&' if its condition is always true if(s->open(a, f) && (target = fopen(fname, "wb"))) ^~~~~~~~~~~~~~~~ zip.cpp:553:17: note: initialize the variable 'target' to silence this warning FILE *target; ^ = NULL 1 warning generated. bot/bot_util.cpp:194:16: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion] if(!&mmi || !mmi.h) continue; ~ ^~~ 1 warning generated. server.cpp:2183:139: warning: address of array 'v->action->desc' will always evaluate to 'true' [-Wpointer-bool-conversion] ...clients[v->owner]->name, v->action && v->action->desc ? v->action->desc : "[... ~~ ~~~~~~~~~~~^~~~ server.cpp:2190:152: warning: address of array 'v->action->desc' will always evaluate to 'true' [-Wpointer-bool-conversion] ...clients[v->owner]->name, v->action && v->action->desc ? v->action->desc : "[... ~~ ~~~~~~~~~~~^~~~ server.cpp:3460:56: warning: address of array 'vi->text' will always evaluate to 'true' [-Wpointer-bool-conversion] char *vmap = newstring(vi->text ? behindpath(vi-... ~~~~^~~~ ~ 3 warnings generated. crypto.cpp: In member function 'void ecjacobian::normalize()': crypto.cpp:308:33: warning: array subscript is above array bounds [-Warray-bounds] digit tmp = x.digits[i+dig+1]; ^ crypto.cpp:308:33: warning: array subscript is above array bounds [-Warray-bounds] digit tmp = x.digits[i+dig+1]; ^ crypto.cpp: In member function 'void ecjacobian::normalize()': crypto.cpp:308:33: warning: array subscript is above array bounds [-Warray-bounds] digit tmp = x.digits[i+dig+1]; ^ crypto.cpp:308:33: warning: array subscript is above array bounds [-Warray-bounds] digit tmp = x.digits[i+dig+1]; ^ In file included from rendermodel.cpp:7:0: tristrip.h: In member function 'void vertmodel::mesh::genstrips()': tristrip.h:213:62: warning: array subscript is below array bounds [-Warray-bounds] to = findedge(first, triangles[cur], first.v[from]); ^
(In reply to comment #9) > This piece can probably go in separately but there're more warnings with clang > 3.6 or gcc 4.9 + -O3. At least one of them proved fatal for games/cube (see bug > 197604). I didn't mean to fix all the warnings, that more modern LLVM/Clang wrote, which is work of actual developers of the software. The software builds even without my patch. I saw, that across the source code the word "Couldn't" used, but in one place it is "Couldn`t", which was logical to fix, just for completeness. My approved patch in attachment 152933 [details] is complete and this is what I think should be commited. Not the parts of it in review D1831, which requires PORTREVISION bump, while stated it is not.
Thanks for the feedback. Please, keep it all in the bug (vs. private mail) for accounting and in case the review gets stalled. I've restored your changes in review D1831. If you still have some issues with the wording of some items in commit message, whitespace or anything else please say so.
(In reply to comment #10) The overall meaning of your commit message related to games/assaultcube is the same, but with different wording. Thank you for change and response.
comment #12 -> comment #11.
Submitter is committer, re-assign Does one of these patches need to be obsoleted? Attachment 152921 [details] is still set to maintainer-approval?
(In reply to comment #14) > Attachment 152921 [details] is still set to maintainer-approval? I think, my maintainer-approval could be removed for it after commit, in case Jan Beich didn't do this or obsolete the attachment.
Comment on attachment 152921 [details] v1 Rejected in comment 4 and obsolete since comment 11. For maintainer-approval? in general I've filed bug 197656.
(In reply to comment #16) Ok, thanks.
A commit references this bug: Author: jbeich Date: Sun Mar 15 11:51:09 UTC 2015 New revision: 381324 URL: https://svnweb.freebsd.org/changeset/ports/381324 Log: Improve style, consistency and fix minor issues in Cube-based ports - Convert to option helpers - Drop unused libGLU dependency (except for games/cube) - Track direct dependency on libX11 (for XFetchBytes) - Drop redundant MKDIR before COPYTREE_* macros [1] - Wrap cd related commands with parentheses [1] - Wrap lines exceeding 80 characters - Bump PORTREVISION to pick up changes in dependencies, plist (assaultcube) and catch regressions early [1] - games/assaultcube: CLIENT uses openal/vorbisfile *instead* of SDL_mixer - games/assaultcube: drop unused gettext-tools dependency [1] - games/assaultcube: convert to PLIST_DIRS [1] - games/assaultcube: add patch for source/src/bot/bot_waypoint.cpp file to fix "invalid source encoding" warning [1] - games/{cube,bloodfrontier}: MASTER or SERVER don't need libX11 - games/redeclipse: tell how large the package is in IGNORE message - games/redeclipse: use PORTDATA to hold list of dirs for COPYTREE_SHARE Differential Revision: https://reviews.freebsd.org/D1831 PR: 197582 [1] PR: 197583 [2] Submitted by: lightside@gmx.com [1] Requested by: lightside@gmx.com [1] Reviewed by: amdmi3, lightside@gmx.com (maintainers) Approved by: maintainer timeout (1 month) [2] Approved by: bapt (mentor) Changes: head/games/assaultcube/Makefile head/games/assaultcube/files/patch-source_src_bot_bot_waypoint.cpp head/games/bloodfrontier/Makefile head/games/cube/Makefile head/games/redeclipse/Makefile head/games/sauerbraten/Makefile