Pull in parser code from godep as part of not using build #149
Conversation
Can probably get rid of Multiple Package case but leaving for now / testing purposes.
Current coverage is 76.59% (diff: 82.71%)@@ master #149 diff @@
==========================================
Files 25 25
Lines 3759 3782 +23
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 2923 2897 -26
- Misses 633 686 +53
+ Partials 203 199 -4
|
There was a problem hiding this comment.
ok so, first, thanks a ton for pulling this over. it doesn't quite fit as-is, because it is still a goal of ListPackages() to identify cases where there's a legitimately bogus package due to multiple package names...despite, ugh, that being a really infrequent case that's basically always a mistake.
i do like that this gets us started in the direction of #99, though.
| } | ||
| } | ||
|
|
||
| //TODO: Needed in GPS? |
There was a problem hiding this comment.
nope, won't be needed - anything version-related is all higher-level than and transparent to what's happening in ListPackages()
There was a problem hiding this comment.
The version mentioned here is the go version, not the package version.
There was a problem hiding this comment.
derp derp 🐑
nevertheless, i think we can safely drop it for now. the goal of ListPackages() is generally to be as permissive as possible in describing what sits on disk. decisions about what to do with that information are generally left to later parts of the system.
| @@ -155,9 +156,18 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { | |||
| ip := filepath.ToSlash(filepath.Join(importRoot, strings.TrimPrefix(path, fileRoot))) | |||
|
|
|||
| // Find all the imports, across all os/arch combos | |||
| p, err := ctx.ImportDir(path, analysisImportMode()) | |||
| ap, err := filepath.Abs(path) | |||
There was a problem hiding this comment.
curious what your reasoning was about adding this - is there something later relying on an absolute path?
There was a problem hiding this comment.
oh wait i think i know - wanting to make sure that there's no confusion about what goes in the pkgcache.
so, that cache is actually a problem, heh - this same path may well get used later on for a different version of the same code. when we need to inspect different versions of the code for their characteristics, we swap them out in-place.
there's caches in place higher up, though, so dropping that cache would be fine.
| var pkg Package | ||
| if err == nil { | ||
| if p.Name == "" { | ||
| p.Name = importRoot |
There was a problem hiding this comment.
seems weird thatp.Name could be empty? if so, though, then it should probably be assigned path.Base(importRoot), so that it's just getting the last element.
There was a problem hiding this comment.
This was necessary to pass one of your tests. ;-) path.Base(importRoot) may be more correct though.
| fname := filepath.Base(file) | ||
| for _, c := range pf.Comments { | ||
| ct := c.Text() | ||
| if i := strings.Index(ct, buildMatch); i != -1 { |
There was a problem hiding this comment.
would this also pick up +build that was in the middle of a comment block?
There was a problem hiding this comment.
Perhaps. I can probably look at the comments and if they aren't above the package statement abort.
|
I'm missing some background here. Why are we moving away from the build package? Seems like the wrong direction? |
|
@adg basically b/c I don't think this is a problem with |
|
OK
…On 20 January 2017 at 14:06, sam boyer ***@***.***> wrote:
@adg <https://github.com/adg> basically b/c go/build
<https://goto.google.com/build> was really designed to operate within the
framework of a concrete set of os/arch/release/build tags. It's not great
for dealing with *everything* that's in a directory; the UseAllFiles
behavior has problems.
I don't think this is a problem with go/build
<https://goto.google.com/build>, per se - just that it really wasn't
designed for this purpose, and shouldn't be. I get into a more specifics
here: #138 (comment)
<#138 (comment)>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#149 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIDilUAoPH70bR9WXnfUszMFAb1nZ91rks5rUCS8gaJpZM4Lo1jg>
.
|
|
Updated some stuff, needs more review... WRT Multiple Package names, In the wild I've seen that's not always the case when ignoring build tags. I can't remember the packages though in question that taught me this lesson. :-( Related ... The code as originally imported does ignore code with the |
| @@ -173,6 +184,7 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { | |||
| } | |||
| return nil | |||
| case *build.MultiplePackageError: | |||
| fmt.Println("HOW DID WE GET HERE?") | |||
There was a problem hiding this comment.
AFAICT a lot of these cases can go away IMO.
Pull in parser code from godep as part of not using build
Can probably get rid of Multiple Package case but leaving
for now / testing purposes.
This is kinda a WIP and we should discuss a bit.