Bug 197605 - games/{assaultcube,bloodfrontier,redeclipse,sauerbraten}: unbundle libenet
Summary: games/{assaultcube,bloodfrontier,redeclipse,sauerbraten}: unbundle libenet
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jan Beich
URL:
Keywords: feature
Depends on:
Blocks:
 
Reported: 2015-02-13 23:52 UTC by Jan Beich
Modified: 2015-05-26 15:46 UTC (History)
5 users (show)

See Also:


Attachments
v1 (16.39 KB, patch)
2015-02-14 06:00 UTC, Jan Beich
jbeich: maintainer-approval? (lightside)
jbeich: maintainer-approval? (amdmi3)
jbeich: maintainer-approval? (acm)
Details | Diff
Proposed patch (since 374303 revision): for games/assaultcube (3.86 KB, patch)
2015-02-14 15:01 UTC, lightside
lightside: maintainer-approval+
Details | Diff
The poudriere testport log (FreeBSD 10 amd64): for games/assaultcube (8.81 KB, application/x-bzip2)
2015-02-14 15:03 UTC, lightside
no flags Details
The poudriere testport log (FreeBSD 10 amd64, with ENET option enabled): for games/assaultcube (8.47 KB, application/x-bzip2)
2015-02-14 15:04 UTC, lightside
no flags Details
The games/assaultcube-enet port in shar format (748 bytes, text/plain)
2015-02-14 21:15 UTC, lightside
no flags Details
assaultcube-only, system libenet as an option (3.96 KB, patch)
2015-02-15 00:06 UTC, Jan Beich
no flags Details | Diff
Proposed patch (since 374303 revision): for games/assaultcube (3.74 KB, patch)
2015-02-15 09:16 UTC, lightside
lightside: maintainer-approval+
Details | Diff
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube (8.75 KB, application/x-bzip2)
2015-02-15 09:18 UTC, lightside
no flags Details
The poudriere testport log (FreeBSD 10 amd64, with PORT_ENET option enabled): games/assaultcube (6.32 KB, application/x-bzip2)
2015-02-15 09:19 UTC, lightside
no flags Details
Proposed patch (since 374303 revision): for games/assaultcube (3.77 KB, patch)
2015-02-15 20:33 UTC, lightside
lightside: maintainer-approval+
Details | Diff
Proposed patch (since 374303 revision): for games/assaultcube (3.76 KB, patch)
2015-02-15 21:20 UTC, lightside
lightside: maintainer-approval+
Details | Diff
Proposed patch (since 374303 revision): for games/assaultcube (5.12 KB, patch)
2015-02-15 23:31 UTC, lightside
lightside: maintainer-approval+
Details | Diff
The games/assaultcube-enet port in shar format (1.78 KB, text/plain)
2015-02-16 18:41 UTC, lightside
no flags Details
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube-enet (6.32 KB, application/x-bzip2)
2015-02-16 18:44 UTC, lightside
no flags Details
The games/assaultcube-enet port in shar format (1.78 KB, text/plain)
2015-02-16 19:21 UTC, lightside
no flags Details
The games/assaultcube-enet port in shar format (1.80 KB, text/plain)
2015-02-16 19:49 UTC, lightside
no flags Details
The games/assaultcube-enet port in shar format (2.04 KB, text/plain)
2015-03-02 06:14 UTC, lightside
no flags Details
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube-enet (6.57 KB, application/x-bzip2)
2015-03-02 06:15 UTC, lightside
no flags Details
Proposed patch (since 381324 revision): for games/assaultcube (5.18 KB, patch)
2015-03-15 18:29 UTC, lightside
lightside: maintainer-approval+
Details | Diff
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube (8.49 KB, application/x-bzip2)
2015-03-15 18:30 UTC, lightside
no flags Details
The poudriere testport log (FreeBSD 10 amd64, with PORT_ENET option enabled): games/assaultcube (6.08 KB, application/x-bzip2)
2015-03-15 18:30 UTC, lightside
no flags Details
Proposed patch (since 381324 revision): for games/assaultcube (5.18 KB, patch)
2015-03-15 19:07 UTC, lightside
lightside: maintainer-approval+
Details | Diff
Proposed patch (since 381324 revision): for games/assaultcube (7.28 KB, patch)
2015-04-13 19:02 UTC, lightside
lightside: maintainer-approval+
Details | Diff
Proposed patch (since 385392 revision): for games/assaultcube (1.81 KB, patch)
2015-05-04 16:52 UTC, lightside
lightside: maintainer-approval+
Details | Diff
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube (8.40 KB, application/x-bzip2)
2015-05-04 16:53 UTC, lightside
no flags Details
The poudriere testport log (FreeBSD 10 amd64, with PORT_ENET option enabled): games/assaultcube (5.93 KB, application/x-bzip2)
2015-05-04 16:53 UTC, lightside
no flags Details
Proposed patch (since 385392 revision): for games/assaultcube (1.78 KB, patch)
2015-05-09 06:20 UTC, lightside
lightside: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer 2015-02-13 23:52:47 UTC
Ports of Cube-based games all build libenet, newer version of which is available as net/enet port. It maybe worth to investigate API difference[1] with bundled versions, convert to system one and check interoperability with existing servers/clients doesn't degrade.

