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

Fix setup.py and use of requirements files and add a docker test container #826

Merged
merged 3 commits into from
May 5, 2019

Conversation

jeking3
Copy link
Contributor

@jeking3 jeking3 commented Jan 15, 2019

Before:

jking@ubuntu:~/GitPython$ python3 setup.py check
/usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'test_requirements'
  warnings.warn(msg)
running check

Note that setup.py was not even using the contents of requirements.txt or test-requirements.txt. Now it does, and it properly identifies the tests-require field. The requirements.txt file declared a runtime dependency on a testing package. I'm assuming that is an error (although it could be used in production code, I don't know...) so I removed that.

When I ran tox locally on Python 3.6.7 I saw a flake8 error F821 which I disabled as it looks bogus - that exception is defined in the documentation.

Added a docker container that you can use with a couple new make targets. This allows you to run python 2.7, 3.4, 3.5, 3.6, or 3.7 tox even if you are on a local branch not named "master" (some tests require this).

Finally, found and fixed the issue on CI with python 3.7 / Xenial / travis. The root cause is that in git 2.20 new behavior was introduced to stop "git fetch --tags" from clobbering local tags that have changed: git/git@0bc8d71

The unit tests expect the old behavior, so now we pass --force down if the version is 2.20 or higher (for the remote test only, the code change is in the test code). It took a while to find this because for some reason when GitCommandError returns an error code, stderr isn't captured. If it was, it would have been easier to figure this out...

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 15, 2019

It looks like CI isn't healthy. Do you want some help with that?

@Byron
Copy link
Member

Byron commented Jan 20, 2019

Thanks a bunch for uncovering this, and for the fix!

It looks like one travis failure legit, as it is flake not liking some whitespace.
The other travis failure looks like it is related to the - now - flaky Remote test, which also fails consistently in #825 .

Please have a look at the comment there - maybe we can make a fix here so #825 can then be merged, too.

CC @cclauss

@codecov-io
Copy link

codecov-io commented Jan 20, 2019

Codecov Report

Merging #826 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #826      +/-   ##
==========================================
- Coverage   94.79%   94.67%   -0.13%     
==========================================
  Files          59       59              
  Lines        9603     9386     -217     
==========================================
- Hits         9103     8886     -217     
  Misses        500      500
Impacted Files Coverage Δ
git/cmd.py 83.83% <100%> (-0.18%) ⬇️
git/objects/tree.py 59.64% <0%> (-0.93%) ⬇️
git/index/util.py 90.69% <0%> (-0.61%) ⬇️
git/index/typ.py 96.82% <0%> (-0.58%) ⬇️
git/refs/log.py 93.84% <0%> (-0.53%) ⬇️
git/objects/base.py 95.83% <0%> (-0.33%) ⬇️
git/refs/tag.py 96.66% <0%> (-0.31%) ⬇️
git/test/lib/helper.py 91.86% <0%> (-0.28%) ⬇️
git/refs/reference.py 95.74% <0%> (-0.26%) ⬇️
git/objects/commit.py 91.59% <0%> (-0.25%) ⬇️
... and 27 more

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 1f66e25...21b712a. Read the comment docs.

@jeking3 jeking3 force-pushed the fix-requirements branch 2 times, most recently from 023b17e to c045246 Compare January 20, 2019 15:58
@cclauss
Copy link
Contributor

cclauss commented Jan 24, 2019

Run Python 3.7 in allow_failures mode until #827 is fixed?

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 25, 2019

What about the windows failures?

@cclauss
Copy link
Contributor

cclauss commented Jan 25, 2019

FileNotFoundError: [WinError 2] The system cannot find the file specified:
    'C:\\Miniconda35-x64\\lib\\site-packages\\setuptools-27.2.0-py3.5.egg'

But the lines just above that say that the current version of setuptools (40.6.3) was just installed. Either don't upgrade setup tools or eliminate the hardcoding of setuptools-27.2.0.

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 25, 2019

