This repository has been archived by the owner. It is now read-only.

Extract PackageTree and stuff into subpackage #189

Merged
merged 11 commits into from Mar 29, 2017

Conversation

Projects
None yet
2 participants
@narqo
Copy link
Contributor

narqo commented Mar 10, 2017

I've started working on #166 to get some overview of the project's internals. While the process hasn't finished yet, I feel it might be a good time to get some early feedback.

Tests for solve_bimodal_test.go are failing as they rely on private workmap of pkgtree. I feel we could refactor this piece, but I'm not sure what is the best way to do it. Definitely, need to dive more deeply into the project's code base.

P. S. It looks like fs package might be a candidate for having fastWalk function from #180, doesn't it?

analysis.go Outdated
@@ -30,943 +20,3 @@ func init() {
archListString := "386 amd64 amd64p32 arm armbe arm64 arm64be ppc64 ppc64le mips mipsle mips64 mips64le mips64p32 mips64p32le ppc s390 s390x sparc sparc64"
archList = strings.Split(archListString, " ")

This comment has been minimized.

@narqo

narqo Mar 10, 2017

Contributor

@sdboyer

I haven't found any references to these variables. Could you provide some details, why they are here?

This comment has been minimized.

@sdboyer

sdboyer Mar 11, 2017

Owner

Ah, yeah. I thought I was going to get to this earlier, but I still haven't.

Basically, they're supposed to be for this: golang/dep#291

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 11, 2017

woohoo more contributors! welcome! 👏 👏 🎉

I think the best route with the wmToReach() call in computeBimodalExternalMap() is to just bite the bullet and fake up a PackageTree that'll produce the same ReachMap when ToReachMap() is called on it as we currently get from calling wmToReach(). I have a nagging memory of some other plan for refactoring it, but it's been a while, and that seems sanest when I look back at it now.

P. S. It looks like fs package might be a candidate for having fastWalk function from #180, doesn't it?

Let's lump it in with the pkgtree package for now, unexported. We can consider factoring it out later, but I don't want to expose it, because then we have to support it/can't change it.

)

// Stored as a var so that tests can swap it out. Ugh globals, ugh.
var IsStdLib = doIsStdLib

This comment has been minimized.

@sdboyer

sdboyer Mar 11, 2017

Owner

This is one of those things I'd really strongly prefer not to export, but we have the call to it in the solver.

If we can't think of any clever solutions, then I'd prefer we duplicate the function in both packages than have it exposed. The only solution I can think of is adding a stdlib filtering param to ToReachMap()...but that doesn't take care of the calls in rootdata.go.

@narqo narqo force-pushed the narqo:extract-package-tree branch from 17dd765 to 00ee443 Mar 14, 2017

Vladimir Varankin

@narqo narqo force-pushed the narqo:extract-package-tree branch from 00ee443 to f3a4a56 Mar 14, 2017

@narqo narqo changed the title (wip!) Extract PackageTree and stuff into subpackage Extract PackageTree and stuff into subpackage Mar 14, 2017

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 14, 2017

Codecov Report

Merging #189 into master will decrease coverage by 0.62%.
The diff coverage is 57.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   78.03%   77.41%   -0.63%     
==========================================
  Files          25       27       +2     
  Lines        3989     3989              
==========================================
- Hits         3113     3088      -25     
- Misses        658      683      +25     
  Partials      218      218
Impacted Files Coverage Δ
types.go 82.05% <ø> (ø) ⬆️
hash.go 81.35% <ø> (ø) ⬆️
bridge.go 76.16% <0%> (ø) ⬆️
source_manager.go 91.38% <100%> (ø) ⬆️
internal/internal.go 100% <100%> (ø)
trace.go 72.22% <100%> (ø) ⬆️
vcs_source.go 73.06% <100%> (ø) ⬆️
pkgtree/pkgtree.go 84.98% <11.11%> (ø)
internal/fs/fs.go 27.71% <27.27%> (ø)
source.go 35.08% <33.33%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afc5fe4...9e5eeb6. Read the comment docs.

@@ -47,6 +49,8 @@ func overrideIsStdLib() {
isStdLib = func(path string) bool {
return false
}
// NOTE(narqo): this is an ugly hack! One have to think about better way to do cross-package mocking.
pkgtree.MockIsStdLib()

This comment has been minimized.

@narqo

narqo Mar 14, 2017

Contributor

I really don't like this, but currently, I don't see another way to make fixtures work.

This comment has been minimized.

@sdboyer

sdboyer Mar 20, 2017

Owner

Yknow...I think maybe we could just expose var IsStdlib on the subpackage, then have tests overwrite it in much the way they do now. As long as there's a doc on it indicating its intended use, and that overwriting it except for test purposes will cause undefined behavior, then I'm not too bothered.

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 14, 2017

(btw, i'd prefer that you just push up commits as you go, rather than amending/squashing and force pushing. i realize i might be weird in this regard, but it's easier for me to keep track of a running diff)

@narqo

This comment has been minimized.

Copy link
Contributor

narqo commented Mar 15, 2017

[..] i'd prefer that you just push up commits as you go [..]

No problem. Will try to do some meaningful commits in future, not some random lazy "wip!"s. :)

