Bug 196304 - games/0ad: Use system spidermonkey24
Summary: games/0ad: Use system spidermonkey24
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: Guido Falsi
URL:
Keywords: needs-qa, patch
Depends on:
Blocks:
 
Reported: 2014-12-27 19:09 UTC by Kevin Zheng
Modified: 2014-12-29 23:46 UTC (History)
1 user (show)

See Also:
madpilot: maintainer-feedback+


Attachments
Patch to use system spidermonkey24 (2.44 KB, patch)
2014-12-27 19:09 UTC, Kevin Zheng
no flags Details | Diff
Poudriere build log (161.96 KB, text/x-log)
2014-12-27 19:11 UTC, Kevin Zheng
no flags Details
Updated patch with no sanity-checking at all (1.81 KB, patch)
2014-12-28 22:55 UTC, Kevin Zheng
no flags Details | Diff
0ad.diff (2.66 KB, patch)
2014-12-29 19:17 UTC, Guido Falsi
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Zheng 2014-12-27 19:09:24 UTC
Created attachment 150998 [details]
Patch to use system spidermonkey24

0AD uses SpiderMonkey, but ships with its own copy. Use the port tree's version.
Comment 1 Bugzilla Automation freebsd_committer 2014-12-27 19:09:24 UTC
Auto-assigned to maintainer madpilot@FreeBSD.org
Comment 2 Kevin Zheng 2014-12-27 19:11:43 UTC
Created attachment 150999 [details]
Poudriere build log

This patch builds and runs the game correctly. Is a PORTREVISION bump required?
Comment 3 Guido Falsi freebsd_committer 2014-12-28 21:29:15 UTC
Hi,

The patch looks correct but I'll need a little time to test it. I'll report back on testing soon.

While at it, I'd rather apply this as an option to the port. The 0ad development team does not support using external spidermonkey.

I have no objection to supporting it but I'd rather have this as a non default option due to this. In this case no PORTREVISION bump would be required.

Thanks!
Comment 4 Guido Falsi freebsd_committer 2014-12-28 21:34:33 UTC
Hi again,

I also noticed the patch adds a LICENSE section, but I'm not sure this software can be said to be GPLv2 as a whole.

It contains code from various sources which comes in a mix of licenses. That's the reason I did never add a LICENSE line.

You can check the licensing details in the LICENSE.txt file at the root of the distribution and see that it's quite difficult to define the license of the project as a whole.

So I'll still avoid to add a LICENSE to the port.
Comment 5 Kevin Zheng 2014-12-28 22:55:42 UTC
Created attachment 151052 [details]
Updated patch with no sanity-checking at all

Ahh, I didn't realize that the LICENSE issue was a bit more complex. What about splitting up the data into a separate port? That would also mean being able to remove the MANUAL_PACKAGE_BUILD line in the main port.

I've attached a tentative, *untested* patch. This adds the SPIDERMONKEY option. I couldn't figure out how to get the inverted-logic OPTIONS_SUB.
Comment 6 Guido Falsi freebsd_committer 2014-12-29 19:15:48 UTC
(In reply to Kevin Zheng from comment #5)
> Created attachment 151052 [details]
> Updated patch with no sanity-checking at all
> 
> Ahh, I didn't realize that the LICENSE issue was a bit more complex. What
> about splitting up the data into a separate port? That would also mean being
> able to remove the MANUAL_PACKAGE_BUILD line in the main port.

Splitting the data would not solve the license part, since the binary itself contains and uses mixed license pieces.

I'm not too sure splitting the port is a good idea anyway. The executable without the data is of no use, and the data part would anyway need the MANUAL_PACKAGE_BUILD option.

I also remember some discussion about such practice being discouraged for new ports, but I don't know if that's still the general consensus.

I'd rather keep the port in one single piece though and I don't think that LICENSE is a good reason to split the port.

The LICENSE framework isn't really up to the task of tracking projects with complex license mixes.

> 
> I've attached a tentative, *untested* patch. This adds the SPIDERMONKEY
> option. I couldn't figure out how to get the inverted-logic OPTIONS_SUB.

OPTIONS_SUB produces both OPTION and NO_OPTION substitutions. You can check it by using "make -V PLIST_SUB" in the directory of a port with options.

Also your patch uses SPIDERMONKEY_CONFIGURE_WITH, but that helper will only add the --with/--without to "CONFIGURE_ARGS", which this port does not use. I use my own variable and modify the pre-build target.

I'm testing a patch almost equivalent to the one you posted, which takes advantage of the NO_OPTION construct. I'm also using a different option name.

I'll also bump PORTREVISION, so users will notice the new option.

I'll attach my patch to this but report shortly, and commit it as soon as I'm done with testing.

Please allow me a little time for testing. I'm away from home and have limited resources with me, so I'll be a little slow, but I'm working on it in the free time.
Comment 7 Guido Falsi freebsd_committer 2014-12-29 19:17:56 UTC
Created attachment 151084 [details]
0ad.diff

Patch I'm testing.
Comment 8 Guido Falsi freebsd_committer 2014-12-29 23:46:05 UTC
Committed. Thanks!
Comment 9 commit-hook freebsd_committer 2014-12-29 23:46:05 UTC
A commit references this bug:

Author: madpilot
Date: Mon Dec 29 23:45:41 UTC 2014
New revision: 375810
URL: https://svnweb.freebsd.org/changeset/ports/375810

Log:
  Add option to allow linking to lang/spidermonkey24 port instead of
  using the boundled one.

  PR:		196304
  Submitted by:	Kevin Zheng <kevinz5000 at gmail.com>

Changes:
  head/games/0ad/Makefile
  head/games/0ad/pkg-plist