Page MenuHomeFreeBSD

[NEW PORT] www/py-bench: Frappe / ERPNext apps setup tool
ClosedPublic

Authored by loader on Jul 17 2015, 6:21 PM.

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.

Event Timeline

loader updated this revision to Diff 7054.Jul 17 2015, 6:21 PM
loader retitled this revision from to [New Port] www/py-bench.
loader updated this object.
loader edited the test plan for this revision. (Show Details)
loader added a reviewer: koobs.
koobs added inline comments.Jul 27 2015, 5:35 AM
www/py-bench/Makefile
4 ↗(On Diff #7054)

This doesn't appear to be the same as PyPI's bench package, and thus is incorrectly named and conflicts:

https://pypi.python.org/pypi/bench

  • Needs to be renamed
  • I would strongly suggest creating an upstream issue to request the author submits this package to PyPI, thereby giving us a correct canonical package name to use, rather than making one up now and having to rename it later.
5 ↗(On Diff #7054)

I would look at whether DISTVERION might be better than PORTVERSION here due to the letter in the version

10 ↗(On Diff #7054)

Upstream (and setup.py) description is:

Benchmark resources usage

30 ↗(On Diff #7054)

Some Python ports install setup.py:console_script's (in LOCALBASE/bin), as this one does:

https://github.com/frappe/bench/blob/master/setup.py#L21

Try using USE_PYTHON=concurrent, which will ensure that installed script filenames have the correct Python version suffix for the users installed/desired version of Python.

Note: Some python packages (not this one) install already version-suffixed scripts. In this case, one should stop (rm) those from being installed, and use USE_PYTHON=concurrent to do it instead.

Everything else aside, the end result of a Python port installation (that supports multiple versions of Python) should always be:

  • A non-version-suffixed script, which symlinks to script-<versionsuffix>
  • A version-suffixed script for the version of Python the port has been built with (ports), or installed for (package)
www/py-bench/files/patch-setup.py
8 ↗(On Diff #7054)

Why as this needed?

If it's a bug fix, has it been submitted upstream?

If so, add patch header breadcrumbs/comments with description and link references

17 ↗(On Diff #7054)

I would probably use >= in this case, rather than no version at all.

Update the RUN_DEPENDS version accordingly.

koobs retitled this revision from [New Port] www/py-bench to [NEW PORT]] www/py-bench: Frappe / ERPNext apps setup tool.Jul 27 2015, 5:35 AM
koobs added a subscriber: Python.
koobs requested changes to this revision.Jul 27 2015, 5:38 AM
koobs retitled this revision from [NEW PORT]] www/py-bench: Frappe / ERPNext apps setup tool to [NEW PORT] www/py-bench: Frappe / ERPNext apps setup tool.
koobs edited edge metadata.
This revision now requires changes to proceed.Jul 27 2015, 5:39 AM
loader added inline comments.Jul 27 2015, 7:57 AM
www/py-bench/Makefile
4 ↗(On Diff #7054)
5 ↗(On Diff #7054)

The last release version v0.92 on GitHub needs a lot changes to work, what is the best solution for this kind of ports if I have to use the master branch? can I just the version number in setup.py + date, like PORTVERSION= 0.1.20150713? or is it possible to use the ${GH_TAGNAME} as the port version?

10 ↗(On Diff #7054)

This is the package on PyPI :)

30 ↗(On Diff #7054)

Thanks for the tip. I will add it.

www/py-bench/files/patch-setup.py
8 ↗(On Diff #7054)

I'm new to this, please correct me if I'm wrong.

py_modules=['foo'] installs foo.py to lib/site-packages/foo.py
packages=['foo'] installs foo directory to lib/site-packages/foo/

and this port needs to install the bench/ directory.

17 ↗(On Diff #7054)

Okay, I will upload a new patch when I got their reply for new port name. Thank your help.

koobs added inline comments.Jul 27 2015, 10:41 AM
www/py-bench/Makefile
5 ↗(On Diff #7054)

The only reason for suggesting DISTVERSION was because of 's' character in PORTVERSION, but I can't tell where it came from. Typo? Intentional?

Regardless of the reason, using DISTVERSION for these kinds of non-numeric version strings, automatically derives a 'sane' PORTVERSION value, that is less prone to issues.

Specifically, this is related to the requirement for PORTVERSION strings only to 'increase in their values, never decrease (thus requiring a PORTEPOCH bump), and the need to be able to say a version is less-than or greater-than another value, which is more difficult with alphabetic characters in version strings.

See: http://www2.au.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/book.html#idp57382352

Key part:

Some vendor's distribution names which do not fit into the ${PORTNAME}-${PORTVERSION}-scheme can be handled automatically by setting DISTVERSION. PORTVERSION will be derived from it automatically.

Having said all that (above), and since looking at the upstream code, it appears that the version scheme upstream is using is currently X.YZ, and that the version value (0.1) in setup.py is just plain wrong, and needs to be fixed to match the current 'proper' version.

An issue should be created for that.

This brings us to the last bit, that is, "the latest 'release' version is broken or doesn't have fixes that haven't been released yet'.

In this case, something like PORTVERSION.<DATESTAMP> is OK, but only if the new version that is greater-than the previous.

I'm not particularly a fan of 'making up version strings', and I tend to use the following alternatives:

  • Backport the patches, pending a new release, OR
  • Ask upstream to tag a new release (and update PyPI if its there)

TLDR: Upstream needs to fix their packaging, versioning and release practices

10 ↗(On Diff #7054)

Sorry, I meant to delete this comment. *shakes fist at phabricator*. Ignore.

27 ↗(On Diff #7054)

If you end up getting upstream to tag a new release, use that tag here.

www/py-bench/files/patch-setup.py
8 ↗(On Diff #7054)

I just wanted to understand the need (certainly I know packages=['foo'] is the most common way), and whether the port doesn't work or breaks other things without this patch ..

If things don't work without this patch, or the package doesn't install the way, or where it is supposed to according to Python packaging rules/conventions, the upstream should be fixed, and links/references/description added to the patch.

loader updated this revision to Diff 7544.Jul 31 2015, 11:29 AM
loader edited edge metadata.

Hi @koobs, thanks so much for your help. I really appreciate it.

  • changed PORTNAME to py-frapp-bench [0]
  • changed PORTVERSION to the latest release 0.92 and removed GH_TAGNAME
  • patch-a93acec is a patch of changes from v0.92 to master head
  • submitted setup.py py_modules => pkackages to upstream [1]
  • updated the version number in setup.py to 0.92 [2]
  • submitted contab(1) fix for FreeBSD to upstream [3]

[0]: https://github.com/frappe/bench/issues/127
[1]: https://github.com/frappe/bench/pull/129
[2]: https://github.com/frappe/bench/pull/131
[3]: https://github.com/frappe/bench/pull/132

Ooops, sorry, it should be be py-frappe-bench, I will upload another patch....

loader updated this revision to Diff 7545.Jul 31 2015, 1:19 PM
loader edited edge metadata.

s|frapp|frappe|

poudriere log: http://p.defau.lt/?qW72rgomGnEbrkrx8nNEIQ
% portlint -AC
looks fine.

koobs accepted this revision.Jul 31 2015, 1:30 PM
koobs edited edge metadata.

Thanks @loader, this looks much better! You're doing AWESOME :)

If this builds successfully, im happy for you to commit.

Regarding commit log conventions/formats, I suggest:

category/port: Short summary

* Itemized changes
* Itemized changes

In NEW PORT cases:

[NEW] category/portname: COMMENT

`cat pkg-descr`
This revision is now accepted and ready to land.Jul 31 2015, 1:30 PM
This revision was automatically updated to reflect the committed changes.