I think I'm done with #166 for now and waiting for your review.

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 15, 2017

OK, great. Might take me a couple days, but I'll definitely try to get this to happen before moving gps into dep (golang/dep#300).

@sdboyer
Copy link
Owner

sdboyer left a comment

Sorry it took me so long to get to a review - in general, this looks great! There are a few changes I've noted with inline the comments.

The bigger thing, though, is that the circle.yaml needs to be updated so that the subpackages are also tested. Ideally, that would also include the coverage information...but that takes a little doing, because you have to stitch together output from individual tests to make it work.

I'd love it if you could take care of that part, too, but if not, I can work it out 😄

@@ -47,6 +49,8 @@ func overrideIsStdLib() {
isStdLib = func(path string) bool {
return false
}
// NOTE(narqo): this is an ugly hack! One have to think about better way to do cross-package mocking.
pkgtree.MockIsStdLib()

This comment has been minimized.

@sdboyer

sdboyer Mar 20, 2017

Owner

Yknow...I think maybe we could just expose var IsStdlib on the subpackage, then have tests overwrite it in much the way they do now. As long as there's a doc on it indicating its intended use, and that overwriting it except for test purposes will cause undefined behavior, then I'm not too bothered.

if len(em) > 0 {
panic(fmt.Sprintf("pkgs with errors in reachmap processing: %s", em))
}
fmt.Printf("reachmap: %+v\n", reachmap)

This comment has been minimized.

@sdboyer

sdboyer Mar 20, 2017

Owner

Left over from debugging?

analysis.go Outdated
@@ -1,17 +1,7 @@
package gps

This comment has been minimized.

@sdboyer

sdboyer Mar 20, 2017

Owner

let's just get rid of this file entirely - move it into solver.go.

fs/fs.go Outdated
@@ -1,7 +1,7 @@
package gps
package fs

This comment has been minimized.

@sdboyer

sdboyer Mar 20, 2017

Owner

I think I'd rather make this internal/fs, at least for now - these funcs aren't really intended to be a part of the public surface area.

@narqo

This comment has been minimized.

Copy link
Contributor

narqo commented Mar 21, 2017

Ready for next iteration.

Vladimir Varankin added some commits Mar 21, 2017

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 28, 2017

(sorry for delay - very very busy week, changing jobs et. all)

Looks like the change you made to the coverage counter ended up just reporting the coverage from the pkgtree subdir :( Get that fixed, and we can merge!

Also, I decided on a much more pleasant way to deal with the isStdLib() func - we'll add a parameter to Flatten() that lets the caller inject a filter func - but can take care of that in a follow-up PR.

@narqo narqo force-pushed the narqo:extract-package-tree branch from e1e4428 to ad41126 Mar 28, 2017

@narqo

This comment has been minimized.

Copy link
Contributor

narqo commented Mar 28, 2017

This is weird, AppVeyor's tests are failing with "nil pointer dereference" from os/exec. But it doesn't look like the changes are the reason. @sdboyer could you help me here?

Vladimir Varankin
@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 28, 2017

That seems like it's an ephemeral error. (Which is itself not reassuring, but...)

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 28, 2017

Let's remove that latest revert (9e5eeb6), we do want those errors renamed. With any luck, tests will pass on re-run.

@sdboyer sdboyer added the enhancement label Mar 28, 2017

@narqo

This comment has been minimized.

Copy link
Contributor

narqo commented Mar 29, 2017

Let's remove that latest revert (9e5eeb6), we do want those errors renamed

After spending some time digging through go's standard library internals, it seems that naming error types as SomethingError is actually a common practice. See https://talks.golang.org/2014/names.slide#15 If you don't mind, I'd like to consider that my initial renaming in the fist commit of the PR, was a mistake.

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 29, 2017

@sdboyer

This comment has been minimized.

Copy link
Owner

sdboyer commented Mar 29, 2017

OK, pulling this in. Thanks for all your work and patience on this!

@sdboyer sdboyer merged commit eb55c2a into sdboyer:master Mar 29, 2017

3 of 5 checks passed

codecov/patch 57.69% of diff hit (target 78.03%)
Details
codecov/project 77.41% (-0.63%) compared to afc5fe4
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
hound No violations found. Woof!

kris-nova pushed a commit to kris-nova/dep that referenced this pull request Apr 21, 2017

Merge pull request sdboyer/gps#189 from narqo/extract-package-tree
Extract PackageTree and stuff into subpackage
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.