What I was asking is do you want to ignore those too?

I'd prefer to merge this, and then deal with the CI failures, instead of bundling them all together... these changes improve (and fix) the setup.py issues.

@cclauss
Copy link
Contributor

cclauss commented Jan 25, 2019

You are asking the wrong guy... I have never been a Windows fan. ;-) I would vote to move forward as is.

@jeking3
Copy link
Contributor Author

jeking3 commented Feb 5, 2019

Do we need the last three windows builds for "miniconda" or "git running in cygwin on windows"? The latter just looks like a test environment failure as it is passing windows paths (from the windows native python 3.5-x64) into a program looking for a unix path. I have no idea what the "conda" ones are or if they provide any real value.

@jeking3
Copy link
Contributor Author

jeking3 commented Feb 5, 2019

The test suite makes a very bad assumption that you are on the master branch. When I work on a project, I fork it, then I make a branch for each of the changes I am making, and submit pull requests from the branch into the upstream master. This is best practices for github development.

I cannot run the test suite if I have anything else checked out. The test suite fails. It is making some bad assumptions. When I run inside a docker container and skip past those, even with python 3.6, I'm seeing other issues like utf8 issues.

@jeking3 jeking3 force-pushed the fix-requirements branch 2 times, most recently from 0ede297 to 03342ce Compare February 5, 2019 21:58
@jeking3
Copy link
Contributor Author

jeking3 commented Feb 5, 2019

I worked around the issues by checking out a copy of the repository from github into the container at a well-known location. This way the unit tests can run against a fresh clone of the repository and succeed.

@jeking3 jeking3 force-pushed the fix-requirements branch 4 times, most recently from 56a06d8 to e5b03d4 Compare February 5, 2019 22:45
@jeking3 jeking3 changed the title Fix setup.py and use of requirements files. Fix setup.py and use of requirements files, add a docker test container, and fix a git 2.20 test issue Feb 6, 2019
@jeking3
Copy link
Contributor Author

jeking3 commented Feb 6, 2019

I'm going to tease this apart into smaller bits...

@jeking3 jeking3 changed the title Fix setup.py and use of requirements files, add a docker test container, and fix a git 2.20 test issue -DO NOT MERGE] Fix setup.py and use of requirements files, add a docker test container, and fix a git 2.20 test issue Feb 6, 2019
@jeking3 jeking3 closed this Mar 15, 2019
@jeking3 jeking3 reopened this May 5, 2019
@jeking3 jeking3 changed the title -DO NOT MERGE] Fix setup.py and use of requirements files, add a docker test container, and fix a git 2.20 test issue Fix setup.py and use of requirements files and add a docker test container May 5, 2019
@jeking3
Copy link
Contributor Author

jeking3 commented May 5, 2019

@Byron consider merging this. The appveyor jobs are passing, at least the ones known to pass, and there are some that are known failures; it would still be nice to get all the CI builds to green since that makes it so much easier to determine if a pull request is even worth looking into.

@Byron Byron merged commit 52ee33a into gitpython-developers:master May 5, 2019
@Byron
Copy link
Member

Byron commented May 5, 2019

Hi @jeking3 , thanks so much for your persistency. Right now I am very short on time and am far away from this, and any other of my open source projects, which must be frustrating for GitPython's user base.

Maybe it would help if there were more collaborators - I have invited you in case this sounds feasible to you.

@jeking3
Copy link
Contributor Author

jeking3 commented May 5, 2019

I'll do what I can. Apart from founding a new company, I'm also involved in boost, apache thrift, and a couple other efforts. At the very least if I can get the CI builds to run clean and have folks rebase their pull requests, it would be a lot easier to qualify things in the pipeline. Thanks!

@jeking3 jeking3 deleted the fix-requirements branch May 5, 2019 16:31
@Byron Byron added this to the v2.1.12 - Bugfixes milestone Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants