Import glide configuration during init #500
Conversation
|
First, yay!! Glad you're moving forward on this - I was actually just about to post something on one of the other issues 😄 I can't remember if I said this anywhere, but repeating just in case - the ideal place to attach this behavior would be as a method on the |
Now, looking over the PR as-is - it's fine to have the bulk of the logic in |
|
I've rebased to take in the gps merge. Tomorrow I'll move my hook out of init and into the Analyzer. |
carolynvs
force-pushed
the
import-glide
branch
2 times, most recently
from
fd6d7ff
to
4a51515
Compare
|
yay! |
|
@sdboyer UPDATE: I found what I was looking for in the ensure command. I'll see if I can get the branch/tag/version deduction working tonight. |
|
Hurray for progress! It now
Both of that is rolled into the same function used by ensure so that everyone (ensure, importers, etc) all benefit. I think all that's left is:
|
cmd/dep/init.go
Outdated
| Analyzers: []rootProjectAnalyzer{ | ||
| newGopathAnalyzer(ctx.Loggers, ctx, pkgT, cpr, sm), | ||
| newImportAnalyzer(ctx.Loggers, sm), | ||
| }} |
There was a problem hiding this comment.
Just trying to understand, the compositeAnalyzer would also be added to analyzer.go's DeriveManifestAndLock() so that the dependency projects are also analyzed for dep, glide, godep, etc config in them, right?
There was a problem hiding this comment.
The way I have things organized as of right now:
dep/analyzer.gohandles identifying if a project uses dep and importing its config.dep/cmd/dep/importAnalyzer.gohandles organizing all the importers for other tools together and importing config for them recursively for all dependencies. The first tool which identities that its config is present is used, and the remaining tools are not used.dep/cmd/dep/gopathAnalyzer.gocontains all the existing logic for generating an initial manifest and lock based on what is on disk under GOPATH.dep/cmd/dep/compositeAnalyzer.golets us combine multiple analyzers, where the results of each is overlaid on top of the results of the previous, combining the results of an importer with whatever is found in the GOPATH.
The init command uses either just the gopathAnalyzer when -skip-tools is present (which is the behavior in master) for the root project and dependencies, or makes a compositeAnalyzer to determine the root manifest (first inspecting the GOPATH, then overlaying the import results) and uses the importAnalyzer for any dependencies.
Things are still in-flux as I've been developing in small steps. 😀 I haven't decided yet if it makes sense to combine the gopathAnalazyer with compositeAnalyzer, because they are only used together, but I kind of prefer to not have a hodge-podge struct that does all the things.
Let me know if that makes sense or not.
There was a problem hiding this comment.
Yep, totally makes sense. I have started writing godep cofig over this PR. 🙂
But I was wondering if we should help gps analyze the dependency projects too with the new importers. From my understanding, gps calls analyzer's DeriveManifestAndLock() for every (direct & transitive) project, and tries to analyze their dependency constraints. Right now it receives nil for manifest and lock of those projects. But as projects adopt dep, and for those projects that already use some other tool, gps should be able to analyze them, right?
Please correct me if I misunderstood 😅
There was a problem hiding this comment.
I think we are both trying to say the same thing. 👍 That's what I was trying to say with "using the importers for both the root project and the dependencies".
The solver takes an an initial manifest and lock for the root project, which I am generating using either just the gopathAnalyzer (for -skip-tools) or the compositeAnalayzer. But it also takes an analyzer to be applied to any dependencies it finds. When not skipping importing from other tools, I give the solver the importAnalyzer so that the same import logic we applied at the root is also applied to dependencies.
Thank you for asking about this! I forget to add an integration test for this scenario.
cmd/dep/ensure.go
Outdated
| } | ||
| } | ||
|
|
||
| // If not a plain SHA1 or bzr custom GUID, assume a plain version. | ||
| // TODO: if there is amgibuity here, then prompt the user? | ||
| return gps.NewVersion(s) | ||
| // return gps.NewVersion(s) |
There was a problem hiding this comment.
this is now cruft, let's remove
cmd/dep/glideConfig.go
Outdated
| @@ -0,0 +1,148 @@ | |||
| // Copyright 2016 The Go Authors. All rights reserved. | |||
There was a problem hiding this comment.
snake_case for filenames, please
|
@sdboyer The analyzer name returned by For example: |
|
@carolynvs I haven't reviewed your changes yet, so I don't know how much this lines up with what you've already done - so, speaking in generalities 😄 My instinctive preference is to have one actual analyzer struct that, perhaps based on some flag property, decides whether it will look for all existing tool metadata files, or only dep's. The name should similarly vary based on the state of that flag. When passing the analyzer to For reference, the purpose of the name and version fields is really just to provide a simple way of categorizing the compatibility of solutions. More formally, their meaning would be:
My plan is to expose these fields directly in Gopkg.lock in #508, so that saner errors messages can be provided. |
|
Thanks! That helps guide my next steps then. I was tempted to merge the two analyzers (skipping tools vs the current behavior) and that sounds like the way to go. |
cmd/dep/ensure_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| constraints := map[string]gps.Constraint{ | ||
| "v1.2.3": sv, | ||
| "v0.8.1": sv, | ||
| "master": gps.NewDefaultBranch("master"), |
There was a problem hiding this comment.
Not necessarily opposed to this, but...
Is making it a default branch actually material to the test here? Branch default-ness is something that's supposed to only really matter in internal sorting; that's why the func for it isn't currently exported from gps.
There was a problem hiding this comment.
Derp! I don't know why I didn't try using c.String() != expected.String() to workaround default causing the equality check to fail. That fixes the test without having to expose NewDefaultBranch, I'll revert that change.
cmd/dep/glide_config.go
Outdated
|
|
||
| // Unsupported fields that we will warn if used | ||
| Subpackages []string `yaml:"subpackages"` | ||
| OS string `yaml:"os"` |
There was a problem hiding this comment.
Just noting here that we may be able to deal with these eventually - #291 - and if we warn on seeing these, we should probably include a link to that issue.
cmd/dep/glide_config.go
Outdated
| func (g *glideFiles) load(projectDir string) error { | ||
| y := filepath.Join(projectDir, glideYamlName) | ||
| if g.loggers.Verbose { | ||
| g.loggers.Err.Printf("glide: Loading %s", y) |
There was a problem hiding this comment.
We want the output here to follow the pattern generally described in the spec, initial implementation in #512. Maybe this becomes the "section header" message? (If so, though, that header is printed regardless of verbose flag)
cmd/dep/glide_config.go
Outdated
|
|
||
| func (g *glideFiles) buildProjectConstraint(pkg glidePackage, sm gps.SourceManager) (pc gps.ProjectConstraint, err error) { | ||
| if pkg.Name == "" { | ||
| err = errors.New("glide: Invalid glide configuration, package name is required") |
There was a problem hiding this comment.
ehhh, i'm on the fence about this. does glide care that much about having this value present? i can't actually recall.
There was a problem hiding this comment.
This is to detect thje invalid configuration below, where an import section failed to set the package name. I don't require the root package name to be set. Granted it's unlikely to ever happen... Let me know if you still want the check removed.
import:
- package: ""
version: 1.0.0
cmd/dep/glide_config.go
Outdated
| g.loggers.Err.Printf("glide: The %s package specified an arch, but that isn't supported by dep, and will be ignored.\n", pkg.Name) | ||
| } | ||
| if len(pkg.Subpackages) > 0 { | ||
| g.loggers.Err.Printf("glide: The %s package specified subpackages, but that is calcuated by dep, and will be ignored.\n", pkg.Name) |
There was a problem hiding this comment.
subpackages are basically dead information in glide right now, anyway. I think it's fine to just ignore them entirely; no need to warn.
cmd/dep/gopath_analyzer.go
Outdated
| if newProject { | ||
| // Check if it's in notondisk project map. These are direct deps, should | ||
| // be added to manifest. | ||
| if _, ok := a.pd.notondisk[pr]; ok { |
There was a problem hiding this comment.
The notondisk map isn't what we want to check here - rather, we need to be looking at the set of external roots. For that, we need something like what I've added over in my WIP ensure PR - https://github.com/sdboyer/dep/blob/1a546666f038281855255a52a3ff8532ff758b1b/cmd/dep/ensure.go#L356-L375
There was a problem hiding this comment.
I think a couple of your comments may not be applicable due to the confusion over what gopath_analyzer does? It's just me extracting functions out of the init command into it's own struct and isn't for import at all.
There was a problem hiding this comment.
Yep, this probably doesn't apply anymore
cmd/dep/gopath_analyzer.go
Outdated
| func (a *gopathAnalyzer) DeriveRootManifestAndLock(path string, n gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) { | ||
| var err error | ||
|
|
||
| a.pd, err = getProjectData(a.ctx, a.pkgT, a.cpr, a.sm) |
There was a problem hiding this comment.
getProjectData is really only for the GOPATH detection route, not when we're importing from another tool. (we should probably rename it...)
There was a problem hiding this comment.
This is for the gopath detection route (see file name: gopath_analyzer.go). I had extracted the logic for init into multiple analyzers (because as I was working on it, made it easier for me to think about). Based on your stated preference for one analyzer, I am now working to combine them back into a single struct.
There was a problem hiding this comment.
ah! ok, i clearly didn't grok that reorganization from context here 😄
cmd/dep/gopath_analyzer.go
Outdated
| // be added to manifest. | ||
| if _, ok := a.pd.notondisk[pr]; ok { | ||
| m.Dependencies[pr] = getProjectPropertiesFromVersion(x.Version()) | ||
| feedback(x.Version(), pr, fb.DepTypeDirect, a.ctx) |
There was a problem hiding this comment.
Also, though we had some confusion in #512 (I wasn't clear about this initially), the goal of the feedback is to report what we learned by importing from the other tool before we go into the solve, not what the ultimate results are. We want to tell the user what we extracted from glide - what we ultimately end up with is a different question.
There was a problem hiding this comment.
I'll keep an eye out for the follow-up PR for #512 to make sure that any code I moved is merged appropriately.
cmd/dep/import_analyzer.go
Outdated
| return depAnalyzer.DeriveManifestAndLock(dir, pr) | ||
| } | ||
|
|
||
| // The assignment back to an interface prevents interface-based nil checks from failing later |
There was a problem hiding this comment.
Yeah I struggled to figure out how to reuse that logic without causing typed nil troubles. Let me know if it's preferable to copy/paste code vs. calling the function and coercing the nils.
There was a problem hiding this comment.
I think we may end up wanting to change the return pattern, anyway, per the discussion over in #594, which may help with this.
carolynvs
changed the title
|
@sdboyer Okay I just finished consolidating the little analyzer structs I had made into a single analyzer in I stopped short of combining that with Working on your other feedback right now. |
cmd/dep/glide_config_test.go
Outdated
|
|
||
| rev := lpv.Underlying() | ||
| if rev != "6a741be0cc55ecbe4f45690ebfd606a956d5f14a" { | ||
| t.Fatalf("Expected locked revision to be '6a741be0cc55ecbe4f45690ebfd606a956d5f14a', got %s", lv) |
There was a problem hiding this comment.
Change lv to rev? lv results in the version string, in this case v1.0.0.
There was a problem hiding this comment.
Thank you for the second set of 👀 !
cmd/dep/glide_config_test.go
Outdated
|
|
||
| ver := lpv.String() | ||
| if ver != "v1.0.0" { | ||
| t.Fatalf("Expected locked version to be 'v1.0.0', got %s", lv) |
There was a problem hiding this comment.
Maybe change lv to ver? This would still be fine because lv and ver have same string values.
| ], | ||
| "error-expected": "", | ||
| "gopath-initial": { | ||
| "github.com/sdboyer/deptestdos": "3f4c3bea144e112a69bbe5d8d01c1b09a544253f", |
There was a problem hiding this comment.
"github.com/sdboyer/deptest" 👀
|
@sdboyer When |
|
@sdboyer Another question. 😊 Now that we have the nifty feedback in place, the text displayed is misleading because the importer doesn't currently have all the facts up-front. I am relying on the solver doing what it does best and then making sure the manifest is cleaned up to match before writing to disk. I populate the initial root manifest with everything I find in Now with instant-feedback, I either need to pass more info to the importer to correctly display feedback, or tweak the feedback text to make it more clear that it's not final. Here's an example:
Current (Misleading) Output Option A: More vague output which is refined later during solve Option B: Importers use the packageTree(?) to get it right the first time I'm leaning towards option b. What do you think? |
|
@sdboyer This is ready for review.
Overview of changes
|
|
Reviewing now 😄 As a starter, looking at the appveyor failures, it looks like a couple tests might've forgotten a call to |
|
Doh! Thanks that was it. |
Yeah, I just flipped the switch on the CC analyzer for FIXMEs, TODOs, etc. It only applies to changed lines, not existing ones. We need to buckle down and be diligent about following these rules, at least with the new stuff we introduce. So yes, let's make sure we're correctly documenting new exported identifiers. |
analyzer.go
Outdated
| func (a Analyzer) DeriveManifestAndLock(path string, n gps.ProjectRoot) (gps.Manifest, gps.Lock, error) { | ||
| // TODO: If we decide to support other tools manifest, this is where we would need | ||
| // to add that support. | ||
| func (a Analyzer) HasConfig(path string) bool { |
There was a problem hiding this comment.
Let's call this HasDepMetadata() instead; HasConfig() is a bit generic.
cmd/dep/ensure.go
Outdated
| if !found { | ||
| return constraint, errors.Errorf("%s is not a valid version for the package %s", versionStr, arg) | ||
| for _, v := range versions { | ||
| if v.Type() == gps.IsBranch { |
There was a problem hiding this comment.
We're not guaranteed sorted output from ListVersions() - the first branch we encounter might not be the default. Need to gps.SortPairedForUpgrade() on the slice to get the outcome we want.
| // Use of this source code is governed by a BSD-style | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| package main |
There was a problem hiding this comment.
fyi, i'm not gonna review the actual glide conversion logic in detail. if there are problems, i'd likely miss them; we'd end up needing to rely on user testing and incremental improvement anyway. so 😄
cmd/dep/gopath_scanner.go
Outdated
| } | ||
| } | ||
|
|
||
| // Perform analysis of the filesystem tree rooted at path, with the |
There was a problem hiding this comment.
Doc text should start with "InitializeRootManifestAndLock"
cmd/dep/init.go
Outdated
| If configuration files for other dependency management tools are found, they | ||
| are used to pre-populate the manifest and lock. Specify -skip-tools to disable | ||
| this behavior. | ||
|
|
||
| A Gopkg.toml file will be written with inferred version constraints for all |
There was a problem hiding this comment.
This text needs to be updated, as well. (Can be a little sloppy about it, given that it's going to change again once we add the -gopath flag.)
cmd/dep/root_analyzer.go
Outdated
| "github.com/golang/dep/internal/gps" | ||
| ) | ||
|
|
||
| // importer handles importing configuration from other dependency managers into our format |
There was a problem hiding this comment.
Just a note - I think I'd eventually like to see this exported.
cmd/dep/root_analyzer.go
Outdated
| // importer handles importing configuration from other dependency managers into our format | ||
| type importer interface { | ||
| Name() string | ||
| Import(path string, pr gps.ProjectRoot) (*dep.Manifest, *dep.Lock, error) |
There was a problem hiding this comment.
Seems like importer should just compose gps.ProjectAnalyzer, rather than having a new method called Import with the same (well, an equivalent) signature, and the same basic purpose, as DeriveManifestAndLock. Is there a reason not to do that?
There was a problem hiding this comment.
I did it this way for a couple reasons:
- I needed the importers to return a dep manifest and lock, not the gps interfaces. Otherwise I would have to cast it immediately back.
- I didn't want the importers to implement functions that would go unused,
Analyzer.Info.
So while they are similar, but I didn't see anything to be gained by reusing that interface.
cmd/dep/root_analyzer.go
Outdated
| HasConfig(dir string) bool | ||
| } | ||
|
|
||
| // rootAnalyzer supplies manifest/lock data both from dep and external tool's config files |
There was a problem hiding this comment.
add periods; complete sentences is generally 👍
| @@ -0,0 +1,10 @@ | |||
| ignored = ["github.com/sdboyer/dep-test","github.com/golang/notexist/samples"] | |||
There was a problem hiding this comment.
you do not, sorry, shouldn't have mentioned it 😄
|
|
||
| package samples | ||
|
|
||
| import dt "github.com/carolynvs/go-dep-test" |
There was a problem hiding this comment.
ok, as long as you solemnly swear on your maintainer badge that this repo is frozen and will never change or go away once we merge this ❄️ 😺
There was a problem hiding this comment.
Haha, I can swear to that. Or you can clone/fork it and put it with sdboyer/deptest for consistency?
|
hmm - looking at we still have this thing where we produce feedback after the solve. either i'm forgetting something about the spec i wrote, or there's something fishy there. why are we not even applying constraints at all until after solving? |
This makes it easier for the importers to reuse its logic
I tweaked ensure so that the existing behavior of ensure X applies the any (*) constraint.
|
@sdboyer There are a couple problems with the output. 😊
|
* init imports by default unless -skip-tools is specified * glide config in dependencies is taken into account during solve
|
I've addressed all your feedback, rebased and squashed. Here's the latest output of running |
The question of whether we read e.g. glide files in deps is a separate one from whether we read them from the current project during Also, either way, I don't think we want to print there, as it interrupts the trace output. |
* Ignore glide's test case for importing during solve
|
I've disabled importing during solve. This means that if a dependency has glide config, it is not used at all. Previously -skip-tools controlled import both for the root project and dependencies, now it only controls at the root, and we never import for dependencies. I've also tweaked the importer so that nothing is logged during solve. So if we eventually use the importer during solve, it won't interrupt the output. Here's the new output: |
|
OK so there is just NO apparent reason for that test windows test failure. Github is up...I don't know. Either way, this is close enough, and has gone on way too long. It's time to merge. We can work out the rest incrementally 😄 |
During
dep initifglide.yamlandglide.lockare present, then use the glide configuration when initializing dep's manifest and lock. Importing config from other dependency manager's will be the default, specify-skip-toolsto ignore existing configuration.glide.yamland populate initial manifest constraints.glide.yamlbut isn't imported.glide.lockand populate initial lock revisions.