Fixes #907 Warn when an importer cannot preserve a constraint #1086
Conversation
cmd/dep/glide_importer.go
Outdated
| @@ -202,7 +202,8 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project | |||
| } | |||
| pc.Constraint, err = g.sm.InferConstraint(pkg.Reference, pc.Ident) | |||
| if err != nil { | |||
| return | |||
| g.logger.Printf("Unable to interpret revision specifier '%s' for package %s: %s", pkg.Name, pc.Ident, err.Error()) | |||
There was a problem hiding this comment.
Should these have trailing newlines \n?
I also wonder if it would be helpful to explicitly mention that Any is being used instead, or if that is the implied default or sufficiently documented elsewhere.
There was a problem hiding this comment.
Yup, the default for the importers is to use the any constraint whenever they can't determine something more specific. This ensures that imported dependencies (direct only) are always included in the generated manifest.
This behavior is not documented because we don't currently doc how the importers work. I am knee deep in redoing the importer plumbing, and when I submit that PR, will add doc for the general heuristic used by all importers.
There was a problem hiding this comment.
This will need a unit test for each importer to verify that the function continues, returning a lock/manifest and prints the warning.
Here is an example of a similar unit test:
https://github.com/golang/dep/blob/master/cmd/dep/glide_importer_test.go#L185
The "bad constraint" version of that test will need a new config file (that has a unparsable constraint) and golden output file that contains your warning.
cmd/dep/glide_importer.go
Outdated
| @@ -202,7 +202,8 @@ func (g *glideImporter) buildProjectConstraint(pkg glidePackage) (pc gps.Project | |||
| } | |||
| pc.Constraint, err = g.sm.InferConstraint(pkg.Reference, pc.Ident) | |||
| if err != nil { | |||
| return | |||
| g.logger.Printf("Unable to interpret revision specifier '%s' for package %s: %s, (using the 'any' constraint instead)\n", pkg.Name, pc.Ident, err.Error()) | |||
There was a problem hiding this comment.
- The wrong variable is being passed in as the first fmt argument. We want to print the value passed to
InferConstraintwhich waspkg.Reference. - Change "revision specifier" to "constraint" in the warning.
cmd/dep/godep_importer.go
Outdated
| @@ -158,7 +158,8 @@ func (g *godepImporter) buildProjectConstraint(pkg godepPackage) (pc gps.Project | |||
| pc.Ident = gps.ProjectIdentifier{ProjectRoot: gps.ProjectRoot(pkg.ImportPath)} | |||
| pc.Constraint, err = g.sm.InferConstraint(pkg.Comment, pc.Ident) | |||
| if err != nil { | |||
| return | |||
| g.logger.Printf("Unable to interpret revision specifier '%s' for package %s: %s, (using the 'any' constraint instead)\n", pkg.ImportPath, pc.Ident, err.Error()) | |||
There was a problem hiding this comment.
Same as above, only this time it's pkg.Comment for the first argument.
|
Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks! |
What does this do / why do we need it?
This fixes issue #907
What should your reviewer look out for in this PR?
nothin
Do you need help or clarification on anything?
nope
Which issue(s) does this PR fix?
#907