On 07/06/16 22:49, Barry Warsaw wrote: > On Jun 07, 2016, at 10:00 PM, Gordon Ball wrote: > >>> The packaging looks really good. I noticed the setting of http_proxy in >>> override_dh_auto_build. You probably don't strictly need that because I >>> believe pybuild does that automatically. It can't hurt though and some >>> maintainers prefer to be explicit about that. >> >> The http_proxy bit was cargo-culted from >> https://wiki.debian.org/Python/LibraryStyleGuide > > Ah, I'd forgotten that this was still needed for sphinx-build. > >> Good idea. The test suite is extensive but it hadn't occurred to me that >> it is entirely based on importing it as a library rather than actually >> running the binary. I'd tested the installed package but clearly on a >> system with python3-pkg-resources already installed. >> >> I found that this fails in an autopkgtest schroot though - xonsh fails >> to start if $HOME is not writable (which is possibly a bug), so I've >> added a wrapper which sets $HOME=$ADTTMP (and added a couple of extra >> examples). > > Looks great, much better than my quick hack. :) > >> On hold for the moment then. I'm happy to have it listed as team maintained >> at a future point though. I'm already subscribed to debian-python, I'll apply >> to join the relevant teams soon. > > Cool. It'll be easy to move later. > > One possible minor issue is that DPMT did adopt git-dpm for packages, but > since that decision several years ago, git-dpm has apparently stopped being > developed. It hasn't quite bitrotted yet, but I am advocating that DPMT drop > git-dpm and use gbp-pq for patch management. I don't see any reason other > than (hopefully temporary) consistency for PAPT to adopt git-dpm once that > switch happens. We'll have to see what the team says. > > For now, let's just assume gbp-pq will be adopted for PAPT. > >> Given the script is generated based on `entry_points` in setup.py, >> shouldn't this dependency be generated by pybuild/dh_python? > > Actually, that should come from an install_requires section or a > requirements.txt file. I don't see either of those in the xonsh repo. But > also pkg_resources is kind of a special case on Debian. It really comes as > part of setuptools, but in Debian we split it out into a separate binary > package, so I don't know that it would be automatically detected in any case. > Clearly it doesn't since it crashes without an explicit Depends, but we'd have > to ask Piotr on debian-python@ to know whether that's intended behavior or > not. TBH, I always just add it explicitly. > >> There are a couple of remaining (possibly non-) issues: >> >> * lintian reports privacy-breach-generic on the documentation (google >> webfont links). I can't see this explicitly configured anywhere in the >> rst files or conf.py, so I *think* this is being added by >> sphinx/cloud-sptheme > > Interesting. I built the package from the git repo on unstable and ran > lintian over the amd64.changes file. I don't get any lintian warnings.
I don't see anything locally, but I do see it when uploading a binary package to mentors.d.n - I'm unsure how their setup differs from `lintian --pedantic`. Anyway. > >> * building the documentation generates (I think) inconsequential errors >> since the xonsh pygments lexer is not available at build time; this >> could be fixed by build-depending on itself, but I'm not sure this is >> worth adding a dependency cycle for. > > I see that too. Agreed it's probably not worth worrying about right now. > >> * xonsh supports installing itself as a jupyter kernel, which I'm >> interested to include but for the moment (parts of) jupyter is only in >> experimental, so it can probably wait until a future xonsh upload > > +1 > > Everything else lgtm. Let me know when you want me to sponsor an upload; I'm > happy to do it any time. It will have to go through NEW of course, but it > would be nice to get this into peoples hands soon. (I know I'm not the only > Debuntunista who wants to use it after watching the Pycon talk. :) > If you think it looks in good shape, let's go for an upload. Thanks for your time reviewing the packaging.
