Use binary strings when writing setup.py as its required by distutils.

Review Request #240 — Created April 17, 2014 and submitted

travis
pants
pants-reviews
ity
Use binary strings when writing setup.py as its required by distutils.

Attempting to pip install pantsbuild.pants currently fails with "TypeError: 'package' must be a string (dot-separated), list, or tuple". Looking into the issue this is because setup.py is written with unicode strings, which distutils does not support. Here we convert strings to binary just before writing out setup.py.
I built sdist's before/after this change, and I was able to pip install pants with this change.
  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
WI
  1. 
      
  2. src/python/pants/commands/setup_py.py (Diff revision 1)
     
     
    pretty sure there's something in compatibility for this.
    
    also, unicode doesn't exist in python 3.x
    
    mba=~=; python3.4
    Python 3.4.0 (default, Apr 15 2014, 11:21:44) 
    [GCC 4.2.1 Compatible Apple Clang 4.0 ((tags/Apple/clang-421.0.60))] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> unicode
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    NameError: name 'unicode' is not defined
    
  3. 
      
TR
TR
  1. There's an issue - this turns zip_safe to a string (need to only convert binary/unicode strings). Not sure how this worked when I published (maybe did that wrong?). Will update the diff...
  2. 
      
TR
WI
  1. 
      
  2. src/python/pants/commands/setup_py.py (Diff revisions 2 - 3)
     
     
     
     
     
    I highly recommending watching Ned Batchelder's unicode sandwich talk from Pycon 2012:
    
    http://nedbatchelder.com/text/unipain.html
    
    I don't think convert() is necessary at all.  You should just do either:
    
    chroot.write((SETUP_BOILERPLATE % {...}).encode('utf-8'))
    
    *or*
    
    let write do it for you.  write takes a mode, and you can just do mode='w' instead of the default mode='wb'.  mode='w' accepts strings but mode='wb' only accepts bytes, hence the need for the .encode(encoding)
    1. My initial approach was trying to do this at write time. Here's what I believe is the issue with that approach, and why I switched to "convert":
      
      hoth:~ travis$ python
      Python 2.6.8 (unknown, Aug 25 2013, 00:04:29) 
      [GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.0.68)] on darwin
      Type "help", "copyright", "credits" or "license" for more information.
      >>> import pprint
      >>> data = {u'entry_points': {u'console_scripts': [u'pants = pants.bin.pants_exe:main']}}
      >>> pprint.pformat(data, indent=4)
      "{   u'entry_points': {   u'console_scripts': [   u'pants = pants.bin.pants_exe:main']}}"
      >>> 
      
      Notice how the u's are inside the string. Regardless of what encoding we write this string as we'll have the u's. I'm happy to do this in the writer if you can think of a way around this. Thoughts?
    2. Whyyy are we using __future__ unicode_literals here?  This is almost always bad news bears in python 2.6.
    3. They just sort of showed up when moving into pantsbuild :/
      
      What do you think is the right path forward here? As is pantsbuild.pants is unusable via sdist so I'd like to resolve this in whatever way we think best.
    4. Let's figure out how to move forward here. pantsbuild.pants is unusable from sdist at present and we need a solution. I propose we make this targeted fix and we can have a broader discussion about unicode separately.
    5. I am ok with this approach. We are currently blocked on this, all for getting this out as soon as possible without sacrificing correctness.
    6. I'm happy with this targeted fix as well and a TODO / issue.  Patrick championed unicode_literals as part of the standard boilerplate future imports that were added in the commons -> pantsbuild move.  I'm unclear of the reasoning, but thats a battle for another day.
  3. 
      
TR
JS
  1. Ship It!
  2. 
      
TR
Review request changed

Status: Closed (submitted)

Change Summary:

commit 88aff968133f9247b5b3ea10b24d1c1921480e87
Author: Travis Crawford <traviscrawford@gmail.com>
Date:   Thu Apr 17 15:11:03 2014 -0700

    Use binary strings when writing setup.py as its required by distutils.
    
    Attempting to pip install pantsbuild.pants currently fails with "TypeError:
    'package' must be a string (dot-separated), list, or tuple". Looking into the
    issue this is because setup.py is written with unicode strings, which
    distutils does not support. Here we convert strings to binary just before
    writing out setup.py.
    
    Testing Done:
    I built sdist's before/after this change, and I was able to pip install pants
    with this change.
    
    Reviewed at https://rbcommons.com/s/twitter/r/240/
Loading...