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

cmd/go: go list should return more information about relative import paths #14348

Open
perillo opened this Issue Feb 16, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@perillo

perillo commented Feb 16, 2016

Suppose you want to implement a tool like golint using go list.
When reporting a problem, golint prints the full path to a source file, but when the user specified a relative import path, golint prints a relative path.

Unfortunately the Package struct returned from go list does not allow this.
The only way to obtain a full path to a .go source file is to use the Dir field, but this is always an absolute path.

The Package struct used by go list should have the following additional fields:

Cmdline  bool   `json:",omitempty"` // defined by files listed on command line
Local    bool   `json:",omitempty"` // imported via relative local path (./ or ../)
SrcDir   string `json:",omitempty"` // a local relative path is interpreted relative to SrcDir

@ianlancetaylor ianlancetaylor changed the title from go list should return more informations to cmd/go: go list should return more information about relative import paths Feb 16, 2016

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Feb 16, 2016

@rsc

This comment has been minimized.

Contributor

rsc commented Feb 16, 2016

I don't think I understand what is being requested. As I understand it, all the information being requested is already easily derived from what is reported already:

  • p.Cmdline = p.ImportPath == "command-line-arguments"
  • p.Local = strings.HasPrefix(p.ImportPath, "_/")
  • p.SrcDir = p.Dir
@perillo

This comment has been minimized.

perillo commented Feb 16, 2016

  • p.ImportPath == "command-line-arguments" is not documented, and it is not part of the API.
  • go list -f '{{.ImportPath}}' . prints, as an example, github.com/perillo/goprint, not _/github.com/perillo/goprint
  • p.SrcDir was a mistake. What I want is the raw import path as specified on the command line. As an example go list -f '{{.Path}}' ../... will print ../pkg1 and so on.

The raw import path is necessary, IMHO, to correctly report relative paths to source files, as it is done by the golint command.

@rsc

This comment has been minimized.

Contributor

rsc commented Feb 16, 2016

Sorry, bear with me, but I still don't understand. It sounds like you want to be able to distinguish the output of the two go list commands in:

cd $GOROOT/src/io
go list -json ./ioutil
go list -json io/ioutil

and also that you want

cd $GOROOT/src/io
go list ./...

to report about a package "./ioutil" instead of "io/ioutil". I don't really see why that's useful. The standard name of the package is io/ioutil, not ./ioutil. The fact that you can refer to it as "./ioutil" or "./..." from within the $GOROOT/src/io directory is a convenience only. It shouldn't propagate into the rest of the toolchain.

I don't know what golint does for reporting relative paths to source files, but I can tell you what the go command itself does, and I would suggest golint do the same.

The go command uses this code, which works very well in practice. At least I can't remember a single complaint about the decisions it makes:

// shortPath returns an absolute or relative name for path, whatever is shorter.
func shortPath(path string) string {
    if rel, err := filepath.Rel(cwd, path); err == nil && len(rel) < len(path) {
        return rel
    }
    return path
}

// relPaths returns a copy of paths with absolute paths
// made relative to the current directory if they would be shorter.
func relPaths(paths []string) []string {
    var out []string
    pwd, _ := os.Getwd()
    for _, p := range paths {
        rel, err := filepath.Rel(pwd, p)
        if err == nil && len(rel) < len(p) {
            p = rel
        }
        out = append(out, p)
    }
    return out
}
@perillo

This comment has been minimized.

perillo commented Feb 16, 2016

On Tue, Feb 16, 2016 at 6:31 PM, Russ Cox notifications@github.com wrote:

Sorry, bear with me, but I still don't understand. It sounds like you want
to be able to distinguish the output of the two go list commands in:

cd $GOROOT/src/io
go list -json ./ioutil
go list -json io/ioutil

and also that you want

cd $GOROOT/src/io
go list ./...

to report about a package "./ioutil" instead of "io/ioutil". I don't
really see why that's useful. The standard name of the package is
io/ioutil, not ./ioutil. The fact that you can refer to it as "./ioutil" or
"./..." from within the $GOROOT/src/io directory is a convenience only. It
shouldn't propagate into the rest of the toolchain.

