[wltrimbl]

This fixes #1735 for me (and I suspect #1565) in ubuntu:16.10 docker containers that were failing to build because of unicode in the author list.
I don't think the problem is lack of unicode support, it's that python3.5 is enforcing more strict handling of encodings.

• Is it mergeable?
• make test Did it pass the tests?
• make clean diff-cover If it introduces new functionality in
  scripts/ is it tested?
• make format diff_pylint_report cppcheck doc pydocstyle Is it well
  formatted?
• Did it change the command-line interface? Only backwards-compatible
  additions are allowed without a major version increment. Changing file
  formats also requires a major version number increment.
• For substantial changes or changes to the command-line interface, is it
  documented in CHANGELOG.md? See keepachangelog
  for more details.
• Was a spellchecker run on the source code and documentation after
  changes were made?
• Do the changes respect streaming IO? (Are they
  tested for streaming IO?)

[wltrimbl]

This fix is incompatible with python2.7, but my python2.7 build of khmer have been failing since 924f19d / #1595.

If you're using python2.7, it is useful to checkout v2.1.1

[ctb]

thanks for this @wltrimbl! happy to fix - but isn't master up to date with v2.1.1? our CI builds python2.7 - what do you think is the problem?

[codecov-io]

Codecov Report

Merging #1737 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1737   +/-   ##
======================================
  Coverage    0.05%   0.05%           
======================================
  Files          90      90           
  Lines       11336   11336           
  Branches     2992    2992           
======================================
  Hits            6       6           
  Misses      11330   11330

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a1f3ec...9f9c2db. Read the comment docs.

[wltrimbl]

Best guess for the 2.7 build failures: subtle and unimportant differences in environment.

The Dockerfile in
https://github.com/dib-lab/2016-sourmash-tax/tree/master/make_khmer
does not build for June 2017 versions of khmer (though it can be rescued by specifying an older version tag)

The dockerfile uses ubnutu-wily, while CI uses debian-trusty.

This suggests that when publishing Dockerfiles that you want to work for a few years git clone && build is not the best idea; specifying a version of the repo in the Dockerfile confers stability.

[ctb]

On Sun, Jun 25, 2017 at 09:41:51AM -0700, Will Trimble wrote: Best guess for the 2.7 build failures: subtle and unimportant differences in environment. The Dockerfile in https://github.com/wltrimbl/2016-sourmash-tax/tree/master/make_khmer does not build for June 2017 versions of khmer (though it can be rescued by specifying an older version tag)

see also #1333. 2016-sourmash-tax is pretty out of date at this point.

This suggests that when publishing Dockerfiles that you want to work for a few years git clone && build is not the best idea; specifying a version of the repo in the Dockerfile confers stability.

yes, when we publish dockerfiles that we want to work for a few years we will do that :)

[betatim]

There are two problems (I think) and they only appear if you have a terminal that does not have (or claims not to have) unicode support.

This snippet can read in the authors on python 2 and 3 (when I unset LANG in my terminal):

In [5]: with codecs.open('authors.csv', 'rb', encoding='utf-8') as csvin:
   ...:     authors = csvin.readlines()
   ...:     authors = [a.strip().split(',') for a in authors]
   ...:     authorstr = ', '.join([row[0] for row in authors])

the encoding specifies the encoding of the file, so I think that shouldn't change between versions.

The problem is that when python thinks the terminal does not support unicode it will use ascii when printing (which is what pip show uses). If we can find a way to detect if printing a utf-8 string will succeed (unfortunately sys.getdefaultencoding() isn't the way to do that 😢) or not we can use errors='ignore' when reading in the authors and make sure we are always on a machine that supports unicode when making a release.

[betatim]

Maybe the way to go is to try and print a unicode character before reading in the authors.

[wltrimbl]

The pip show khmer error (#1565) is a problem with writing the author string.
This pip install khmer error (#1735) is a problem reading the author string during build.

This patch fixes the install error, but I don't know how to test the for the pip show khmer bug.

[betatim]

Fundamentally both these problems occur because python is trying to read utf-8 encoded bytes and using the ascii codec instead. If we specify utf-8 as encoding to codecs.open it will always use the correct encoding to read in the data.

The csv module seems to be broken with respect to handling unicode so I think we should ditch it instead of adding tests for which version of python is being used and then reading in the data with the wrong encoding.

[wltrimbl]

This dockerfile will reproduce the unicode error during build, and also shows that this PR fixes it.

FROM ubuntu:16.10
RUN apt-get -y update && \
    apt-get install -y python3.5 python3.5-dev python3.5-venv python3-setuptools make \
    libc6-dev g++ zlib1g-dev git
RUN python3.5 -m venv ~/py3 && \
    . ~/py3/bin/activate && pip3 install -U pip
RUN . ~/py3/bin/activate && cd && \
    git clone https://github.com/wltrimbl/khmer && cd khmer && \
    git checkout acea494 &&  \
#    git checkout 42f0930 &&  \
    python setup.py install

# acea494 Merge pull request #1734 from dib-lab/fix/cyimport     FAILS with
# UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 261: ordinal not in range(128)
# 42f0930 Different encoding strings for python2 and python3 for authors.csv  FIXED

[wltrimbl]

@betatim you are right, ditching csv.reader is the Right Thing To Do.

[wltrimbl]

Ready for review

[standage]

@betatim, can you handle this review?

[betatim]

LGTM. In a terminal "without unicode" python2 and 3 python setup.py install both work. On py2 pip show khmer works and on py3 it fails when printing out the authors (as expected). If I setup my $LANG=en_GB.UTF-8 it starts working on py3 as well.

Thanks for this!

Also if some of my earlier comments were a bit combative in their wording sorry, I was in a seriously foul mood that spilled over :-/

[ctb]

On Thu, Jul 13, 2017 at 08:51:31AM -0700, Tim Head wrote: Also if some of my earlier comments were a bit combative in their wording sorry, I was in a seriously foul mood that spilled over :-/

thanks for the note! I didn't notice anything personally but I appreciate the thought!

[betatim]

Merging this before I need to approve it for a third time :)