New port: devel/py-cdg
ClosedPublic

Authored by jonathan on Jul 20 2017, 7:29 PM.

Details

Reviewers
bdrewery
koobs
Group Reviewers
Python
Commits
rP452495: New port: devel/py-cdg
Summary

Although it doesn't (yet) look like much, this is a Python library for
manipulating control- and data-flow graphs. The long-term goal is to
provide a language-agnostic data format for describing control-flow
graphs (including callgraphs) and data-flow graphs as an intermediate
representation between language-facing tools like LLVM and user-facing
tools like Cytoscape, Dot and (maybe someday) a better cscope.

Test Plan

portlint: OK (looks fine.)
testport: OK (poudriere: 0.1.3, amd64, no options, Python v2.7, https://allendale.engr.mun.ca/poudriere-logs/cadets-head-ports-cadets/2017-09-09_14h24m18s/build.html)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jonathan created this revision.Jul 20 2017, 7:29 PM
bdrewery requested changes to this revision.Jul 20 2017, 10:41 PM
bdrewery edited reviewers, added: Python; removed: portmgr.
bdrewery added a subscriber: bdrewery.
bdrewery added inline comments.
devel/py-cdg/Makefile
22 ↗(On Diff #31024)

This include doesn't seem needed

This revision now requires changes to proceed.Jul 20 2017, 10:41 PM
koobs added a subscriber: koobs.Jul 22 2017, 5:24 AM
koobs added inline comments.
devel/py-cdg/Makefile
9 ↗(On Diff #31024)

Should match setup.py:description (ideally short_description) as closely as possible within the context of portlint/COMMENT format compliance. ie, currently, Software callgraph manipulation

Update upstream description/short_description if the comment can be improved

11 ↗(On Diff #31024)

Add LICENSE to MANIFEST.in upstream (along with any other files that should be in the sdist), so that LICENSE_FILE=${WRKSRC}/LICENSE can be added in the port.

12 ↗(On Diff #31024)

'install_requires': ['networkx', 'nose', 'pygraphviz'], are listed in setup.py:install_requires but are not referenced in RUN_DEPENDS

Also, its very unlikely nose is an install_requires (its only listed as a TEST_DEPENDS here). If it is a test only requirement (and a compulsory one at that), it can/should be listed in setup.py:tests_require.

Extra but non-compulsory requirements should be listed instead setup.py:extras_require

13 ↗(On Diff #31024)

There is no such FreeBSD port/packagenamepy-nosetests/pyXY-nosetests. The port name is py-nose, the package name is pyXY-nose.

If you intended to depend on nose script/binary name nosetests, then depending on a python binary without a version suffix requires that the port is 'concurrent safe', ie; installs *both* scriptname and scriptname-X.Y.

This only occurs with packages who's upstream code does this themselves, or ports that explicitly use USE_PYTHON=concurrent.

Even for ports/packages that install version-suffixed script names, it is not recommended to use a version-suffix-LESS scriptname (ie; nosetests), as the version of Python this script invokes can be changed at anytime after installation by the user.

One should explicitly either use scriptname-${PYTHON_VER}, or better in almost all circumstances, depend on the python-version-specific-package-name, like:

TEST_DEPENDS?= ${PYTHON_PKGNAMEPREFIX}nose>0:devel/py-nose

16 ↗(On Diff #31024)

Use CHEESESHOP (PyPI) as the default MASTER_SITES for Python package ports unless there is an explicit and compelling reason to use another source (in this case GitHub).

An example of a legitimate (albeit temporary) reason is that important files (example: test/test data files) are not included in the source distribution (sdist) from PyPI, and so tests can't be run without using the github sources until upstream fixes the issue (adding the files/dirs to MANIFEST.in and re-uploading the sdist in a future version).

17 ↗(On Diff #31024)

Given this package has a 'scripts': ['bin/cdg-dot'], (ie; provides an end-user usable script), the port should be made "concurrent safe". That is, all files it installs should be named uniquely so that multiple installations of the port for use with multiple python versions concurrently do not conflict. This can be done with USE_PYTHON=concurrent and verifying the pkg-plist contents (make makeplist)

24 ↗(On Diff #31024)

Use do-test: instead, and the build target dependency is no longer requires (since test framework elements was added to ports infrastructure)

devel/py-cdg/pkg-descr
1 ↗(On Diff #31024)

Correct (upstream, registered pypi name) name is cdg

koobs requested changes to this revision.Jul 22 2017, 5:24 AM
jonathan marked 9 inline comments as done.Aug 11 2017, 9:30 AM

Thanks very much for the feedback: I learned a lot about Python packaging!

I've addressed the issues that mentioned locally but arcanist is currently segfaulting for me. Hopefully I'll manage to upload my fixes soon.

jonathan updated this revision to Diff 31935.Aug 11 2017, 10:02 AM
  • New port: devel/py-cdg
  • Address comments in Phab review
koobs requested changes to this revision.Aug 11 2017, 10:41 AM

This looks much better :)

Please confirm this port passes (QA) by adding the following to TEST PLAN sections after testing:

portlint: OK (looks fine.)
testport: OK (poudriere: <versions>, <archs>, <options>, <python-versions> tested)
This revision now requires changes to proceed.Aug 11 2017, 10:41 AM
koobs added inline comments.Aug 11 2017, 10:43 AM
devel/py-cdg/Makefile
15 ↗(On Diff #31935)

Shouldn't use/need ?= unless this is a master port

sunpoet added inline comments.
devel/py-cdg/Makefile
7 ↗(On Diff #31935)

Please uncomment PKGNAMEPREFIX.

jonathan updated this revision to Diff 32597.Sep 1 2017, 6:26 PM
  • Change an unnecessary '?=' to '='.
  • Uncomment PKGNAMEPREFIX as requested by sunpoet.
  • Update py-cdg to HEAD
jonathan edited the test plan for this revision. (Show Details)Sep 1 2017, 6:27 PM
jonathan edited the test plan for this revision. (Show Details)Sep 9 2017, 5:14 PM
jonathan marked 2 inline comments as done.

Ok, I think that addresses the last of the reviewer comments?

koobs accepted this revision.Sep 11 2017, 5:41 AM
Closed by commit rP452495: New port: devel/py-cdg (authored by jonathan, committed by ). · Explain WhyOct 20 2017, 12:01 AM
This revision was automatically updated to reflect the committed changes.