Incorrect version is selected

[AlekSi]

$ dep ensure -v
Root project is "project"
 11 transitively valid internal packages
 11 external packages imported from 10 projects
(0)   ✓ select (root)
(1)	? attempt git.hosting.com/platform/package.git with 1 pkgs; at least 1 versions to try
(1)	    try git.hosting.com/platform/package.git@v0.7.0
(1)	    try git.hosting.com/platform/package.git@v0.6.0
(1)	    try git.hosting.com/platform/package.git@v0.5.4
(1)	    try git.hosting.com/platform/package.git@v0.5.3
(1)	    try git.hosting.com/platform/package.git@v0.5.2
(1)	    try git.hosting.com/platform/package.git@v0.5.1
(1)	    try git.hosting.com/platform/package.git@v0.5.0
(1)	    try git.hosting.com/platform/package.git@v0.4.0
(1)	    try git.hosting.com/platform/package.git@v0.3.0
(1)	    try git.hosting.com/platform/package.git@v0.2.0
(1)	    try git.hosting.com/platform/package.git@v0.1.0
(1)	✓ select git.hosting.com/platform/package.git@v0.1.0 w/1 pkgs
…

dep from commit 4773988, Gopkg.lock does not contain this package.

Can I make logging more verbose?

[sdboyer]

Hi, thanks for posting an issue! 😄

And, eeek - that's the whole, unmodified trace output? If so, there's definitely a bug somewhere; it shouldn't be possible for the solver to move through those versions without reporting a failure reason. (There's no more verbose mode than that)

[AlekSi]

It is both modified (project name, URLs are changed) and truncated (at …), but versions and error messages are not.

Here is another one:

Root project is "project"
 11 transitively valid internal packages
 11 external packages imported from 10 projects
(0)   ✓ select (root)
…
(11)  ? attempt git.hosting.com/platform/package.git with 1 pkgs; 12 versions to try
(11)      try git.hosting.com/platform/package.git@v0.7.0
(11)      try git.hosting.com/platform/package.git@v0.6.0
(11)      try git.hosting.com/platform/package.git@v0.5.4
(11)      try git.hosting.com/platform/package.git@v0.5.3
(11)      try git.hosting.com/platform/package.git@v0.5.2
(11)      try git.hosting.com/platform/package.git@v0.5.1
(11)      try git.hosting.com/platform/package.git@v0.5.0
(11)      try git.hosting.com/platform/package.git@v0.4.0
(11)      try git.hosting.com/platform/package.git@v0.3.0
(11)      try git.hosting.com/platform/package.git@v0.2.0
(11)      try git.hosting.com/platform/package.git@v0.1.0
(11)  ✓ select git.hosting.com/platform/package.git@v0.1.0 w/1 pkgs
…
(17)  ? attempt git.hosting.com/platform/package2.git with 1 pkgs; 7 versions to try
(17)      try git.hosting.com/platform/package2.git@v0.10.0
(17)  ✓ select git.hosting.com/platform/package2.git@v0.10.0 w/1 pkgs
…

Both project and project2 are not present in lock file before running this command. Manifest is empty.
I tried to add tag v0.10.0 to project, but this did not help.

[AlekSi]

I added traceInfo to solver.findValidVersion, new output:

(27)  ? attempt git.hosting.com/platform/project.git with 1 pkgs; 13 versions to try
(27)      try git.hosting.com/platform/project.git@v0.10.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.7.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.6.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.5.4
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.5.3
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.5.2
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.5.1
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.5.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.4.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.3.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.2.0
(27)  ✗   "." is not a valid import path
(27)      try git.hosting.com/platform/project.git@v0.1.0
(27)  ✓ select git.hosting.com/platform/project.git@v0.1.0 w/1 pkgs

It doesn't make sense for me: all versions are importable, and v0.1.0 is not so different from v0.2.0.

[AlekSi]

Ok, I tracked that one down (there was a lot of red herring on the way). Library package project contain main.go which starts like this:

// +build ignore

package main

import (
	"."
)

Import path . is then passed to intersectConstraintsWithImports -> DeduceProjectRoot -> deduceRootPath -> deduceKnownPaths -> normalizeURI. Latest method returns error, but it's not printed for some reason.

What I think can be improved:

1. There should be a way to enable debug logging for, well, debugging and error reporting. Something like "Please try to run it with DEP_DEBUG=1`.
2. github.com/pkg/errors could be helpful in some places in GPS.
3. I wonder why this error is not printed?
4. Some part of the call chain above (probably intersectConstraintsWithImports) should ignore relative imports.

[sdboyer]

Thanks for putting the time in on figuring this one out - you really dug in deep! 🎉

There should be a way to enable debug logging for, well, debugging and error reporting. Something like "Please try to run it with DEP_DEBUG=1`.

The question is, what more information would you want to see? Everything here kinda revolves around the solve tracer, which is tricky.

Solve tracing has generally been sufficient so far - as you discovered, once you instrumented it and added that error message, the issue here became a lot clearer. It's sufficient, at least, to understand where the logical problem was.

The problem is, it's output that's not designed for reprocessing or extension; the visualization it presents would be pretty torn up by just dumping data into the trace. But we kinda need the trace to contextualize the information we have.

One class of things we might want to see is "warnings" - I have a standalone issue for them, which touches on the question of how they should be recombined with solve tracing: sdboyer/gps#77

github.com/pkg/errors could be helpful in some places in GPS.

Probably, yeah. Useless in the solver, I think, but potentially quite useful in the SourceManager.

I've avoided it thus far because I have a longer-term plan with errors that I've been...well, it's probably time to get moving on it, now - sdboyer/gps#20. But I deferred using pkg/errors because it wasn't clear to me how it would integrate later. Not a great reason.

I wonder why this error is not printed?

A fine question, and the one I was planning on figuring out 😄

Some part of the call chain above (probably intersectConstraintsWithImports) should ignore relative imports.

Well, it depends on which part. The import "." is currently allowed by the parsing system, but other dot imports are not. As the comments there indicate, I'm fuzzy on how the compiler itself deals with relative imports (apart from the fact that they seem to be something that're now frowned upon, judging from the docs). I want to align the implementation gps uses as closely with the spec as possible.

That said, we can't budge on relative imports that escape the tree. If they ascend past the project root, they have to be discarded.

All of this, though, is ignoring the basic fact that we don't have handling for the "." import. And that's just a bug. My first guess is that we should be able to discard it long before it makes it into the solver, in PackageTree.ToReachMap(), as a dot import like that is...I mean, what does that even do? 😄

[carolynvs]

What with gps moving into dep, and dep already depending on pkg/errors (which may be my favorite lib of all time)... I'd be happy to help pepper it with wraps. 😇

[AlekSi]

The question is, what more information would you want to see?

I think it should log as much as possible. The main use case for that information is a reporting of issues:

• if something is not clear from that log, more logging should be added;
• if there is a clear bug, it should be fixed;
• if there is a clear user's error, it should be logged normally in user-friendly language, not only in this debug log.

as you discovered, once you instrumented it and added that error message, the issue here became a lot clearer

I don't think we could expect it from all users. Debug log should be enough, in my opinion.

[sdboyer]

I think it should log as much as possible. The main use case for that information is a reporting of issues:

Sure, I'm on board with these principles, but I was asking a more specific question (in fulfillment of the first bullet, really). I'm trying to understand if, for this particular case, there's more logging you think to be necessary beyond the bit you added.

[AlekSi]

I think "try" error should always be logged when -v flag is present.

All of this, though, is ignoring the basic fact that we don't have handling for the "." import. And that's just a bug.

+1. So in this particular case bug should be fixed, not error message. 😆

[sdboyer]

yep! we may need to wait until we get gps merged in directly, but that should hopefully happen pretty soon, now.

[sdboyer]

OK, new gps is merged in, so gonna fix this up tonight.