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

Use gitpython for tags #4052

Merged
merged 14 commits into from May 29, 2018

Conversation

Projects
None yet
6 participants
@stsewd
Member

stsewd commented May 2, 2018

Fix #1820

It has been previous attempts to fix the issue with annotated tags (#3302).

I'm using git python to get the tags, I tested this with a local repo with annotated and lightweight tags and it works as expected :).

I'm little worried about pyhon2 here since we need to use str to get the info. I did two builds using py2 (one with tags and other with no tags) and works as expected.

Refs #3839

@stsewd

This comment has been minimized.

Member

stsewd commented May 2, 2018

We may want to change all the class with gitpython in the future and have a repo object shared across methods.

@stsewd stsewd referenced this pull request May 2, 2018

Merged

Update Git on prod #3615

@agjohnson

Looks good so far. Perhaps it's better to use future's str via from builtins import str? I'm not sure what error you're hitting there otherwise.

Also, i might be good to look at #3839. #2997 is another. The attempt there is to support utf8 branches, which break because of the csvreader. The tests probably won't translate, but we should be trying to break the gitpython support in tests probably -- so perhaps throw some utf8 tags in?

This PR will translate pretty easily to parsing branches as well, that might be a good next target.

VCSVersion(self, str(tag.commit), str(tag))
for tag in repo.tags
]
return versions

This comment has been minimized.

@agjohnson

agjohnson May 3, 2018

Contributor

❤️ so much cleaner and no csv parsing hacks!

This comment has been minimized.

@techtonik

techtonik May 4, 2018

Contributor

But no tests.

@agjohnson agjohnson added this to the 2.4 milestone May 3, 2018

@humitos

This comment has been minimized.

Member

humitos commented May 3, 2018

Shouldn't we have a new test for the new "parser". I mean, at least a test that check that given a valid repo it returns a list if VCSVersion objects?

@stsewd

This comment has been minimized.

Member

stsewd commented May 4, 2018

@humitos I hesitated to add a test here because basically, we would be testing an external library, but I'll try to add a test to check if we return VCSVersion objects.

@stsewd

This comment has been minimized.

Member

stsewd commented May 7, 2018

Perhaps it's better to use future's str via from builtins import str? I'm not sure what error you're hitting there otherwise

No problem at all, I'll try to do a manual build with a unicode tag.

Test added (I took the unicode test from #2997 :) )

@stsewd

This comment has been minimized.

Member

stsewd commented May 7, 2018

The Unicode test did not pass :/ on py2

@humitos

This comment has been minimized.

Member

humitos commented May 8, 2018

The Unicode test did not pass :/ on py2

Why it does fail in py2? I think we should support the unicode tags in py2. Is it to hard to make it work?

Also, the test seems that doesn't pass in py3 either: https://travis-ci.org/rtfd/readthedocs.org/jobs/376123608

chdir(path)
return directory
def create_tag(directory, tag, annotated=False):

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

create_git_tag maybe?

and I think that directory can be renamed to path also, to keep the same wording.

This comment has been minimized.

@stsewd

stsewd May 8, 2018

Member

Directory makes reference to the tmp directory, and path is used for the cwd,

create_tag(repo_path, 'release-ünîø∂é')
repo = self.project.vcs_repo()
# Hack the repo path
repo.working_dir = repo_path

This comment has been minimized.

@humitos

humitos May 8, 2018

Member

Why do we need to do this here?

This comment has been minimized.

@stsewd

stsewd May 16, 2018

Member

Because we don't make the clone part, so the vcs object will try to search the repo in /project/checkouts/.... Sorry for the late reply, I was focused in fixin the unicode problem 😁

@stsewd

This comment has been minimized.

Member

stsewd commented May 8, 2018

Also, the test seems that doesn't pass in py3 either: https://travis-ci.org/rtfd/readthedocs.org/jobs/376123608

Not sure about that, tests are passing locally with py3, I'll push a test for py3 only

@stsewd

This comment has been minimized.

Member

stsewd commented May 8, 2018

Why it does fail in py2? I think we should support the unicode tags in py2. Is it to hard to make it work?

The tests on py2 aren't passing because of the subprocess call, I was reading the docs, and didn't found anything useful.

@stsewd

This comment has been minimized.

Member

stsewd commented May 8, 2018

I tested gitpython on py2 and py3, in both versions gitpython work with unicode tags, the problem here is with the subprocess call on the tests, going to take a deep look there

env=env).communicate()[0]
def check_output(l, env=None):
output = subprocess.Popen(
l, stdout=subprocess.PIPE, universal_newlines=True, env=env

This comment has been minimized.

@stsewd
@stsewd

This comment has been minimized.

Member

stsewd commented May 8, 2018

Well, at least tests are passing on py2 with unicode tags 😪

@stsewd

This comment has been minimized.

Member

stsewd commented May 11, 2018

OK, I figured out, this error is because of a python bug on 3.6.3 (which run on travis). On my local machine, I have 3.6.5. I tested this with the help of pyenv :)

