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

gvt (and gb-vendor) importer #1149

Merged
merged 5 commits into from
Sep 28, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/dep/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ specified, use the current directory.
When configuration for another dependency management tool is detected, it is
imported into the initial manifest and lock. Use the -skip-tools flag to
disable this behavior. The following external tools are supported:
glide, godep, vndr, govend.
glide, godep, vndr, govend, gb, gvt.

Any dependencies that are not constrained by external configuration use the
GOPATH analysis below.
Expand Down
28 changes: 28 additions & 0 deletions cmd/dep/testdata/harness_tests/init/gvt/case1/final/Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions cmd/dep/testdata/harness_tests/init/gvt/case1/final/Gopkg.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

[[constraint]]
name = "github.com/sdboyer/deptest"
source = "https://github.com/carolynvs/deptest"

[[constraint]]
name = "github.com/sdboyer/deptestdos"

[[constraint]]
branch = "v2"
name = "gopkg.in/yaml.v2"
20 changes: 20 additions & 0 deletions cmd/dep/testdata/harness_tests/init/gvt/case1/initial/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package main

import (
"fmt"

"github.com/sdboyer/deptest"
"github.com/sdboyer/deptestdos"
"gopkg.in/yaml.v2"
)

func main() {
var a deptestdos.Bar
var b yaml.MapItem
var c deptest.Foo
fmt.Println(a, b, c)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"version": "0",
"dependencies": [
{
"importpath": "github.com/sdboyer/deptest",
"repository": "https://github.com/carolynvs/deptest",
"revision": "3f4c3bea144e112a69bbe5d8d01c1b09a544253f",
"branch": "HEAD"
},
{
"importpath": "github.com/sdboyer/deptestdos",
"repository": "https://github.com/sdboyer/deptestdos",
"revision": "5c607206be5decd28e6263ffffdcee067266015eXXX",
"branch": "master"
},
{
"importpath": "gopkg.in/yaml.v2",
"repository": "https://gopkg.in/yaml.v2",
"revision": "f7716cbe52baa25d2e9b0d0da546fcf909fc16b4",
"branch": "v2"
}
]
}
13 changes: 13 additions & 0 deletions cmd/dep/testdata/harness_tests/init/gvt/case1/testcase.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"commands": [
["init", "-no-examples"]
],
"error-expected": "",
"gopath-initial": {
"github.com/sdboyer/deptest": "3f4c3bea144e112a69bbe5d8d01c1b09a544253f"
},
"vendor-final": [
"github.com/sdboyer/deptest",
"github.com/sdboyer/deptestdos"
]
}
2 changes: 1 addition & 1 deletion docs/FAQ.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ about what's going on.
During `dep init` configuration from other dependency managers is detected
and imported, unless `-skip-tools` is specified.

The following tools are supported: `glide`, `godep`, `vndr` and `govend`.
The following tools are supported: `glide`, `godep`, `vndr`, `govend`, `gb` and `gvt`.

See [#186](https://github.com/golang/dep/issues/186#issuecomment-306363441) for
how to add support for another tool.
Expand Down
27 changes: 26 additions & 1 deletion internal/importers/base/importer.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,18 @@ func (i *Importer) ImportPackages(packages []ImportedPackage, defaultConstraintF
}

for _, prj := range projects {
source := prj.Source
if len(source) > 0 {
isDefault, err := i.isDefaultSource(prj.Root, source)
if err == nil && isDefault {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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())

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

source = ""
}
}

pc := gps.ProjectConstraint{
Ident: gps.ProjectIdentifier{
ProjectRoot: prj.Root,
Source: prj.Source,
Source: source,
},
}

Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@michael-go michael-go Sep 23, 2017

Choose a reason for hiding this comment

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

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 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return true, nil
}

sourceURLs, err := i.sm.SourceURLsForPath(string(projectRoot))
if err != nil {
return false, err
}
// The first url in the slice will be the default one (usually https://...)
if len(sourceURLs) > 0 && sourceURL == sourceURLs[0].String() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@michael-go michael-go Sep 23, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

return true, nil
}

return false, nil
}
129 changes: 129 additions & 0 deletions internal/importers/gvt/importer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
// Copyright 2017 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package gvt

import (
"encoding/json"
"io/ioutil"
"log"
"os"
"path/filepath"

"github.com/golang/dep"
"github.com/golang/dep/internal/gps"
"github.com/golang/dep/internal/importers/base"
"github.com/pkg/errors"
)

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

// Importer imports gvt configuration into the dep configuration format.
type Importer struct {
*base.Importer
gvtConfig gvtManifest
}

// NewImporter for gvt.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

func NewImporter(logger *log.Logger, verbose bool, sm gps.SourceManager) *Importer {
return &Importer{Importer: base.NewImporter(logger, verbose, sm)}
}

type gvtManifest struct {
Deps []gvtPkg `json:"dependencies"`
}

type gvtPkg struct {
ImportPath string
Repository string
Revision string
Branch string
}

// Name of the importer.
func (g *Importer) Name() string {
return "gvt"
}

// HasDepMetadata checks if a directory contains config that the importer can handle.
func (g *Importer) HasDepMetadata(dir string) bool {
y := filepath.Join(dir, gvtPath)
if _, err := os.Stat(y); err != nil {
return false
}

return true
}

// Import the config found in the directory.
func (g *Importer) Import(dir string, pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) {
err := g.load(dir)
if err != nil {
return nil, nil, err
}

return g.convert(pr)
}

func (g *Importer) load(projectDir string) error {
g.Logger.Println("Detected gb/gvt configuration files...")
j := filepath.Join(projectDir, gvtPath)
if g.Verbose {
g.Logger.Printf(" Loading %s", j)
}
jb, err := ioutil.ReadFile(j)
if err != nil {
return errors.Wrapf(err, "unable to read %s", j)
}
err = json.Unmarshal(jb, &g.gvtConfig)
if err != nil {
return errors.Wrapf(err, "unable to parse %s", j)
}

return nil
}

func (g *Importer) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) {
g.Logger.Println("Converting from vendor/manifest ...")

packages := make([]base.ImportedPackage, 0, len(g.gvtConfig.Deps))
for _, pkg := range g.gvtConfig.Deps {
// Validate
if pkg.ImportPath == "" {
err := errors.New("invalid gvt configuration, ImportPath is required")
return nil, nil, err
}

if pkg.Revision == "" {
err := errors.New("invalid gvt configuration, Revision is required")
return nil, nil, err
}

var contstraintHint = ""
if pkg.Branch == "HEAD" {
// gb-vendor sets "branch" to "HEAD", if the package was feteched via -tag or -revision,
// we pass the revision as the constraint hint
contstraintHint = pkg.Revision
} else if pkg.Branch != "master" {
// both gvt & gb-vendor set "branch" to "master" unless a different branch was requested.
// so it's not realy a constraint unless it's a different branch
contstraintHint = pkg.Branch
}

ip := base.ImportedPackage{
Name: pkg.ImportPath,
Source: pkg.Repository,
LockHint: pkg.Revision,
ConstraintHint: contstraintHint,
}
packages = append(packages, ip)
}

err := g.ImportPackages(packages, true)
if err != nil {
return nil, nil, err
}

return g.Manifest, g.Lock, nil
}
Loading