Add -gopath flag to init #497

darkowlzz · 2017-05-01T19:53:44Z

Separate network mode and GOPATH mode of operations.
In GOPATH mode, fetch projectData by searching through the current
GOPATH. Solve the dependency constraints based on the projects found on
disk and use network to solve for projects not found on disk.
In Network mode, do not check GOPATH or any ondisk projects. Solve the
dependency constraints completely over network.

Fixes #496

darkowlzz · 2017-05-01T19:58:25Z

This is implementation of the 2nd approach mentioned in #496 . Looking at code might help in better understanding :)

Integration tests are failing due to change in behavior (Network mode by default). Will fix them once we have a properly discussed fix.

darkowlzz · 2017-05-01T20:50:23Z

Also, should dep tell the user about the execution mode, or how it's solving the dependency constraints?
Something like:

$ dep init -gopath
......
Searching GOPATH to solve dependencies
......

sdboyer · 2017-05-02T00:28:04Z

This is implementation of the 2nd approach mentioned in #496 .

As noted over there, I think this is the right approach to start with 😄

Integration tests are failing due to change in behavior (Network mode by default). Will fix them once we have a properly discussed fix.

👍

Also, should dep tell the user about the execution mode, or how it's solving the dependency constraints?

Definitely. I'd go a step further, actually - I'll update the spec doc with some things to this effect.

sdboyer

Looks like you're headed in a good direction 😄

sdboyer · 2017-05-02T00:33:37Z

cmd/dep/init.go

-		P: make([]gps.LockedProject, 0, len(pd.ondisk)),
-	}
+	if cmd.gopath {
+		pd, err = getProjectData(ctx, pkgT, cpr, sm)


The implementation of getProjectData() is likely to need a fair bit of modification, as it should really only be filling in holes in the graph. I suspect that's going to result in nontrivial changes to that graph traversal algorithm.

I thought I left a reply here 🤔 maybe my machine got disconnected and it wasn't saved.

So here again, would be better to create a dedicated issue to discuss how we modify getProjectData() 😬. Would be great if you could add some more details about the ideas you have in making it better. The function is huge and a lot of stuff seems to be happening in it. 😅

For sure, it's a rabbit hole. Separate issue makes sense.

sdboyer · 2017-05-02T00:35:02Z

cmd/dep/init.go

-		)
+		for pr, v := range pd.ondisk {
+			// That we have to chop off these path prefixes is a symptom of
+			// a problem in gps itself


This comment might actually be out of date, I think I changed subpackage naming a while back. It wouldn't be a terrible thing if you checked on it while you were poking around in this 😄

yes, I came across this while working on the previous init PR. I observed that the Lock obtained after a solve contains LockedProject with proper list of packages required from the project, no chopping required. But I couldn't figure out how to create LockedProject's package list using gps, to avoiding the above chopping code block.

Can you create a new issue with some more details?

But I couldn't figure out how to create LockedProject's package list using gps, to avoiding the above chopping code block.

Yeah there's a family of functions I've been considering adding for this, but right now everything's waiting behind getting gps moved into dep.

Can you create a new issue with some more details?

Sure, but...which part? Just the whole thing?

No, for now just that chopping code block part. With little details about how to create LockedProject with proper list of packages using gps, to replace the outdated comment and code.

With little details about how to create LockedProject with proper list of packages using gps

Well, this is kinda what I'm getting at - the "properness" of the package list in LockedProject's isn't separable from a full Solve(). NewLockedProject() will let you create instances as needed, but being proper/correct is only defined in the context of an overall dependency set, computed by solving.

So, is the question here about simply creating instances, or creating correct ones?

Yeah, from what I have seen, a full solve is required to get a proper package list. I was wondering if there's a way to partially run solve (or some gps function) and get the same package list that we need to create locks.

Nope, unfortunately, the problem can't be partitioned (in fact, it's NP-complete). It's all-or-nothing; hence why gps.Solution is a superset of gps.Lock.

darkowlzz · 2017-05-02T21:35:59Z

Added a single line message when -gopath mode is executed.
Fixed breaking tests.
Added a new test for the default network mode.

Will implement itemized feedback in another PR. Would create another issue for the same.

darkowlzz · 2017-05-02T21:52:09Z

Also, should we explicitly print the default network mode, "Using network for projects...", kinda message?
IMO, since it's default, it's not necessary. Only when we go away from default, like passing -gopath, we should inform the user about it.

sdboyer · 2017-05-02T23:42:30Z

IMO, since it's default, it's not necessary. Only when we go away from default, like passing -gopath, we should inform the user about it.

👍 👍 👍