@stsewd

This comment has been minimized.

Member

stsewd commented May 11, 2018

Also replicated on 3.6.4

@stsewd

This comment has been minimized.

Member

stsewd commented May 11, 2018

So, I tested this with python 3.6.5 from pyenv, same error, so I guess the same is done on travis?

@stsewd

This comment has been minimized.

Member

stsewd commented May 16, 2018

According to the docs the LC_CTYPE var isn't passed by default https://tox.readthedocs.io/en/latest/config.html#confval-passenv=SPACE-SEPARATED-GLOBNAMES. So that fixed the problem.

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 22, 2018

This looks like a solid step forward. I am a bit scared because we blew up projects previously with a similar change to this. @stsewd have you tested this locally on a few projects and confirmed it's working well?

I'm definitely +1 on this change, but don't want to break tag updating again :)

@stsewd

This comment has been minimized.

Member

stsewd commented May 22, 2018

@ericholscher I going to do more manual tests and also with github integration and report back (I think the last time it broke was because the git version that was used in production, I'm testing with 2.17.0).

@stsewd

This comment has been minimized.

Member

stsewd commented May 22, 2018

@ericholscher I did some manual tests, this fixes the Edit on GitHub link, but fails to parse unicode tags (actually it fails on tag.commit, which is weird because repo.tags works perfectly and both steps pass on the tests :/). Maybe the environment variables are affecting this (I tried setting some, but still fails)?

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

@stsewd is there a good way for us to at least skip unicode tags, instead of breaking the build completely? That would at least be a good step forward, and less disruptive.

@stsewd

This comment has been minimized.

Member

stsewd commented May 24, 2018

@ericholscher I think we can catch the non ascii exception, but I'm not sure if that's better for the users since they wouldn't be notified about that. I'm still thinking that this just need some ajustments in the environment variables, going to tests comparing the environment from tox and my pc.

@humitos

This comment has been minimized.

Member

humitos commented May 24, 2018

I did some manual tests, this fixes the Edit on GitHub link, but fails to parse unicode tags

Not sure I'm following here. The tests do work on Travis, but they don't work on your local tox or, they work on Travis and in your local tox but the manual QA test fails?

I could add more info here by testing it in my local instance to check for potential environment variable issues. Let me know what would be the steps to follow and test.

@stsewd

This comment has been minimized.

Member

stsewd commented May 24, 2018

@humitos they pass in tox, but manual QA fails in the same code that is tested.

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

I don't think we should block this on unicode though, since it's already broken. We should follow up on unicode support in #3060, but I think I'm +1 on merging this.

@stsewd

This comment has been minimized.

Member

stsewd commented May 25, 2018

I wasn't able to make it work setting and unsetting some variables, I think we'll figure out on prod if we can use unicode tags p:

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 29, 2018

I think unicode will break things downstream, so it is likely best to skip them for now. But we should discuss this more in #3060, and see if we can at least skip them gracefully.

@ericholscher ericholscher merged commit 242ccae into rtfd:master May 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:use-git-python-for-tags branch May 29, 2018

@rawtaz

This comment has been minimized.

rawtaz commented May 29, 2018

What is needed for this fix to be effective in the various projects' documentation that use readthedocs.org (e.g. https://restic.readthedocs.io/en/stable/ which has a broken Edit button)?

@stsewd

This comment has been minimized.

Member

stsewd commented May 29, 2018

@rawtaz just need to wait for the code to be deployed and then I think you probably need to rebuild your project too.

@rawtaz

This comment has been minimized.

rawtaz commented May 29, 2018

@stsewd Thanks :) Any idea when the code will be deployed, or more specifically what should I keep an eye on to know when it happens?

@stsewd

This comment has been minimized.

Member

stsewd commented May 29, 2018

RTD is deployed from the rel branch https://github.com/rtfd/readthedocs.org/commits/rel, I don't know exactly when, but I hope in this week.

https://docs.readthedocs.io/en/latest/faq.html#what-commit-of-read-the-docs-is-in-production

@stsewd

This comment has been minimized.

Member

stsewd commented May 30, 2018

@rawtaz today this was deployed, let us know if there is a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment