gvt (and gb-vendor) importer #1149

michael-go · 2017-09-10T14:08:45Z

What does this do / why do we need it?

It imports the config (vendor/manifest) of https://github.com/FiloSottile/gvt

What should your reviewer look out for in this PR?

gvt and gb-vendor use the same filename for their manifests, and have a pretty similar structure, as gvt is based on gb-vendor.
There already is a PR open for gb: #818, however it's based on the code prior to the recently merged "Standardize the importers" #1100, and @carolynvs suggested I opened a new PR anyway.

Do you need help or clarification on anything?

Every import in the gvt manifest has a repository field, which has the URL of the repository. On one project I tested the importer with, it had the repository field of all golang.org/x/crypto packages point to https://go.googlesource.com/crypto. To make the import more accurate, I tried to init the importedPackage struct via Source: pkg.Repository, however it fails with:

unable to list versions for golang.org/x/crypto(https://go.googlesource.com/crypto): unable to deduce repository and source type for "https://go.googlesource.com/crypto": unable to read metadata: go-import metadata not found

Thanks!

Which issue(s) does this PR fix?

fixes #679

googlebot · 2017-09-10T14:08:48Z

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.

michael-go · 2017-09-10T14:34:06Z

I signed it!

googlebot · 2017-09-10T14:34:09Z

CLAs look good, thanks!

sdboyer · 2017-09-10T22:08:27Z

hmm, so, re: the Source issue, I think we may need to do a little extra work in order to determine if the named source is one of the default possible ones for a given import path, and if so, omit it. that will require a new method on SourceManager

carolynvs · 2017-09-11T03:39:18Z

So gband gvt use the exact same file format, and gvt indicates that it created the file by inserting "generator": "github.com/FiloSottile/gvt", into the manifest? If that's right, then we won't need two importers and both can be handled by a single importer.

I don't know enough about either project to say for sure, and it shouldn't impact this PR. Just thinking about whether or not we can close the gb PR when this merges.

carolynvs · 2017-09-11T03:42:36Z

RE: the source field, this is why the original gb PR got hung up. @michael-go Let us know if you need have questions or help implementing what Sam is suggesting.

carolynvs

This is really good! 💖

I am so very sorry that my upcoming PR is going to require more changes to your PR. That's on me, but hopefully it's not too much work to rebase!

carolynvs · 2017-09-11T03:52:52Z

cmd/dep/gvt_importer.go

+		}
+
+		var contstraintHint = ""
+		if pkg.Branch != "master" {


You don't need to validate this, and can set ConstraintHint to pkg.Branch directly. The base importer handles detecting if it's a branch, and ignore garbage like "HEAD" or constraints that don't match the revision.

I added this line because it actually affected the behavior.
for example, I ran it on a gvt project with this in the manifest:

{ "importpath": "github.com/blang/semver", "repository": "https://github.com/blang/semver", "vcs": "", "revision": "aea32c919a18e5ef4537bbd283ff29594b1b0165", "branch": "master" },

by passing the "master" as a ConstraintHint, dep init chose a newer version:

[[projects]] name = "github.com/blang/semver" packages = ["."] revision = "2ee87856327ba09384cabd113bc6b5d174e9ec0f" version = "v3.5.1"

but by not passing "master" as a hint with the check above, dep init chose the correct version:

[[projects]] name = "github.com/blang/semver" packages = ["."] revision = "aea32c919a18e5ef4537bbd283ff29594b1b0165" version = "v3.1.0"

I think that may be a bug in the base importer then? I'll use that a testcase and see if I can figure out what's going on. 😀

carolynvs · 2017-09-11T03:52:59Z

cmd/dep/gvt_importer.go

+
+const gvtPath = "vendor" + string(os.PathSeparator) + "manifest"
+
+type gvtImporter struct {


Heads up: there's one more PR pending that will impact yours. I am so sorry about the churn! 😥

When #1145 is merged (hopefully tomorrow morning), here's what will need to change:

Make a directory for gvt under internal/importers.

Move gvt_importer.go into that directory and rename it to importer.go. Same for it's test file.

Add an import for github.com/golang/dep/internal/importers/base.

The *baseImporter field should be renamed to *base.Importer.

Any fields you were using from the base importer are now exported and need to be upper-cased (e.g. g.logger -> g.Logger, g.manifest -> g.Manifest).

Rename gvtImporter to Importer.

Rename newGvtImporter() to NewImporter(). This function is now called in internal/importers/importers.go -> BuildAll().

Move your testdata to internal/importers/gvt/testdata.

carolynvs · 2017-09-11T03:56:42Z

cmd/dep/gvt_importer_test.go

+	}{
+		"package without comment": {
+			convertTestCase{
+				wantConstraint: importerTestV1Constraint,


After that PR is merged, you will need to import github.com/golang/dep/internal/importers/importertest, and then the testcases will look like this:

importertest.TestCase{ WantConstraint: importertest.V1Constraint, WantRevision: importertest.V1Rev, WantVersion: importertest.V1Tag, },

carolynvs · 2017-09-11T03:58:27Z

cmd/dep/gvt_importer_test.go

+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			err := testCase.Exec(t, func(logger *log.Logger, sm gps.SourceManager) (*dep.Manifest, *dep.Lock, error) {


After the PR this function has been renamed to Execute.

carolynvs · 2017-09-11T03:59:12Z

cmd/dep/gvt_importer_test.go

+		name := name
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()


We've had trouble with running the importer tests in parallel (see #1143). So please remove this line.

michael-go · 2017-09-12T07:49:26Z

@sdboyer, regarding the Source. So if I understand correctly, you suggest a new method in SourceManger, something like IsDefaultSource(ProjectIdentifier) bool, and this method will be called from baseImporter.importPackages(), and if returns true, the .Source will be cleared from the gps.ProjectIdentifier prior the first call to InferConstraint()?
If so, you would like this to be part of this PR?
Thanks!

and @carolynvs, thanks! I'll rebase my branch with the changes above after #1145 is merged

sdboyer · 2017-09-12T09:30:54Z

yep! that sounds ideal.

…

On September 12, 2017 3:49:27 AM EDT, michael-go ***@***.***> wrote: @sdboyer, regarding the `Source`. So if I understand correctly, you suggest a new method in `SourceManger`, something like `IsDefaultSource(ProjectIdentifier) bool`, and this method will be called from `baseImporter.importPackages()`, and if returns true, the `.Source` will be cleared from the `gps.ProjectIdentifier` prior the first call to `InferConstraint()`? If so, you would like this to be part of this PR? Thanks! and @carolynvs, thanks! I'll rebase my branch with the changes above when #1145 is merged -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1149 (comment)

michael-go · 2017-09-14T21:04:49Z

@carolynvs I rebased the branch following the merge of #1445.

I still check that the .Branch isn't "master" before passing it as a ConstraintHint, because otherwise it overrides the imported revision, as in the example I gave above (#1149 (comment)). dep init prints:

(0)   ✓ select (root)
(1)	? attempt github.com/blang/semver with 1 pkgs; at least 1 versions to try
(1)	    try github.com/blang/[email protected]
(2)	✗   github.com/blang/[email protected] not allowed by constraint master:
(2)	    master from (root)
(1)	    try github.com/blang/[email protected]
(1)	✓ select github.com/blang/[email protected] w/1 pkgs

Regarding the repository field in gvt's manifest, I'm waiting for the merge of #1166 opened following the helpful advice of @sdboyer.

Thanks!

carolynvs · 2017-09-15T02:10:12Z

I was able to reproduce the issue of the master constraint.

The problem is that imported revision is tagged with v3.1.0, and the base importer is using that version in the lock, while still using a branch constraint in the manifest, which is invalid dep configuration. Solve will detect that v3.1.0 doesn't satisfy the branch constraint, and pick a different revision that satisfies the constraint.

I am still thinking about how to resolve this:

The base importer can ignore that the revision is tagged, and instead of setting version = "v3.1.0" in the lock, use branch = "master" instead.
The base importer can ignore the master constraint, and continue setting version = "v3.1.0" in the lock. It could then either use an any constraint, or ^3.1.0 in the manifest.

Either of those solutions would preserve the correct locked revision. The first is "truer" to the original imported config, preserving the master branch constraint. However, dep would much prefer to move people onto using semver constraints.

Plus I'm not sure that gvt really meant "stay on master" instead of "this revision happened to be the tip of master when gvt was run". 🤔

carolynvs

The rebase looks great! 👍

Please update the help text and doc to mention that gvt and gb(right?) are supported:

dep/cmd/dep/init.go

Lines 31 to 32 in d62440d

    
           disable this behavior. The following external tools are supported: 
        
           glide, godep, vndr, govend.

https://github.com/golang/dep/blob/master/docs/FAQ.md#what-external-tools-are-supported

michael-go · 2017-09-15T10:39:10Z

Thanks @carolynvs.
I updated the docs with gvt and also added the integration test for it, which b.t.w. catches the "branch": "master" issue with https://github.com/sdboyer/deptestdos

regarding gb - I'll look later at #818 to make sure I cover here all the cases for it

michael-go · 2017-09-16T19:51:22Z

OK, so after looking at #818, I added a special handling to a case where "branch" is set "HEAD", and updated the docs (and console output) to include gb

carolynvs · 2017-09-19T14:10:42Z

internal/importers/gvt/importer.go

+
+		ip := base.ImportedPackage{
+			Name: pkg.ImportPath,
+			//TODO: temporarly ignore .Repository. see https://github.com/golang/dep/pull/1166


I think once you get a chance to incorporate your PR back into this, we can merge.

thanks @carolynvs ! will try to add a commit tomorrow

michael-go · 2017-09-20T13:31:31Z

@carolynvs I rebased over the new master following the merge of #1166 and handled the Respository issue. Thanks! 🙏

1.8.x on Travis fails with go test -race on TestIntegration/ensure/empty/case1 .. I'm pretty sure it's not related to this PR

carolynvs · 2017-09-22T14:30:44Z

internal/importers/base/importer.go

+		source := prj.Source
+		if len(source) > 0 {
+			isDefault, err := i.isDefaultSource(prj.Root, source)
+			if err == nil && isDefault {


Let's print a warning, and ignore the imported source. Then they can review the warning and tweak if necessary.

Also add a testcase in TestBaseImporter_ImportProjects to verify that WantWarning contains a substring from our warning message and that the source was set to "".

e.g.

Ignoring imported source https://github.com/example/foo.blorp for github.com/example/foo: <error string>

Do you mean it for every time we set Source to "" , or just when isDefaultSource() returns an error? (and it should actually never return an error, as DeduceRootProject() that is called earlier in loadPackages calls the same underlying deduceRootPath())

Sorry that wasn't clear! Yes, I was referring to when isDefaultSource returns an error. Even though it may never happen, I'd prefer to handle it regardless.

carolynvs · 2017-09-22T14:31:51Z

internal/importers/base/importer.go

@@ -291,3 +299,20 @@ func (i *Importer) convertToConstraint(v gps.Version) gps.Constraint {
 	}
 	return v
 }
+
+func (i *Importer) isDefaultSource(projectRoot gps.ProjectRoot, sourceURL string) (bool, error) {
+	if sourceURL == "https://"+string(projectRoot) {


Can you help me understand why this is necessary? I would have thought that SourceURLsForPath would cover this as well.

mainly for gopkg.in imports, as for example importing gopkg.in/yaml.v2 via gvt will appear in the manifest as:

{ "importpath": "gopkg.in/yaml.v2", "repository": "https://gopkg.in/yaml.v2", "vcs": "", "revision": "f7716cbe52baa25d2e9b0d0da546fcf909fc16b4", "branch": "v2" }

but SourceURLsForPath() will return [https://github.com/airbrake/gobrake http://github.com/airbrake/gobrake]

In addition, this small condition will usually return true and will save save some CPU cycles 😄

Would you please add a comment explaining that we are essentially checking for gopkg.in URLs?

carolynvs · 2017-09-22T14:37:58Z

internal/importers/base/importer.go

+		return false, err
+	}
+	// The first url in the slice will be the default one (usually https://...)
+	if len(sourceURLs) > 0 && sourceURL == sourceURLs[0].String() {


If the first entry in sourceURLs doesn't exist, does dep try the other ones? e.g. If the imported source matched the second entry, but we don't import it and left the source blank, would dep automatically try the first, realize it doesn't exist and fall back to that second entry?

I am a bit confused as to why only the first entry is the only "default". I thought they all were defaults that dep would try?

regarding you first question, yes, I believe dep will try the next URL in order, if one doesn't exist.

I compare only to the first one following the guidance of @sdboyer - good chance I misunderstood something though. My guess for this, is that if a tool choose not the https: URL, but the ssh one for example, we better respect that and not try fetching via https even if it works.

Sounds good to me, thanks!

carolynvs · 2017-09-22T14:39:04Z

internal/importers/gvt/importer.go

+	gvtConfig gvtManifest
+}
+
+// NewImporter for gvt.


So we don't forget, let's include in this comment that it also automatically handles gb as well.

carolynvs · 2017-09-22T14:40:17Z

internal/importers/gvt/importer_test.go

+				},
+			},
+		},
+		"package with alternate repository": {


I believe this is really testing the logic that you added to the base importer, right? Let's move this testcase into the base importer's tests, as it's not specific to gvt.

hm... this tests also tests that I use the Source field in gvt importer, i.e. it tests this line: https://github.com/michael-go/dep/blob/1cfc27fb29c6369b42d99a51e06e680f1539e819/internal/importers/gvt/importer.go#L116

maybe we just need to added another test in the base importer in addition?

Yeah that sounds good, would you please add a test to the base importer as well?

michael-go · 2017-09-27T19:37:02Z

Thanks @carolynvs, I handled all your comments, except I didn't know how to write the test for isDefaultSource() error handling, as there is no possible code flow to make the warning to be printed. I can mock isDefaultSource() for this, but thought that maybe the 2 tests I added in the last commit are enough. 🙏

carolynvs · 2017-09-27T20:20:49Z

Yes, that's enough, thanks!

michael-go requested a review from carolynvs as a code owner September 10, 2017 14:08

googlebot added the cla: no label Sep 10, 2017

michael-go force-pushed the gvt-importer branch from e4bce9d to 80f5fad Compare September 10, 2017 14:14

googlebot added cla: yes and removed cla: no labels Sep 10, 2017

michael-go force-pushed the gvt-importer branch from 80f5fad to bdd2f6b Compare September 10, 2017 17:44

carolynvs suggested changes Sep 11, 2017

View reviewed changes

michael-go mentioned this pull request Sep 13, 2017

add SourceURLsForPath() to SourceManager interface #1166

Merged

michael-go force-pushed the gvt-importer branch from bdd2f6b to a7103ca Compare September 14, 2017 20:47

carolynvs suggested changes Sep 15, 2017

View reviewed changes

michael-go requested a review from sdboyer as a code owner September 15, 2017 10:32

michael-go force-pushed the gvt-importer branch 2 times, most recently from b9d8969 to a7eff3c Compare September 16, 2017 19:48

michael-go changed the title ~~[WIP] gvt importer~~ [WIP] gvt (and gb-vendor) importer Sep 16, 2017

carolynvs reviewed Sep 19, 2017

View reviewed changes

michael-go added 3 commits September 20, 2017 11:09

import gvt manifest on init

34b8ee6

add gvt importer integration test and update docs

28f55f7

gvt importer imports gb-vendor too

956f34d

import alternate repo url from gb/gvt manifest

1cfc27f

michael-go force-pushed the gvt-importer branch from a7eff3c to 1cfc27f Compare September 20, 2017 13:16

carolynvs removed the request for review from sdboyer September 22, 2017 14:17

carolynvs suggested changes Sep 22, 2017

View reviewed changes

gvt/gb importer PR fixes

cbf282b

carolynvs approved these changes Sep 28, 2017

View reviewed changes

carolynvs changed the title ~~[WIP] gvt (and gb-vendor) importer~~ gvt (and gb-vendor) importer Sep 28, 2017

carolynvs merged commit 6fb987b into golang:master Sep 28, 2017

carolynvs mentioned this pull request Sep 28, 2017

gb project importer #818

Closed

zknill pushed a commit to zknill/dep that referenced this pull request Oct 6, 2017

gvt (and gb-vendor) importer (golang#1149)

c87c7b4

This was referenced Oct 7, 2017

Switch from gvt to dep digitalocean/doctl#259

Closed

Switch from gvt to dep digitalocean/doctl#262

Merged


		const gvtPath = "vendor" + string(os.PathSeparator) + "manifest"

		type gvtImporter struct {

	disable this behavior. The following external tools are supported:
	glide, godep, vndr, govend.

gvt (and gb-vendor) importer #1149

gvt (and gb-vendor) importer #1149

Conversation

michael-go commented Sep 10, 2017

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

googlebot commented Sep 10, 2017

michael-go commented Sep 10, 2017

googlebot commented Sep 10, 2017

sdboyer commented Sep 10, 2017

carolynvs commented Sep 11, 2017

carolynvs commented Sep 11, 2017

carolynvs left a comment

Choose a reason for hiding this comment

carolynvs Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-go commented Sep 12, 2017 • edited Loading

sdboyer commented Sep 12, 2017 via email

michael-go commented Sep 14, 2017 • edited Loading

carolynvs commented Sep 15, 2017

carolynvs left a comment

Choose a reason for hiding this comment

michael-go commented Sep 15, 2017

michael-go commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-go commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-go Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-go Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-go commented Sep 27, 2017

carolynvs commented Sep 27, 2017

carolynvs Sep 11, 2017 •

edited

Loading

michael-go commented Sep 12, 2017 •

edited

Loading

michael-go commented Sep 14, 2017 •

edited

Loading

michael-go commented Sep 20, 2017 •

edited

Loading

michael-go Sep 23, 2017 •

edited

Loading

michael-go Sep 23, 2017 •

edited

Loading