games/cube switched a long time ago (see ports r173704). It no longer uses static library since net/libenet started providing shared one.

games/tesseract (in bug 189829) switched by me. It only required the following:

  LIB_DEPENDS=          libenet.so:${PORTSDIR}/net/enet

  USES+=                pkgconfig
  EXTRACT_AFTER_ARGS=   --exclude src/enet

  post-patch:
          @${REINPLACE_CMD} \
                  -e 's,-Ienet/include,`pkg-config --cflags libenet`,' \
                  -e 's,-Lenet -lenet,`pkg-config --libs libenet`,' \
                  -e 's,enet/libenet.a,${LOCALBASE}/lib/libenet.so,' \
                  ${BUILD_WRKSRC}/Makefile

with the following intent:
- do not extract src/enet to avoid accidentally picking up its headers
- replace flags with whatever pkg-config knows about libenet
- force make target to believe libenet was already built

[1] http://upstream.rosalinux.ru/versions/enet.html
Comment 1 Bugzilla Automation freebsd_committer 2015-02-13 23:52:47 UTC
Maintainers CC'd
Comment 3 Jan Beich freebsd_committer 2015-02-14 06:00:21 UTC
Created attachment 152957 [details]
v1

Based on games/cube patches. May also help reduce the scope of bug 197610.

games/assaultcube bundles libenet 1.3.6 with Windows fix from 1.3.7
games/bloodfrontier bundles libenet 1.3 snapshot (just after 1.2.1)
games/cube bundles vanilla libenet 7-18-2005 (before 1.0)
games/redeclipse bundles vanilla libenet 1.3.7
games/sauerbraten bundles libenet 1.3.6 with Windows fix from 1.3.7

poudriere bulk -t for 101i386 logs:
http://slexy.org/view/s20SmxrhdM assaultcube-1.2.0.2_3.log
http://slexy.org/view/s21yQepnbe bloodfrontier-b2_10.log
http://slexy.org/view/s20HJ7nk9F cube-2005.08.29_15.log
http://slexy.org/view/s2fyzuy8ye redeclipse-1.4_3.log
http://slexy.org/view/s2pWodiHJ4 sauerbraten-20130203_3.log
Comment 4 lightside 2015-02-14 08:11:48 UTC
Comment on attachment 152957 [details]
v1

This is possible, but I think, that this is a bad idea in case of games/assaultcube.

Currently, it defines "PROTOCOL_VERSION 1201" (inside of source/src/protocol.h file), the changes of which is coupled with bundled enet source code. There is a need a compatibility between client and server protocol and their packets, in case the game is used for multiplayer.

This what is written (as excerpt) inside of source/README_CUBEENGINE.txt file:
-8<--
CHEATING
========
If you want to use cube as a base for a game where the multiplayer aspect is
important and used by a large community, you need to be aware that cube's
thick client - thin server architecture is extremely cheat sensitive. If you
release a cube based game with source code equivalent to the binaries, some
minor changes can give anyone an aimbot or other cheats in online games.
There are several ways to make this less easy, some of which are:

1. only distribute binaries (the ZLIB license allows this). Executables can still
   be hacked, but unless you have a really large online community, noone will
   probably bother.
2. write a network proxy, such as qizmo used with QuakeWorld (whose sources are
   also open source). The proxy is a small closed source program that checksums
   the executable, and maybe also some game media. You can then make servers
   or game admins request this information and ban cheaters.
3. release the sources with an incompatible network protocol or other changes
   compared to the binaries you release.
4. build serious cheat detection into the game. Since all clients are their own
   "servers", you can make them all keep track of stats for the other players,
   such as health etc. you can make all sorts of consistency checks on shots,
   movement speed and items. If the discrepancy for a certain client becomes
   too big, all clients but the cheater can send their "vote" to the server
   for having him banned. Even better, you can add server side stat checking.