No, I don't want to change how standard packages are handled.

I don't know what golint does for reporting relative paths to source files,

Suppose that, inside a directory "foo", you do golint ../foo. A file path
will be printed as "../foo/bar.go"

That is, all the paths are prefixed with "..", since this is how I
specified the import paths on the command line.

but I can tell you what the go command itself does, and I would suggest

golint do the same.

One of the cause of all this confusion is IMHO that some tools like golint
do not use go list internally.
I have written some tools that required to enumerate installed packages,
and it was a pain to understand how to get the correct implementation.

The go command uses this code, which works very well in practice. At least
I can't remember a single complaint about the decisions it makes:

// shortPath returns an absolute or relative name for path, whatever is shorter.
func shortPath(path string) string {
if rel, err := filepath.Rel(cwd, path); err == nil && len(rel) < len(path) {
return rel
}
return path
}

Using shortPath in the previous example, golint would print "bar.go", and
not "../foo/bar.go"

I prefer the golint solution, since it seems more coherent.

Thanks!

@rsc

This comment has been minimized.

Contributor

rsc commented Feb 16, 2016

On golang-dev you asked:

It is possible that I'm misunderstanding what "local" means, for the go tool.

The default for imports in Go is to be "absolute" or "fully-qualified" in the sense that import "X" has a fixed meaning regardless of the directory in which it appears (ignoring vendoring). Within GOROOT and GOPATH, this is the only permitted import form.

Outside GOROOT and GOPATH, mainly to allow simple experiments and throwaway programs, the go command permits an import to use a relative import path like "./X", which obviously changes meaning based on the directory in which it appears. But directory a/b/c might import "./d" and directory a/b might import "./c/d" and directory a/b/c/x might import "../d" and those all refer to the same directory, so internally the go command must construct one canonical import path for that directory. The path it constructs is _/, which you see, for example, in the output of go list. The go command refers to packages found outside GOROOT/GOPATH as "local", in the sense that they are not part of the GOROOT/GOPATH space.

Code in GOROOT/GOPATH needs to be self-contained, so it cannot import local packages, although of course imports in the other direction are fine.

All that concerns import paths found in import statements in source code.

The go command line processes import paths as well, and there it is convenient to allow mentioning a directory (for example, in GOROOT/src/io, saying . or ./ioutil) as a shorthand for the standard, absolute import path of that directory (for that example, io or io/ioutil). The shorthand is limited to command line argument processing. The code in GOROOT/src/io cannot import "./ioutil".

@rsc

This comment has been minimized.

Contributor

rsc commented Feb 16, 2016

OK, I guess it comes down to this:

Suppose that, inside a directory "foo", you do golint ../foo. A file path will be printed as "../foo/bar.go"

That is, all the paths are prefixed with "..", since this is how I specified the import paths on the command line.

...

Using shortPath in the previous example, golint would print "bar.go", and not "../foo/bar.go"

That's right, that's how the go command works today:

$ cd $GOROOT/src/io/ioutil
$ echo broken >bad.go
$ go build ../ioutil
can't load package: package io/ioutil: 
bad.go:1:1: expected 'package', found 'IDENT' broken
$ 

The error message refers to bad.go, not ../ioutil/bad.go.

Now I understand what you want from the go command, but I don't see a good way to get it to you. The fact is, the model you're asking for is contrary to how the go command thinks about import paths, about packages, and about files. It could go out of its way to do something it doesn't need, for clients that want to be different, but the tool ecosystem would be more consistent if clients followed the go command's lead.

I don't think it's worth adding complexity to the go command just to allow golint to (continue to?) print file names in an idiosyncratic way.

@perillo

This comment has been minimized.

perillo commented Feb 16, 2016

On Tue, Feb 16, 2016 at 8:20 PM, Russ Cox notifications@github.com wrote:

On golang-dev you asked:

