Bug 220596 - [NEW PORT] shells/xonsh: Python-ish BASH-wards shell
Summary: [NEW PORT] shells/xonsh: Python-ish BASH-wards shell
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: Matthew Seaman
URL: https://reviews.freebsd.org/D8475
Keywords: feature, needs-patch, needs-qa
Depends on:
Blocks:
 
Reported: 2017-07-10 09:58 UTC by Roberto Fernandez Cueto
Modified: 2017-09-22 14:58 UTC (History)
4 users (show)

See Also:
koobs: maintainer-feedback? (roberfern)


Attachments
Diff to the port tree (1.45 KB, text/plain)
2017-07-10 09:58 UTC, Roberto Fernandez Cueto
no flags Details
Output from make makeplist (24.00 KB, text/plain)
2017-07-10 13:03 UTC, Roberto Fernandez Cueto
no flags Details
Actual patch of xonsh (1.30 KB, patch)
2017-07-10 13:29 UTC, Roberto Fernandez Cueto
no flags Details | Diff
Fix outstanding problems with the port (1.40 KB, patch)
2017-08-02 14:32 UTC, Matthew Seaman
matthew: maintainer-approval? (roberfern)
Details | Diff
Final diff of xonsh for FreeBSD (2.85 KB, patch)
2017-09-22 14:06 UTC, Roberto Fernandez Cueto
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roberto Fernandez Cueto 2017-07-10 09:58:23 UTC
Created attachment 184224 [details]
Diff to the port tree

I would like to have the xonsh shell as part of the port tree.

I have created some time ago a differential for the port tree (D8475).

The diff is attached also  as plain text.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 10:17:11 UTC
If this is the same xonsh as listed in PyPI [1]:

- Update version: 0.5.12
- Add PKGNAMEPREFIX = PYTHON_PKGNAMEPREFIX
- Add 'python' as secondary (virtual) category
- Use CHEESESHOP (PyPI) as the MASTER_SITES (removing USE_GITHUB) unless there is a compelling reason not to use it (eg: important files missing from sdist)
- It appears upstream builds/tests (Travis CI) with 3.4+. Current patch is limited to 3.4 only. Set to 3.4+

Please also confirm that this port/change passes QA (portlint and poudriere in particular). 

For more information and instructions see: https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/testing.html

