Skip to content

Check for submodules #3661

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.

Sign up for GitHub

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 12 commits into from
Feb 27, 2018
Merged

Check for submodules #3661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e31d37f
Check for submodules
stsewd Feb 23, 2018
5723d91
Add test
stsewd Feb 23, 2018
1c94f3d
Quick test for travis
stsewd Feb 23, 2018
c1f78ce
Add branch for submodule
stsewd Feb 23, 2018
f7ab4f0
Assert for old version
stsewd Feb 23, 2018
2de6a7d
Better tests
stsewd Feb 23, 2018
fb47f5c
Add repo itself as submodule
stsewd Feb 23, 2018
a67f024
Remove assert
stsewd Feb 23, 2018
301547e
Move comment
stsewd Feb 23, 2018
c9a33a4
Add comment about checkout
stsewd Feb 23, 2018
c4a8262
Checkout to master again
stsewd Feb 23, 2018
44c4d93
Typo
stsewd Feb 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions readthedocs/rtd_tests/tests/test_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ 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()

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

# The submodule branch contains one submodule
repo.checkout('submodule')
self.assertTrue(repo.submodules_exists())


class TestHgBackend(RTDTestCase):
def setUp(self):
Expand Down
7 changes: 7 additions & 0 deletions readthedocs/rtd_tests/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ 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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

log.info(check_output(['git', 'add', '.'], env=env))
log.info(check_output(['git', 'commit', '-m"Add submodule"'], env=env))
# Checkout to master branch again
log.info(check_output(['git', 'checkout', 'master'], env=env))
chdir(path)
return directory

Expand Down
11 changes: 8 additions & 3 deletions readthedocs/vcs_support/backends/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

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 •
edited
Loading

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

I figure out

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.


def fetch(self):
code, _, _ = self.run('git', 'fetch', '--tags', '--prune')
if code != 0:
Expand Down Expand Up @@ -187,9 +191,10 @@ def checkout(self, identifier=None):
self.run('git', 'clean', '-d', '-f', '-f')

# Update submodules
self.run('git', 'submodule', 'sync')
self.run('git', 'submodule', 'update',
'--init', '--recursive', '--force')
if self.submodules_exists():
self.run('git', 'submodule', 'sync')
self.run('git', 'submodule', 'update',
'--init', '--recursive', '--force')

return code, out, err

Expand Down