For the cube's own game I chose option 3, i.e. you can only play the official
cube game using the binaries supplied by me, and you can't compile your own clients
for multiplayer use (you can still make custom clients that work with matching
custom servers, or play cube single player maps compatible with the real thing).
This situation is not ideal, but there is no easy way around it.

This scheme is probably very easy to defeat if you are capable of using disassemblers
and packet sniffers, but please contrain yourself and don't ruin the fun of the
cube multiplayer community. Thanks.
-->8-

The AssaultCube developer(s) might have their own opinion(s) about this, but I don't think that we need to pose FreeBSD binaries as cheating, in case of network source code changes, which gives advantage.

I already said in bug 197582, that I can't change maintainer-approval for files which I didn't own or don't have sufficient privileges to do this. So, I reject the parts of games/assaultcube changes in comment for attachment.
Comment 5 lightside 2015-02-14 15:01:52 UTC
Created attachment 152968 [details]
Proposed patch (since 374303 revision): for games/assaultcube

On the other hand, if this is possible, it could be implemented.

I created the proposed patch for games/assaultcube, which allows to use Enet library from ports or bundled one. But I think, that ENET option shouldn't be enabled by default.
Comment 6 lightside 2015-02-14 15:03:17 UTC
Created attachment 152969 [details]
The poudriere testport log (FreeBSD 10 amd64): for games/assaultcube
Comment 7 lightside 2015-02-14 15:04:18 UTC
Created attachment 152970 [details]
The poudriere testport log (FreeBSD 10 amd64, with ENET option enabled): for games/assaultcube
Comment 8 lightside 2015-02-14 21:15:27 UTC
Created attachment 152986 [details]
The games/assaultcube-enet port in shar format

For ENET enabled ports it is possible to create companion ports with "PKGNAMESUFFIX=-enet", "OPTIONS_SET+=ENET" and "CONFLICTS=${PORTNAME}-[0-9]*". This might allow to use them in DragonFly, for example, without the need to (accidentally) change default options, when it is disabled for the (multiplayer) reason.

I attached this kind of port for games/assaultcube-enet.
Comment 9 Jan Beich freebsd_committer 2015-02-15 00:06:29 UTC
Created attachment 152993 [details]
assaultcube-only, system libenet as an option

I may need an input from someone with games@ hat as well to know if the option makes sense and for which ports. Here's a bit different implementation for games/assaultcube:

http://slexy.org/view/s2sFXUmFTo PORT_ENET=off
http://slexy.org/view/s2tnuOtXvX PORT_ENET=on

> -OPTIONS_DEFINE=	DOCS
> +OPTIONS_DEFINE=	DOCS ENET

ENET option name is misleading, as if by default the port is built without libenet support. Another port may add ENET option with different meaning and it would clash if the user has global OPTIONS_SET+=ENET in make.conf.

Alas, no consistency in naming an option to use a system lib:

  devel/gdb: PORT_READLINE
  lang/python*: LIBFFI
  mail/spamass-milter: SENDMAIL_PORT
  net/freeradius*: HEIMDAL_PORT
  net/ngrep: PORTS_PCAP
  net/rsync: POPT_PORT
  games/0ad: PORTSSM

> ENET_DESC=	Use Enet library from ports, instead of bundled one

Maybe include bundled version unless it quickly becomes obsolete. Just in case of CVEs. However, doing so would prevent the _DESC migrating to bsd.options.desc.mk.
Comment 10 lightside 2015-02-15 09:13:46 UTC
Comment on attachment 152993 [details]
assaultcube-only, system libenet as an option

The following line is not needed, in my opinion:
EXTRACT_AFTER_ARGS+=	--exclude enet
But if so, it should be:
EXTRACT_AFTER_ARGS+=	--exclude source/enet

The extra patch with hardcoded DragonFly OPSYS is not needed also. Instead of fixing for one platform, try to fix it for all:
${REINPLACE_CMD} -e '/defined/s|FreeBSD|${OPSYS}|g' \
		${WRKSRC}/source/enet/unix.c
Thanks for the idea how to fix it, by the way.

The following line is mistake:
PORT_ALL_TARGET_OFF=		libenet
Should be:
PORT_ENET_ALL_TARGET_OFF=	libenet
While it worked even without it, because of Makefile dependencies.

The hardcoded version in following option description is also not needed:
PORT_ENET_DESC=	Use libenet from net/enet over bundled 1.3.6
Should be:
PORT_ENET_DESC=	Use libenet from net/enet over bundled
Because newer version of port might use different bundled version.

I will propose my changes in the next comment.
Comment 11 lightside 2015-02-15 09:16:00 UTC
Created attachment 152999 [details]
Proposed patch (since 374303 revision): for games/assaultcube

