Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Respect canonical import comments #1017

Merged
merged 8 commits into from
Oct 7, 2017

Conversation

rtfb
Copy link
Contributor

@rtfb rtfb commented Aug 15, 2017

What does this do / why do we need it?

This adds capability to pkgtree to extract canonical import path comments from the packages. That will later be used as additional constraints when solving the dependencies (will come in a separate PR).

(And a couple unrelated typo fixes and code cleanups along the way)

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

I was wondering, at the very end of ListPackages, where the package gets added to the Packages map, should I also add a copy with a canonical path key like blow? So that solver could see both versions available?

if pkg.CommentPath != "" {
	ptree.Packages[pkg.CommentPath] = PackageOrErr{
		P: pkg,
	}
}

Also, is there a way to develop from my fork? Can't even get tests running:

$ go test $(go list ./internal/gps/pkgtree/...)
# github.com/rtfb/dep/internal/gps/pkgtree
internal/gps/pkgtree/pkgtree_test.go:20:2: use of internal package not allowed
FAIL	github.com/rtfb/dep/internal/gps/pkgtree [setup failed]

It works fine on the main golang/dep repo, but then transplanting pieces of code to another repo before committing is tedious.

Which issue(s) does this PR fix?

This is phase 1 for issue #902.

We already import and use build package here, so there's no harm in
reuse. The code is both shorter and more readable.
Look for import comments following the package declaration. The
implementation of findImportComment() very closely follows go/build's
findImportComment().

Tests and validation against importRoot to come in a later commit.
Added a check and an error type for importRoot to be a prefix of
canonical import path, tests for the new code paths.

Fixed one unrelated bug along the way:
    Sprintf("%q", strings.Join(strs, "\", \""))
doesn't work as intended, since the %q verb will escape the inner
doublequotes.
@rtfb rtfb requested a review from sdboyer as a code owner August 15, 2017 19:41
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask 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.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@rtfb
Copy link
Contributor Author

rtfb commented Aug 16, 2017 via email

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@sdboyer
Copy link
Member

sdboyer commented Aug 16, 2017

woohoo! so glad we're making progress on this.

Also, is there a way to develop from my fork? Can't even get tests running:

yep, the way i do it is to develop directly in $GOPATH/src/github.com/golang/dep, but add a second remote that points to my fork. i use my fork as origin, and this repo as upstream.

should I also add a copy with a canonical path key like blow? So that solver could see both versions available?

nope, we don't want that. the goal of ListPackages() is to provide an accurate picture of what's actually on disk - a virtual representation of the filesystem. we rely on that design principle in a lot of places; adding an entry that undermines that rule would just confuse things.

i'll try to do a review in the next day or two 😄

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry sorry it took me so long! the mechanics of this look pretty good, but we need to change a bit about how the return semantics work.

}

// ConflictingImportComments indicates that the package declares more than one
// different canonical paths.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/paths/path/

}

// NonCanonicalImportRoot reports the situation when the dependee imports a
// package via something else that the package's declared canonical path.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/else that/other than/

@@ -152,18 +154,23 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) {
}
}

if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) {
return &NonCanonicalImportRoot{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs softening a bit, as right now, the only cases where we return an actual error from ListPackages() itself (which is what this would result in) are more physical than logical - e.g., we can't actually read the filesystem. the rest of the API is built around that kind of assumption, too.

so, instead of returning the error directly, let's make this an error that we put in the PackageOrErr.Err slot on each package where we encounter the problem.

Copy link
Contributor Author

@rtfb rtfb Sep 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume an equivalent change should be made to prevent ConflictingImportComments from leaking as well?

@ibrasho ibrasho assigned ibrasho and unassigned ibrasho Sep 9, 2017
@rtfb
Copy link
Contributor Author

rtfb commented Sep 10, 2017

sorry sorry it took me so long!

No worries, I haven't been able to spend significant time on it either :-). Just enough to address your feedback...

AppVeyor tests are failing at something that doesn't seem to be related. Can this be due to my branch falling too far behind? Should I rebase?

@jmank88 jmank88 closed this Sep 10, 2017
@jmank88 jmank88 reopened this Sep 10, 2017
@sdboyer
Copy link
Member

sdboyer commented Sep 13, 2017

it wasn't you, it was other problems we were having. appveyor's been whipped back into line, now 😄

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

back, after more delay! basically wording and comment nits, but the mechanics look good. lots more that we could add with this later.

@@ -210,6 +220,7 @@ func fillPackage(p *build.Package) error {

var testImports []string
var imports []string
var importComments []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so i was originally going to suggest that we just keep a single string, rather than a []string, and upon encountering later values, just compare against that one string and error out immediately if we find an error. i was thinking that it might escape, but i ran it through with the optimization profiler (-gcflags '-m -m'), and turns out, it doesn't.

so, this is fine. just thought you might be interested that i checked that through 😄

}

func quotedPaths(ps []string) string {
var quoted []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

microoptimization nit: quoted := make([]string, 0, len(ps))

// ConflictingImportComments indicates that the package declares more than one
// different canonical path.
type ConflictingImportComments struct {
ImportPath string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the provenance of these errors can be a little confusing, let's include comments explaining the intent of the information recorded in each of the struct fields here, analogous to Package.

// different canonical path.
type ConflictingImportComments struct {
ImportPath string
ImportComments []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For more clarity, let's name this ConflictingImportComments.

// NonCanonicalImportRoot reports the situation when the dependee imports a
// package via something other than the package's declared canonical path.
type NonCanonicalImportRoot struct {
ImportRoot string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the above, let's include brief field-by-field comments on these to explain where they come from.

}

func (e *NonCanonicalImportRoot) Error() string {
return fmt.Sprintf("importing via path %q, but package insists on a canonical path %q",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this phrasing may end up being a bit confusing, as it would come out:

importing via path "github.com/foo/Bar", but package insists on a canonical path "github.com/foo/bar/bleep/blorp/bopkins"

(case variation here used only to provide a clear example, this probably isn't how it'll be seen most of the time)

i don't have a specific wording suggestion, but we should tweak this a little bit to accommodate the fact that we're contrasting a root path with what could potentially be a subpackage path.

@sdboyer
Copy link
Member

sdboyer commented Oct 6, 2017

bump

@rtfb
Copy link
Contributor Author

rtfb commented Oct 6, 2017

Yeah, I'm on it. I was travelling when you last commented, and came around to sit down with it couple days back, only to find out that I was locked out of my GH 2FA during phone transition. You know, little joys of life :-)

I'll try to push an update later today.

@rtfb rtfb force-pushed the respect-import-comments-issue-902 branch from 5198b48 to 47e42c7 Compare October 6, 2017 17:50
@rtfb
Copy link
Contributor Author

rtfb commented Oct 6, 2017

The build failed only on 1.9.x, with a lint error, while on others it passed. Weird :-/

@sdboyer
Copy link
Member

sdboyer commented Oct 7, 2017

no worries - thanks for keeping at this! and yeah, we only run the linters on the 1.9 travis build. it should just be that one issue, then: https://travis-ci.org/golang/dep/jobs/284321611#L547 - the newline byte slice var wasn't used.

i've no idea why the CLA bot is misbehaving. it's been doing this for a week or so now :(

@gmlewis
Copy link

gmlewis commented Oct 7, 2017

I apologize for the CLA problems - we are working on resolving the issues. I've rescanned this PR and it is now cleared.

@sdboyer
Copy link
Member

sdboyer commented Oct 7, 2017

@gmlewis oh, i appreciate the update - and i didn't even have to go looking for anybody to ping about it! 👍 😄

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great! i think this is ready to go 🎉

@sdboyer sdboyer merged commit 667d662 into golang:master Oct 7, 2017
@darkowlzz darkowlzz changed the title [WIP] Respect canonical import comments Respect canonical import comments Oct 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants