Skip to content

Updated tox file and changed Travis CI to use tox #597

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

Merged
merged 13 commits into from
Jun 4, 2019
Merged

Updated tox file and changed Travis CI to use tox #597

merged 13 commits into from
Jun 4, 2019

Conversation

jadchaar
Copy link
Member

@jadchaar jadchaar commented Jun 3, 2019

Along with these tox configuration changes, I also updated the isort config to not conflict with Black (which wraps import lines beyond 88 characters and uses trailing commas).

Closes: #589

@jadchaar
Copy link
Member Author

jadchaar commented Jun 3, 2019

@asottile I took your advice and decided to tweak our tox environment and use it for our Travis builds, but I am not sure why the builds are failing. Am I using tox incorrectly on Travis?

I tried to use pytest as an example and made sure to include the requirements.txt file for modules such as dateutil, but I am still having some issues with the build failing with the error:

ERROR: FAIL could not package project - v = InvocationError(u'/home/travis/virtualenv/python2.7.15/bin/python setup.py sdist --formats=zip --dist-dir /home/travis/build/crsmithdev/arrow/.tox/dist', 1)

Setting the environment variable and running tox --recreate locally works fine. Also, dateutils is in the requirements.txt file as python-dateutils.

@asottile
Copy link

asottile commented Jun 3, 2019

the failure actually points out an important point of the packaging (and one of the many motivations for using tox in the first place!): the packaging for arrow is currently broken when installing from source

this line in the setup.py: https://github.com/crsmithdev/arrow/blob/cfafc0e4b2f2fcf2b004cb0ab2a67e2bb5feb56d/setup.py#L4

that attempts to import arrow/__init__.py

which in turn imports most of the arrow packaging

You generally have ~3 approaches here:

  1. Read the file in setup.py instead of importing (PRO: no dependencies, CON: slightly janky? ish?)

    and you actually have two sub-approaches here, you can either use the ast or you can just read it and use a regex, the regex usually works fine 🤷‍♂

    version_re = re.compile('(?<=__version__ = ")([^"]+)(?=")')
    with open('arrow/__init__.py') as f:
        version = version_re.search(f.read()).group()

    here's a way with an ast parser

    class Visitor(ast.NodeVisitor):
        def visit_Assign(self, node):
            if (
                    len(node.targets) == 1 and
                    isinstance(node.targets[0], ast.Name) and
                    node.targets[0].id == '__version__' and
                    isinstance(node.value, ast.Str)
            ):
                self.version = node.value.s
    
    with open('arrow/__init__.py', 'rb') as f:
        visitor = Visitor()
        visitor.visit(ast.parse(f.read(), filename='arrow/__init__.py'))
        version = visitor.version

    it's more code, but it's more likely to not break due to regex fragility

  2. only define the version in setup.py, and at runtime use importlib_metadata.version(__name__) to retrieve the version

    (this is the way I've used in my projects with success, though it does come with a slight import-time cost) -- for example: https://github.com/pre-commit/pre-commit/blob/a75fe69984b1e1508cb89c41139bb8f384c86a53/pre_commit/constants.py#L21

  3. Use setuptools-scm (or similar tool)

I don't know enough about how to set this up, but it's apparently a reasonable option


To demonstrate that it's broken from source:

$ pip install arrow --no-binary :all:
Collecting arrow
  Downloading https://files.pythonhosted.org/packages/a0/3b/0b5c7771a619ee4eae1f894669dd5f7c0aae9c92d87891c4c746937f9cc4/arrow-0.14.0.tar.gz (102kB)
     |████████████████████████████████| 112kB 2.4MB/s 
    ERROR: Complete output from command python setup.py egg_info:
    ERROR: Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-install-pki32a8v/arrow/setup.py", line 4, in <module>
        from arrow import __version__
      File "/tmp/pip-install-pki32a8v/arrow/arrow/__init__.py", line 3, in <module>
        from .api import get, now, utcnow
      File "/tmp/pip-install-pki32a8v/arrow/arrow/api.py", line 10, in <module>
        from arrow.factory import ArrowFactory
      File "/tmp/pip-install-pki32a8v/arrow/arrow/factory.py", line 15, in <module>
        from dateutil import tz as dateutil_tz
    ModuleNotFoundError: No module named 'dateutil'
    ----------------------------------------
ERROR: Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-install-pki32a8v/arrow/

cheers! hope this helps :)

@codecov-io
Copy link

codecov-io commented Jun 3, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #597   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines        1501   1501           
  Branches      225    225           
=====================================
  Hits         1501   1501

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 95f2be6...99fbf48. Read the comment docs.

@systemcatch
Copy link
Collaborator

@jadchaar does this need to go in 0.14.1 as well or can it wait until the next release?

@jadchaar
Copy link
Member Author

jadchaar commented Jun 3, 2019

We can wait until the next release since this is more of a backend/housekeeping thing.

Copy link

@asottile asottile left a comment

Choose a reason for hiding this comment

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

otherwise looks good!

asottile added a commit to pytest-dev/pytest that referenced this pull request Jun 3, 2019
Looks like this has been in the history since the beginning of time, but we should always get a blank slate anyway

Noticed this in arrow-py/arrow#597
@jadchaar
Copy link
Member Author

jadchaar commented Jun 4, 2019

@systemcatch mind reviewing these changes so we can get them merged in for release 0.14.2? Andrew found another regression with setup.py and locales (#601).

@jadchaar
Copy link
Member Author

jadchaar commented Jun 4, 2019

Note: I replaced codecs with io in setup.py in the latest commit after some research (namely https://stackoverflow.com/questions/5250744/difference-between-open-and-codecs-open-in-python).

@systemcatch systemcatch merged commit 26a8af8 into arrow-py:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display stdlib errors on Travis run
4 participants