Remove duplicate root projects when importing from glide #898
Conversation
carolynvs
force-pushed
the
root-imported-constraints
branch
from
ed1b50f to
ad8686c
Compare
internal/gps/project_root_cache.go
Outdated
| // source manager, which is quite helpful when repeatedly converting from | ||
| // import paths to root projects. | ||
| type ProjectRootCache struct { | ||
| prs map[string]ProjectRoot |
There was a problem hiding this comment.
this information should already be cached inside of a SourceManager (or, at least, it definitely is in sourceMgr) - what's the benefit of layering this on top?
There was a problem hiding this comment.
that logic is definitely complex, because it has to be concurrency-safe, but yes, the deductionCoordinator.rootxt radix tree is the cache, and this is basically the cache "get".
we use the radix tree, rather than a map, so that we only ever have to actually deduce a project root once, even if we haven't seen a literal path before. so, the sequence goes something like this:
- a call comes in to deduce
k8s.io/kubernetes/pkg/foo - we make the necessary HTTP call and find that
k8s.io/kubernetesis the root, and record that in the radix tree - a subsequent call comes in to deduce
k8s.io/kubernetes/pkg/bar
even though no exact call happened for k8s.io/kubernetes/pkg/bar before, or even for its actual root, k8s.io/kubernetes, a deduction call to k8s.io/kubernetes/pkg/bar after an earlier, completed one to k8s.io/kubernetes/pkg/foo still results in a cache hit.
this is one of the things afforded to us by the assumption that projects are repo root-only, and therefore necessarily cannot be nested.
|
i'm guessing this is just backburnered a bit, right? |
|
Yup! I'll jump back on this (and others) next week. |
carolynvs
force-pushed
the
root-imported-constraints
branch
from
ad8686c to
9485a09
Compare
|
I've removed the unnecessary cache and rebased. |
cmd/dep/godep_importer_test.go
Outdated
|
|
||
| if result != c.want { | ||
| t.Fatalf("projectExistsInLock result is not as expected: \n\t(GOT) %v \n\t(WNT) %v", result, c.want) | ||
| t.Errorf("projectExistsInLock result is not as expected: \n\t(GOT) %v \n\t(WNT) %v", result, c.want) |
There was a problem hiding this comment.
Move this whole test function TestGodepConfig_ProjectExistsInLock() as part of root_analyzer_test.go?
There was a problem hiding this comment.
Also, any reason for using Errorf here? Just curious to know when to use which one 😊
There was a problem hiding this comment.
This test isn't using proper subtests, so I was using Errorf (which allows the other testcases to execute after a failure). I'll switch to a subtest and fatalf.
carolynvs
force-pushed
the
root-imported-constraints
branch
from
9485a09 to
3dbbfbd
Compare
|
Rebased and fixed that one test. |
|
weee! |
I (naively) assumed that since glide supports listing the subpackages used, that it people's glide.yaml/lock wouldn't have multiple entries for the same root package...
The glide importer now checks for duplicate root projects.
I've introduced a cache of the resolution from import path to root project, because it's guaranteed that paths will be double resolved (for glide and any other future importer that uses a config + lock).