Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add future lib and port src/base #6067

Merged
merged 8 commits into from
Jul 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions 3rdparty/python/requirements.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
ansicolors==1.0.2
beautifulsoup4>=4.3.2,<4.4
cffi==1.11.1
configparser==3.5.0
contextlib2==0.5.5
coverage>=4.5,<4.6
docutils>=0.12,<0.13
fasteners==0.14.1
faulthandler==2.6
future==0.16.0
futures==3.0.5
isort==4.2.5
Markdown==2.1.1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

import cgi
import collections
import json
import os
Expand Down Expand Up @@ -354,7 +355,9 @@ def generate_generated(config, here):

def render_html(dst, config, soups, precomputed, template):
soup = soups[dst]
renderer = Renderer()
renderer = Renderer(escape=cgi.escape) # TODO(python3port): cgi.escape is deprecated in favor html.escape
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a tough bug to find. Pystache by default uses cgi.escape() in Python2, but html.escape() in Python3. Because future adds a backport to html (which doesn't exist in Python2), Pystache successfully was importing the module and using html.escape().

html.escape() escapes ' unlike cgi.escape(), so one of our tests was failing.

For now, I'm having us stay with the old semantics.

# (the default used by Pystache on Python3). html.escape() escapes single
# quotes, whereas cgi.escape() does not. This will need to be addressed.
title = precomputed.page[dst].title
topdots = ('../' * dst.count('/'))
if soup.body:
Expand Down
23 changes: 22 additions & 1 deletion src/python/pants/base/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ python_library(
name = 'build_environment',
sources = ['build_environment.py'],
dependencies = [
'3rdparty/python:future',
':build_root',
'src/python/pants/scm',
'src/python/pants/scm:git',
Expand Down Expand Up @@ -75,6 +76,7 @@ python_library(
name = 'fingerprint_strategy',
sources = ['fingerprint_strategy.py'],
dependencies = [
'3rdparty/python:future',
':deprecated',
'src/python/pants/util:meta',
]
Expand All @@ -85,19 +87,24 @@ python_library(
sources = ['generator.py'],
dependencies = [
':mustache',
'3rdparty/python:future',
'3rdparty/python:pystache',
]
)

python_library(
name = 'hash_utils',
sources = ['hash_utils.py'],
dependencies = [
'3rdparty/python:future',
]
)

python_library(
name = 'mustache',
sources = ['mustache.py'],
dependencies = [
'3rdparty/python:future',
'3rdparty/python:pystache',
'3rdparty/python:six',
]
Expand All @@ -106,17 +113,24 @@ python_library(
python_library(
name = 'parse_context',
sources = ['parse_context.py'],
dependencies = [
'3rdparty/python:future',
]
)

python_library(
name = 'payload',
sources = ['payload.py'],
dependencies = [
'3rdparty/python:future',
]
)

python_library(
name = 'payload_field',
sources = ['payload_field.py'],
dependencies = [
'3rdparty/python:future',
'3rdparty/python/twitter/commons:twitter.common.collections',
':deprecated',
':hash_utils',
Expand All @@ -136,12 +150,16 @@ python_library(
python_library(
name = 'revision',
sources = ['revision.py'],
dependencies = [
'3rdparty/python:future',
]
)

python_library(
name = 'run_info',
sources = ['run_info.py'],
dependencies = [
'3rdparty/python:future',
':build_environment',
'src/python/pants/util:dirutil',
'src/python/pants:version',
Expand All @@ -152,6 +170,7 @@ python_library(
name = 'cmd_line_spec_parser',
sources = ['cmd_line_spec_parser.py'],
dependencies = [
'3rdparty/python:future',
'3rdparty/python/twitter/commons:twitter.common.collections',
':build_file',
':specs',
Expand All @@ -171,6 +190,7 @@ python_library(
name = 'worker_pool',
sources = ['worker_pool.py'],
dependencies = [
'3rdparty/python:future',
'src/python/pants/reporting:report', # TODO(pl): Bust this out
],
)
Expand All @@ -179,7 +199,7 @@ python_library(
name = 'workunit',
sources = ['workunit.py'],
dependencies = [
'3rdparty/python:six',
'3rdparty/python:future',
'src/python/pants/util:dirutil',
'src/python/pants/util:memo',
'src/python/pants/util:rwbuf',
Expand All @@ -200,6 +220,7 @@ python_library(
name='exiter',
sources=['exiter.py'],
dependencies=[
'3rdparty/python:future',
'3rdparty/python:faulthandler',
'src/python/pants/util:dirutil'
]
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/build_environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import logging
import os
import sys
from builtins import str

from pants.base.build_root import BuildRoot
from pants.scm.scm import Scm
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/cmd_line_spec_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
unicode_literals, with_statement)

import os
from builtins import object

from pants.base.specs import DescendantAddresses, SiblingAddresses, SingleAddress

Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/exiter.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import signal
import sys
import traceback
from builtins import object

import faulthandler
import six
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/fingerprint_strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import logging
from abc import abstractmethod
from builtins import object

from pants.util.meta import AbstractClass

Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
unicode_literals, with_statement)

import pprint
from builtins import object

import pystache

Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/hash_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import hashlib
import json
from builtins import object


def hash_all(strs, digest=None):
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/mustache.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import os
import pkgutil
from builtins import object

import pystache
import six
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/parse_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import functools
import threading
from builtins import object


class Storage(threading.local):
Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/base/payload.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

from builtins import object
from hashlib import sha1


Expand All @@ -30,7 +31,7 @@ def __init__(self):

@property
def fields(self):
return self._fields.items()
return list(self._fields.items())

def as_dict(self):
"""Return the Payload object as a dict."""
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/payload_field.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
unicode_literals, with_statement)

from abc import abstractmethod
from builtins import object
from hashlib import sha1

from twitter.common.collections import OrderedSet
Expand Down
33 changes: 21 additions & 12 deletions src/python/pants/base/revision.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@
unicode_literals, with_statement)

import re
from itertools import izip_longest
from builtins import map, object, str
from functools import total_ordering

from future.moves.itertools import zip_longest


@total_ordering
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implements all 6 comparison dunders, given __eq__ and __lt__.

There is a slight performance penalty. If Revision is used a lot, I can manually add the other comparisons.

class Revision(object):
"""Represents a software revision that is comparable to another revision describing the same
software.
Expand Down Expand Up @@ -74,7 +78,7 @@ def lenient(cls, rev):
"""
rev = re.sub(r'(\d)([a-zA-Z])', r'\1.\2', rev)
rev = re.sub(r'([a-zA-Z])(\d)', r'\1.\2', rev)
return cls(*map(cls._parse_atom, re.split(r'[.+_\-]', rev)))
return cls(*list(map(cls._parse_atom, re.split(r'[.+_\-]', rev))))

def __init__(self, *components):
self._components = components
Expand All @@ -87,21 +91,26 @@ def components(self):
"""
return list(self._components)

def __cmp__(self, other):
for ours, theirs in izip_longest(self._components, other._components, fillvalue=0):
difference = cmp(ours, theirs)
if difference != 0:
return difference
return 0
def _is_valid_operand(self, other):
return hasattr(other, '_components')

def __repr__(self):
return '{}({})'.format(self.__class__.__name__, ', '.join(map(repr, self._components)))

def __eq__(self, other):
return hasattr(other, '_components') and tuple(self._components) == tuple(other._components)

def __ne__(self, other):
return not self.__eq__(other)
if not self._is_valid_operand(other):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This slightly changes the behavior from before. When comparing to non-Revisions, this returns NotImplemented, which is the expected behavior. instead of the prior False.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any python3 compat reason to mix in this behavior change? As an example of future conversion PRs, it would be good to stick to pure semantic-preserving automated conversion if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I made the change is for the@total_ordering decorator, so that we treat __eq__ and __lt__ more similarly. I believe I can revert back to original semantics if you prefer, though.

The only Py3 change we need is replacing __cmp__ with __lt__ or one of the other rich comparison functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer it if just to keep semantics changes out of the conversion PRs. Especially since Revision is :API: public and the semantic shift is False -> raise. We can't know how this will break API users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@Eric-Arellano Eric-Arellano Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted __eq__ to return False

But am keeping __lt__ as returning NotImplemented. The prior implementation in __cmp__ would return AttributeError if other._components is not defined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I think I can buy the AttributeError case as not being part of the public api, so this change is probably OK. That said - I'd avoid that sort of improvement in the conversion PRs in particular going forward. Broken out as separate changes is a different story, that is definitely great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and the semantic shift is False -> raise.

I was mistaken there. The semantic shift is subtle to non-existant depending on how other implements __eq__. That said, still a fan of nixing improvements in these PRs. TODO is ok, but it really should be attributed and / or - better - assigned an issue with the issue link noted in the TODO. Your spirit of wanting to fix is admirable and I'm sure you'll see more of these. Perhaps just start 1 umbrella issue to capture all the small TODO's you find then you or someone else can circle back and do the fixes in unrelated PR's.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I reverted everything back to identical semantics from before (just more explicit).

I think it's a good policy to avoid any changes in these PRs because they get buried. Instead, will add notes in form TODO(python3port) and open an issue at #6071.

return False # TODO(python3port): typically this should return NotImplemented.
# Returning False for now to avoid changing prior API.
return tuple(self._components) == tuple(other._components)

def __lt__(self, other):
if not self._is_valid_operand(other):
return AttributeError # TODO(python3port): typically this should return NotImplemented.
# Returning AttributeError for now to avoid changing prior API.
for ours, theirs in zip_longest(self._components, other._components, fillvalue=0):
if ours != theirs:
return ours < theirs
return False

def __hash__(self):
return hash(self._components)
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/base/run_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import re
import socket
import time
from builtins import object, str

from pants import version
from pants.base.build_environment import get_buildroot, get_scm
Expand Down
8 changes: 5 additions & 3 deletions src/python/pants/base/worker_pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@
import multiprocessing
import signal
import sys
import thread
import threading
from builtins import next, object
from multiprocessing.pool import ThreadPool

from future.moves import _thread

from pants.reporting.report import Report


Expand Down Expand Up @@ -103,7 +105,7 @@ def error(e):

# We filter out Nones defensively. There shouldn't be any, but if a bug causes one,
# Pants might hang indefinitely without this filtering.
work_iter = iter(filter(None, work_chain))
work_iter = (_f for _f in work_chain if _f)

def submit_next():
try:
Expand Down Expand Up @@ -149,7 +151,7 @@ def _do_work(self, func, args_tuple, workunit_name, workunit_parent, on_failure=
except KeyboardInterrupt:
# If a worker thread intercepts a KeyboardInterrupt, we want to propagate it to the main
# thread.
thread.interrupt_main()
_thread.interrupt_main()
raise
except Exception as e:
if on_failure:
Expand Down
3 changes: 1 addition & 2 deletions src/python/pants/base/workunit.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@
import re
import time
import uuid
from builtins import object, range
from collections import namedtuple

from six.moves import range

from pants.util.dirutil import safe_mkdir_for
from pants.util.memo import memoized_method
from pants.util.rwbuf import FileBackedRWBuf
Expand Down
5 changes: 5 additions & 0 deletions tests/python/pants_test/base/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ python_tests(
name = 'exclude_target_regexp_integration',
sources = [ 'test_exclude_target_regexp_integration.py' ],
dependencies = [
'3rdparty/python:future',
'src/python/pants/util:process_handler',
'tests/python/pants_test:int-test',
],
Expand All @@ -58,6 +59,7 @@ python_library(
name = 'context_utils',
sources = ['context_utils.py'],
dependencies = [
'3rdparty/python:future',
'3rdparty/python/twitter/commons:twitter.common.collections',
'src/python/pants/base:workunit',
'src/python/pants/build_graph',
Expand All @@ -70,6 +72,7 @@ python_tests(
name = 'deprecated',
sources = ['test_deprecated.py'],
dependencies = [
'3rdparty/python:future',
'src/python/pants/base:deprecated',
'src/python/pants:version',
]
Expand Down Expand Up @@ -109,6 +112,7 @@ python_tests(
name = 'hash_utils',
sources = ['test_hash_utils.py'],
dependencies = [
'3rdparty/python:future',
'src/python/pants/base:hash_utils',
'src/python/pants/util:contextutil',
]
Expand Down Expand Up @@ -185,6 +189,7 @@ python_tests(
name = 'worker_pool',
sources = ['test_worker_pool.py'],
dependencies = [
'3rdparty/python:future',
'src/python/pants/base:worker_pool',
'src/python/pants/util:contextutil',
]
Expand Down
Loading