[1] https://pypi.python.org/pypi/xonsh
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 10:19:07 UTC
Additional, the port should be marked concurrent safe (using USE_PYTHON=concurrent) if files are uniquely named and dont conflict when multiple python versions (eg: py34-xonsh and py35-xonsh) are installed at the same time
Comment 3 Roberto Fernandez Cueto 2017-07-10 11:07:48 UTC
(In reply to Kubilay Kocak from comment #1)
How do I put the CHEESESHOP as the MASTER_SITES?

The rest is more or less easy to follow.
Comment 4 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 11:16:19 UTC
(In reply to Roberto Fernandez Cueto from comment #3)

MASTER_SITES=CHEESEHOP

USE_GITHUB and GH_* variables can then be removed. The tarball from PyPI (CHEESESHOP) may have a different checksum, so run make makesum to update distinfo if necessary
Comment 5 Roberto Fernandez Cueto 2017-07-10 12:30:45 UTC
I am following the steps under https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/testing.html and I have found the following:

Checking for pkg-plist issues (check-plist)
===> Parsing plist
===> Checking for items in STAGEDIR missing from pkg-plist
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/__amalgam__.py
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/__pycache__/__amalgam__.cpython-36.%%PYTHON_PYOEXTENSION%%
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/__pycache__/__amalgam__.cpython-36.pyc
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/completers/__amalgam__.py
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/completers/__pycache__/__amalgam__.cpython-36.%%PYTHON_PYOEXTENSION%%
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/completers/__pycache__/__amalgam__.cpython-36.pyc
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/history/__amalgam__.py
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/history/__pycache__/__amalgam__.cpython-36.%%PYTHON_PYOEXTENSION%%
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/history/__pycache__/__amalgam__.cpython-36.pyc
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/prompt/__amalgam__.py
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/prompt/__pycache__/__amalgam__.cpython-36.%%PYTHON_PYOEXTENSION%%
Error: Orphaned: %%PYTHON_SITELIBDIR%%/xonsh/prompt/__pycache__/__amalgam__.cpython-36.pyc
===> Checking for items in pkg-plist which are not in STAGEDIR
===> Error: Plist issues found.

Is it usual to find this kind of errors on python ports or how can I solve this issue so this message disappears?
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 12:38:28 UTC
(In reply to Roberto Fernandez Cueto from comment #5)

This is normally a symptom of not using autoplist

What is the output (add as attachment, not comment) of `make makeplist`?
Comment 7 Matthew Seaman freebsd_committer 2017-07-10 12:57:56 UTC
(In reply to Roberto Fernandez Cueto from comment #5)

Almost all python ports use autoplist -- definitely recommended.
Comment 8 Roberto Fernandez Cueto 2017-07-10 13:03:02 UTC
Created attachment 184229 [details]
Output from make makeplist
Comment 9 Roberto Fernandez Cueto 2017-07-10 13:05:23 UTC
I do not think that it is a problem of autoplist, because it is already in the Makefile.
Comment 10 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 13:10:58 UTC
(In reply to Roberto Fernandez Cueto from comment #8)

To take one example of the errors from comment 5, we see the following listed in makeplist output:

%%PYTHON_SITELIBDIR%%/xonsh/__pycache__/__amalgam__.cpython-36.%%PYTHON_PYOEXTENSION%%
%%PYTHON_SITELIBDIR%%/xonsh/__pycache__/__amalgam__.cpython-36.pyc

I'm not sure why the errors are being seen. 

Could you attach the latest version of your patch (and obsolete attachment 184229 [details] in the process)
Comment 11 Roberto Fernandez Cueto 2017-07-10 13:27:49 UTC
Comment on attachment 184229 [details]
Output from make makeplist

Obsolete output
Comment 12 Roberto Fernandez Cueto 2017-07-10 13:29:21 UTC
Created attachment 184232 [details]
Actual patch of xonsh

Updating the patch
Comment 13 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 13:52:10 UTC
(In reply to Roberto Fernandez Cueto from comment #12)

I can reproduce the errors in comment 5.

FreeBSD Python ports (using autoplist) rely on setuptools' --record functionality to produce a list of installed files.

It appears that xonsh's setup.py does some 'stuff' that produces/results in incorrect --record output.

It likely has something to do with amalgamate_source() and/or

TABLES = ['xonsh/lexer_table.py', 'xonsh/parser_table.py',
          'xonsh/__amalgam__.py',
          'xonsh/completers/__amalgam__.py',
          'xonsh/history/__amalgam__.py',
          'xonsh/prompt/__amalgam__.py']

Additionally, we see the following output during the 'install' target:

running install
Removed xonsh/parser_table.py
Removed xonsh/__amalgam__.py
Removed xonsh/completers/__amalgam__.py
Removed xonsh/history/__amalgam__.py
Removed xonsh/prompt/__amalgam__.py

I'd start looking here.

On a side note, this reminds me of upstream issue 20397 [1], patches for which we've included in all Python versions for a while now.

[1] https://bugs.python.org/issue20397

Additionally minor nits:

- LICENSE=BSD should be BSD[234]CLAUSE (upstream license file indicates a 2-clause license
- LICENSE_FILE should be added ponting to ${WRKSRC}/path/to/license when a license file is included with the distribution files (one is in this case.
Comment 14 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 14:02:31 UTC
Just noticed that a license is *not* included in the PyPI source distribution (sdist). LICENSE_FILE doesn't need to be set.
Comment 15 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-10 14:06:50 UTC
(In reply to Kubilay Kocak from comment #14)

As a test case (not necessarily a workaround or solution), removing the clean_tables() function call from the xinstall class prevents the errors:

--- setup.py.orig       2017-07-10 14:03:10 UTC
+++ setup.py
@@ -182,7 +182,6 @@ def restore_version():
 class xinstall(install):
     """Xonsh specialization of setuptools install class."""
     def run(self):
-        clean_tables()
         build_tables()
         amalgamate_source()
         # add dirty version number

This is the functionality that is resulting in --record output not matching what is installed
Comment 16 Matthew Seaman freebsd_committer 2017-07-10 22:33:57 UTC
(In reply to Kubilay Kocak from comment #15)

This is where is all goes wrong:
```
===>  Staging for py36-xonsh-0.5.12
===>   py36-xonsh-0.5.12 depends on package: py36-setuptools>0 - found
===>   py36-xonsh-0.5.12 depends on file: /usr/local/bin/python3.6 - found
===>   Generating temporary packing list
running install
Removed xonsh/parser_table.py
Removed xonsh/__amalgam__.py
Removed xonsh/completers/__amalgam__.py
Removed xonsh/history/__amalgam__.py
Removed xonsh/prompt/__amalgam__.py
Building lexer and parser tables.
Could not install Jupyter kernel spec, please install Jupyter/IPython.
running build
running build_py
copying xonsh/parser_table.py -> build/lib/xonsh
Generating LALR tables
WARNING: 707 shift/reduce conflicts
WARNING: 113 reduce/reduce conflicts
WARNING: reduce/reduce conflict in state 20 resolved using rule (test_or_star_expr -> test)
WARNING: rejected rule (testlist -> test) in state 20
WARNING: reduce/reduce conflict in state 346 resolved using rule (test_or_star_expr -> test)
WARNING: rejected rule (testlist -> test) in state 346
WARNING: reduce/reduce conflict in state 373 resolved using rule (string_tok -> STRING)
WARNING: rejected rule (subproc_arg_part -> STRING) in state 373
WARNING: reduce/reduce conflict in state 1333 resolved using rule (nodedent -> INDENT any_dedent_toks DEDENT)
WARNING: rejected rule (any_dedent_tok -> DEDENT) in state 1333
Could not import amalgamate, skipping.    <<<-----*****
```

Missing Jupyter/iPython appears to be no big deal.

But it does look like there's an extra module needs porting and installing
as a BUILD_DEPENDS for xonsh:

https://pypi.python.org/pypi/amalgamate/0.1.3

(Written by the same authors as xonsh)

Also, it's a BSD2CLAUSE license (This is on Github, but not in the pypi tarballs: https://github.com/xonsh/xonsh/blob/master/license)
Comment 17 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-11 05:28:35 UTC
(In reply to Matthew Seaman from comment #16)

Yep I saw that too. 

If its a compulsory build requirement, it should be listed in setup.py:setup_requires and added to BUILD_DEPENDS accordingly. It would be worth clarifying the dependency type with upstream and getting the appropriate upstream changes made.

If it's an optional or rather non-compulsory requirement, amalgamate should be added to extras_require if it is indeed an 'extra' and the build should be fixed.

The fact that the amalgamate step is 'skipped' rather than being fatal indicates that it's OK to continue.
Comment 18 Roberto Fernandez Cueto 2017-07-11 11:24:56 UTC
(In reply to Kubilay Kocak from comment #15)

As I see in GitHub, the patch is already applied to the source code, so it is apparently a bug that has been fixed by the developers.

I was thinking in put the amalgamate python module as a dependency of xonsh, but I do not know if I should put it under devel/amalgamate and use it as an optional dependency with the patch from Kubilay Kocak for the off case.

Any ideas?
Comment 19 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-11 11:27:04 UTC
(In reply to Roberto Fernandez Cueto from comment #18)

Can you point us to the commit referenced?
Comment 21 Kubilay Kocak freebsd_committer freebsd_triage 2017-07-11 11:47:02 UTC
(In reply to Roberto Fernandez Cueto from comment #20)

That commit/code was made in 2015 and has been mostly replaced since then (looking at 0.5.12 code). In particular that commit just moved the clean_tables() call from main() to the custom install class (xinstall(install)), which is the code we see causing the issue today.
Comment 22 Roberto Fernandez Cueto 2017-07-11 12:20:00 UTC
(In reply to Kubilay Kocak from comment #21)
I have mentally misplaced the - symbol from the diff, my bad, I am sorry.
Comment 23 Roberto Fernandez Cueto 2017-07-17 12:31:40 UTC
I have wrote the developers and I am waiting for an answer. Here it is the link to the conversation:
https://groups.google.com/d/msg/xonsh/4C8OROXSdK8/8i8cGQqwBgAJ
Comment 24 Roberto Fernandez Cueto 2017-07-27 10:46:41 UTC
I finally got an answer, amalgamate is meant to be a build dependency of xonsh, that means that it should be ported as well. Which leads me to the following question, where should be amalgamate imported? into devel?
Comment 25 commit-hook freebsd_committer 2017-08-02 14:08:13 UTC
A commit references this bug:

Author: matthew
Date: Wed Aug  2 14:07:49 UTC 2017
New revision: 447104
URL: https://svnweb.freebsd.org/changeset/ports/447104

Log:
  A package-based, source code amalgamater for collapsing Python
  packages into a single module.

  The big idea here is to glue most of the source files in a package or
  subpackage together into a single module, called
  __amalgam__.py. Combined with some hooks in the __init__.py, this
  should dramatically reduce the number of files that are being searched
  for inside of the package. This is critical in larger projects where
  import times are the major startup time cost.

  WWW: https://github.com/xonsh/amalgamate

  PR:		220596

Changes:
  head/devel/Makefile
  head/devel/py-amalgamate/
  head/devel/py-amalgamate/Makefile
  head/devel/py-amalgamate/Makefile~
  head/devel/py-amalgamate/distinfo
  head/devel/py-amalgamate/pkg-descr
Comment 26 Matthew Seaman freebsd_committer 2017-08-02 14:32:02 UTC
Created attachment 184957 [details]
Fix outstanding problems with the port

This adds:

   * BUILD_DEPENDS on the new devel/py-amalgamate port
   * Mark as NO_ARCH
   * Fix LICENSE

This compiles for me and seems to work, but it generates a nasty looking exception on startup:

```
% xon.sh-3.6
Exception in thread Thread-2:
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/usr/local/lib/python3.6/site-packages/xonsh/history/__amalgam__.py", line 331, in run
    files = self.files(only_unlocked=True)
  File "/usr/local/lib/python3.6/site-packages/xonsh/history/__amalgam__.py", line 353, in files
    boot = uptime.boottime()
  File "/usr/local/lib/python3.6/site-packages/xonsh/xoreutils/uptime.py", line 270, in boottime
    up = uptime()
  File "/usr/local/lib/python3.6/site-packages/xonsh/xoreutils/uptime.py", line 260, in uptime
    up = (_uptime_bsd() or _uptime_plan9() or _uptime_linux() or
  File "/usr/local/lib/python3.6/site-packages/xonsh/xoreutils/uptime.py", line 56, in _uptime_linux
    if xp.LIBC.sysinfo(buf) < 0:
AttributeError: 'NoneType' object has no attribute 'sysinfo'


                      Welcome to the xonsh shell (0.5.12)

                     ~ All that is and all that shell be ~

--------------------------------------------------------------------------------
xonfig tutorial    ->    Launch the tutorial in the browser
xonfig wizard      ->    Run the configuration wizard and claim your shell
(Note: Run the Wizard or create a ~/.xonshrc file to suppress the welcome screen)

matthew@lucid-nonsense ~/work/ports/shells/xonsh $
```

If you can confirm that it functions correctly and if possible come up with
a fix for the exception, then I think we're pretty close to having this commitable.
Comment 27 Roberto Fernandez Cueto 2017-08-08 14:45:31 UTC
Ok, I have tracked this error until the following conclusion:
I have run the following code in python3.6 and python2.7,

import ctypes
import ctypes.util
libc = ctypes.CDLL(find_library('libc.so'), use_errno=True)
sz = ctypes.c_unit(0)
libc.sysctlbyname('kern.boottime', None, ctypes.byref(sz), None, 0)

On python3.6 the result is -1 and errno=ENOENT, but in python2.7 the sysctlbyname(3) function returns 0 and the sz variable correctly set.

Xonsh assumes that out machine is not a BSD machine because this function
does not return a valid parameter.

I do not know which will be the best solution.
Comment 28 Matthew Seaman freebsd_committer 2017-08-08 14:55:47 UTC
(In reply to Roberto Fernandez Cueto from comment #27)

Hmmm... This looks like a bug in python-3.6 to me.  Kubilay -- 
what do you think?
Comment 29 Shane 2017-09-18 10:39:29 UTC
(In reply to Roberto Fernandez Cueto from comment #27)

Your example use of find_library is wrong.

From pydocs -
> name is the library name without any prefix like lib, suffix like .so, .dylib or version number (this is the form used for the posix linker option -l)

sysctlbyname takes a char* as the first arg - in ctypes get that from create_string_buffer(), also prefixing it as bytes works - b'kern.boottime'

While it doesn't actually make a difference, using ctypes.c_size_t for sz would be more correct.

So for the proper results in 2.7 and 3.x use

import ctypes
import ctypes.util
libc = ctypes.CDLL(ctypes.util.find_library('c'), use_errno=True)
sz = ctypes.c_size_t(0)

Then either
sysctl_name = ctypes.create_string_buffer(b'kern.boottime')
libc.sysctlbyname(sysctl_name, None, ctypes.byref(sz), None, 0)

or
libc.sysctlbyname(b'kern.boottime', None, ctypes.byref(sz), None, 0)


So to get rid of the exception on starting xonsh -

You will find in _uptime_bsd() xp.LIBC is None so that points to the first fix being back in __amalgam__.py:LIBC()

Adjust the ON_BSD test to use find_library() - the same as ON_DARWIN

elif ON_BSD:
    try:
        libc = ctypes.CDLL(ctypes.util.find_library("c"))

Then in _uptime_bsd() either prefix the sysctlname with b or use create_string_buffer()

xp.LIBC.sysctlbyname(b'kern.boottime', None, ctypes.byref(sz), None, 0)
Comment 30 Roberto Fernandez Cueto 2017-09-22 14:06:16 UTC
Created attachment 186617 [details]
Final diff of xonsh for FreeBSD

Ok, I tested the proposed changes on the port and it works just fine without throwing any error.
Comment 31 commit-hook freebsd_committer 2017-09-22 14:58:08 UTC
A commit references this bug:

Author: matthew
Date: Fri Sep 22 14:57:42 UTC 2017
New revision: 450378
URL: https://svnweb.freebsd.org/changeset/ports/450378

Log:
  xonsh is a Python-ish, BASHwards-looking shell language and command prompt.

  The language is a superset of Python 3.4+ with additional shell primitives.
  xonsh (pronounced conch) is meant for the daily use of experts and novices
  alike.

  WWW: http://xon.sh

  PR:		220596
  Submitted by:	roberfern@gmail.com

Changes:
  head/shells/Makefile
  head/shells/xonsh/
  head/shells/xonsh/Makefile
  head/shells/xonsh/distinfo
  head/shells/xonsh/files/
  head/shells/xonsh/files/patch-xonsh_platform.py
  head/shells/xonsh/files/patch-xonsh_xoreutils_uptime.py
  head/shells/xonsh/pkg-descr
Comment 32 Matthew Seaman freebsd_committer 2017-09-22 14:58:40 UTC
Committed, thanks!