Extract PackageTree and stuff into subpackage #189

narqo · 2017-03-10T23:10:12Z

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?

narqo · 2017-03-10T23:13:09Z

analysis.go

@@ -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, " ")


@sdboyer

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

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 · 2017-03-11T03:26:06Z

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.

sdboyer · 2017-03-11T03:22:57Z

pkgtree/pkgtree.go

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


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.

fix tests

codecov · 2017-03-14T22:19:07Z

Codecov Report

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

@@            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.

narqo · 2017-03-14T22:23:02Z

solve_test.go

@@ -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()


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

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 · 2017-03-14T23:17:22Z

(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 · 2017-03-15T01:43:01Z

[..] 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 · 2017-03-15T02:15:05Z

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

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 😄

sdboyer · 2017-03-20T17:34:12Z

solve_test.go

@@ -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()


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 · 2017-03-20T17:36:14Z

solve_bimodal_test.go

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


Left over from debugging?

sdboyer · 2017-03-20T17:43:40Z

analysis.go

@@ -1,17 +1,7 @@
 package gps


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

sdboyer · 2017-03-20T17:46:00Z

fs/fs.go

@@ -1,7 +1,7 @@
-package gps
+package fs


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 · 2017-03-21T22:05:29Z

Ready for next iteration.

sdboyer · 2017-03-28T11:32:59Z

(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 · 2017-03-28T22:04:25Z

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?

sdboyer · 2017-03-28T23:16:16Z

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

sdboyer · 2017-03-28T23:24:02Z

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

narqo · 2017-03-29T00:17:59Z

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 · 2017-03-29T01:04:23Z

indeed! i'm glad that's the standard, i rather like that better :D OK, i think i should be able to merge this tonight, then. just finishing up a couple things elsewhere, first.

…

On 3/28/17 20:17, Vladimir Varankin wrote: Let's remove that latest revert (9e5eeb6 <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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#189 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABUX6kxNmPzBF4TxPUnsi-BCa_j3_0hks5rqaM3gaJpZM4MZ-Hk>.

sdboyer · 2017-03-29T11:00:09Z

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

Extract PackageTree and stuff into subpackage

narqo commented Mar 10, 2017

View reviewed changes

sdboyer reviewed Mar 11, 2017

View reviewed changes

narqo force-pushed the extract-package-tree branch from 17dd765 to 00ee443 Compare March 14, 2017 22:06

extract PackageTree to pkgtree package

f3a4a56

fix tests

narqo force-pushed the extract-package-tree branch from 00ee443 to f3a4a56 Compare March 14, 2017 22:18

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

narqo commented Mar 14, 2017

View reviewed changes

sdboyer suggested changes Mar 20, 2017

View reviewed changes

Vladimir Varankin added 6 commits March 21, 2017 22:57

remove unused stuff

8722033

move analysis into solver

1fc0e45

move fs to internal/fs

67dc4f8

move isStdLib to internal pkg

cb48479

fix ci configs to run tests in sub packages

9ef2b50

fix typo in circle.yml

b482791

Vladimir Varankin added 2 commits March 22, 2017 01:08

ci: remove temporary stuff during coverage aggregation

aa26e43

get rid of unused mocks

af2c0b4

ci: fix coverage generation in circle.yml

ad41126

narqo force-pushed the extract-package-tree branch from e1e4428 to ad41126 Compare March 28, 2017 21:23

revert pkgtree error types renaming

9e5eeb6

sdboyer approved these changes Mar 28, 2017

View reviewed changes

sdboyer added the enhancement label Mar 28, 2017

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

krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017

Merge pull request sdboyer/gps#189 from narqo/extract-package-tree

e8e777c

Extract PackageTree and stuff into subpackage

sdboyer mentioned this pull request May 17, 2017

Global state: internal/gps/internal/internal.go var IsStdLib golang/dep#597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract PackageTree and stuff into subpackage #189

Extract PackageTree and stuff into subpackage #189

narqo commented Mar 10, 2017

narqo Mar 10, 2017

sdboyer Mar 11, 2017

sdboyer commented Mar 11, 2017

sdboyer Mar 11, 2017

codecov bot commented Mar 14, 2017 •

edited

Loading

narqo Mar 14, 2017

sdboyer Mar 20, 2017

sdboyer commented Mar 14, 2017 •

edited

Loading

narqo commented Mar 15, 2017 •

edited

Loading

sdboyer commented Mar 15, 2017

sdboyer left a comment

sdboyer Mar 20, 2017

sdboyer Mar 20, 2017

sdboyer Mar 20, 2017

sdboyer Mar 20, 2017

narqo commented Mar 21, 2017

sdboyer commented Mar 28, 2017

narqo commented Mar 28, 2017

sdboyer commented Mar 28, 2017

sdboyer commented Mar 28, 2017

narqo commented Mar 29, 2017

sdboyer commented Mar 29, 2017 via email

sdboyer commented Mar 29, 2017

		@@ -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, " ")

Extract PackageTree and stuff into subpackage #189

Extract PackageTree and stuff into subpackage #189

Conversation

narqo commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Mar 11, 2017

Choose a reason for hiding this comment

codecov bot commented Mar 14, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Mar 14, 2017 • edited Loading

narqo commented Mar 15, 2017 • edited Loading

sdboyer commented Mar 15, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narqo commented Mar 21, 2017

sdboyer commented Mar 28, 2017

narqo commented Mar 28, 2017

sdboyer commented Mar 28, 2017

sdboyer commented Mar 28, 2017

narqo commented Mar 29, 2017

sdboyer commented Mar 29, 2017 via email

sdboyer commented Mar 29, 2017

codecov bot commented Mar 14, 2017 •

edited

Loading

sdboyer commented Mar 14, 2017 •

edited

Loading

narqo commented Mar 15, 2017 •

edited

Loading