The proposed patch contains some changes from attachment 152993 [details], with fixes and additions, including some changes from attachment 152968 [details].
Comment 12 lightside 2015-02-15 09:18:06 UTC
Created attachment 153001 [details]
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube
Comment 13 lightside 2015-02-15 09:19:35 UTC
Created attachment 153002 [details]
The poudriere testport log (FreeBSD 10 amd64, with PORT_ENET option enabled): games/assaultcube
Comment 14 lightside 2015-02-15 09:20:43 UTC
Comment on attachment 152986 [details]
The games/assaultcube-enet port in shar format

Obsoleted by attachment 152999 [details].
Comment 15 lightside 2015-02-15 20:33:18 UTC
Created attachment 153016 [details]
Proposed patch (since 374303 revision): for games/assaultcube

(In reply to comment #10)
> Instead of fixing for one platform, try to fix it for all
Not for all, but for some BSD operating systems, including DragonFly.

- Added OPSYS check for FreeBSD.
Comment 16 lightside 2015-02-15 21:20:02 UTC
Created attachment 153018 [details]
Proposed patch (since 374303 revision): for games/assaultcube

Changed PORT_ENET_DESC value to "Use libenet from net/enet", which is more concise, in my opinion. The so called "bundled" version is used as a static library, but net/enet is used as a shared library. No need to oppose them to each other (in description).

I guess, the patch needs to be updated after resolution of the bug 197582 or vice versa. Otherwise there is a need for a merge mechanism, based on the same revision. If this will be the case, then don't hesitate to ask about final result.
Comment 17 Jan Beich freebsd_committer 2015-02-15 21:29:06 UTC
Oh, LICENSE+=MIT should be moved under !PORT_ENET.

(In reply to lightside from comment #10)

> The following line is not needed, in my opinion:

Agree but it's a good insurance nothing else would try to use bundled
library when PORT_ENET is enabled.

> EXTRACT_AFTER_ARGS+=    --exclude enet
> But if so, it should be:
> EXTRACT_AFTER_ARGS+=    --exclude source/enet

Works both ways. The former can be copied as is among Cube ports.

  $ mkdir -p foo/source/enet bar/source/enet
  $ tar cf - foo bar | tar tf - --exclude enet
  foo/
  foo/source/
  bar/
  bar/source/
  $ tar cf - foo bar | tar tf - --exclude source/enet
  foo/
  foo/source/
  bar/
  bar/source/
  $ tar cf - foo bar | tar tf - --exclude foo/source/enet
  foo/
  foo/source/
  bar/
  bar/source/
  bar/source/enet/

>
> The extra patch with hardcoded DragonFly OPSYS is not needed also. Instead of
> fixing for one platform, try to fix it for all:

This should be examined case-by-case. For one, gethostby*_r() has one
argument less on Solaris, NetBSD and OpenBSD. Not that anyone tries to
use FreeBSD ports there.

Also, files/*patch-foo makes it easier to notice when upstream
integrates the changes.

(In reply to lightside from comment #11)

> +	${REINPLACE_CMD} -e '/^CXXFLAGS=/d ; /^CXX=/d ; \
> +		/^PLATFORM_PREFIX=/s|native|${OPSYS}|; \

Maybe refactor out the line. PLATFORM_PREFIX is only used by vendor
install which is completely ignored by do-install.

> +		/^INCLUDES=/s|$$| -I${LOCALBASE}/include| ; \
> +		s|sdl-config|${SDL_CONFIG}| ; \
> +		s|-lcurl|& -lintl| ; \
> +		s|$$(USRLIB)|${LOCALBASE}/lib| ; \
> +		/^SERVER_LIBS=/s|-lz|& -L${LOCALBASE}/lib|' \

Why not keep using the same style? Both within vendor's Makefile and
among substitutions in the port's Makefile. SDL_CONFIG and LOCALBASE
are already exported via MAKE_ENV.

	s|sdl-config|$$(SDL_CONFIG)| ; \
	s|$$(USRLIB)|$$(LOCALBASE)/lib| ; \
	/^INCLUDES=/s|$$| -I$$(LOCALBASE)/include| ; \
	/^CLIENT_LIBS=/s|$$| -lintl| ; \
	/^SERVER_LIBS=/s|$$| -L$$(LOCALBASE)/lib| ; \

> +		${BUILD_WRKSRC}/Makefile
> +.if ${PORT_OPTIONS:MPORT_ENET}

Thanks for removing hard to read ${SED_ENET_RE} from attachment 152968 [details].
Comment 18 lightside 2015-02-15 23:29:00 UTC
(In reply to comment #17)
> Agree but it's a good insurance nothing else would try to use bundled
> library when PORT_ENET is enabled.

The bundled enet library included for a reason, some of which is explained in comment #4. There could be compatibility issues also, when new version of the port might come with newer version of enet library, than ports net/enet version. That's is why PORT_ENET option is disabled by default, in my opinion (for games/assaultcube).

(In reply to comment #17)
> Works both ways. The former can be copied as is among Cube ports.

Yes, it works for the current distfile. I just wanted to be more specific.

(In reply to comment #17)
> Maybe refactor out the line. PLATFORM_PREFIX is only used by vendor
> install which is completely ignored by do-install.

Yes, I spotted this also. Seems like, I did it a long time ago.

(In reply to comment #17)
> Why not keep using the same style? Both within vendor's Makefile and
> among substitutions in the port's Makefile. SDL_CONFIG and LOCALBASE
> are already exported via MAKE_ENV.

The ${BUILD_WRKSRC}/Makefile file doesn't use ${SDL_CONFIG} and ${LOCALBASE} variables. They was added by a static patch. The sed patch just adds them in a direct way, which is the same, if you don't plan to use a different values on the patch and build stages, of course. But it could be returned, with a different style, as you mentioned.

(In reply to comment #17)
> This should be examined case-by-case. For one, gethostby*_r() has one
> argument less on Solaris, NetBSD and OpenBSD. Not that anyone tries to
> use FreeBSD ports there.
> Also, files/*patch-foo makes it easier to notice when upstream
> integrates the changes.

I did it in a sed patch, because it is position independent and requires less maintenance, in case of proper changes. But if this is a case and newer version of net/enet includes these DragonFly changes, then it is more likely, that this patch might be removed in a next version(s).

(In reply to comment #17)
> Thanks for removing hard to read ${SED_ENET_RE} from attachment 152968 [details]
It wasn't hard for maintainer, obviously.
Comment 19 lightside 2015-02-15 23:31:41 UTC
Created attachment 153021 [details]
Proposed patch (since 374303 revision): for games/assaultcube

- Returned files/patch-source_enet_unix.c file from attachment 152993 [details].
- Removed PLATFORM_PREFIX changes from sed patch.
- Changed curly braces to parentheses for defined variables of ${BUILD_WRKSRC}/Makefile sed patch.
Comment 20 lightside 2015-02-15 23:45:33 UTC
(In reply to comment #19)
> - Returned files/patch-source_enet_unix.c file from attachment 152993 [details].

I renamed the "extra-source_enet_unix.c" to "patch-source_enet_unix.c" filename. This means, that it will be applied independently from PORT_ENET option. It will be easier to check the changes for a newer version, in my opinion.
Comment 21 lightside 2015-02-16 18:41:43 UTC
Created attachment 153045 [details]
The games/assaultcube-enet port in shar format

I created another version of games/assaultcube-enet port, without conflicts with games/assaultcube.
It depends from games/assaultcube port and just installs ${PORTNAME}_enet_* variants of files.
So, it is just like a component port, which allows more choice, without excluding each other.
Comment 22 lightside 2015-02-16 18:44:03 UTC
Created attachment 153046 [details]
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube-enet
Comment 23 lightside 2015-02-16 19:21:25 UTC
Created attachment 153048 [details]
The games/assaultcube-enet port in shar format

The equality comparison (==) for version check of run-time dependency also works.
The changed games/assaultcube-enet port attached.
Comment 24 lightside 2015-02-16 19:49:00 UTC
Created attachment 153056 [details]
The games/assaultcube-enet port in shar format

(In reply to comment #23)
> The equality comparison (==) for version check of run-time dependency
> also works.

No, the "greater or equal" comparison (>=) is needed for this case.
Also, there is need to consider @dir for empty directories.
Comment 25 commit-hook freebsd_committer 2015-02-20 06:54:13 UTC
A commit references this bug:

Author: jbeich
Date: Fri Feb 20 06:53:39 UTC 2015
New revision: 379412
URL: https://svnweb.freebsd.org/changeset/ports/379412

Log:
  - Fix invalid dereferencing of null reference which causes startup
    crash for cube_client when built with clang 3.6 + -O1 or higher [1]
  - Properly track libenet dependency [2]

  PR:		197604 [1]
  PR:		197605 [2]
  Submitted by:	dim [1]

Changes:
  head/games/cube/Makefile
  head/games/cube/files/patch-entities.cpp
  head/games/cube/files/patch-physics.cpp
  head/games/cube/files/patch-protos.h
  head/games/cube/files/patch-rendermd2.cpp
Comment 26 Dmitry Marakasov freebsd_committer 2015-02-27 20:54:47 UTC
Is there a real reason to unbundle enet? Looks dangerous to me as games may use local modifications or depend on specific version.

I'd contact authors on per-port basis and ask if unbundling enet is OK, and whether such option could be implemented upstream. On the ports side, creating XXX-enet ports would be bad idea, I'd go with an option.
Comment 27 lightside 2015-02-27 21:42:09 UTC
(In reply to comment #26)
> Is there a real reason to unbundle enet?

I think, the one of the reasons is a availability of new enet version in ports.

(In reply to comment #26)
> Looks dangerous to me as games may use local modifications or depend on
> specific version.

I think the same. Therefore, I don't recommend to enable this by default, e.g. for compatibility with existing servers.

(In reply to comment #26)
> On the ports side, creating XXX-enet ports would be bad idea,
> I'd go with an option.

It depends from what kind of XXX-enet port you create. In case of games/assaultcube-enet version, I managed to create a component's kind of port, which just installs *-enet version of files and don't conflict with games/assaultcube version.

This is just my opinion, which might be different from opinion of PR's creator.
Comment 28 Jan Beich freebsd_committer 2015-02-28 12:28:33 UTC
(In reply to Dmitry Marakasov from comment #26)
> Is there a real reason to unbundle enet?

In order to maintain and track security issues in only one version.
Bug 197610 and CVE-2006-119{4,5} (in games/cube) are examples or
games/bloodfrontier failing to mention MIT license of bundled libenet.

https://www.freebsd.org/doc/en/books/porters-handbook/bundled-libs.html

> Looks dangerous to me as games may use local modifications

Instead of speculating I did check in comment 3 and there're none.

> or depend on specific version.

Only games/cube and games/bloodfrontier required patching API usage.
And since libenet-1.3.6 enet/protocol.h had only minor adjustments:

https://github.com/lsalzman/enet/commit/e19dc9f
https://github.com/lsalzman/enet/commit/ea8d41f

Maintainers can nudge upstream to update bundled copy to gain
more confidence.

> I'd contact authors on per-port basis and ask if unbundling enet is
> OK, and whether such option could be implemented upstream.

The burden is partially alleviated by other distributions already
using system libenet. Keep in mind that upstream often bundle
libraries because of Windows or to minimize dependencies.

> On the ports side, creating XXX-enet ports would be bad idea,

Agree. PORT_ENET would have very minor if any user impact. Slave ports
may make more sense for SDL 1.2 vs. SDL 2.0 case.

> I'd go with an option.

As long as the option name is consistent across different ports...
Also, backporting bug 197610 or any future fix for ports consumers
is a must then.

Thanks for feedback. Now I have an idea how to proceed to v2.
Comment 29 lightside 2015-02-28 13:59:46 UTC
(In reply to comment #28)
> Maintainers can nudge upstream to update bundled copy to gain
> more confidence.

I don't think so. This is your personal opinion.
The same for most parts of comment #28.

I think, better to "unbundle" changes for different ports and maintainers, instead of nudging them as a group, where some maintainer(s) have a different opinion(s) about proposed changes and how they need to be implemented. How these changes might be committed is a different story.
Comment 30 lightside 2015-02-28 17:08:31 UTC
(In reply to comment #29)

Looks like, I commented on the reply, which is not based on my comment(s).
There is no need to repeat my opinion. I approved the changes for games/assaultcube port, which might be committed. Other changes might be discussed.
Comment 31 lightside 2015-03-02 06:14:29 UTC
Created attachment 153656 [details]
The games/assaultcube-enet port in shar format

- Added comment about purpose of games/assaultcube-enet port to Makefile
- Explicitly set PORTREVISION [*]
- Excluded DOCS option
- Changed run-time dependency to DATADIR file
- Removed changes to _LICENSE_DIR, because it based on PKGNAME, which is different for the master port

* There is a need to use "?=" instead of "=" for PORTREVISION of master port, in case of changes for *-enet version.

This attachment considered experimental. It shows, that there is a possibility to allow different opinions without the need to choose only one version (with or without PORT_ENET option) or introduce conflicts on ports/packages level.
Comment 32 lightside 2015-03-02 06:15:27 UTC
Created attachment 153657 [details]
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube-enet
Comment 33 Dmitry Marakasov freebsd_committer 2015-03-02 17:32:10 UTC
(In reply to lightside from comment #27)
> It depends from what kind of XXX-enet port you create.

It doesn't. If the original port still uses bundled enet, this solves so problem, and it adds unnecessary complexity for both maintainers and users. I suspect *-enet port will never be used anyway. So I'm strictly against it.

>> Maintainers can nudge upstream to update bundled copy to gain more confidence.

I agree. Such intrusive changes should only be performed after at least consulting with upstream. If their safety is confirmed, enet may be unbundled by default (I'd still leave an option to keep changes isolated and for the case if unbundled version breaks for some reason in the future).
Comment 34 lightside 2015-03-03 05:29:51 UTC
(In reply to comment #33)
> It doesn't. If the original port still uses bundled enet, this solves so
> problem, and it adds unnecessary complexity for both maintainers and users. I
> suspect *-enet port will never be used anyway. So I'm strictly against it.

In my case, the *-enet version allow to use both types of executables. This version might be built as package also. On the ports level, the users might change options on their preference and decide what they really want. At least, I have an answer to users who want to use package with PORT_ENET option and users who doesn't, without possible PRs (or accidental commits), which want exact option to enable or disable. This is kind of port(s), which might be used for multiplayer with cheat protection mechanisms, where the answer is not quite easy. This is what described inside of source/README.txt of games/assaultcube:
-8<--
The cheat protection described in README_CUBEENGINE.txt is not used in AssaultCube. Binaries 
compiled from source are compatible with the official binaries. This means, the client can 
easily be hacked, on the other hand, you can compile AssaultCube for different platforms and 
fix bugs yourself.
You MUST NOT connect to servers that are not yours with a modified client that contains changes 
that were not made for platform-compatibility or to fix bugs. This would be cheating. Thank you 
for supporting fair play gaming.
-->8-
The new version of net/enet doesn't mean it's better (even if it is, from developer point of view, where current 1.3.6 and 1.3.12 versions are quite compatible). At least, the user might decide and have their consequences, but not because of maintainer.
Personally, I tested local AssaultCube server with 1.3.6 (1.3.12) version and connected to it from client with 1.3.12 (1.3.6) version and it worked from player perspective. This kind of test was possible, because of available *-net version of port, which doesn't conflict with master port.

(In reply to comment #33)
> I agree. Such intrusive changes should only be performed after at least
> consulting with upstream. If their safety is confirmed, enet may be unbundled
> by default (I'd still leave an option to keep changes isolated and for the case
> if unbundled version breaks for some reason in the future).

In my case, the version of enet is 1.3.6 still, even after more than 2 years of development:
https://github.com/assaultcube/AC/blob/3ff01a1/source/enet/ChangeLog
I guess, the new version might be committed with newer version of AssaultCube, based on previous commits, and there is a reason to do so.
Comment 35 lightside 2015-03-03 07:14:23 UTC
(In reply to comment #34)

In case of games/assaultcube, there is a so called "next" development branch:
https://github.com/assaultcube/AC/commits/next
According to it, there are some changes to 1.3.6 version, e.g. "add incremental crc calculation":
https://github.com/assaultcube/AC/commit/64f3781
The net/enet v1.3.12 doesn't have "enet_crc32_inc" function, for example:
https://github.com/lsalzman/enet/blob/9895a4f/include/enet/enet.h
Therefore, in case of these changes and not the same upstream changes for net/enet, the newer version of AssaultCube might be not compatible. While this is not the case for current version.
Comment 36 Dmitry Marakasov freebsd_committer 2015-03-03 14:03:14 UTC
(In reply to lightside from comment #34)
> In my case, the *-enet version allow to use both types of executables.

As I've said, that's useless.

-8<--
> You MUST NOT connect to servers that are not yours with a modified client that
> contains changes 
> that were not made for platform-compatibility or to fix bugs. This would be
> cheating. Thank you 
> for supporting fair play gaming.
-->8-

This doesn't prohibit using newer enet version.

Summarizing, if the port works and/or upstream says it's OK, the port should have option which unbundles enet enabled, instead disabled.
Comment 37 Dmitry Marakasov freebsd_committer 2015-03-03 14:05:11 UTC
(In reply to lightside from comment #35)
That is also why you need option - if update can't work with unbundled enet, you just disable/remove the option. Otherwise, you'll have broken -enet port.
Comment 38 lightside 2015-03-03 17:45:54 UTC
(In reply to comment #36)
> This doesn't prohibit using newer enet version.

And doesn't prohibit to use "bundled" enet version, which used to build Linux, Mac OS X and Windows executables.

(In reply to comment #37)
> That is also why you need option - if update can't work with unbundled enet,
> you just disable/remove the option. Otherwise, you'll have broken -enet port.

I already implemented such kind of option (based on proposed changes), if you saw the patches. The *-enet version has different purpose, which I already explained. In my opinion, the master port (games/assaultcube) should be built with bundled version by default. But I don't share this opinion to the ports, which you maintain, because this is you, who decide. I think, if there were more frequent updates (with related changes to the protocol version), this kind of PR might be not so important.
Comment 39 lightside 2015-03-03 19:32:21 UTC
(In reply to comment #38)
> But I don't share this opinion to the ports, which you maintain,
> because this is you, who decide.
In other words:
What I said in this PR is related to changes of games/assaultcube only. I don't force my opinion to other ports in this PR. There are different maintainers (with their opinions), who could decide on their own.

What might be committed is depends from PR's creator (who is committer also).
This PR is reasonable and possible to implement.
Comment 40 lightside 2015-03-15 18:29:47 UTC
Created attachment 154399 [details]
Proposed patch (since 381324 revision): for games/assaultcube

The proposed patch updated after ports r381324 changes.
Comment 41 lightside 2015-03-15 18:30:22 UTC
Created attachment 154400 [details]
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube
Comment 42 lightside 2015-03-15 18:30:52 UTC
Created attachment 154401 [details]
The poudriere testport log (FreeBSD 10 amd64, with PORT_ENET option enabled): games/assaultcube
Comment 43 lightside 2015-03-15 19:07:08 UTC
Created attachment 154402 [details]
Proposed patch (since 381324 revision): for games/assaultcube

Slightly changed sed patches with the same result.
Comment 44 lightside 2015-04-13 19:02:44 UTC
Created attachment 155563 [details]
Proposed patch (since 381324 revision): for games/assaultcube

Adapt patches in files directory for games/assautlcube port, after ports r383894 changes for ports-mgmt/portlint v2.16.3.
Comment 45 lightside 2015-05-04 16:52:00 UTC
Created attachment 156341 [details]
Proposed patch (since 385392 revision): for games/assaultcube

Updated the patch for games/assaultcube after ports r385392 changes.
Comment 46 lightside 2015-05-04 16:53:00 UTC
Created attachment 156342 [details]
The poudriere testport log (FreeBSD 10 amd64): games/assaultcube
Comment 47 lightside 2015-05-04 16:53:31 UTC
Created attachment 156343 [details]
The poudriere testport log (FreeBSD 10 amd64, with PORT_ENET option enabled): games/assaultcube
Comment 48 Dmitry Marakasov freebsd_committer 2015-05-09 02:27:06 UTC
Well this looks much better than assaultcube-enet nonsense. Though I suggest to not use options helpers for PORT_ENET option here: logic should not be split into multiple chunks as it's hard to understand and manage, and *_OFF helpers are especially counter-intuitive. E.g. use

.if ${PORT_OPTIONS:MPORT_ENET}
LIB_DEPENDS+=
.else
GNU_CONFIGURE=
CONFIGURE_WRKSRC=
CONFIGURE_ARGS+=
ALL_TARGET+=
.endif
Comment 49 lightside 2015-05-09 06:20:26 UTC
Created attachment 156532 [details]
Proposed patch (since 385392 revision): for games/assaultcube

(In reply to comment #48)
> Well this looks much better than assaultcube-enet nonsense.

Actually, it looks the same as before (attachment 155563 [details]), but some parts was merged in ports r385392.
The games/assaultcube-enet port (in shar format) is a different port as a component for games/assaultcube, which allows to coexist two packages: with and without external libenet.so dependency. I don't propose this port, if there are no interest in it, but it is there, because I created it (and it works).

(In reply to comment #48)
> I suggest to not use options helpers for PORT_ENET option here:
> logic should not be split into multiple chunks as it's hard to understand and manage

Ok, I attached this variant of proposed patch.
Comment 50 commit-hook freebsd_committer 2015-05-09 12:44:12 UTC
A commit references this bug:

Author: amdmi3
Date: Sat May  9 12:43:56 UTC 2015
New revision: 385887
URL: https://svnweb.freebsd.org/changeset/ports/385887

Log:
  - Add optional support for using newer libenet from ports

  PR:		197605
  Submitted by:	lightside@gmx.com (maintainer)

Changes:
  head/games/assaultcube/Makefile
Comment 51 Dmitry Marakasov freebsd_committer 2015-05-09 12:45:30 UTC
Great, committed. Similar changes are welcome for other cube-based ports. I propose to handle them in separate PRs and close this one.
Comment 52 Dmitry Marakasov freebsd_committer 2015-05-26 15:46:53 UTC
Closing it then.