It is possible that I'm misunderstanding what "local" means, for the go
tool.

The default for imports in Go is to be "absolute" or "fully-qualified" in
the sense that import "X" has a fixed meaning regardless of the directory
in which it appears (ignoring vendoring). Within GOROOT and GOPATH, this is
the only permitted import form.

Outside GOROOT and GOPATH, mainly to allow simple experiments and
throwaway programs, the go command permits an import to use a relative
import path like "./X", which obviously changes meaning based on the
directory in which it appears. But directory a/b/c might import "./d" and
directory a/b might import "./c/d" and directory a/b/c/x might import
"../d" and those all refer to the same directory, so internally the go
command must construct one canonical import path for that directory. The
path it constructs is _/, which you see, for example, in the output of go
list. The go command refers to packages found outside GOROOT/GOPATH as
"local", in the sense that they are not part of the GOROOT/GOPATH space.

Thanks. This makes things more clear.
My confusion was probably caused by the fact that I was reading the source
code from the point of view of go command line processing.

By the way: is this documented somewhere? I don't remember reading a
document where this was well explained.
All I can remember is that I always found go tool hard to understand,
probably because it has evolved a lot.

@perillo

This comment has been minimized.

perillo commented Feb 16, 2016

On Tue, Feb 16, 2016 at 8:27 PM, Russ Cox notifications@github.com wrote:

OK, I guess it comes down to this:

Suppose that, inside a directory "foo", you do golint ../foo. A file path
will be printed as "../foo/bar.go"

That is, all the paths are prefixed with "..", since this is how I
specified the import paths on the command line.

...

Using shortPath in the previous example, golint would print "bar.go", and
not "../foo/bar.go"

That's right, that's how the go command works today:

$ cd $GOROOT/src/io/ioutil
$ echo broken >bad.go
$ go build ../ioutil
can't load package: package io/ioutil:
bad.go:1:1: expected 'package', found 'IDENT' broken
$

The error message refers to bad.go, not ../ioutil/bad.go.

Now I understand what you want from the go command, but I don't see a good
way to get it to you.

In my working patch, I added a new Path (or RawPath or RawImportPath)
fields to the Package struct. Path contains the original "import path"
as specified by the user.
The only issue is that, using your example:

$ cd $GOROOT/src/io/ioutil
$ go list ../ioutil

should print ioutil, not ../ioutil

The fact is, the model you're asking for is contrary to how the go command
thinks about import paths, about packages, and about files. It could go out
of its way to do something it doesn't need, for clients that want to be
different, but the tool ecosystem would be more consistent if clients
followed the go command's lead.

I don't think it's worth adding complexity to the go command just to allow
golint to (continue to?) print file names in an idiosyncratic way.

I agree.
I still prefer the golint idiosyncratic way of printing file names, but it
is more important to have a standard.

I found this issue only because I was trying to do the "right" thing in my
tool, and golint seemed the standard when reporting full source file paths.
By the way, golint not only reports file paths differently from go command,
but it also interprets command line arguments differently.

@rsc

This comment has been minimized.

Contributor

rsc commented Feb 16, 2016

The only issue is that, using your example:

$ cd $GOROOT/src/io/ioutil
$ go list ../ioutil

should print ioutil, not ../ioutil

The go command prints source file (*.go) paths are printed relative to the current directory (when that makes them shorter). The basic motivation here is that when you run 'go build' (to build the code in the current directory) you want to see a compiler error reported in 'foo.go' and not 'c:\the\directory\you\are\already\in\foo.go'.

Import paths are different: they are always printed fully qualified (even including unvendoring them). And go list is really just shorthand for go list -f '{{.ImportPath}}'.

@perillo

This comment has been minimized.

perillo commented Feb 16, 2016

This is my latest informal patch to add Path and Cmdline fields to the Package struct:
http://pastebin.com/Zkr1yevd

I don't know if it is worth the additional complexity; probably not.
If there is interest I can create a CL, but I'm fine if this issue is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment