New port: devel/py-cdg
Needs ReviewPublic

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

Details

Reviewers
bdrewery
koobs
Group Reviewers
Python
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

Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11334
Build 11699: arc lint + arc unit
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
23

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
10

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

12

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.

13

'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

14

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

17

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).

18

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)

25

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
2

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
16

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

sunpoet added inline comments.
devel/py-cdg/Makefile
8

Please uncomment PKGNAMEPREFIX.

jonathan updated this revision to Diff 32597.Fri, Sep 1, 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)Fri, Sep 1, 6:27 PM
jonathan edited the test plan for this revision. (Show Details)Sat, Sep 9, 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.Mon, Sep 11, 5:41 AM