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

add Gomfile importer #746

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mattn
Copy link
Member

mattn commented Jun 13, 2017

Adds Gomfile importer support to init.

TODO

* add detection for production/test environments

@googlebot googlebot added the cla: yes label Jun 13, 2017

@mattn mattn force-pushed the mattn:gom-importer branch from 572af7c to 41445d7 Jun 13, 2017

@mattn mattn changed the title [WIP] add Gomfile importer [WIP][DO NOT MERGE] add Gomfile importer Jun 13, 2017

@sdboyer sdboyer added the area: init label Jun 13, 2017

@carolynvs carolynvs self-requested a review Jun 13, 2017

lp := gps.NewLockedProject(pi, version, nil)
lock.P = append(lock.P, lp)
}
manifest.Constraints[pc.Ident.ProjectRoot] = gps.ProjectProperties{Source: pc.Ident.Source, Constraint: nil}

This comment has been minimized.

@carolynvs

carolynvs Jun 13, 2017

Collaborator

Did you intend to always leave the constraint in the manifest nil? When the gomfile specifies :tag => '1.0.0', or :branch => 'master' for example, both are valid constraints that can be figured out using deduceConstraint.

@mattn

This comment has been minimized.

Copy link
Member

mattn commented Jun 14, 2017

I consider about production/develop environments for gom. But maybe, all of packages writen in Gomfile should be exported in Gopkg.toml. So I'll remove [WIP] and [DO NOT MERGE].

@mattn mattn changed the title [WIP][DO NOT MERGE] add Gomfile importer add Gomfile importer Jun 14, 2017

@carolynvs
Copy link
Collaborator

carolynvs left a comment

Thanks for adding support for gom! Really excited to get this incorporated. 🎉

lock := &dep.Lock{}

for _, pkg := range g.goms {
// Obtain ProjectRoot. Required for avoiding sub-package imports.

This comment has been minimized.

@carolynvs

carolynvs Jun 16, 2017

Collaborator

Is it valid config to specify a sub-package? For example:

Gomfile

gom 'github.com/davecgh/go-spew/spew', :tag => 'v1.1.0'

If not, then you can remove the logic that detects the project root and check if the project exists in the lock.

This comment has been minimized.

@mattn

mattn Jul 22, 2017

Member

Sorry, I don't clearly understand what you mean. Yes, it's valid entry in Gomfile.

g.logger.Printf(warn.Error())
version = revision
}
feedback(version, pi.ProjectRoot, fb.DepTypeImported, g.logger)

This comment has been minimized.

@carolynvs

carolynvs Jun 16, 2017

Collaborator

You'll need to rebase to get the latest changes to the feedback function. It has been deprecated. Here's an example of the updated feedback usage.

It is new separate calls for logging feedback for a constraint and a locked project, so you'll need to add more feedback after setting the constraint in the manifest.

}
}

func TestGomConfig_ProjectExistsInLock(t *testing.T) {

This comment has been minimized.

@carolynvs

carolynvs Jun 16, 2017

Collaborator

This is a duplicate test of

func TestGodepConfig_ProjectExistsInLock(t *testing.T) {
and can be removed.

}

// Compares two slices of gomPackage and checks if they are equal.
func equalGomImports(a, b []gomPackage) bool {

This comment has been minimized.

@carolynvs

carolynvs Jun 16, 2017

Collaborator

This looks like it's unused?

@@ -0,0 +1,2 @@
gom "github.com/sdboyer/deptest", :commit => "3f4c3bea144e112a69bbe5d8d01c1b09a544253f"

This comment has been minimized.

@carolynvs

carolynvs Jun 16, 2017

Collaborator

I notice that this only has testdata for :commit and not :tag or :branch. Since we also need an integration test for dep init importing a gomfile, see godep for an integration test example, you could include those in that new integration test.

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

It is important that tests for :branch and :tag are added, in addition to an integration test.

@carolynvs carolynvs referenced this pull request Jun 23, 2017

Closed

Alternate init modes #186

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 21, 2017

bump

mattn added some commits Jun 13, 2017

@mattn mattn force-pushed the mattn:gom-importer branch from f31346a to 92dca1a Jul 22, 2017

@mattn mattn requested a review from sdboyer as a code owner Jul 22, 2017

revision := gps.Revision(rev)
version, err := lookupVersionForLockedProject(pi, pc.Constraint, revision, g.sm)
if err != nil {
warn := errors.Wrapf(err, "Unable to lookup the version represented by %s in %s. Falling back to locking the revision only.", rev, pi.ProjectRoot)

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

The warning message, and falling back to the revision is now handled in lookupVersionForLockedProject, so you can just print the err if it's present, and remove the version = revision assignment.

See the godep importer for an example.

@@ -0,0 +1,4 @@
Detected Gomfile...
Converting from Gomfile ...
Trying v0.8.1 (3f4c3be) as initial lock for imported dep github.com/sdboyer/deptest

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

There should be 2 extra lines in the output, looks like it is missing the constraints and is only printing the locked projects.

pc.Ident = pi

if rev != "" {
pc.Constraint, err = g.sm.InferConstraint(rev, pc.Ident)

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

When revision == "", you should set the constraint to gps.Any().

This comment has been minimized.

@mattn

mattn Jul 25, 2017

Member

Ah, I didn't know gps.Any(). Okay, will do it in later.

This comment has been minimized.

@carolynvs

carolynvs Jul 26, 2017

Collaborator

After thinking about this more, I think the right answer is to do that in InferConstraint, so that when an empty string is passed, it returns gps.Any(). I'll submit a PR for that in a bit.

This comment has been minimized.

@carolynvs

carolynvs Jul 26, 2017

Collaborator

I've submitted #901 so that all importers get this for free when calling InferConstraint.

return nil, nil, err
}

revision := gps.Revision(rev)

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

I am not sure what you are doing on this line? This won't be valid when rev is a branch or a tag.

I noticed that you first try to load a gomfile.lock and fallback to a gomfile. Does the gomfile.lock contain just :commit entries and the gomefile contain entries for :branch and :tag? With other dependency managers, like glide, there are two files and we use both. One to import constraints and the other to know which revision to put in the lock.

At a high level what needs to happen here is this:

  1. If a constraint such as a branch or tag is available, it should be used as the constraint in the manifest. Otherwise, use gps.Any(). We don't want to put a revision/commit in the constraint.
  2. Log the constraint using f := fb.NewConstraintFeedback(pc, fb.DepTypeImported).LogFeedback(g.logger).
  3. Call lookupVersionForLockedProject passing in the project, the constraint from the first step and the revision (:commit) from the lock.
  4. Make a locked project and log it.
@@ -0,0 +1,2 @@
gom "github.com/sdboyer/deptest", :commit => "3f4c3bea144e112a69bbe5d8d01c1b09a544253f"

This comment has been minimized.

@carolynvs

carolynvs Jul 25, 2017

Collaborator

It is important that tests for :branch and :tag are added, in addition to an integration test.

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