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

Check for submodules #3661

Merged
merged 12 commits into from Feb 27, 2018
Merged

Check for submodules #3661

merged 12 commits into from Feb 27, 2018

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 23, 2018

Closes #3627

@@ -78,6 +78,10 @@ def test_parse_git_tags(self):
self.project.vcs_repo().parse_tags(data)]
self.assertEqual(expected_tags, given_ids)

def test_check_for_submodules(self):
repo = self.project.vcs_repo()
self.assertFalse(repo.submodules_exists())
Copy link
Member Author

@stsewd stsewd Feb 23, 2018

I'm not sure how to check when there is a submodule, should I create a submodule on other branch on the git repo setup and then check here?

Copy link
Member Author

@stsewd stsewd Feb 23, 2018

Actually I don't think I'm really testing here, I think this command can be only executed inside checkout.

Copy link
Member Author

@stsewd stsewd Feb 23, 2018

And I see on the logs that make_test_git says there is not git configuration (name and email) when running log.info(check_output(['git', 'add', '.'], env=env))

This works on travis?

@@ -55,6 +55,10 @@ def repo_exists(self):
code, _, _ = self.run('git', 'status', record=False)
return code == 0

def submodules_exists(self):
code, out, _ = self.run('git', 'submodule', 'status', record=False)
return code == 0 and bool(out)
Copy link
Member Author

@stsewd stsewd Feb 23, 2018

This command always returns 0, but the output is empty when there isn't submodules, I think is better to rely on git than checking for .gitmodules file.

Copy link
Member Author

@stsewd stsewd Feb 23, 2018

Well, actually checking for .gitmodules file makes the tests more easy to write :/. Please let me know if you want me to go for that path.

Copy link
Member Author

@stsewd stsewd Feb 23, 2018

I figure out

Copy link
Member

@humitos humitos Feb 23, 2018

I test this locally in a repo with 126 submodules and I found this command is too slow and very CPU consuming:

git submodule status  0,73s user 0,50s system 100% cpu 1,228 total

Even in the RTD repository:

$ time git submodule status
git submodule status  0,08s user 0,05s system 103% cpu 0,129 total

I like following the pattern here by using git commands, but this is something that we will want to consider over just checking the existance of the file.

@agjohnson what do you think?

Copy link
Contributor

@agjohnson agjohnson Feb 27, 2018

I thought I commented here, but I think i got side tracked determining how git submodule status worked, and I think i got stuck in that rabbit hole. Relying on git here is fine. If we find it could be refactored, I still think checking for a top level .gitmodules works just as fine also.

@stsewd stsewd force-pushed the check-for-submodules branch from 0c7c713 to 5284a3d Feb 23, 2018
log.info(check_output(['git', 'add', '.'], env=env))
output = check_output(['git', 'add', '.'], env=env)
log.info(output)
assert '*** Please tell me who you are.' not in output
Copy link
Member Author

@stsewd stsewd Feb 23, 2018

Just to prove my point, this assert only works for python3 (but the output on python2 is the same that python3)

Copy link
Member Author

@stsewd stsewd Feb 23, 2018

well, all the tests passed, I'm not sure why this happens on my local instance.

@stsewd stsewd force-pushed the check-for-submodules branch from 5284a3d to 1c94f3d Feb 23, 2018
log.info(check_output(['git', 'commit', '-m"init"'], env=env))
log.info(check_output(['git', 'checkout', '-b', 'submodule'], env=env))
Copy link
Member Author

@stsewd stsewd Feb 23, 2018

Any idea how to add a submodule here? maybe creating other repo?

@stsewd stsewd force-pushed the check-for-submodules branch from ff4dcc2 to 301547e Feb 23, 2018
@@ -34,6 +34,11 @@ def make_test_git():
log.info(check_output(['git', 'init'] + [directory], env=env))
log.info(check_output(['git', 'add', '.'], env=env))
log.info(check_output(['git', 'commit', '-m"init"'], env=env))
# Add repo itself as submodule
log.info(check_output(['git', 'checkout', '-b', 'submodule'], env=env))
log.info(check_output(['git', 'submodule', 'add', '-b', 'master', './', 'submodule'], env=env))
Copy link
Member Author

@stsewd stsewd Feb 23, 2018

I added the repo itself as submodule on other branch for test the submodule existence

Copy link
Member

@humitos humitos Feb 23, 2018

Isn't it needed to re-checkout master after creating the submodule and adding it? I mean, so other tests keep using the master branch as they did before.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 23, 2018

I was able to write the tests, but on my local instance the tests fail

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

Any idea what's going on?

Copy link
Member

@humitos humitos left a comment

I like this PR. I think it's a good improvement.

I left a couple of comments and one question to Anthony since I'm not sure what we do prefer (checking by os.path.exists or git submodule status). I think the later is better for consisntency but it seems that it's too slow.

@@ -55,6 +55,10 @@ def repo_exists(self):
code, _, _ = self.run('git', 'status', record=False)
return code == 0

def submodules_exists(self):
code, out, _ = self.run('git', 'submodule', 'status', record=False)
return code == 0 and bool(out)
Copy link
Member

@humitos humitos Feb 23, 2018

I test this locally in a repo with 126 submodules and I found this command is too slow and very CPU consuming:

git submodule status  0,73s user 0,50s system 100% cpu 1,228 total

Even in the RTD repository:

$ time git submodule status
git submodule status  0,08s user 0,05s system 103% cpu 0,129 total

I like following the pattern here by using git commands, but this is something that we will want to consider over just checking the existance of the file.

@agjohnson what do you think?

@@ -34,6 +34,11 @@ def make_test_git():
log.info(check_output(['git', 'init'] + [directory], env=env))
log.info(check_output(['git', 'add', '.'], env=env))
log.info(check_output(['git', 'commit', '-m"init"'], env=env))
# Add repo itself as submodule
log.info(check_output(['git', 'checkout', '-b', 'submodule'], env=env))
log.info(check_output(['git', 'submodule', 'add', '-b', 'master', './', 'submodule'], env=env))
Copy link
Member

@humitos humitos Feb 23, 2018

Isn't it needed to re-checkout master after creating the submodule and adding it? I mean, so other tests keep using the master branch as they did before.

repo.checkout()
self.assertFalse(repo.submodules_exists())

repo.checkout('submodule')
Copy link
Member

@humitos humitos Feb 23, 2018

Can you add a comment here explaining why you checkout submodule here and what's the difference with the other branch? Just for future reference.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 23, 2018

@humitos were you able to run the tests successfully on your local instance? Just want to be sure this isn't something that affects everyone testing on their local instance.

@humitos
Copy link
Member

@humitos humitos commented Feb 26, 2018

@stsewd I didn't run the tests locally, but what's your concern? The test are ran in Travis and they did pass.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 26, 2018

@humitos someone trying to submit a PR would see their tests fail on their local instance, probably this will cause a bad experience for who is trying to collaborate or setup his local instance.

@humitos
Copy link
Member

@humitos humitos commented Feb 26, 2018

@stsewd all the tests run without problem in my instance. Try re-running them by re-creating the environment (tox -r)

  py27: commands succeeded
  py36: commands succeeded
  lint: commands succeeded
  docs: commands succeeded
  congratulations :)

Running your branch.

@humitos
Copy link
Member

@humitos humitos commented Feb 26, 2018

@agjohnson can you review this PR since you created the issue? I already approve this one, but I left a comment regarding the usage of CPU that you may want to take a look.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 26, 2018

Still fails on my local instance :/ but glad that passed on yours :), probably it's some problem with my Fedora.

Copy link
Contributor

@agjohnson agjohnson left a comment

Yeah this looks great! I think the git command for a check is a good addition, there are probably some configuration cases that i'm not considering when I say we should just checkout the top-level .gitmodules file.

@@ -55,6 +55,10 @@ def repo_exists(self):
code, _, _ = self.run('git', 'status', record=False)
return code == 0

def submodules_exists(self):
code, out, _ = self.run('git', 'submodule', 'status', record=False)
return code == 0 and bool(out)
Copy link
Contributor

@agjohnson agjohnson Feb 27, 2018

I thought I commented here, but I think i got side tracked determining how git submodule status worked, and I think i got stuck in that rabbit hole. Relying on git here is fine. If we find it could be refactored, I still think checking for a top level .gitmodules works just as fine also.

@agjohnson agjohnson merged commit e7dbb2b into readthedocs:master Feb 27, 2018
1 check passed
@stsewd stsewd deleted the check-for-submodules branch Mar 4, 2018
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.

None yet

4 participants