sdboyer · 2017-05-03T03:45:46Z

cmd/dep/init.go

-		P: make([]gps.LockedProject, 0, len(pd.ondisk)),
-	}
+	if cmd.gopath {
+		fmt.Fprintln(os.Stderr, "Searching GOPATH for projects...")


Let's use internal.Logf() here

oh! didn't see that existed. Found fmt.Fprintln() being used in main.go. So used the same 😅

oh...yeah, hah, i think that's older stuff there. it should be updated 😄

darkowlzz · 2017-05-04T01:50:14Z

@sdboyer are we waiting for some changes here? 😅 Just making sure no change requests are pending. You can take your time :)

sdboyer · 2017-05-04T02:15:53Z

@darkowlzz sorry, I meant to ask you the same 😄

We do need the general CLI helptext to be updated. We're in a transitional state with this, so it needn't be flawless, but we can't mislead users about behavior.

darkowlzz · 2017-05-04T02:52:17Z

@sdboyer oops! forgot about that 😅 Updated!

sdboyer · 2017-05-13T19:40:59Z

OK, needs just a little catchup after I merged #512 - then I can pull it into the feature branch that I mentioned before 😄

darkowlzz · 2017-05-13T19:53:32Z

@sdboyer yep, already working on it.

darkowlzz · 2017-05-13T21:17:01Z

@sdboyer squashed, rebased and adjusted to work with current code.

sdboyer · 2017-05-14T13:10:42Z

awesome. it looks like i should be able to get the feature branch up this afternoon. thanks!

sdboyer · 2017-06-06T01:15:28Z

oh, we're totally ready for this now! this is in @carolynvs's domain, so marking her for review 😄

carolynvs

Sorry, so much of the logic that you are building on has moved that a rebase is needed before I can review further.

carolynvs · 2017-06-06T22:45:47Z

cmd/dep/init.go


 - 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, version of


In this mode, the version of...

carolynvs · 2017-06-06T22:50:15Z

cmd/dep/init.go

-	l := &dep.Lock{
-		P: make([]gps.LockedProject, 0, len(pd.ondisk)),
-	}
+	if cmd.gopath {


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!

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

darkowlzz · 2017-06-07T13:42:28Z

Rebased and made changes to work with new code.

I feel the need to have some kind of loading animation or as simple as incremental dots (....) because in network mode, unlike in gopath mode, there's no intermediate output. Feedback constraints are printed only after a solution is obtained.

darkowlzz · 2017-06-07T14:45:50Z

cmd/dep/testdata/harness_tests/init/case4/final/Gopkg.toml

+
+[[constraint]]
+  name = "github.com/sdboyer/deptest"
+  version = "1.0.0"


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.

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.

carolynvs · 2017-06-07T14:59:22Z

cmd/dep/init.go

+	if cmd.gopath {
+		gs.FinalizeRootManifestAndLock(m, l)
+	} else {
+		// Pick the direct dependencies from the above solution and add to manifest


Can you help me understand why you added this else clause? It looks like the old code from the gopath scanning but in this case is being applied to the results of importing config from an external tool.

IIRC, this was done to generate the final lock memo with the obtained solution.
When there's no external tool and -gopath is not passed, we use an empty manifest in the solver to get a solution. Then we use the lock obtained from the solution and pick direct dependencies from there and create a new manifest, using which we generate the final lock memo.

Now, in case of an external tool, our initial manifest may or may not be empty. So we use the solution lock to fill up the manifest with direct dependencies. In case, manifest was empty, it gets filled. And in case, manifest already had packages, there wouldn't be any change. Or if there were any missing direct dependencies in manifest, they get added too.

Hope this answers your question.

@carolynvs I just figured out why you asked this question. Went through the epic #186 and came across

ignore all the existing local information - GOPATH, existing tools, everything - and just have the solver pick out the latest versions it can.

😅 which is totally different from what's being done here. Lemme change that.

To be honest I'm not entirely sure what's the right approach. 😊 I was just asking because I was confused.

What I am pretty sure about is:

The logic should be in rootAnalyzer.FinalizeManifestAndLock instead of the init command directly.

When someone runs dep init -skip-tools -gopath it shouldn't result in a populated lock and an empty manifest.

The logic should be in rootAnalyzer.FinalizeManifestAndLock instead of the init command directly.

Yeah, I felt I made init dirty. It became elegant with all the new stuff and with all these changes, it felt dirty 😅 I'll change that.

When someone runs dep init -skip-tools -gopath it shouldn't result in a populated lock and an empty manifest.

We have Integration/init/case3 to handle this scenarios 😊

darkowlzz · 2017-06-10T12:17:28Z

Made rootAnalyzer and gopathScanner -gopath flag aware and skip usual operations when in network mode.

Much cleaner now :)

carolynvs

Apologies if my comments sent you off on the wrong path... 😊

This is super close, and should be ready to merge after shuffling things back a bit to the way they were initially.

carolynvs · 2017-06-11T13:29:47Z

cmd/dep/root_analyzer.go

 		ctx:        ctx,
 		sm:         sm,
 		directDeps: directDeps,
 	}
 }

 func (a *rootAnalyzer) InitializeRootManifestAndLock(dir string, pr gps.ProjectRoot) (rootM *dep.Manifest, rootL *dep.Lock, err error) {
-	if !a.skipTools {
+	// Use importer tools only in gopath mode
+	if a.gopath && !a.skipTools {


This doesn't match the behavior outlined in the updated command spec

If -gopath is passed, then when the legacy tool import is complete (or skipped, in the event that -skip-tools is also passed), an additional search pass should be made through the current (as inferred from the cwd) GOPATH

As I read it, -gopath and -skip-tools are completely independent.

I added a comment discussing this in the google doc: dep: updated command spec.

carolynvs · 2017-06-11T13:33:48Z

cmd/dep/root_analyzer.go

-			if pr == y.Ident().ProjectRoot {
-				used = true
-				break
+	if a.gopath {


I think both paths of this if/else are safe to run unconditionally. If a direct dep was added during solve, either because the imported config was incomplete or due to "network solving", then it's appropriate to add it to the manifest. Same for removing unused dependencies from the manifest.

If we stop using the a.gopath in both functions, we can remove it entirely from the struct.

We need some way to make root_analyzer aware of -gopath flag. If else part of this is kept here itself, in root_analyzer, it would execute the feedback code. So, in gopath mode, feedbacks would be logged twice.

carolynvs · 2017-06-11T13:36:39Z

cmd/dep/gopath_scanner.go


 	pd    projectData
 	origM *dep.Manifest
 	origL *dep.Lock
 }

-func newGopathScanner(ctx *dep.Ctx, directDeps map[string]bool, sm gps.SourceManager) *gopathScanner {
+func newGopathScanner(ctx *dep.Ctx, directDeps map[string]bool, sm gps.SourceManager, gopath bool) *gopathScanner {


I think this went a bit too far in trying to keep init "clean". 😀 Let's have the init command handle the -gopath flag and call gopathScanner or not based on its value.

darkowlzz · 2017-06-15T14:24:39Z

cmd/dep/root_analyzer.go

+			f = fb.NewLockedProjectFeedback(y, fb.DepTypeTransitive)
+		}
+		f.LogFeedback(a.ctx.Err)
+	}


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 🤔

@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. 😁

rootAnalyzer.importManifestAndLock

Import constraints and locked projects from external config.

gopathScanner.InitializeManifestAndLock

Add constraints and locked projects from GOPATH.

SOLVE

Uses dark magic to add more goodies to the lock.

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.

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?

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 🙌

darkowlzz · 2017-06-17T14:12:46Z

cmd/dep/init.go

@@ -164,8 +175,8 @@ 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.RemoveTransitiveDependencies(p.Manifest)


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

- Removes gopathScanner's FinalizeManifestAndLock() and its usage. - Adds using removeTransitiveDependencies() to remove unused constraints. - Changes rootAnalyzer's FinalizeManifestAndLock to log feedback for new constraints and locked projects only.

darkowlzz · 2017-06-17T14:35:29Z

cmd/dep/root_analyzer.go

-				used = true
-				break
+func (a *rootAnalyzer) FinalizeRootManifestAndLock(m *dep.Manifest, l *dep.Lock, ol dep.Lock) {
+	a.removeTransitiveDependencies(m)


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

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.

carolynvs · 2017-06-17T16:12:13Z

cmd/dep/root_analyzer.go

+					pc := gps.ProjectConstraint{Ident: y.Ident(), Constraint: pp.Constraint}
+					f = fb.NewConstraintFeedback(pc, fb.DepTypeDirect)
+				} else {
+					f = fb.NewLockedProjectFeedback(y, fb.DepTypeDirect)


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.

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

- Removes `removeTransitiveDependencies()` from `FinalizeRootManifestAndLock`. - Moves locked project feedback for new direct deps out of conditional clause.

carolynvs · 2017-06-17T20:10:38Z

I opened #763 for one of the failed tests, I've seen it randomly fail a few times. Looks good after restarting the build!

carolynvs · 2017-06-17T20:12:31Z

Thank you for staying on top of this and your patience with the uncertain expected behaviors! 💖

googlebot added the cla: yes label May 1, 2017

sdboyer reviewed May 2, 2017

View reviewed changes

darkowlzz force-pushed the 496-gopath-flag-init branch from d3c8160 to c4e11d0 Compare May 2, 2017 21:23

sdboyer reviewed May 3, 2017

View reviewed changes

darkowlzz force-pushed the 496-gopath-flag-init branch from c4e11d0 to 347af65 Compare May 3, 2017 03:46

darkowlzz force-pushed the 496-gopath-flag-init branch from 15e4e54 to 588f47e Compare May 4, 2017 02:50

sdboyer mentioned this pull request May 5, 2017

Add itemized feedback for dep resolution in init #512

Merged

darkowlzz force-pushed the 496-gopath-flag-init branch 2 times, most recently from 3b22acb to 396dc2a Compare May 13, 2017 20:55

darkowlzz mentioned this pull request May 28, 2017

init print not in GOPATH only when notondisk > 0 #668

Merged

sdboyer requested a review from carolynvs June 6, 2017 01:15

carolynvs suggested changes Jun 6, 2017

View reviewed changes

darkowlzz force-pushed the 496-gopath-flag-init branch 2 times, most recently from 537b95b to 8a82838 Compare June 7, 2017 13:30

darkowlzz commented Jun 7, 2017

View reviewed changes

carolynvs reviewed Jun 7, 2017

View reviewed changes

darkowlzz force-pushed the 496-gopath-flag-init branch 2 times, most recently from 3fdd31c to b858322 Compare June 10, 2017 12:13

carolynvs suggested changes Jun 11, 2017

View reviewed changes

darkowlzz force-pushed the 496-gopath-flag-init branch from b858322 to 8915341 Compare June 15, 2017 13:44

darkowlzz commented Jun 15, 2017

View reviewed changes

carolynvs added the area: init label Jun 16, 2017

Add -gopath flag to init

b674913

darkowlzz force-pushed the 496-gopath-flag-init branch 2 times, most recently from d56d41b to 0821c76 Compare June 17, 2017 14:08

darkowlzz commented Jun 17, 2017

View reviewed changes

darkowlzz force-pushed the 496-gopath-flag-init branch from 0821c76 to 6ace212 Compare June 17, 2017 14:32

Generalize FinalizeManifestAndLock

3eb07c3

- Removes gopathScanner's FinalizeManifestAndLock() and its usage. - Adds using removeTransitiveDependencies() to remove unused constraints. - Changes rootAnalyzer's FinalizeManifestAndLock to log feedback for new constraints and locked projects only.

darkowlzz force-pushed the 496-gopath-flag-init branch from 6ace212 to 3eb07c3 Compare June 17, 2017 14:34

darkowlzz commented Jun 17, 2017

View reviewed changes

carolynvs reviewed Jun 17, 2017

View reviewed changes

Print locked project feedback for all new dir deps

3c34e8f

- Removes `removeTransitiveDependencies()` from `FinalizeRootManifestAndLock`. - Moves locked project feedback for new direct deps out of conditional clause.

carolynvs approved these changes Jun 17, 2017

View reviewed changes

carolynvs merged commit 52acb7e into golang:master Jun 17, 2017

Add -gopath flag to init #497

Add -gopath flag to init #497

Conversation

darkowlzz commented May 1, 2017

darkowlzz commented May 1, 2017

darkowlzz commented May 1, 2017

sdboyer commented May 2, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented May 2, 2017

darkowlzz commented May 2, 2017

sdboyer commented May 2, 2017

sdboyer May 3, 2017 • edited Loading

Choose a reason for hiding this comment

darkowlzz May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented May 4, 2017

sdboyer commented May 4, 2017

darkowlzz commented May 4, 2017

sdboyer commented May 13, 2017

darkowlzz commented May 13, 2017

darkowlzz commented May 13, 2017

sdboyer commented May 14, 2017

sdboyer commented Jun 6, 2017

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Jun 7, 2017 • edited Loading

darkowlzz Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz commented Jun 10, 2017

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs commented Jun 17, 2017

carolynvs commented Jun 17, 2017

darkowlzz May 2, 2017 •

edited

Loading

sdboyer May 3, 2017 •

edited

Loading

darkowlzz May 3, 2017 •

edited

Loading

darkowlzz commented Jun 7, 2017 •

edited

Loading

darkowlzz Jun 7, 2017 •

edited

Loading

darkowlzz Jun 15, 2017 •

edited

Loading

darkowlzz Jun 17, 2017 •

edited

Loading