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.

Sign up for GitHub

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

Add future lib and port src/base #6067

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
31167f9
Add future and configparser libraries to dependencies for python2-3 c…
Jul 2, 2018
06b0db0
Port src/base to Python3.
Jul 5, 2018
0c0e8f7
Remove unnecessary calls to list()
Jul 5, 2018
ce2216b
Revert to prior semantics of Revision __eq__
Jul 5, 2018
77ae991
Missed removing an unnecessary list
Jul 5, 2018
7f07f94
Fix failing sitegen test due to changes in standard lib's escape func…
Jul 6, 2018
8bc62a4
Fully restore prior semantics with revision.py and improve TODOs
Jul 6, 2018
70d2547
Simplify generator expression
Jul 6, 2018
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
View file
Open in desktop
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
23 changes: 22 additions & 1 deletion src/python/pants/base/BUILD
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
31 changes: 19 additions & 12 deletions src/python/pants/base/revision.py
View file
Open in desktop
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,24 @@ 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.

More on :API: public here: https://www.pantsbuild.org/deprecation_policy.html

Copy link
Contributor Author

@Eric-Arellano Eric-Arellano Jul 5, 2018 •
edited
Loading

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: typically this should return NotImplemented. Returning False to avoid changing prior API.
return tuple(self._components) == tuple(other._components)

def __lt__(self, other):
if not self._is_valid_operand(other):
return NotImplemented
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
View file
Open in desktop
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
View file
Open in desktop
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 = iter([_f for _f in work_chain if _f])
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be just work_iter = (_f for _f in work_chain if _f); ie directly construct an iter (generator) vs create a list and then ask it for an iter. If this change was automated however, I'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it. If we don't change it now, it will probably never get changed and might be confusing to later devs why we're using this roundabout approach.

I do agree with you that we should be more rigorous though with keeping 100% semantic parity (as possible). But for small things like removing extraneous list(), I think it's worth fixing what futurize generates.


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
View file
Open in desktop
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
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion tests/python/pants_test/base/context_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import logging
import sys
from builtins import map, object
from contextlib import contextmanager

from twitter.common.collections import maybe_list
Expand Down Expand Up @@ -107,7 +108,7 @@ def subproc_map(self, f, items):
:API: public
"""
# Just execute in-process.
return map(f, items)
return list(map(f, items))


def create_context_from_options(options, target_roots=None, build_graph=None,
Expand Down
1 change: 1 addition & 0 deletions tests/python/pants_test/base/test_deprecated.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import unittest
import warnings
from builtins import object, str
from contextlib import contextmanager

from pants.base.deprecated import (BadDecoratorNestingError, BadRemovalVersionError,
Expand Down
Loading