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 itemized feedback for dep resolution in init #512

Merged
merged 1 commit into from May 13, 2017

Conversation

Projects
None yet
3 participants
@darkowlzz
Copy link
Collaborator

darkowlzz commented May 3, 2017

  • Compares manifest, lock and packageTree to separate direct and transitive
    dependencies. Based on the constraints, logs them as feedback.
  • Creates ConstraintFeedback struct to hold dep constraint data.
  • Adds GetUsingFeedback() and GetLockingFeedback() functions to generate
    feedback text.

Fixes #503

@googlebot googlebot added the cla: yes label May 3, 2017

// 3. if not exists in manifest, check in pkgT
// 3.a. if exists in pkgT, it's direct dep, hint constraint, lock only
// 3.b. if not exists in pkgT, it's transitive dep, lock only
// 4. log feedback

This comment has been minimized.

@darkowlzz

darkowlzz May 3, 2017

Collaborator

Is this a good algo to separate direct and transitive deps? Any other simpler solution?

This comment has been minimized.

@sdboyer

sdboyer May 4, 2017

Member

Imports are the only thing responsible for deciding what's direct vs. transitive - not the manifest. (Well, required packages in the manifest have the same effect, but it's not possible for there to be any here, so that's a wash). So, I think you can just drop 1 & 2 and go right to 3.

3.a. isn't quite on the mark either, but I think that what I wrote in the spec doc might be clearer with 1 & 2 out of the way? if not, i can clarify further 😄

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 3, 2017

  • Added the feedback implementation in internal/internal.go for now. Not sure if it should be moved to a separate internal/ file or combined with some existing module ?

Output of init with this change:

$ dep init -no-examples
Cached golang.org/x/sys
Cached github.com/darkowlzz/openurl
Cached github.com/fsnotify/fsnotify
dep: Locking in master (ca83bd2cb9abb47839b50eb4da612f00158f5870) for transitive dep golang.org/x/sys
dep: Using master as constraint for direct dep github.com/darkowlzz/openurl
dep: Locking in master (673a04d14da017f965f0b97f87ab369ec4a1fd08) for direct dep github.com/darkowlzz/openurl
dep: Using master as constraint for direct dep github.com/fsnotify/fsnotify
dep: Locking in master (fd9ec7deca8bf46ecd2a795baaacf2b3a9be1197) for direct dep github.com/fsnotify/fsnotify
  • The revision is in its long form. Would the default git short SHA length (7) be good enough? Shorten with rev[0:7].
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 4, 2017

Cached golang.org/x/sys
Cached github.com/darkowlzz/openurl
Cached github.com/fsnotify/fsnotify

Let's also get rid of this feedback as part of the PR. We added it initially to provide some feedback to the user while work was being done, but the feedback we're adding here is much better.

The revision is in its long form. Would the default git short SHA length (7) be good enough? Shorten with rev[0:7].

Generally yes, but we can't actually rely on only having git SHA1s - there's also hg, bzr, svn, and whatever else we come up with in the future.

hg also has SHA1 revs, but svn has simple integers, and bzr has a weird thing. I'd say that it's fine to trim if you can positively identify something shaped like a SHA1, but in the other cases, leave it as-is. deduceConstraint() in ensure.go has some logic you could follow on that.

