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

gb project importer #818

Closed
wants to merge 7 commits into from
Closed
Changes from 1 commit
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
31 changes: 23 additions & 8 deletions cmd/dep/gb_importer.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,20 +115,21 @@ func (i *gbImporter) convert(pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, erro

// Deduce the project root. This is necessary because gb manifests can have
// multiple entries for the same project root, one for each imported subpackage
var ip gps.ProjectRoot
var root gps.ProjectRoot
var err error
if ip, err = i.sm.DeduceProjectRoot(pkg.Importpath); err != nil {
if root, err = i.sm.DeduceProjectRoot(pkg.Importpath); err != nil {
return nil, nil, err
}

// Set the proper import path back on the dependency
pkg.Importpath = string(ip)
pkg.Importpath = string(root)

// If we've already locked this project root then we can skip
if projectExistsInLock(lock, pkg.Importpath) {
continue
}

// Otherwise, attempt to convert this specific package, which returns a constraint and a lock
pc, lp, err := i.convertOne(pkg)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -156,6 +157,11 @@ func (i *gbImporter) convertOne(pkg gbDependency) (pc gps.ProjectConstraint, lp

However, if we can infer a tag that points to the revision or the branch, we may be able
to use that as the constraint

So, the order of operations to convert a single dependency in a gb manifest is:
- Find a specific version for the revision (and branch, if set)
- If there's a branch available, use that as the constraint
- If there's no branch, but we found a version from step 1, use the version as the constraint
Copy link
Collaborator

Choose a reason for hiding this comment

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

The inline doc is very helpful! 👍

*/
pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.Importpath), Source: pkg.Repository}

Expand All @@ -164,7 +170,7 @@ func (i *gbImporter) convertOne(pkg gbDependency) (pc gps.ProjectConstraint, lp

// But if the branch field is not "HEAD", we can use that as the initial constraint
var constraint gps.Constraint
if pkg.Branch != "HEAD" {
if pkg.Branch != "" && pkg.Branch != "HEAD" {
constraint = gps.NewBranch(pkg.Branch)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method is missing a conditional branch that uses the constraint if a branch is set, and otherwise uses the result of sm.InferConstraint. Right now, constraint is set to master, then immediately discarded a few lines down.

Here are the steps we need:

  1. Default the constraint to the branch if possible.
  2. Use that constraint to lookup the version to put in the lock.
  3. If the constraint is empty, try to infer it from the locked version.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I misunderstood some of the functions I was calling. I'll rejigger it.

}

Expand All @@ -175,12 +181,21 @@ func (i *gbImporter) convertOne(pkg gbDependency) (pc gps.ProjectConstraint, lp
i.logger.Println(err.Error())
}

// And now try to infer a constraint from the returned version
pc.Constraint, err = i.sm.InferConstraint(version.String(), pc.Ident)
if err != nil {
return
// If the constraint is nil (no branch), but there's a version, infer a constraint from there
if constraint == nil && version != nil {
constraint, err = i.sm.InferConstraint(version.String(), pc.Ident)
if err != nil {
return
}
}

// If there's *still* no constraint, set the constraint to the revision
Copy link
Author

Choose a reason for hiding this comment

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

Is this the proper behavior? Should there just not be a constraint if there's no branch specified and no tag that points to the revision?

Copy link
Collaborator

@carolynvs carolynvs Jul 28, 2017

Choose a reason for hiding this comment

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

In general, dep tries to prevent setting a constraint to a specific revision, that's what the lock is for. The manifest tries to only hold hints on how to update a package, like "I am following the v2 branch" or "Anything less than 2.0.0 please".

Instead of not adding a constraint, what we should do in this case (i.e. they haven't picked a branch and are using a revision that isn't tagged) is to set constraint = gps.Any().

The result will be an entry in the manifest without any restriction on it:

[[constraint]]
  name = "github.com/carolyn/lovesponies"

if constraint == nil {
constraint = revision
}

pc.Constraint = constraint

lp = gps.NewLockedProject(pc.Ident, version, nil)

fb.NewConstraintFeedback(pc, fb.DepTypeImported).LogFeedback(i.logger)
Copy link
Collaborator

@carolynvs carolynvs Jul 18, 2017

Choose a reason for hiding this comment

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

The convention is to log the (general) constraint first, and then the (specific) lock, giving us output that looks like this:

Using master as initial constraint for imported dep github.com/golang/lint
Trying master (cb00e56) as initial lock for imported dep github.com/golang/lint

Expand Down