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

cmd/doc: issues related to vendor ambiguity #12423

Closed
rsc opened this issue Aug 31, 2015 · 4 comments
Closed

cmd/doc: issues related to vendor ambiguity #12423

rsc opened this issue Aug 31, 2015 · 4 comments
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Aug 31, 2015

Test case:

go get rsc.io/pdf
go doc pdf.Text

I get:

$ go doc pdf.Text
package pdf // import "rsc.io/pdf"

WARNING: package source is installed in "golang.org/x/arch/vendor/rsc.io/pdf"
type Text struct {
    Font     string  // the font used
    FontSize float64 // the font size, in points (1/72 of an inch)
    X        float64 // the X coordinate, in points, increasing left to right
    Y        float64 // the Y coordinate, in points, increasing bottom to top
    W        float64 // the width of the text, in points
    S        string  // the actual UTF-8 text
}

    A Text represents a single piece of text drawn on a page.
$

There are three issues.

First, import comments are no-ops in vendor source trees, so the WARNING should be omitted.

Second, the import statement on the first line should probably give the full path of the selected package (golang.org/x/arch/vendor/rsc.io/pdf). Even though that's not how a source file would import it, the expanded path is what the tools like "go list" show. Perhaps to avoid confusion the line could drop the "import", so not:

package pdf // import "golang.org/x/arch/vendor/rsc.io/pdf"

but instead:

package pdf // golang.org/x/arch/vendor/rsc.io/pdf

Third, there should be some kind of prioritization that depends on where in the source tree "go doc" is run. If run in $GOPATH/src/golang.org/x/arch/x86, then absolutely that package is a good resolution for "go doc pdf.Text". But outside $GOPATH/src/golang.org/x/arch, that package is not importable, so if there is non-vendored copy or a closer vendored copy, that should be preferred. In my case, plain $GOPATH/src/rsc.io/pdf should take priority over the vendored arch copy, again possibly unless run inside the arch subtree.

@rsc rsc added this to the Go1.6Early milestone Aug 31, 2015
@robpike
Copy link
Contributor

robpike commented Sep 29, 2015

I cannot reproduce the first problem. Not sure why:

% go get -d rsc.io/pdf
% go doc pdf.Text
package pdf // import "rsc.io/pdf"

type Text struct {
    Font     string  // the font used
    FontSize float64 // the font size, in points (1/72 of an inch)
    X        float64 // the X coordinate, in points, increasing left to right
    Y        float64 // the Y coordinate, in points, increasing bottom to top
    W        float64 // the width of the text, in points
    S        string  // the actual UTF-8 text
}
    A Text represents a single piece of text drawn on a page.

The vendoring issues are very unclear to me, and I think there are a number of other issues around tooling and vendoring that need to be resolved. I see no reason this needs to be done soon, so I will downgrade to Go1.6 from Go1.6Early and remove myself as assignee.

@robpike robpike modified the milestones: Go1.6, Go1.6Early Sep 29, 2015
@robpike robpike removed their assignment Sep 29, 2015
@robpike
Copy link
Contributor

robpike commented Sep 29, 2015

The reason I can't reproduce the WARNING is that the package is not installed in a vendored directory and I don't know why it is in your situation. So there likely is an issue here, and maybe all it takes to resolve is that the WARNING be prefixed by a check that there is no "/vendor/" in the actual path.

But also: I tried an example that is in a vendor directory and it works fine:

% pwd
/Users/r/go/src/cmd/vendor/golang.org/x/arch/x86/x86asm
% go doc
package x86asm // import "cmd/vendor/golang.org/x/arch/x86/x86asm"

Package x86asm implements decoding of x86 machine code.

const AAA ...
const PrefixImplicit Prefix = 0x8000 ...
const AL ...
var ErrInvalidMode = errors.New("invalid x86 mode in Decode") ...
func GNUSyntax(inst Inst) string
func GoSyntax(inst Inst, pc uint64, symname func(uint64) (string, uint64)) string
func IntelSyntax(inst Inst) string
func Decode(src []byte, mode int) (inst Inst, err error)
type Arg interface { ... }
type Args [4]Arg
type Imm int64
type Inst struct { ... }
type Mem struct { ... }
type Op uint32
type Prefix uint16
type Prefixes [14]Prefix
type Reg uint8
type Rel int32

I believe the real issue here is indeed priority, but I am not sure what the right answer is.

@gopherbot
Copy link

CL https://golang.org/cl/17691 mentions this issue.

robpike added a commit that referenced this issue Dec 10, 2015
This is a simple change to the command that should resolve problems like finding
vendored packages before their non-vendored siblings. By searching in breadth-first
order, we find the matching package lowest in the hierarchy, which is more likely
to be correct than the deeper one, such as a vendored package, that will be found
in a depth-first scan.

This may be sufficient to resolve the issue, and has the merit that it is very easy
to explain. I will leave the issue open for now in case my intuition is wrong.

Update #12423

Change-Id: Icf69e8beb1845277203fcb7d19ffb7cca9fa41f5
Reviewed-on: https://go-review.googlesource.com/17691
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc
Copy link
Contributor Author

rsc commented Dec 17, 2015

The new rules fixed the immediate problem I was running into.

@rsc rsc closed this as completed Dec 17, 2015
@golang golang locked and limited conversation to collaborators Dec 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants