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

gb project importer #818

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@avidal
Copy link

avidal commented Jul 15, 2017

What does this do / why do we need it?

This adds a project importer for gb, one more step towards converting all existing dependency managers.

What should your reviewer look out for in this PR?

This was based on a combination of the PR that added the glide importer plus the code in glide that added the gb importer: https://github.com/Masterminds/glide/blob/gps-integration/gb/gb.go

gb (well, more particularly, the gb-vendor plugin) doesn't save the tag if you fetch by tag, instead it fills in the branch key for the dependency with "HEAD", same if you fetch by a specific revision. This PR tries to infer a constraint based on just the revision, but I'm not sure if that's the appropriate behavior or not.

references #186

@avidal avidal requested a review from carolynvs as a code owner Jul 15, 2017

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Jul 15, 2017

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.

@googlebot googlebot added the cla: no label Jul 15, 2017

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 15, 2017

I signed it!

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Jul 15, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jul 15, 2017

@avidal avidal force-pushed the avidal:gb-importer branch from ba11bbe to 4916449 Jul 15, 2017

@avidal avidal changed the title [WIP] gb project importer gb project importer Jul 15, 2017

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 15, 2017

Hm. I rebased to clean up my commits and now I'm blocked on the CLA check again.

edit: Looks like the CLA check is just a bit laggy right now. Lots of other open PRs pending a CLA check.

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 15, 2017

Switched to only blessed repositories for the unit tests, as well as added an integration test (thanks @carolynvs !)

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 17, 2017

Hmm. Both this PR and my other one #829 are waiting on the cla/google check since Friday..

@sdboyer sdboyer closed this Jul 18, 2017

@sdboyer sdboyer reopened this Jul 18, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 18, 2017

would be great if we could get a review from @davecheney on this - marking him as a reviewer 🙏

@sdboyer sdboyer requested a review from davecheney Jul 18, 2017

@carolynvs
Copy link
Collaborator

carolynvs left a comment

Yay, this is so close! I have just a few minor nits and questions.

// load the gb manifest
func (i *gbImporter) load(projectDir string) error {
i.logger.Println("Detected gb manifest file...")
var err error

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

small nit: It would help to have err defined closer to its assignment, in this case next to where 'buf' is declared.


// See if we can get a version from that constraint
version, err := lookupVersionForLockedProject(pc.Ident, revision, revision, i.sm)
if err != nil {

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

The other importers treat being unable to determine whether the revision is tagged, or the tip of a branch, as a warning and do this:

if err != nil {
	// Only warn about the problem, it is not enough to warrant failing
	g.logger.Println(err.Error())
}
lp = gps.NewLockedProject(pc.Ident, version, nil)

fb.NewLockedProjectFeedback(lp, fb.DepTypeImported).LogFeedback(i.logger)
fb.NewConstraintFeedback(pc, fb.DepTypeImported).LogFeedback(i.logger)

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

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
var revision = gps.Revision(pkg.Revision)

// See if we can get a version from that constraint
version, err := lookupVersionForLockedProject(pc.Ident, revision, revision, i.sm)

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

The second parameter c gps.Constraint should be nil, instead of passing in the revision again.

The purpose of the constraint parameter is to help pick the appropriate tag when a revision has multiple tags, by filtering with the semver constraint from the manifest (which we don't have with gb). To be clear, it doesn't cause the function to fail, but we know that since it's a revision, it's never going to help in that case. 😀

@@ -0,0 +1,6 @@
Detected gb manifest file...
Converting from gb manifest...
Trying master (a0196ba) as initial lock for imported dep github.com/sdboyer/deptestdos

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

As noted above, we would like to be consistent in how these are logged and have the constraints listed before the locked projects.


// gbManifest represents the manifest file for GB projects
type gbManifest struct {
Version int `json:"version"`

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

Is this used anywhere?

// Branch may be HEAD or an actual branch. In the case of HEAD, that means
// the user vendored a dependency by specifying a tag or a specific revision
// which results in a detached HEAD
Branch string `json:"branch"`

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

It doesn't look like Branch is being used. If it's set to a real branch name that we can find in the repo, should it be used as the constraint in the manifest?

This comment has been minimized.

@avidal

avidal Jul 24, 2017

I added support for using the branch as the initial constraint (if it's not HEAD); but I don't have a solid repository to use in the tests. Ideally there'd be a blessed test repository with two branches, then I can have a test that pins to a revision that is not on the master branch.

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

Would https://github.com/carolynvs/deptest work for your test? It has a master and v2 branch, and it's "blessed" for using in dep's tests.

This comment has been minimized.

@avidal

avidal Jul 25, 2017

I'll add a test that uses it and see how the tooling responds. Thanks.

// Path is used when the actual used package exists in a subpath
// For importing purposes, this isn't necessary as dep will figure
// out which packages are actually being used during resolving
Path string `json:"path,omitempty"`

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

Since we are ignoring Path, and don't print a warning about the fact that we ignore it, we can just remove it. 😄

version = "1.0.0"

[[constraint]]
branch = "master"

This comment has been minimized.

@carolynvs

carolynvs Jul 18, 2017

Collaborator

Right now, we are seeing "branch = master" here because the revision is the tip of master (which lookupVersionForLockedProject identifies). I am thinking that if the revision wasn't the tip however, we'd still like the constraint to be master. If we end up adding logic for using the branch from gb, using an earlier revision from master would be one way to test that out.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 21, 2017

how are we doing with this?

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 24, 2017

@sdboyer @carolynvs Thanks for the feedback; been really busy now that I'm back at the office but I'll take a stab at addressing the comments once I get a chance.

@avidal avidal requested a review from sdboyer as a code owner Jul 24, 2017

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 24, 2017

@carolynvs @sdboyer I just pushed a handful of commits that should address all of the feedback. Good call on using a non-tip revision of deptestdos, the test is much better now.

@avidal avidal force-pushed the avidal:gb-importer branch from b8fd555 to f40b53f Jul 24, 2017

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 24, 2017

Rebased to a single commit with all PR feedback addressed.

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

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

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.

This comment has been minimized.

@avidal

avidal Jul 25, 2017

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

@@ -0,0 +1,6 @@
Detected gb manifest file...
Converting from gb manifest...
Using ^2.0.0 as initial constraint for imported dep github.com/sdboyer/deptestdos

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

Once the constraint logic is fixed, this will change to Using master as initial constraint.

if err != nil {
return nil, nil, err
}
manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{Source: pc.Ident.Source, Constraint: pc.Constraint}

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

In the gb manifest file, is it possible that the import path is a sub-package? e.g. the manifest has an entry for for "github.com/pkg/foo/cats" and "github.com/pkg/foo/dogs".

If so, then your importer will need to check if the root project has already been added, and skip subsequent imports for the same root package. For a reference, see how the godep importer handles this scenario.

This comment has been minimized.

@avidal

avidal Jul 25, 2017

Yes, I believe that the gb manifest has a "importpath" which contains a subpackage, and then the combination of "repository" and "path" points to the source for that specific subpackage. I dropped the path in the importer because dep will handle that already; and I misunderstood gps.ProjectIdentifier. I'll check the godep importer.

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

I dorked this up for the glide importer, just opened #898 to fix. 😊

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 25, 2017

Just wanted to give a huge 👍 to @carolynvs for being so thorough in the reviews. dep is a bit of a beast, and there are plenty of opportunities to misconstrue the architecture in an importer so having such a thorough maintainer for the subsystem is a huge help. Thanks again!

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 26, 2017

@avidal you are correct, @carolynvs is awesome 😄

thank you for sticking with it! there's obviously a lot of change in dep right now, which has definitely undermined our ability to generate good documentation - and what documentation we have is largely oriented towards users, not contributors.

if you feel like you've gained some valuable insights through this process that might be helpful for other new contributors, please please do share, and we'll see if we can turn those into docs, too 😄

avidal added some commits Jul 15, 2017

@avidal avidal force-pushed the avidal:gb-importer branch from f40b53f to 5ac982a Jul 27, 2017

@avidal

This comment has been minimized.

Copy link

avidal commented Jul 27, 2017

Hmm, the appveyor build failed because of an unrelated change. Is that expected?

}
}

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

This comment has been minimized.

@avidal

avidal Jul 27, 2017

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?

This comment has been minimized.

@carolynvs

carolynvs Jul 28, 2017

Collaborator

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"
@carolynvs
Copy link
Collaborator

carolynvs left a comment

We are chugging along! 🚂

}
}

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

This comment has been minimized.

@carolynvs

carolynvs Jul 28, 2017

Collaborator

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"
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

This comment has been minimized.

@carolynvs

carolynvs Jul 28, 2017

Collaborator

The inline doc is very helpful! 👍

package main

import (
cdt "github.com/carolynvs/deptest"

This comment has been minimized.

@carolynvs

carolynvs Jul 28, 2017

Collaborator

You don't have to change what you have here. I just wanted to share another pattern for making tests for dep:

package main

import (
  _ "github.com/carolynvs/deptest"
  _ "github.com/sdboyer/deptest"
)

func main() {}
}
}

func TestGbConfig_Convert_BadInput(t *testing.T) {

This comment has been minimized.

@carolynvs

carolynvs Jul 28, 2017

Collaborator

Would you please split this into two tests? Or single test with subtests whichever you are more comfortable with. One for the empty package name and another for the empty revision.

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

This comment has been minimized.

@carolynvs

carolynvs Jul 28, 2017

Collaborator

These source fields shouldn't be present, because they aren't forks or non-standard repo locations.

It looks like gb always has pkg.Repository set... We need to come up some way to tell when we should set source. 🤔 @sdboyer I assume that gps already has logic that does that but I'm not sure where or it it's exposed.

@carolynvs

This comment has been minimized.

Copy link
Collaborator

carolynvs commented Sep 28, 2017

@avidal Thank for for the head start! @michael-go discovered when he added support for gvt that it also works for gb-vendor. We were able to use some of your logic here in this PR and got support for both merged in #1149.

@carolynvs carolynvs closed this Sep 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment