Check for submodules #3661

stsewd · 2018-02-23T04:44:04Z

stsewd · 2018-02-23T04:45:49Z

readthedocs/rtd_tests/tests/test_backend.py

@@ -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())


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?

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

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?

stsewd · 2018-02-23T04:47:36Z

readthedocs/vcs_support/backends/git.py

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


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.

~~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.~~

I figure out

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?

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 · 2018-02-23T06:29:52Z

readthedocs/rtd_tests/utils.py

-    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


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

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

stsewd · 2018-02-23T07:00:52Z

readthedocs/rtd_tests/utils.py

    log.info(check_output(['git', 'commit', '-m"init"'], env=env))
+    log.info(check_output(['git', 'checkout', '-b', 'submodule'], env=env))


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

stsewd · 2018-02-23T07:27:04Z

readthedocs/rtd_tests/utils.py

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


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

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 · 2018-02-23T07:29:31Z

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 "[email protected]"
  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?

humitos

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.

humitos · 2018-02-23T15:50:59Z

readthedocs/vcs_support/backends/git.py

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


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?

humitos · 2018-02-23T15:55:56Z

readthedocs/rtd_tests/utils.py

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


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.

humitos · 2018-02-23T15:56:24Z

readthedocs/rtd_tests/tests/test_backend.py

+        repo.checkout()
+        self.assertFalse(repo.submodules_exists())
+
+        repo.checkout('submodule')


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 · 2018-02-23T16:37:37Z

@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 · 2018-02-26T01:44:07Z

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

stsewd · 2018-02-26T01:48:39Z

@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 · 2018-02-26T02:02:18Z

@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 · 2018-02-26T02:03:14Z

@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 · 2018-02-26T02:10:19Z

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

agjohnson

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.

agjohnson · 2018-02-27T07:12:13Z

readthedocs/vcs_support/backends/git.py

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


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 added 2 commits February 22, 2018 23:42

Check for submodules

e31d37f

Add test

5723d91

stsewd commented Feb 23, 2018

View reviewed changes

stsewd force-pushed the check-for-submodules branch from 0c7c713 to 5284a3d Compare February 23, 2018 06:28

stsewd commented Feb 23, 2018

View reviewed changes

Quick test for travis

1c94f3d

stsewd force-pushed the check-for-submodules branch from 5284a3d to 1c94f3d Compare February 23, 2018 06:41

stsewd added 3 commits February 23, 2018 01:55

Add branch for submodule

c1f78ce

Assert for old version

f7ab4f0

Better tests

2de6a7d

stsewd commented Feb 23, 2018

View reviewed changes

stsewd added 3 commits February 23, 2018 02:16

Add repo itself as submodule

fb47f5c

Remove assert

a67f024

Move comment

301547e

stsewd force-pushed the check-for-submodules branch from ff4dcc2 to 301547e Compare February 23, 2018 07:24

stsewd commented Feb 23, 2018

View reviewed changes

RichardLitt added the PR: work in progress Pull request is not ready for full review label Feb 23, 2018

humitos reviewed Feb 23, 2018

View reviewed changes

stsewd added 2 commits February 23, 2018 11:30

Add comment about checkout

c9a33a4

Checkout to master again

c4a8262

Typo

44c4d93

humitos added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Feb 26, 2018

humitos requested a review from agjohnson February 26, 2018 02:02

humitos approved these changes Feb 26, 2018

View reviewed changes

agjohnson approved these changes Feb 27, 2018

View reviewed changes

agjohnson merged commit e7dbb2b into readthedocs:master Feb 27, 2018

stsewd deleted the check-for-submodules branch March 4, 2018 03:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for submodules #3661

Check for submodules #3661

stsewd commented Feb 23, 2018

stsewd Feb 23, 2018

stsewd Feb 23, 2018

stsewd Feb 23, 2018

stsewd Feb 23, 2018

stsewd Feb 23, 2018 •

edited

Loading

stsewd Feb 23, 2018

humitos Feb 23, 2018

agjohnson Feb 27, 2018

stsewd Feb 23, 2018 •

edited

Loading

stsewd Feb 23, 2018

stsewd Feb 23, 2018

stsewd Feb 23, 2018

humitos Feb 23, 2018

stsewd commented Feb 23, 2018

humitos left a comment

humitos Feb 23, 2018

humitos Feb 23, 2018

humitos Feb 23, 2018

stsewd commented Feb 23, 2018

humitos commented Feb 26, 2018

stsewd commented Feb 26, 2018

humitos commented Feb 26, 2018

humitos commented Feb 26, 2018

stsewd commented Feb 26, 2018

agjohnson left a comment

agjohnson Feb 27, 2018

		log.info(check_output(['git', 'commit', '-m"init"'], env=env))
		log.info(check_output(['git', 'checkout', '-b', 'submodule'], env=env))

Check for submodules #3661

Check for submodules #3661

Conversation

stsewd commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Feb 23, 2018

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Feb 23, 2018

humitos commented Feb 26, 2018

stsewd commented Feb 26, 2018

humitos commented Feb 26, 2018

humitos commented Feb 26, 2018

stsewd commented Feb 26, 2018

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Feb 23, 2018 •

edited

Loading

stsewd Feb 23, 2018 •

edited

Loading