code review 4626064: goinstall: Add support for generic hosts using special ...

goinstall: Add support for generic hosts using special import form

This change extends goinstall to support "magic" package names of the
form:
        <host>/<repo>.<vcs>/<path>

Where <host> is the hostname, <repo> the path to the repository, <vcs>
the type of vcs (git, hg, bzr or svn), and <path> is the path inside the
repository that contains the source code for the package.

For example: "example.com/pub/foo.hg/src" means download the Mercurial
repository at either pub/foo.hg or pub/foo from example.com and then
build and install the source files from src inside the repository
checkout.

Repositories on the built-in hostings sites (github, bitbucket,
launchpad and googlecode) must still use the old form (i.e.
github.com/xxx/yyy.git/src will be rejected).

[quantumfyre, 2011-06-21 20:38:52]

Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[quantumfyre, 2011-06-27 14:11:22]

On Tue, 21 Jun 2011 20:38:52 +0000, julian@quantumfyre.co.uk wrote:
> Reviewers: adg, rsc,
>
> Message:
> Hello adg@golang.org, rsc@golang.org (cc: 
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> goinstall: Add support for generic hosts using special import form

Are there any comments on this?  Is it work pursuing?

The implementation submitted doesn't currently match the proposed 
documentation - but it doesn't seem worth trying to make it match if the 
whole thing is flawed or taking the wrong approach.

-- 
Julian

[rsc, 2011-06-27 14:37:33]

> Are there any comments on this?  Is it work pursuing?

It's worth pursuing.  I think Andrew will be sending you
some comments today or tomorrow.

Thanks for keeping at this - I remember you proposing
this many months ago.

Russ

[adg, 2011-06-27 23:59:41]

Thanks for this.

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
src/cmd/goinstall/download.go:154: func tryCommand(v *vcs, prefix string)
*vcsMatch {
Should some of this be parallelized? Perhaps we should try different protocols
simultaneously, and use the one that comes back fastest. I don't think we should
do the with/without suffix tests in parallel, though.

I think you can put all the parallelization in just this function.

match := make(chan *vcsMatch)
for _, proto := range v.protocols {
    go func(proto string) {
        for _, suffix := range []{v.suffix, ""} { // check w/ suffix first
            // run the command etc, if successful then
               match <- &vcsMatch{...
        }
    }(proto)
}

for _ := range v.protocols {
    if m := <-match; m != nil {
        return m
    }
}
return nil

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
src/cmd/goinstall/download.go:168: func findTools() {
We don't need to search for the tools because we know the ones we're looking
for. The exec will fail in tryCommand, and that's where we should log an error.

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
src/cmd/goinstall/download.go:187: for _, v := range vcsList {
This loop is a bit labored.

What about something like

parts := strings.Split(pkg, "/")
for i := range parts {
    if i == 0 {
        continue // skip domain
    }
    for _, v := range vcsList {
        if !strings.HasSuffix(parts[i], v.suffix) {
            continue
        }
        prefix := strings.Join(parts[:i+1], "/")[:len(v.suffix)]
        return tryCommand(v, prefix)
    }
}

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
src/cmd/goinstall/download.go:235: m = findRepo(pkg)
below this add
dashpath = ""

(We don't want to report privately operated repos)

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
src/cmd/goinstall/download.go:282: maybeReportToDashboard(dashpath)
if dashpath != "" {
    maybeReportToDashboard(dashpath)
}

[rsc, 2011-06-28 00:00:53]

> Should some of this be parallelized? Perhaps we should try different
> protocols simultaneously, and use the one that comes back fastest.

If there is enough work for parallelization
then we are doing something wrong.  Let's do
one at a time, so that the choice is predictable.

Russ

[adg, 2011-06-28 00:47:44]

On 28 June 2011 10:00, Russ Cox <rsc@golang.org> wrote:
>> Should some of this be parallelized? Perhaps we should try different
>> protocols simultaneously, and use the one that comes back fastest.
>
> If there is enough work for parallelization
> then we are doing something wrong.  Let's do
> one at a time, so that the choice is predictable.

Okay, so let's skip this part of it for now and just do it in serial.

Andrew

[quantumfyre, 2011-06-28 11:47:29]

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
src/cmd/goinstall/download.go:168: func findTools() {
On 2011/06/27 23:59:41, adg wrote:
> We don't need to search for the tools because we know the ones we're looking
> for. The exec will fail in tryCommand, and that's where we should log an
error.

except that we then need to determine if the exec failed because the tool is
missing/broken as compared to the tool being ok, but the repo we asked it to
look at not existing - so that we know whether we need to log an error.

[rsc, 2011-06-28 12:07:11]

On 2011/06/28 11:47:29, quantumfyre wrote:
> http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go
> File src/cmd/goinstall/download.go (right):
> 
>
http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
> src/cmd/goinstall/download.go:168: func findTools() {
> On 2011/06/27 23:59:41, adg wrote:
> > We don't need to search for the tools because we know the ones we're looking
> > for. The exec will fail in tryCommand, and that's where we should log an
> error.
> 
> except that we then need to determine if the exec failed because the tool is
> missing/broken as compared to the tool being ok, but the repo we asked it to
> look at not existing - so that we know whether we need to log an error.

There's no difference.  If you were asked to use hg and hg doesn't exist,
you should log that error.

(It's not like before where we were sniffing what tools were around to try.)

[quantumfyre, 2011-06-28 12:54:54]

On 2011/06/28 12:07:11, rsc wrote:
> On 2011/06/28 11:47:29, quantumfyre wrote:
> >
http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go
> > File src/cmd/goinstall/download.go (right):
> > 
> >
>
http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
> > src/cmd/goinstall/download.go:168: func findTools() {
> > On 2011/06/27 23:59:41, adg wrote:
> > > We don't need to search for the tools because we know the ones we're
looking
> > > for. The exec will fail in tryCommand, and that's where we should log an
> > error.
> > 
> > except that we then need to determine if the exec failed because the tool is
> > missing/broken as compared to the tool being ok, but the repo we asked it to
> > look at not existing - so that we know whether we need to log an error.
> 
> There's no difference.  If you were asked to use hg and hg doesn't exist,
> you should log that error.
> 
> (It's not like before where we were sniffing what tools were around to try.)

Yes, for hg that's true.  However, with git we try git:// and http:// and so it
is basically the same as before - except that we know that we want to access a
git repo, so we only care about the functionality of the git tool and don't need
to worry if hg is missing.

[quantumfyre, 2011-06-28 12:59:08]

On 2011/06/28 12:54:54, quantumfyre wrote:
> On 2011/06/28 12:07:11, rsc wrote:
> > On 2011/06/28 11:47:29, quantumfyre wrote:
> > >
> http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go
> > > File src/cmd/goinstall/download.go (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/4626064/diff/4001/src/cmd/goinstall/download.go...
> > > src/cmd/goinstall/download.go:168: func findTools() {
> > > On 2011/06/27 23:59:41, adg wrote:
> > > > We don't need to search for the tools because we know the ones we're
> looking
> > > > for. The exec will fail in tryCommand, and that's where we should log an
> > > error.
> > > 
> > > except that we then need to determine if the exec failed because the tool
is
> > > missing/broken as compared to the tool being ok, but the repo we asked it
to
> > > look at not existing - so that we know whether we need to log an error.
> > 
> > There's no difference.  If you were asked to use hg and hg doesn't exist,
> > you should log that error.
> > 
> > (It's not like before where we were sniffing what tools were around to try.)
> 
> Yes, for hg that's true.  However, with git we try git:// and http:// and so
it
> is basically the same as before - except that we know that we want to access a
> git repo, so we only care about the functionality of the git tool and don't
need
> to worry if hg is missing.

Actually it's not true for hg either - as we try with and without the suffix ...

[rsc, 2011-06-28 13:07:29]

Even so, I don't see how the error handling is different.

err := try(first url)
if err != nil {
    err = try(second url)
}
if err != nil {
    log.Print(err)
}

No?

[quantumfyre, 2011-06-29 21:50:25]

Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[quantumfyre, 2011-06-29 21:51:41]

Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-06-30 01:04:31]

http://codereview.appspot.com/4626064/diff/15001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4626064/diff/15001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:150: func tryCommand(v *vcs, prefix string)
*vcsMatch {
s/tryCommand/findRepo/

http://codereview.appspot.com/4626064/diff/15001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:158: if err == nil {
if err != nil {
  printf("find %s: %s %s %s: %v\n%s\n", prefix, v.cmd, v.check, repo, err, out)
} else {

http://codereview.appspot.com/4626064/diff/15001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:164: errorf("%s returned an error (%v):\n%s\n",
v.name, err, out)
errorf("find %s: couldn't find %s repository", prefix, v.name)

[quantumfyre, 2011-06-30 07:39:51]

Hello adg@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-06-30 08:54:31]

LGTM

[adg, 2011-06-30 08:55:06]

*** Submitted as http://code.google.com/p/go/source/detail?r=c78e6a668db0 ***

goinstall: Add support for generic hosts using special import form

This change extends goinstall to support "magic" package names of the
form:
        <host>/<repo>.<vcs>/<path>

Where <host> is the hostname, <repo> the path to the repository, <vcs>
the type of vcs (git, hg, bzr or svn), and <path> is the path inside the
repository that contains the source code for the package.

For example: "example.com/pub/foo.hg/src" means download the Mercurial
repository at either pub/foo.hg or pub/foo from example.com and then
build and install the source files from src inside the repository
checkout.

Repositories on the built-in hostings sites (github, bitbucket,
launchpad and googlecode) must still use the old form (i.e.
github.com/xxx/yyy.git/src will be rejected).

R=adg, rsc
CC=golang-dev
http://codereview.appspot.com/4626064

Committer: Andrew Gerrand <adg@golang.org>

[gustavo_niemeyer.net, 2011-06-30 10:07:28]

> If there is enough work for parallelization
> then we are doing something wrong.  Let's do
> one at a time, so that the choice is predictable.

The problem is that this penalizes significantly people using the
revision control systems at the end of the list, even more in
countries where latency to the repositories is higher (e.g. Brazil).
I'd really appreciate the approach with paralelization myself.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter

[adg, 2011-06-30 10:23:22]

On 30 June 2011 20:07, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
>> If there is enough work for parallelization
>> then we are doing something wrong.  Let's do
>> one at a time, so that the choice is predictable.
>
> The problem is that this penalizes significantly people using the
> revision control systems at the end of the list, even more in
> countries where latency to the repositories is higher (e.g. Brazil).
> I'd really appreciate the approach with paralelization myself.

What if there were a host cache that recorded which protocol works
with which host? That way you'd only need to probe on the first try.

Andrew

[gustavo_niemeyer.net, 2011-06-30 10:35:35]

> What if there were a host cache that recorded which protocol works
> with which host? That way you'd only need to probe on the first try.

You have to probe for every URL, because the same host may contain
different repository kinds.  It also feels like a workaround for
something that is easy to do in a better way.

If Russ is after predictability, we can wait for the results from all
commands, and sort on the accepted ones, for instance.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter

[quantumfyre, 2011-06-30 10:49:59]

On Thu, 30 Jun 2011 11:35:13 +0100, Gustavo Niemeyer wrote:
>> What if there were a host cache that recorded which protocol works
>> with which host? That way you'd only need to probe on the first try.
>
> You have to probe for every URL, because the same host may contain
> different repository kinds.  It also feels like a workaround for
> something that is easy to do in a better way.
>
> If Russ is after predictability, we can wait for the results from all
> commands, and sort on the accepted ones, for instance.

What's the scale of the problem?  Are we talking 30 seconds vs 2 
minutes?  Or 10 minutes vs 40?

-- 
Julian

[gustavo_niemeyer.net, 2011-06-30 10:56:38]

> What's the scale of the problem?  Are we talking 30 seconds vs 2 minutes?
>  Or 10 minutes vs 40?

The scale is X vs. X*N.  I can't do the real test myself right now
because I'm not in Brazil at the moment, but this should give you an
idea:

$ time bzr info https://www.terra.com.br/
bzr: ERROR: Connection error: curl connection error (couldn't connect to host)
on https://www.terra.com.br/.bzr/smart

real    0m21.619s

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter

[quantumfyre, 2011-06-30 11:09:30]

On Thu, 30 Jun 2011 11:56:16 +0100, Gustavo Niemeyer wrote:
>> What's the scale of the problem?  Are we talking 30 seconds vs 2 
>> minutes?
>>  Or 10 minutes vs 40?
>
> The scale is X vs. X*N.

Yes, where N is currently 4 (or 2 for Mercurial).

> I can't do the real test myself right now
> because I'm not in Brazil at the moment, but this should give you an
> idea:
>
> $ time bzr info https://www.terra.com.br/
> bzr: ERROR: Connection error: curl connection error (couldn't connect
> to host)
> on https://www.terra.com.br/.bzr/smart
>
> real    0m21.619s

Ok, so not a ridiculous delay?
Not saying that we shouldn't make the change, I was just curious about 
the scale.

-- 
Julian

[rsc, 2011-06-30 11:50:13]

On Thu, Jun 30, 2011 at 06:07, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
>> If there is enough work for parallelization
>> then we are doing something wrong.  Let's do
>> one at a time, so that the choice is predictable.
>
> The problem is that this penalizes significantly people using the
> revision control systems at the end of the list, even more in
> countries where latency to the repositories is higher (e.g. Brazil).
> I'd really appreciate the approach with paralelization myself.

I haven't been watching the code so maybe I missed something.

Please give an example where an import line triggers trying
*multiple* revision control systems.  Those were not supposed
to happen anymore.  That was the point of the new approach.

Russ

[gustavo_niemeyer.net, 2011-06-30 15:41:50]

Sorry, I'm probably the one missing something then. I'm in a sprint and
following as I can, and assumed things given a previous review and your
comment.

Going back home, so will check this more carefully soon.
On Jun 30, 2011 12:50 PM, "Russ Cox" <rsc@golang.org> wrote:
> On Thu, Jun 30, 2011 at 06:07, Gustavo Niemeyer <gustavo@niemeyer.net>
wrote:
>>> If there is enough work for parallelization
>>> then we are doing something wrong.  Let's do
>>> one at a time, so that the choice is predictable.
>>
>> The problem is that this penalizes significantly people using the
>> revision control systems at the end of the list, even more in
>> countries where latency to the repositories is higher (e.g. Brazil).
>> I'd really appreciate the approach with paralelization myself.
>
> I haven't been watching the code so maybe I missed something.
>
> Please give an example where an import line triggers trying
> *multiple* revision control systems. Those were not supposed
> to happen anymore. That was the point of the new approach.
>
> Russ

[niemeyer, 2011-06-30 19:33:58]

> Please give an example where an import line triggers trying
> *multiple* revision control systems.  Those were not supposed
> to happen anymore.  That was the point of the new approach.

Ok, just had a chance to review the code now.  Indeed my
comment was a bit off, and I apologize for that.  I still
see a similar issue, though.  I'm not aware of any
svn/hg/bzr repositories which use the respective dot-suffix.

My suggestion, though, is for us to turn the suffix field
into a list, so that we can differentiate the order per vcs.
This would avoid these lags in the vast majority of cases
by using the common convention upfront.

[rsc, 2011-06-30 19:53:38]

> Ok, just had a chance to review the code now.  Indeed my
> comment was a bit off, and I apologize for that.  I still
> see a similar issue, though.  I'm not aware of any
> svn/hg/bzr repositories which use the respective dot-suffix.

I think you missed the part where foo.vcs/bar means
to checkout foo.vcs, *or if that fails*, foo, and then
use the directory bar in the checkout.

Russ

[gustavo_niemeyer.net, 2011-06-30 20:13:23]

> I think you missed the part where foo.vcs/bar means
> to checkout foo.vcs, *or if that fails*, foo, and then
> use the directory bar in the checkout.

Got that.  I was actually suggesting that we use a field like

    suffixes: []string{"", ".bzr"}
    ...
    suffixes: []string{".git", ""}

So that we generally get the right one first.  Pretty much every
single check for "foo.bzr/bar" and "foo.svn/bar" will be wrong, for
instance.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter

[rsc, 2011-06-30 20:19:02]

> Got that.  I was actually suggesting that we use a field like
>
>    suffixes: []string{"", ".bzr"}
>    ...
>    suffixes: []string{".git", ""}
>
> So that we generally get the right one first.  Pretty much every
> single check for "foo.bzr/bar" and "foo.svn/bar" will be wrong, for
> instance.

They should all be consistent, but I'd be happy to
make them all consistently "" and then ".vcs".

[gustavo_niemeyer.net, 2011-06-30 20:24:33]

> They should all be consistent, but I'd be happy to
> make them all consistently "" and then ".vcs".

Ok.. I can see that consistency is important for documenting the
behavior and establishing user expectations, even if I'm a bit sad
that we know we'll be banging on the wrong door all the time.

While I'd like to encourage inverting the current order to make bzr
users happy, it's obvious that git has a significant edge in terms of
published repositories, so if it is to be consistent, the current
order is probably right for the majority of users.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter

[rsc, 2011-06-30 20:27:08]

> Ok.. I can see that consistency is important for documenting the
> behavior and establishing user expectations, even if I'm a bit sad
> that we know we'll be banging on the wrong door all the time.
>
> While I'd like to encourage inverting the current order to make bzr
> users happy, it's obvious that git has a significant edge in terms of
> published repositories, so if it is to be consistent, the current
> order is probably right for the majority of users.

remember that github is already a special case.
where are the others?

[quantumfyre, 2011-06-30 21:00:37]

On Thu, 30 Jun 2011 21:24:11 +0100, Gustavo Niemeyer wrote:
>> They should all be consistent, but I'd be happy to
>> make them all consistently "" and then ".vcs".
>
> Ok.. I can see that consistency is important for documenting the
> behavior and establishing user expectations, even if I'm a bit sad
> that we know we'll be banging on the wrong door all the time.
>
> While I'd like to encourage inverting the current order to make bzr
> users happy, it's obvious that git has a significant edge in terms of
> published repositories, so if it is to be consistent, the current
> order is probably right for the majority of users.

When using the git:// protocol, a repository example.com/foo.git can be 
accessed as example.com/foo, so it's only git over http:// that would 
have to make an extra hit ...

-- 
Julian

[gustavo_niemeyer.net, 2011-07-03 21:37:24]

> When using the git:// protocol, a repository example.com/foo.git can be
> accessed as example.com/foo, so it's only git over http:// that would have
> to make an extra hit ...

Ok, it'd definitely be good to invert the order then, and it looks
like Andrew has already done so in a following revision.  Thanks all.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter