code review 5787055: cmd/go: local import fixes

cmd/go: local import fixes

1) The -D argument should always be a pseudo-import path,
like _/Users/rsc/foo/bar, never a standard import path,
because we want local imports to always resolve to pseudo-paths.

2) Disallow local imports in non-local packages.  Otherwise
everything works but you get two copies of a package
(the real one and the "local" one) in your binary.

[rsc, 2012-03-08 16:58:00]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-03-09 07:12:46]

LGTM
 On Mar 8, 2012 8:58 AM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/go: local import fixes
>
> 1) The -D argument should always be a pseudo-import path,
> like _/Users/rsc/foo/bar, never a standard import path,
> because we want local imports to always resolve to pseudo-paths.
>
> 2) Disallow local imports in non-local packages.  Otherwise
> everything works but you get two copies of a package
> (the real one and the "local" one) in your binary.
>
> Please review this at
http://codereview.appspot.com/**5787055/<http://codereview.appspot.com/5787055/>
>
> Affected files:
>  M src/cmd/go/build.go
>  M src/cmd/go/pkg.go
>
>
> Index: src/cmd/go/build.go
> ==============================**==============================**=======
> --- a/src/cmd/go/build.go
> +++ b/src/cmd/go/build.go
> @@ -383,6 +383,7 @@
>
>        bp, err := ctxt.ImportDir(dir, 0)
>        pkg := new(Package)
> +       pkg.local = true
>        pkg.load(&stk, bp, err)
>        pkg.localPrefix = dirToImportPath(dir)
>        pkg.ImportPath = "command-line-arguments"
> @@ -1202,7 +1203,7 @@
>
>  func (gcToolchain) ld(b *builder, p *Package, out string, allactions
> []*action, mainpkg string, ofiles []string) error {
>        importArgs := b.includeArgs("-L", allactions)
> -       return b.run(p.Dir, p.ImportPath, tool(archChar+"l"), "-o", out,
> importArgs, buildLdflags, mainpkg)
> +       return b.run(".", p.ImportPath, tool(archChar+"l"), "-o", out,
> importArgs, buildLdflags, mainpkg)
>  }
>
>  func (gcToolchain) cc(b *builder, p *Package, objdir, ofile, cfile
> string) error {
> @@ -1284,7 +1285,7 @@
>                ldflags = append(ldflags, afile)
>        }
>        ldflags = append(ldflags, cgoldflags...)
> -       return b.run(p.Dir, p.ImportPath, "gccgo", "-o", out,
> buildGccgoflags, ofiles, "-Wl,-(", ldflags, "-Wl,-)")
> +       return b.run(".", p.ImportPath, "gccgo", "-o", out,
> buildGccgoflags, ofiles, "-Wl,-(", ldflags, "-Wl,-)")
>  }
>
>  func (gccgcToolchain) cc(b *builder, p *Package, objdir, ofile, cfile
> string) error {
> Index: src/cmd/go/pkg.go
> ==============================**==============================**=======
> --- a/src/cmd/go/pkg.go
> +++ b/src/cmd/go/pkg.go
> @@ -279,9 +279,8 @@
>        p.copyBuild(bp)
>
>        // The localPrefix is the path we interpret ./ imports relative to.
> -       // Now that we've fixed the import path, it's just the import path.
>        // Synthesized main packages sometimes override this.
> -       p.localPrefix = p.ImportPath
> +       p.localPrefix = dirToImportPath(p.Dir)
>
>        if err != nil {
>                p.Incomplete = true
> @@ -343,6 +342,16 @@
>                }
>                p1 := loadImport(path, p.Dir, stk, p.build.ImportPos[path])
>                if p1.local {
> +                       if !p.local && p.Error == nil {
> +                               p.Error = &PackageError{
> +                                       ImportStack: stk.copy(),
> +                                       Err: fmt.Sprintf("local import %q
> in non-local package", path),
> +                               }
> +                               pos := p.build.ImportPos[path]
> +                               if len(pos) > 0 {
> +                                       p.Error.Pos = pos[0].String()
> +                               }
> +                       }
>                        path = p1.ImportPath
>                        importPaths[i] = path
>                }
>
>
>

[yiyus, 2012-03-09 08:29:53]

On Thursday, 8 March 2012 17:58:00 UTC+1, rsc wrote:
>
>
> 2) Disallow local imports in non-local packages.  Otherwise
> everything works but you get two copies of a package
> (the real one and the "local" one) in your binary.
>

Wouldn't it be possible to only install it as "the real one"?

As I said in go-nuts 
[https://groups.google.com/d/topic/golang-nuts/7t_KN1LdOSA/discussion], I 
think there are some useful uses of relative import paths (I can explain 
them if you want, maybe there are better ways to do what I have in mind).

If there is no clear decision on how local imports should work, may it be 
left undefined in Go1 so it can be figured out later?

[rsc, 2012-03-09 17:45:06]

On Fri, Mar 9, 2012 at 03:29, yiyus <yiyu.jgl@gmail.com> wrote:
> Wouldn't it be possible to only install it as "the real one"?
>
> As I said in go-nuts
> [https://groups.google.com/d/topic/golang-nuts/7t_KN1LdOSA/discussion], I
> think there are some useful uses of relative import paths (I can explain
> them if you want, maybe there are better ways to do what I have in mind).
>
> If there is no clear decision on how local imports should work, may it be
> left undefined in Go1 so it can be figured out later?

I saw that discussion.  I wrote this CL so that people won't start doing that.
Source files are significantly less portable if you are using relative imports,
because now you can't resolve anything without knowing where the file
came from.

Russ

[yiyus, 2012-03-09 18:06:58]

On Friday, 9 March 2012 18:45:04 UTC+1, rsc wrote:
>
> I saw that discussion.  I wrote this CL so that people won't start doing 
> that.
> Source files are significantly less portable if you are using relative 
> imports,
>

Fair enough.

All my concerns with the go tool would have been solved using links. Since 
that won't happen, I thought local paths may be a good way to have 
"relocatable source", but it looks like that is not possible and the go 
tool is not as flexible with source location as I would like.

I guess that is a feature I will have to learn to appreciate instead of 
trying to fight against it. You have been right in similar arguments 
before, so I will try.

[rsc, 2012-03-11 19:53:47]

*** Submitted as fc524d42fb8c ***

cmd/go: local import fixes

1) The -D argument should always be a pseudo-import path,
like _/Users/rsc/foo/bar, never a standard import path,
because we want local imports to always resolve to pseudo-paths.

2) Disallow local imports in non-local packages.  Otherwise
everything works but you get two copies of a package
(the real one and the "local" one) in your binary.

R=golang-dev, bradfitz, yiyu.jgl
CC=golang-dev
http://codereview.appspot.com/5787055

[rliebling_gmail.com, 2012-10-01 22:10:40]

I'm hoping I misunderstand this.  So, please correct me if so.  Let me give 
an example I think is important, and should be considered important in the 
design of Go.  

Suppose i have a hypothetical project constant_time_sorting which has 3 
packages, the main constant_time_sorting along with subpackages 
magic_algorithm and utilities organized like so:
constant_time_sorting/
    magic_algorithm/
    utilities/

And, i host this project on github at 
github.com/rliebling/constant_time_sorting

If i understand correctly, to import my subpackages (magic_algorithm and 
utilities) i will not be able to use a local import (since my project is 
hosted on github.com, obtained via go get 
github.com/rliebling/constant_time_sorting, it is not a local project). 
 So, i have to specify the full import path for magic_algorithm and 
utilities.  

Now, if someone else were to fork my project (say to fix a bug), they will 
have to change the import paths before being able to build their copy of 
this project (since it is also a remote project).  They fix the bug and 
kindly submit a pull request to me to incorporate into what will quickly 
become a popular Go project.  However, someone has to play some games with 
the pull request (either the submitter or the receiver) since the changes 
they had to make included changing import paths.

This seems like a serious impediment to a common github workflow.  And, 
it's not really unique to github either.  the point is that one cannot just 
fork a project and have it build without first making changes.  And, that 
seems like a bad call.  Admittedly, i don't understand what it would take 
to implement a solution that allows local (relative) imports within a 
remote project but it seems like a very important feature and I would 
encourage the team to make it happen.  I have not done much go programming, 
but it's already negatively impacted me several times.

best
rich


On Sunday, March 11, 2012 12:53:47 PM UTC-7, rsc wrote:
>
> *** Submitted as fc524d42fb8c ***
>
> cmd/go: local import fixes
>
> 1) The -D argument should always be a pseudo-import path,
> like _/Users/rsc/foo/bar, never a standard import path,
> because we want local imports to always resolve to pseudo-paths.
>
> 2) Disallow local imports in non-local packages.  Otherwise
> everything works but you get two copies of a package
> (the real one and the "local" one) in your binary.
>
> R=golang-dev, bradfitz, yiyu.jgl
> CC=golang-dev
> http://codereview.appspot.com/5787055
>
>
> http://codereview.appspot.com/5787055/
>
>

[aam, 2012-10-01 23:28:32]

> Now, if someone else were to fork my project (say to fix a bug), they will
> have to change the import paths before being able to build their copy of
> this project (since it is also a remote project).  They fix the bug and
> kindly submit a pull request to me to incorporate into what will quickly
> become a popular Go project.  However, someone has to play some games with
> the pull request (either the submitter or the receiver) since the changes
> they had to make included changing import paths.

there is a trick to this which most go programmers working with github
are aware of, say that i forked your github repo, my fork will be
called 'github.com/mirtchovski/constant_time_sorting. to work with it
without having to change any import paths i would:

$ cd $GOROOT/src/github.com/rliebling/
$ rm -rf constant_time_sorting
$ git clone github.com/mirtchovski/constant_time_sorting

that replaces your repository with my fork but doesn't change any import paths.

[aam, 2012-10-01 23:30:06]

> $ cd $GOROOT/src/github.com/rliebling/
> $ rm -rf constant_time_sorting
> $ git clone github.com/mirtchovski/constant_time_sorting

that's GOPATH, of course.

[dave_cheney.net, 2012-10-01 23:30:42]

> $ cd $GOROOT/src/github.com/rliebling/
> $ rm -rf constant_time_sorting
> $ git clone github.com/mirtchovski/constant_time_sorting
>
> that replaces your repository with my fork but doesn't change any import
paths.

Or even easier

cd $PKG
git checkout master   #don't blame me for this git facepalm
vim vim vim
git commit
git push $YOURFORK

[rliebling_gmail.com, 2012-10-02 16:26:54]

Thanks everyone for the suggestions of how to cope with this problem. 
 However, while these approaches help an individual cope, they are, in my 
view, not well suited to team development where the team cannot wait for 
the patch to be accepted.  

They will instead import their own fork of the 3rd party library until that 
time. Meanwhile, they will have to have one version to submit a patch, as 
described in the several responses to my original question, and another 
version to actually reference in their code.  Consider also the case where 
a hierarchy of repos is used for a project, with different gatekeepers at 
the different levels (eg used by linux, but presumably used in other 
projects, as well.)  In this case, again, one cannot simply accept standard 
patches from the lower repos because import statements will need to be 
changed to move the code from one repo to another.

Russ stated earlier:

>   Source files are significantly less portable if you are using relative 
> imports,
>   because now you can't resolve anything without knowing where the file 
> came from.

My contention is this addresses the wrong problem.  First, the standard 
unit of reuse should be the package, not the source file. Source files in 
Go are already hard to reuse just by copying to another package, as the 
file likely depends on other files in the same package without any explicit 
reference to those files.  But, more importantly, i think the github 
approach to code reuse is significantly more common and more important.  In 
this approach one uses some open source library (eg using rubygems/bundler 
in ruby), specifying some version constraint, btw. When the need arises to 
make your own changes/fixes/enhancements to that, you fork it, and use your 
own fork up until your changes have been accepted by the original 
maintainer (if ever).  This approach has led to flourishing development 
communities with constantly improving libraries.  Encouraging instead a 
system where developers merely appropriate individual source files for 
their own use runs counter to this approach.

But, maybe the point of all this is that we should just not use "go get" or 
"go install" because the package management aspects leads to these 
difficulties.  I see two paths:
  1.  never use 'remote imports': treat all packages as local so this 
requirement vanishes.  Someone writes a package management tool for Go that 
behaves this way and life gets better.
  2. Separate the notion of the source repo for the code from the directory 
and import naming scheme.  Esssentially this is how java works, where 
packages might be named com.facebook.api.something, but the source code for 
it might come from your local fork of the project or someone else's. 
 Again, someone needs to write a tool to make this easy to get code and 
build it.  (Please don't make everything xml, like java/maven/ant.)
The bottom line seems to be that "go get" and "go install" don't cut it.
  
The current situation creates annoying friction to collaboration with Go. 
 The result will be less collaboration, which will be a loss for the whole 
Go community.  I hope you will reconsider this issue.  The answer need not 
be allowing local imports.  But, something in my view needs to be done to 
eliminate the friction of forking a Go project involving multiple packages.

best,
rich


On Monday, October 1, 2012 4:30:44 PM UTC-7, Dave Cheney wrote:
>
> > $ cd $GOROOT/src/github.com/rliebling/ 
> > $ rm -rf constant_time_sorting 
> > $ git clone github.com/mirtchovski/constant_time_sorting 
> > 
> > that replaces your repository with my fork but doesn't change any import 
> paths. 
>
> Or even easier 
>
> cd $PKG 
> git checkout master   #don't blame me for this git facepalm 
> vim vim vim 
> git commit 
> git push $YOURFORK 
>