cf.DependencyType = internal.DepTypeDirect
cf.ConstraintType = internal.ConsTypeConstraint
} else {
if _, ok := pkgT.Packages[string(projectPath)]; ok {

This comment has been minimized.

@sdboyer

sdboyer May 4, 2017

Member

The Packages map isn't quite what you're looking for - it's the list of packages within the current project, not the imports of those packages. What you want is a bit more elaborate. I've got code that does it over in #489 - https://github.com/sdboyer/dep/blob/1b8edb3436f39dbc29b4fae81dc1d51b2e77a303/cmd/dep/ensure.go#L390-L409 .

// 3. if not exists in manifest, check in pkgT
// 3.a. if exists in pkgT, it's direct dep, hint constraint, lock only
// 3.b. if not exists in pkgT, it's transitive dep, lock only
// 4. log feedback

This comment has been minimized.

@sdboyer

sdboyer May 4, 2017

Member

Imports are the only thing responsible for deciding what's direct vs. transitive - not the manifest. (Well, required packages in the manifest have the same effect, but it's not possible for there to be any here, so that's a wash). So, I think you can just drop 1 & 2 and go right to 3.

3.a. isn't quite on the mark either, but I think that what I wrote in the spec doc might be clearer with 1 & 2 out of the way? if not, i can clarify further 😄

@sdboyer
Copy link
Member

sdboyer left a comment

just a few things

@darkowlzz darkowlzz force-pushed the darkowlzz:503-itemize-init-feedback branch 2 times, most recently from 7676985 to 7222809 May 4, 2017


// Get manifest version if available
if pp, ok := m.Dependencies[lock.Ident().ProjectRoot]; ok {
cf.Version = pp.Constraint.String()

This comment has been minimized.

@darkowlzz

darkowlzz May 4, 2017

Collaborator

@sdboyer any idea what's wrong here?
Integration tests for init case 1 and 3 are failing due to this line. If this assignment is removed, tests pass.
Is there any way to view the errors? go test -v doesn't show the actual error, same as logs in appveyor and travis.

This comment has been minimized.

@sdboyer

sdboyer May 5, 2017

Member

I'm not sure. And unfortunately, I don't think there's a way to see those errors. Would be a great thing to open an issue for 😄

This comment has been minimized.

@darkowlzz

darkowlzz May 5, 2017

Collaborator

PR #523 for the same.

// 2.b.i. if it has a version attached, constraint dep
// 2.b.ii. if it has revision only, hint dep
// 2.c. else it's a transitive dep
// 2.d. log feedback

This comment has been minimized.

@darkowlzz

darkowlzz May 4, 2017

Collaborator

Correct? 😅

This comment has been minimized.

@sdboyer

sdboyer May 5, 2017

Member

i think so, apart from those two nits

@darkowlzz darkowlzz force-pushed the darkowlzz:503-itemize-init-feedback branch from 7222809 to a2ee5e2 May 4, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 4, 2017

  • Changed the algo.
  • Check revision to for valid SHA1 digest and trim to 7 characters.
  • Remove "Cached" message.

This is how the feedback looks now:

$ dep init -no-examples
dep: Locking in v1.0.0 (ff2948a) for transitive dep github.com/sdboyer/deptest
dep: Using master as constraint for direct dep github.com/fsnotify/fsnotify
dep: Locking in master (fd9ec7d) for direct dep github.com/fsnotify/fsnotify
dep: Locking in master (ca83bd2) for transitive dep golang.org/x/sys
dep: Using ^2.0.0 as constraint for direct dep github.com/sdboyer/deptestdos
dep: Locking in v2.0.0 (5c60720) for direct dep github.com/sdboyer/deptestdos
dep: Using master as constraint for direct dep github.com/darkowlzz/openurl
dep: Locking in master (673a04d) for direct dep github.com/darkowlzz/openurl
// 2.b. if it's a direct dep:
// 2.b.i. if it has a version attached, constraint dep
// 2.b.ii. if it has revision only, hint dep
// 2.c. else it's a transitive dep

This comment has been minimized.

@sdboyer

sdboyer May 5, 2017

Member

should be explicit and note that it goes into the lock

// 2. loop through lock projects
// 2.a. check if the projects exists in direct deps list
// 2.b. if it's a direct dep:
// 2.b.i. if it has a version attached, constraint dep

This comment has been minimized.

@sdboyer

sdboyer May 5, 2017

Member

nit: s/constraint/constrain/

// 2.b.i. if it has a version attached, constraint dep
// 2.b.ii. if it has revision only, hint dep
// 2.c. else it's a transitive dep
// 2.d. log feedback

This comment has been minimized.

@sdboyer

sdboyer May 5, 2017

Member

i think so, apart from those two nits


// Get manifest version if available
if pp, ok := m.Dependencies[lock.Ident().ProjectRoot]; ok {
cf.Version = pp.Constraint.String()

This comment has been minimized.

@sdboyer

sdboyer May 5, 2017

Member

I'm not sure. And unfortunately, I don't think there's a way to see those errors. Would be a great thing to open an issue for 😄

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 5, 2017

Guess it's ready?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 5, 2017

Ahh, crap. So instead of just reviewing the code, I finally actually set this up and ran it locally. And I now realize there was a basic miscommunication - I'm sorry 😢

All of the output itself is looks good, but the timing is off. This provides the itemized list of choices after solving has already occurred. That was not what I had in mind; rather, I wanted this output to replace the existing output that's generated during the discovery process. So, that's all of the:

  • Found import of <project>, analyzing...
  • Could not determine version for <project>, omitting from generated manifest
  • Analyzing <project>...

This is why the spec has those header sections, e.g. Searching GOPATH for remaining projects... - I was imagining this output happening concurrently with each search process. Now, I realize that those headers will need to change a bit as the other parts get implemented (hopefully i'll get to #497 today), but unless you think it would be too difficult/cause a bunch of unnecessary work to achieve these incremental output changes until after the logic of init changes as well, then I'd prefer that all this output be moved up from the post-solve segment into the discovery segment.

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 6, 2017

@sdboyer I thought about it when I started implementing this. The feedback should be incremental. I realized that it would require changes in gps for instant feedback while resolving. But then, I thought these feedback should not be part of gps because they are dep specific. Hence, continued with this post-solve feedback.

Maybe the gps solver could be modified to attach some function that would be called to log these feedback at every solving iteration and keep gps generic?

About moving the output from post-solve segment to discovery segment, again I'm not sure how to get the solution data before running a solve.

On another thought, this is a network mode specific issue, I guess, which is the only mode at the moment 😬. VersionInWorkspace() could be used to fetch details of ondisk projects and feedback for those at the time of discovery is feasible (like the code in getProjectData()). And only for the projects that require network, code in this PR (with some changes) can be used. Thoughts?

Not sure how to proceed from here. Maybe put this on hold until the other parts are implemented 🙂

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 6, 2017

oops! it can still be done. My bad. Right now, we do search $GOPATH and go for network for not on disk deps. Which means, feedback can be logged for them, no need to wait for -gopath PR 🤦‍♂️ silly me 😅.

@darkowlzz darkowlzz force-pushed the darkowlzz:503-itemize-init-feedback branch from 289e393 to 57d870b May 7, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 7, 2017

Separated network and $GOPATH based dep's feedback. $GOPATH based deps show up incrementally as they are discovered but network based deps show up together because they are received together from the solver. Still wondering if it would be a good idea to attach a function to the solver to make network deps discovery incremental 🤔

Feedback output still looks the same, with $GOPATH and network headers:

$ dep init -no-examples
dep: Searching GOPATH for projects...
dep: Using master as constraint for direct dep github.com/fsnotify/fsnotify
dep: Locking in master (fd9ec7d) for direct dep github.com/fsnotify/fsnotify
dep: Locking in master (ca83bd2) for transitive dep golang.org/x/sys
dep: Using network for remaining projects...
dep: Using master as constraint for direct dep github.com/darkowlzz/openurl
dep: Locking in master (673a04d) for direct dep github.com/darkowlzz/openurl
dep: Using ^2.0.0 as constraint for direct dep github.com/sdboyer/deptestdos
dep: Locking in v2.0.0 (5c60720) for direct dep github.com/sdboyer/deptestdos
dep: Locking in v1.0.0 (ff2948a) for transitive dep github.com/sdboyer/deptest
  • Found import of , analyzing...
  • Could not determine version for , omitting from generated manifest
  • Analyzing ...

Will remove these once we decide if this new approach is better.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 10, 2017

This obviously needs refactor after #525 (sorry 😞 ), but two nits for when you get around to it:

  1. The dep: prefix doesn't really help anything, let's drop it (which is the default anyway, after incorporating #525)
  2. Let's prefix the individual discovery items with a two space indent, but leave the section "headers" (e.g. Searching GOPATH for projects...) unindented, so it's easier to visually scan and see when the different discovery modes are being used.
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 11, 2017

I'm prepping that feature branch - rebase, when you can? (plus those last couple nits).

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 11, 2017

Will rebase once #544 is merged. Import cycle issues even if I rebase now.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 12, 2017

all merged up :)

Add itemized feedback for dep resolution in init
- Creates ConstraintFeedback struct to hold dep constraint data.
- Adds GetUsingFeedback() and GetLockingFeedback() functions to generate
feedback text.
- Adds incremental feedback for deps found in $GOPATH and separate
feedback for network solved deps.

@darkowlzz darkowlzz force-pushed the darkowlzz:503-itemize-init-feedback branch from 57d870b to 4c785b9 May 13, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 13, 2017

@sdboyer squashed, rebased and made some adjustments to fit with the new code.

  • Created new internal package internal/feedback/.
  • Removed "dep" and added 2 space indent in feedback.
$ dep init
Searching GOPATH for projects...
  Using 673a04d as hint for direct dep github.com/darkowlzz/openurl
  Using master as constraint for direct dep github.com/fsnotify/fsnotify
  Locking in master (fd9ec7d) for direct dep github.com/fsnotify/fsnotify
  Locking in master (ca83bd2) for transitive dep golang.org/x/sys
Using network for remaining projects...
  Using ^2.0.0 as constraint for direct dep github.com/sdboyer/deptestdos
  Locking in v2.0.0 (5c60720) for direct dep github.com/sdboyer/deptestdos
  Locking in v1.0.0 (ff2948a) for transitive dep github.com/sdboyer/deptest
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 13, 2017

OK, new internal/feedback package seems fine enough for now. Thanks for keeping at this - in we go!!!

@sdboyer sdboyer merged commit b86ad16 into golang:master May 13, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sdboyer sdboyer referenced this pull request May 13, 2017

Merged

Add -gopath flag to init #497

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 16, 2017

So, I just ran dep init -v, and realized that what we've put together here meshes really poorly with that output 😄 We need a follow-up to basically scrub out all of the pre-existing output (with the dep: prefix), and have the only change with dep init -v be the trace output.

b/c yeah, this is super confusing:

dep: Finding dependencies for "github.com/Masterminds/glide"...
dep: Found 33 dependencies.
Searching GOPATH for projects...
dep: Building dependency graph...
dep: Found import of "github.com/Masterminds/semver", analyzing...
  Using range-prerelease as constraint for direct dep github.com/Masterminds/semver
  Locking in range-prerelease (8dec626) for direct dep github.com/Masterminds/semver
dep: Found import of "github.com/Masterminds/vcs", analyzing...
  Using master as constraint for direct dep github.com/Masterminds/vcs
  Locking in master (6abbba9) for direct dep github.com/Masterminds/vcs
dep: Found import of "github.com/codegangsta/cli", analyzing...
dep: Could not determine version for "github.com/codegangsta/cli", omitting from generated manifest
dep: Found import of "github.com/sdboyer/gps", analyzing...
  Using master as constraint for direct dep github.com/sdboyer/gps
  Locking in master (4d9c485) for direct dep github.com/sdboyer/gps
dep: Found import of "gopkg.in/yaml.v2", analyzing...
  Using 53feefa as hint for direct dep gopkg.in/yaml.v2
dep: Analyzing transitive imports...
dep: Analyzing "github.com/Masterminds/vcs"...
dep: Analyzing "github.com/codegangsta/cli"...
dep: Analyzing "github.com/sdboyer/gps"...
dep: Analyzing "github.com/Masterminds/semver"...
dep: Analyzing "github.com/armon/go-radix"...
dep: Analyzing "github.com/sdboyer/constext"...
  Locking in master (836a144) for transitive dep github.com/sdboyer/constext
dep: Analyzing "gopkg.in/yaml.v2"...
Using network for remaining projects...
dep: Solving...
Root project is "github.com/Masterminds/glide"
 17 transitively valid internal packages
 5 external packages imported from 5 projects
(0)   ✓ select (root)
(1)	? attempt github.com/Masterminds/semver with 1 pkgs; at least 1 versions to try
(1)	    try github.com/Masterminds/semver@range-prerelease
(1)	✓ select github.com/Masterminds/semver@range-prerelease w/1 pkgs
(2)	? attempt github.com/Masterminds/vcs with 1 pkgs; at least 1 versions to try
(2)	    try github.com/Masterminds/vcs@master
(2)	✓ select github.com/Masterminds/vcs@master w/1 pkgs
(3)	? attempt github.com/sdboyer/gps with 1 pkgs; at least 1 versions to try
(3)	    try github.com/sdboyer/gps@master
(3)	✓ select github.com/sdboyer/gps@master w/4 pkgs
(4)	? attempt github.com/sdboyer/constext with 1 pkgs; at least 1 versions to try
(4)	    try github.com/sdboyer/constext@master
(4)	✓ select github.com/sdboyer/constext@master w/1 pkgs
(5)	? attempt gopkg.in/yaml.v2 with 1 pkgs; at least 1 versions to try
(5)	    try gopkg.in/yaml.v2@53feefa2559fb8dfa8d81baad31be332c97d6c77
(5)	✓ select gopkg.in/yaml.v2@53feefa2559fb8dfa8d81baad31be332c97d6c77 w/1 pkgs
(6)	? attempt github.com/armon/go-radix with 1 pkgs; 1 versions to try
(6)	    try github.com/armon/go-radix@master
(6)	✓ select github.com/armon/go-radix@master w/1 pkgs
(7)	? attempt github.com/codegangsta/cli with 1 pkgs; 47 versions to try
(7)	    try github.com/codegangsta/cli@v1.19.1
(7)	✓ select github.com/codegangsta/cli@v1.19.1 w/1 pkgs
  ✓ found solution with 10 packages from 7 projects

Solver wall times by segment:
         b-list-pkgs: 2.558107961s
              b-gmal: 2.376535693s
         select-root:   1.541794ms
             satisfy:   1.206161ms
         select-atom:   1.109764ms
            new-atom:    158.866µs
     b-list-versions:     49.901µs
  b-deduce-proj-root:     23.482µs
               other:     21.256µs
     b-source-exists:      9.749µs

  TOTAL: 4.938764627s

  Using master as constraint for direct dep github.com/armon/go-radix
  Locking in master (4239b77) for direct dep github.com/armon/go-radix
  Using ^1.19.1 as constraint for direct dep github.com/codegangsta/cli
  Locking in v1.19.1 (0bdedde) for direct dep github.com/codegangsta/cli
@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 16, 2017

oops! 😅 that's really a mess. Will create a follow-up PR.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 16, 2017

cool, thank you :) i'm also inclined to revisit the enumerating of the items "over the network" - that's just listing out what ended up in the solution. this creates misleading output - what's read from GOPATH (and later, other projects' metadata) are hints/inputs that might change, whereas the "from network" output is what's actually been selected.

i think it may instead be better to enumerate the list of dependencies for which the local search method did not turn up a result, enumerate them, and indicate that the dep will instead simply rely on its "default most recent version" logic. Only, yknow, wordsmithed in a way that will be understandable for users 😄

@sdboyer sdboyer referenced this pull request May 17, 2017

Merged

Import glide configuration during init #500

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