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

Use gitpython for tags #4052

Merged
merged 14 commits into from May 29, 2018
Merged

Conversation

@stsewd
Copy link
Member

@stsewd 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
Copy link
Member Author

@stsewd 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.

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

Loading

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

@agjohnson agjohnson May 3, 2018

Choose a reason for hiding this comment

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

❤️ so much cleaner and no csv parsing hacks!

Loading

Copy link
Contributor

@techtonik techtonik May 4, 2018

Choose a reason for hiding this comment

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

But no tests.

Loading

@agjohnson agjohnson added this to the 2.4 milestone May 3, 2018
@humitos
Copy link
Member

@humitos 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?

Loading

@stsewd
Copy link
Member Author

@stsewd 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.

Loading

@stsewd
Copy link
Member Author

@stsewd 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 :) )

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 7, 2018

The Unicode test did not pass :/ on py2

Loading

@humitos
Copy link
Member

@humitos 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

Loading

chdir(path)
return directory


def create_tag(directory, tag, annotated=False):
Copy link
Member

@humitos humitos May 8, 2018

Choose a reason for hiding this comment

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

create_git_tag maybe?

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

Loading

Copy link
Member Author

@stsewd stsewd May 8, 2018

Choose a reason for hiding this comment

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

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

Loading

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

@humitos humitos May 8, 2018

Choose a reason for hiding this comment

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

Why do we need to do this here?

Loading

Copy link
Member Author

@stsewd stsewd May 16, 2018

Choose a reason for hiding this comment

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

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 😁

Loading

@stsewd
Copy link
Member Author

@stsewd 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

Loading

@stsewd
Copy link
Member Author

@stsewd 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.

Loading

@stsewd
Copy link
Member Author

@stsewd 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

Loading

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

@stsewd stsewd May 8, 2018

Choose a reason for hiding this comment

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

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 8, 2018

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

Loading

@stsewd
Copy link
Member Author

@stsewd 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 :)

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 11, 2018

Also replicated on 3.6.4

Loading

@stsewd
Copy link
Member Author

@stsewd 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?

Loading

@stsewd
Copy link
Member Author

@stsewd 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.

Loading

@ericholscher
Copy link
Member

@ericholscher 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 :)

Loading

@stsewd
Copy link
Member Author

@stsewd 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).

Loading

@stsewd
Copy link
Member Author

@stsewd 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)?

Loading

@ericholscher
Copy link
Member

@ericholscher 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.

Loading

@stsewd
Copy link
Member Author

@stsewd 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.

Loading

@humitos
Copy link
Member

@humitos 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.

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 24, 2018

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

Loading

@ericholscher
Copy link
Member

@ericholscher 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.

Loading

@stsewd
Copy link
Member Author

@stsewd 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:

Loading

@ericholscher
Copy link
Member

@ericholscher 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.

Loading

@ericholscher ericholscher merged commit 242ccae into readthedocs:master May 29, 2018
1 check passed
Loading
@stsewd stsewd deleted the use-git-python-for-tags branch May 29, 2018
@rawtaz
Copy link

@rawtaz 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)?

Loading

@stsewd
Copy link
Member Author

@stsewd 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.

Loading

@rawtaz
Copy link

@rawtaz 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?

Loading

@stsewd
Copy link
Member Author

@stsewd 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

Loading

@stsewd
Copy link
Member Author

@stsewd stsewd commented May 30, 2018

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants