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

Feature: shallow git clone #5514

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@SSE4
Copy link
Contributor

commented Jul 18, 2019

Changelog: Feature: shallow git clone
Docs: conan-io/docs#1380
closes: #3770
@PYVERS: Macos@py27, Windows@py36, Linux@py27, py34
@tags: svn, slow
@revisions: 1

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@jgsogo I need some advice on how to write tests for this - e.g. use real git repositories, use mocks and verify commands invoked or something else?

@jgsogo

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@jgsogo I need some advice on how to write tests for this - e.g. use real git repositories, use mocks and verify commands invoked or something else?

I would use a local git repository, it runs very fast and I think that here you'd want to test that the command actually works.

@SSE4 SSE4 force-pushed the SSE4:shallow_clone branch from eb81117 to dc1c646 Jul 19, 2019

@SSE4 SSE4 marked this pull request as ready for review Jul 19, 2019

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@jgsogo I've added some test, please take a look if it's good enough

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

CI error seems to be unrelated: OSError: [Errno 48] Address already in use

@jgsogo

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@jgsogo I've added some test, please take a look if it's good enough

I think it tests what needs to be tested. I don't like the fetch in the checkout function, but probably there is no other way to do it.

@lasote lasote added this to the 1.18 milestone Jul 25, 2019

@lasote
Copy link
Contributor

left a comment

I would like a simpler approach:

  • No conan.conf entry,
  • The SCM model will try to do a shallow clone always, and If it fails, try always to fallback to general approach (but the fallback not in the tool, but in the scm model). If we are doing the non-shallow clone, do not call the checkout (no sense)
  • The tool should be dummy, if I call .checkout and it is a shallow clone, it has to fail.
  • The same if I try to do a shallow clone of a commit and the server doesn't support it.

@SSE4

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Interesting discussion:

I agree it would be nice to simplify and remove conan.conf configuration, but I am concerned it might be breaking somewhere? I do not know about the details and failures modes of shallow clones, specially when you try to do a checkout or another operations after it.

@SSE4 SSE4 force-pushed the SSE4:shallow_clone branch 6 times, most recently from b9f36b2 to 469154b Jul 26, 2019

@lasote
Copy link
Contributor

left a comment

Add test for SVN getting the version.

conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/client/tools/scm.py Outdated Show resolved Hide resolved

@SSE4 SSE4 force-pushed the SSE4:shallow_clone branch from 469154b to 3a22d38 Jul 29, 2019

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

added tests for SVN/Git.get_version

@lasote

lasote approved these changes Jul 29, 2019

@lasote lasote requested a review from jgsogo Jul 29, 2019

@jgsogo
Copy link
Member

left a comment

Please, have a look a the comments, I'm not sure if some of the changes can lead to unexpected effects.

conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/client/tools/scm.py Outdated Show resolved Hide resolved
conans/model/scm.py Show resolved Hide resolved

@SSE4 SSE4 force-pushed the SSE4:shallow_clone branch 2 times, most recently from 0b2e220 to a62b31e Jul 29, 2019

@SSE4 SSE4 referenced this pull request Jul 29, 2019

@SSE4 SSE4 force-pushed the SSE4:shallow_clone branch from a62b31e to 19a1f47 Jul 29, 2019

conans/client/tools/scm.py Outdated Show resolved Hide resolved
- shallow git clone
Signed-off-by: SSE4 <tomskside@gmail.com>

@SSE4 SSE4 force-pushed the SSE4:shallow_clone branch from 19a1f47 to fef6d3a Jul 29, 2019

"attribute in the 'scm'" % self.folder)

output = self.run("init")
output += self.run('remote add origin "%s"' % url)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

(Just thinking aloud:)

I know these are the same commands as before, but this command fails if the remote is already added to the repo:

⇒  git remote -v
origin	git@github.com:sword-and-sorcery/conanquest-fantastic-adventures.git (fetch)
origin	git@github.com:sword-and-sorcery/conanquest-fantastic-adventures.git (push)
[python3]jgsogo@MacBook-Pro-de-Javier:~/dev/conan/issues/tmp5514|master 
⇒  git remote add origin git@github.com:sword-and-sorcery/conanquest-fantastic-adventures.git
fatal: remoto origin ya existe.

Maybe it is just the name of the function, fetch, I know it is not public and won't be executed on its own, so these two lines probably will be removed in the future to the calling function.

@jgsogo

jgsogo approved these changes Jul 29, 2019

Copy link
Member

left a comment

LGTM.

@lasote lasote merged commit ab2d738 into conan-io:develop Jul 30, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.