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

Add -gopath flag to init #497

Merged
merged 3 commits into from
Jun 17, 2017
Merged

Add -gopath flag to init #497

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b674913
Add -gopath flag to init
darkowlzz Jun 7, 2017
3eb07c3
Generalize FinalizeManifestAndLock
darkowlzz Jun 17, 2017
3c34e8f
Print locked project feedback for all new dir deps
darkowlzz Jun 17, 2017
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
21 changes: 0 additions & 21 deletions cmd/dep/gopath_scanner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,27 +134,6 @@ func (g *gopathScanner) overlay(rootM *dep.Manifest, rootL *dep.Lock) {
}
}

func (g *gopathScanner) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock) {
// Iterate through the new projects in solved lock and add them to manifest
// if direct deps and log feedback for all the new projects.
for _, x := range l.Projects() {
pr := x.Ident().ProjectRoot
newProject := true
// Check if it's a new project, not in the old lock
for _, y := range g.origL.Projects() {
if pr == y.Ident().ProjectRoot {
newProject = false
}
}
if newProject {
// If it's in notondisk, add to manifest, these are direct dependencies.
if _, ok := g.pd.notondisk[pr]; ok {
m.Constraints[pr] = getProjectPropertiesFromVersion(x.Version())
}
}
}
}

func trimPathPrefix(p1, p2 string) string {
if fs.HasFilepathPrefix(p1, p2) {
return p1[len(p2):]
Expand Down
28 changes: 19 additions & 9 deletions cmd/dep/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,19 @@ disable this behavior. The following external tools are supported: glide.
Any dependencies that are not constrained by external configuration use the
GOPATH analysis below.

The version of each dependency will reflect the current state of the GOPATH. If
a dependency doesn't exist in the GOPATH, a version will be selected from the
versions available from the upstream source per the following algorithm:
By default, the dependencies are resolved over the network. A version will be
selected from the versions available from the upstream source per the following
algorithm:

- Tags conforming to semver (sorted by semver rules)
- Default branch(es) (sorted lexicographically)
- Non-semver tags (sorted lexicographically)

An alternate mode can be activated by passing -gopath. In this mode, the version
of each dependency will reflect the current state of the GOPATH. If a dependency
doesn't exist in the GOPATH, a version will be selected based on the above
network version selection algorithm.

A Gopkg.toml file will be written with inferred version constraints for all
direct dependencies. Gopkg.lock will be written with precise versions, and
vendor/ will be populated with the precise versions written to Gopkg.lock.
Expand All @@ -52,11 +57,13 @@ func (cmd *initCommand) Hidden() bool { return false }
func (cmd *initCommand) Register(fs *flag.FlagSet) {
fs.BoolVar(&cmd.noExamples, "no-examples", false, "don't include example in Gopkg.toml")
fs.BoolVar(&cmd.skipTools, "skip-tools", false, "skip importing configuration from other dependency managers")
fs.BoolVar(&cmd.gopath, "gopath", false, "search in GOPATH for dependencies")
}

type initCommand struct {
noExamples bool
skipTools bool
gopath bool
}

func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
Expand Down Expand Up @@ -132,13 +139,17 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
if err != nil {
return err
}
gs := newGopathScanner(ctx, directDeps, sm)
err = gs.InitializeRootManifestAndLock(p.Manifest, p.Lock)
if err != nil {
return err

if cmd.gopath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all this logic moved out of init.go into gopath_scanner.go, would you please rebase and move over your changes too? Sorry for the merge trouble!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's where the various bits are in gopath_scanner:

  • Scan to generate project data -> gopathScanner.scanGopathForDependencies
  • Populate a manifest using the project data -> gopathScanner.InitializeRootManifestAndLock
  • Combine the results of the importer with what we found in GOPATH, printing feedback for what was used (things from the importers win) -> gopathScanner.overlay
  • Tweak the lock based on the results of solving -> gopathScanner.FinalizeRootManifestAndLock

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks :)

gs := newGopathScanner(ctx, directDeps, sm)
err = gs.InitializeRootManifestAndLock(p.Manifest, p.Lock)
if err != nil {
return err
}
}

rootAnalyzer.skipTools = true // Don't import external config during solve for now
copyLock := *p.Lock // Copy lock before solving. Use this to separate new lock projects from solved lock

params := gps.SolveParameters{
RootDir: root,
Expand All @@ -164,8 +175,7 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
}
p.Lock = dep.LockFromSolution(soln)

rootAnalyzer.FinalizeRootManifestAndLock(p.Manifest, p.Lock)
gs.FinalizeRootManifestAndLock(p.Manifest, p.Lock)
rootAnalyzer.FinalizeRootManifestAndLock(p.Manifest, p.Lock, copyLock)

// Run gps.Prepare with appropriate constraint solutions from solve run
// to generate the final lock memo.
Expand Down
44 changes: 33 additions & 11 deletions cmd/dep/root_analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"log"

"github.com/golang/dep"
fb "github.com/golang/dep/internal/feedback"
"github.com/golang/dep/internal/gps"
"github.com/pkg/errors"
)
Expand Down Expand Up @@ -125,18 +126,39 @@ func (a *rootAnalyzer) DeriveManifestAndLock(dir string, pr gps.ProjectRoot) (gp
return gps.SimpleManifest{}, nil, nil
}

func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock) {
// Remove dependencies from the manifest that aren't used
for pr := range m.Constraints {
var used bool
for _, y := range l.Projects() {
if pr == y.Ident().ProjectRoot {
used = true
break
func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, ol dep.Lock) {
a.removeTransitiveDependencies(m)
Copy link
Collaborator Author

@darkowlzz darkowlzz Jun 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@carolynvs I used removeTransitiveDependencies() in this PR itself. Hopefully this is how you intended to use it 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK only the importers may add a transitive or unused dep to the manifest, because they are just responsible for converting from the external config to dep's. In rootAnalyzer.importManifestAndLock we are calling removeTransitiveDependencies after the importer is run, to clean up after them.

So unless someplace in the code else may be adding things to the manifest that may not belong, I don't think we need to call it again in FinalizeManifestAndLock.


// Iterate through the new projects in solved lock and add them to manifest
// if they are direct deps and log feedback for all the new projects.
for _, y := range l.Projects() {
var f *fb.ConstraintFeedback
pr := y.Ident().ProjectRoot
// New constraints: in new lock and dir dep but not in manifest
if _, ok := a.directDeps[string(pr)]; ok {
if _, ok := m.Constraints[pr]; !ok {
pp := getProjectPropertiesFromVersion(y.Version())
if pp.Constraint != nil {
m.Constraints[pr] = pp
pc := gps.ProjectConstraint{Ident: y.Ident(), Constraint: pp.Constraint}
f = fb.NewConstraintFeedback(pc, fb.DepTypeDirect)
} else {
f = fb.NewLockedProjectFeedback(y, fb.DepTypeDirect)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When I split up the old feedback function into the constructors, a single call no longer does "double duty". So NewConstraintFeedback will only generate a using entry and NewLockedProjectFeedback will only generate a locking entry.

What we have right here will only print either a using or a locking, but never both. Was that the intent? I expected that if we can deduce a constraint, that we could want to print the constraint and the locked project. If you agree, I think we need to add a LogFeedback call after NewConstraintFeedback and then unconditionally call NewLockedProjectFeedback and LogFeedback instead of putting it in this else clause.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh! yeah, that's right. Didn't realize that got changed 😅 no need of the conditional else.

}
f.LogFeedback(a.ctx.Err)
}
} else {
// New locked projects: in new lock but not in old lock
newProject := true
for _, opl := range ol.Projects() {
if pr == opl.Ident().ProjectRoot {
newProject = false
}
}
if newProject {
f = fb.NewLockedProjectFeedback(y, fb.DepTypeTransitive)
f.LogFeedback(a.ctx.Err)
}
}
if !used {
delete(m.Constraints, pr)
}
}
Copy link
Collaborator Author

@darkowlzz darkowlzz Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Problem with this is that, since rootAnalyzer isn't aware of network/gopath mode of execution, even in gopath mode, these feedbacks would run. Resulting in double feedbacks.

Should this be moved to init? or should we make at least rootAnalyzer aware of gopath flag? or maybe there's another way 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

@darkowlzz I wrote out the workflow for init to help answer this question and decided to dump my notes, instead of just the one part that addresses the question. 😁

  1. rootAnalyzer.importManifestAndLock
    • Import constraints and locked projects from external config.
  2. gopathScanner.InitializeManifestAndLock
    • Add constraints and locked projects from GOPATH.
  3. SOLVE
    • Uses dark magic to add more goodies to the lock.
  4. rootAnalyzer.FinalizeManifestAndLock
    • Remove unused constraints. I'll remove this in a separate PR. I just realized that it's not needed anymore thanks to rootAnalyzer.removeTransitiveDependencies.
    • This PR adds printing feedback for direct deps found during solve.
    • This PR adds printing feedback for transitive deps locked during solve.
  5. gopathScanner.FinalizeManifestAndLock
    • Add constraints found during solve that weren't initially in the manifest
      because they weren't on disk.

I believe that we can generalize the FinalizeManifestAndLock activities to "Identify new constraints and locked projects found during solve". New constraints are direct deps that are in the final lock but not the manifest, and new locked projects are identified by keeping track of the initial lock. The same finalization tasks can be performed of how we got to this state:

  • -gopath wasn't specified and nothing was imported, 100% network solve.
  • The GOPATH didn't have repos for all of the direct deps.
  • The imported config was incomplete and didn't have constraints for all of the direct deps.

Then we could have init keep a copy of the lock given to the solver, and pass it in as an additional argument to rootAnalyzer.FinalizeManifestAndLock. Now all that logic is in one place and we can also remove gopathScanner.FinalizeManifestAndLock.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I remember we used to keep a copy of lock given to the solver and use the same to identify new constraints and projects, by comparing with new lock, at one point of time. I still have my dep fork at that point of time, copyLock 😊

So, yeah, a unified FinalizeManifestAndLock is what we need 🙌

}
Expand Down
2 changes: 1 addition & 1 deletion cmd/dep/testdata/harness_tests/init/case1/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"commands": [
["init", "-no-examples", "-skip-tools"]
["init", "-no-examples", "-skip-tools", "-gopath"]
],
"error-expected": "",
"gopath-initial": {
Expand Down
2 changes: 1 addition & 1 deletion cmd/dep/testdata/harness_tests/init/case2/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"commands": [
["init", "-no-examples", "-skip-tools"]
["init", "-no-examples", "-skip-tools", "-gopath"]
],
"error-expected": "",
"gopath-initial": {
Expand Down
2 changes: 1 addition & 1 deletion cmd/dep/testdata/harness_tests/init/case3/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"commands": [
["init", "-no-examples", "-skip-tools"]
["init", "-no-examples", "-skip-tools", "-gopath"]
],
"error-expected": "",
"gopath-initial": {
Expand Down
21 changes: 21 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case4/final/Gopkg.lock
View file
Open in desktop

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

8 changes: 8 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case4/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

[[constraint]]
name = "github.com/sdboyer/deptest"
version = "1.0.0"
Copy link
Collaborator Author

@darkowlzz darkowlzz Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Previously, this used to be with a ^. Now, I think because of ImpliedCaretString, versions are written as exact. Is it intended?
Also, the feedback that we print for network mode, prints with ^ right now. So there's a mismatch between what the feedback shows version and what's written in manifest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, when init generates the manifest it should not write out the ^. When the manifest is read, the caret is automatically added. Here's some backstory on why we don't include the caret in the manifest.

My preference is for the verbose output to print what's really going on and show the caret, but will leave that call to @sdboyer. If we do end up changing that, let's do it in a separate PR.


[[constraint]]
name = "github.com/sdboyer/deptestdos"
version = "2.0.0"
13 changes: 13 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case4/initial/foo/bar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// 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 foo

import "github.com/sdboyer/deptest"

func Foo() deptest.Foo {
var y deptest.Foo

return y
}
19 changes: 19 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case4/initial/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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/golang/notexist/foo"
"github.com/sdboyer/deptestdos"
)

func main() {
var x deptestdos.Bar
y := foo.FooFunc()

fmt.Println(x, y)
}
12 changes: 12 additions & 0 deletions cmd/dep/testdata/harness_tests/init/case4/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"commands": [
["init", "-no-examples"]
],
"gopath-initial": {
"github.com/sdboyer/deptestdos": "a0196baa11ea047dd65037287451d36b861b00ea"
},
"vendor-final": [
"github.com/sdboyer/deptest",
"github.com/sdboyer/deptestdos"
]
}
2 changes: 1 addition & 1 deletion cmd/dep/testdata/harness_tests/init/glide/case1/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"commands": [
["init", "-no-examples"]
["init", "-no-examples", "-gopath"]
],
"error-expected": "",
"gopath-initial": {
Expand Down
2 changes: 1 addition & 1 deletion cmd/dep/testdata/harness_tests/init/glide/case2/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"commands": [
["init", "-no-examples", "-skip-tools"]
["init", "-no-examples", "-skip-tools", "-gopath"]
],
"error-expected": "",
"gopath-initial": {
Expand Down