Implement vcs similar to go get #1717

outcoldman · 2018-02-25T22:43:42Z

For private repositories and git over ssh repositories implementation of
dep was different from go get.

Examples:

  go get example.com/repo.git

Will use remote

  (scheme)://example.com/repo

The most important part that suffix .git tells go get to access the path
as git, but it drops the suffix to actually access the remote.
Go get will clone this repo as

  git clone git+ssh://example.com/repo $GOPATH/src/example.com/repo.git

It is very unfortunate that you will end up with .git suffixes but at least it
works, this is the only way to make go get to recognize this import as
git.

But in case of dep

  import "example.com/repo.git"

Will result for dep to try to access remote as

  (scheme)://example.com/repo.git

Which makes it impossible to teach go get to work with dep.

This change is making consistent behavior between go get and dep by
always dropping suffix .git for remotes (which is also not ideal, but
at least it works now with go dep).

Other changes related to the schemas, I made the order match to the
default schemes in go get (this one I reverted, as there are a lot of tests failed)

What does this do / why do we need it?

After fixing this issue I believe go get and dep will have good working solution for private git repos (as many people complain in other tickets that it does not work).

If you have your own private git repository available on git+ssh://example.com/repo you will be able to reference it in the go source like

import (
   "example.com/repo.git"
)

That will clone the git+ssh://example.com/repo under $GOPATH/src/example.com/repo.git, where suffix .git is not ideal, but at least it works.

And after that you will be able to run dep init and dep ensure which also behave similarly to go get by cloning example.com/repo.git from remote (scheme)://example.com/repo in path vendor/example.com/repo.git

What should your reviewer look out for in this PR?

Look on the issue and the problem described Inconsistency between go get and dep tools to how remote is resolved for git repos #1716
(this one I reverted, as there are a lot of tests failed) I have changed order of schemes to match it with go get as well vcs.go#L48-L54
Look on regex in go get to load vcs path and scheme vcs.go#L1019-L1022
Look on how go get handles this regex to resolve the remote, see vcs.go#L683-L751

Do you need help or clarification on anything?

None

Which issue(s) does this PR fix?

fixes #1716
fixes #174 (maybe?)

For private repositories and git over ssh repositories implementation of dep was different from `go get`. Examples: go get example.com/repo.git Will use remote (scheme)://example.com/repo The most important part that suffix .git tells go get to access the path as git, but it drops the suffix to actually access the remote. Go get will clone this repo as git clone git+ssh://example.com/repo $GOPATH/src/example.com/repo.git It is very unfortunate that you will end up with .git suffixes but at least it works, this is the only way to make `go get` to recognize this import as git. But in case of dep import "example.com/repo.git" Will result for dep to try to access remote as (scheme)://example.com/repo.git Which makes it impossible to teach go get to work with dep. This change is making consistent behavior between *go get* and *dep* by always dropping suffix *.git* for remotes (which is also not ideal, but at least it works now with *go dep*). Other changes related to the schemas, I made the order match to the default schemes in *go get*

googlebot · 2018-02-25T22:43:46Z

Thanks for your pull request. t looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-02-25T22:43:47Z

Thanks for your pull request. t looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

outcoldman · 2018-02-25T22:52:20Z

I signed it!

googlebot · 2018-02-25T22:52:34Z

CLAs look good, thanks!

googlebot · 2018-02-25T22:52:35Z

CLAs look good, thanks!

This reverts commit ca4c99c.

outcoldman · 2018-03-04T01:59:36Z

@sdboyer curious, did you have a chance to look into this? Just want to know if this is something that can be considered to be merged?

I definitely do not want to maintain my own fork of dep, so if this change cannot be merged - I will try to find different way how to make it work for me.

Also I am happy to iterate, add more explanations if needed, docs for how to work with private repos, etc.

hoshsadiq · 2018-03-04T15:22:14Z

IMO being required to update the import to have a .git suffix is a big no-no. I think if we can check the source first, and ignore the name for location checking (i.e. the name is just a local vendor path when source is declared), surely that would be a fix?

outcoldman · 2018-03-04T16:30:42Z

@hoshsadiq

being required to update the import to have a .git suffix is a big no-no

Could you explain why? Unfortunately go get already requires you to have .git as a suffix for the git path, so if you just want go get to be able to resolve dependencies you need to write import 'example.com/somelib.git/somepackage', where go get will understand that example.com/somelib.git is the git repo and /somepackage is the package in this git repo. It will resolve git repo in $GOPATH/src/example.com/somelib.git. Not having .git in the path does not allow tools to understand what is the repo and what is the path, as import 'example.com/somelib/somepackage' can be a repo=example.com/somelib, package=/somepackage or actually repo=example.com/somelib/somepackage, package=/.

So in my PR the only thing I do just trying to make dep to work exactly the same as go get.

I think if we can check the source first, and ignore the name for location checking (i.e. the name is just a local vendor path when source is declared), surely that would be a fix?

Not sure I understand the proposal, could you explain on example?

hoshsadiq · 2018-03-04T17:03:18Z

Reason being when you develop multiple projects, your $GOPATH/src layout will be:

src/git.myorg.com/team/project1
src/git.myorg.com/team/project2

Let's imagine project2 depends on project1, and you've made a change in projet1.

If you have the .git suffix, dep will force download project1 into project2/vendor/.../project1.git, which means it won't pick up the changes of project1. The only way to force it to pickup changes in project1 is by committing, pushing, and then doing dep ensure -update. Even breakpoints would have to be set in the files inside project2/vendor/.../project1.git as opposed setting them inside src/git.myorg.com/team/project1

Not having the git suffix means go will simply pick up your project1 without having to push or running dep every time you push.

Having this PR merged is certainly better than nothing, but this issue needs to be addressed correctly. Glide for example manages this correctly.

As for the proposal, I've not looked at how dep works internally, however, from what I've observed is that it's sees the name attribute as everything so please correct me if I'm wrong. This is fine because it can imply where we can find the source for the imported library. However, with this issue, this isn't the first thing should check. We should set the source the be the name if and only if there's no source present. I.e. this

[[constraint]]
  name = "github.com/pkg/errors"

is the same as

[[constraint]]
  name = "github.com/pkg/errors"
  source = "github.com/pkg/errors"

Then we resolve stuff, we only care about the source attribute. Finally, once the source location has been resolved, we can check it out in vendor/src/<value of name>. This way, we can set force the source to have a .git suffix, and leave name as it is. Go would pick up everything correctly, and if you already have that project in your $GOPATH it wouldn't clone the repo. Hope that helps. Again, I'm not sure how viable this solution would be considering I'm not really sure how the internals of go get and dep work.

outcoldman · 2018-03-04T17:23:15Z

@hoshsadiq

dep by default actually always goes to remote, not sure if there are way to actually force it to pick up the sources from local folder.

In your scenario you will need to change folder names to be

src/git.myorg.com/team/project1.git
src/git.myorg.com/team/project2.git

At the beginning, and in that case everything else will work.

The main reason why you need .git as a suffix, is because you need to tell go get and dep that
a) they are dealing with git repo, not hg or anything else. (in case of locally checked out repository go get will find it by looking on the .git folder, but in case of remote repo there is no way to tell)
b) to tell what is the git repo and what is the path inside the git repo. So currently there are no way to tell if this is a git repo src/git.myorg.com/team or that src/git.myorg.com/team/project1. This is why go get requires it explicitly.

Also with the scheme

src/git.myorg.com/team/project1
src/git.myorg.com/team/project2

And if project2 depends on project1 you don't have a way to make go get git.myorg.com/team/project2, it will not work (in case if you are using simple ssh access to your git), because go get requires you to tell that this is a git.

With this change the workflow for your example will be:

You can call go get git.myorg.com/team/project2.git, that will clone it to src/git.myorg.com/team/project2.git and resolve project1 to src/git.myorg.com/team/project1.git
You will need to always include as include 'git.myorg.com/team/project1.git' or include 'git.myorg.com/team/project1.git/pkg'.
dep will work out of the box similarly how go get works, no need to introduce any new configurations.

sdboyer

Sorry it's taken me so long to respond - most of my dep time for the past few months has been occupied by rebutting vgo.

The main issue that i'm concerned about with this is how it would interact with the logic in the sourceCoordinator. The on-disk path they're cloned into is derived from the URL, meaning that both the suffixed and unsuffixed paths would end up using the same clone on disk.

On balance, that's a good thing, but we'd have to make sure that there's only one object in memory for that repository, independent of which path we take. i believe we have checking in place that should result in them folding together, but we'll need a test that makes sure of it. You could probably extend TestSourceCreationCounts to this end.

sdboyer · 2018-07-02T03:15:00Z

gps/deduce.go

@@ -73,7 +73,7 @@ var (
 	//gcRegex      = regexp.MustCompile(`^(?P<root>code\.google\.com/[pr]/(?P<project>[a-z0-9\-]+)(\.(?P<subrepo>[a-z0-9\-]+))?)(/[A-Za-z0-9_.\-]+)*$`)
 	jazzRegex         = regexp.MustCompile(`^(?P<root>hub\.jazz\.net(/git/[a-z0-9]+/[A-Za-z0-9_.\-]+))((?:/[A-Za-z0-9_.\-]+)*)$`)
 	apacheRegex       = regexp.MustCompile(`^(?P<root>git\.apache\.org(/[a-z0-9_.\-]+\.git))((?:/[A-Za-z0-9_.\-]+)*)$`)
-	vcsExtensionRegex = regexp.MustCompile(`^(?P<root>([a-z0-9.\-]+\.)+[a-z0-9.\-]+(:[0-9]+)?/[A-Za-z0-9_.\-/~]*?\.(?P<vcs>bzr|git|hg|svn))((?:/[A-Za-z0-9_.\-]+)*)$`)
+	vcsExtensionRegex = regexp.MustCompile(`^(?P<root>(?P<repo>([a-z0-9.\-]+\.)+[a-z0-9.\-]+(:[0-9]+)?(/~?[A-Za-z0-9_.\-]+)+?)\.(?P<vcs>bzr|fossil|git|hg|svn))(/~?[A-Za-z0-9_.\-]+)*$`)


We don't actually support fossil - not sure there's much point in adding it.

outcoldman · 2018-07-02T04:33:44Z

Considering the current state of dep and vgo, I assume we can just close (abandon) this PR

Please just make sure that this will be well covered in vgo ;)

Thank you!

sdboyer-stripe · 2018-07-02T17:23:42Z

Considering the current state of dep and vgo, I assume we can just close (abandon) this PR

Please just make sure that this will be well covered in vgo ;)

Sorry, but i'm not contributing my time voluntarily to vgo, as i believe it is fundamentally the wrong direction. i'll keep this in mind with the successor tool to dep.

sdboyer · 2018-07-11T05:21:48Z

(note that this should not be an issue in vgo, as it uses none of our code. i'm also pretty sure that the concern i raised above is something we handle automatically, so incorporating this should be pretty easy.)

outcoldman added 2 commits February 25, 2018 14:14

Update changelog

4f2cc1b

outcoldman requested a review from sdboyer as a code owner February 25, 2018 22:43

googlebot added the cla: no label Feb 25, 2018

googlebot added cla: yes and removed cla: no labels Feb 25, 2018

This was referenced Feb 25, 2018

"source" declaration not followed? #860

Closed

Support for private/enterprise patterns #286

Closed

Support for private repositories e.g. Github Enterprise #174

Closed

outcoldman added 3 commits February 25, 2018 16:05

Fix tests. Revert back order

4f1f5eb

Set order similar to go get

ca4c99c

Revert "Set order similar to go get"

c2213dc

This reverts commit ca4c99c.

sdboyer suggested changes Jul 2, 2018

View reviewed changes

sdboyer closed this Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement vcs similar to go get #1717

Implement vcs similar to go get #1717

outcoldman commented Feb 25, 2018 •

edited

Loading

googlebot commented Feb 25, 2018

googlebot commented Feb 25, 2018

outcoldman commented Feb 25, 2018

googlebot commented Feb 25, 2018

googlebot commented Feb 25, 2018

outcoldman commented Mar 4, 2018

hoshsadiq commented Mar 4, 2018

outcoldman commented Mar 4, 2018

hoshsadiq commented Mar 4, 2018 •

edited

Loading

outcoldman commented Mar 4, 2018

sdboyer left a comment

sdboyer Jul 2, 2018

outcoldman commented Jul 2, 2018

sdboyer-stripe commented Jul 2, 2018 •

edited

Loading

sdboyer commented Jul 11, 2018

Implement vcs similar to go get #1717

Implement vcs similar to go get #1717

Conversation

outcoldman commented Feb 25, 2018 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

googlebot commented Feb 25, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Feb 25, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

outcoldman commented Feb 25, 2018

googlebot commented Feb 25, 2018

googlebot commented Feb 25, 2018

outcoldman commented Mar 4, 2018

hoshsadiq commented Mar 4, 2018

outcoldman commented Mar 4, 2018

hoshsadiq commented Mar 4, 2018 • edited Loading

outcoldman commented Mar 4, 2018

sdboyer left a comment

Choose a reason for hiding this comment

sdboyer Jul 2, 2018

Choose a reason for hiding this comment

outcoldman commented Jul 2, 2018

sdboyer-stripe commented Jul 2, 2018 • edited Loading

sdboyer commented Jul 11, 2018

outcoldman commented Feb 25, 2018 •

edited

Loading

hoshsadiq commented Mar 4, 2018 •

edited

Loading

sdboyer-stripe commented Jul 2, 2018 •

edited

Loading