gogitdiff: incremental build tool #21

prateek · 2018-04-24T14:42:39Z

Add a new tool, ggd, to compute packages affected by git changes

NB: don't worry about test coverage, this tool uses e2e integration tests using bash to create a separate gopath/git repos (see build-tools/utilities/ggd/test.sh for more details).

codecov · 2018-04-24T14:44:06Z

Codecov Report

Merging #21 into master will decrease coverage by 15.22%.
The diff coverage is 7.44%.

@@             Coverage Diff             @@
##           master      #21       +/-   ##
===========================================
- Coverage   63.63%   48.41%   -15.23%     
===========================================
  Files           3        4        +1     
  Lines         506      694      +188     
===========================================
+ Hits          322      336       +14     
- Misses        169      342      +173     
- Partials       15       16        +1

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 e8fd6b3...07664e2. Read the comment docs.

robskillington · 2018-04-24T19:22:42Z

Nice.

There seems to be a binary file committed btw currently: utilities/gogitdelta/gogitdelta

prateek · 2018-04-24T22:34:20Z

utilities/ggd/lib/dag.go

+		for _, imp := range imports {
+			impPath := imp.Path()
+			if strings.HasSuffix(impPath, "_test") {
+				println(fmt.Sprintf(">> %+v", imp))


prateek · 2018-04-25T17:04:25Z

@jeromefroe ended up using your dag calculation code from #17, it had minor issues but handled the x_test better than https://github.com/kisielk/gotool.

misc cleanup

jeromefroe

Mostly looks good to me, just had a few nits and questions

jeromefroe · 2018-04-26T15:42:18Z

utilities/ggd/README.md

+ggd - GoGitDiff
+===============
+
+This is a tool to compute the packages affected by the difference between two Git


nit: I found this first sentence kind of confusing. What do you think about simplifying to something like the following:

`ggd` is a tool to compute which packages have changed between two Git revisions and their respective dependencies.

How about:

`ggd` is a tool to compute Go packages affected by changes between two Git revisions. It does so by identifying the Go packages changed between two Git revisions, and computing all other packages which depend upon them.

jeromefroe · 2018-04-26T15:57:58Z

utilities/ggd/lib/dag.go

+
+		visited := make(map[string]string)
+
+		// ensure we add the pkg to the graph


Ahh, good catch, yea definitely need to add the package here, as the steps below will only add the things which import it.

Yay for tests :)

jeromefroe · 2018-04-26T16:06:40Z

utilities/ggd/lib/dag.go

+	if !ok {
+		// It's okay for Import to return an error as not all packages that can be found in
+		// a package will necessarily be present. For example, packages imported only by test
+		// files in vendored packages will not be installed. In the case of an error, Import


nit (self-nit?): "In the case of an error, Import always returns a non-nil *Package. In the case of an error it will only contain partial information." -> "In the case of an error, Import always returns a non-nil *Package that contains partial information."

sure thing.

jeromefroe · 2018-04-26T16:11:32Z

utilities/ggd/lib/dag.go

+	edges[to] = struct{}{}
+}
+
+// Closure returns all the transitive closure of all packages reachable


nit: s/returns all the/returns the

jeromefroe · 2018-04-26T16:15:49Z

utilities/ggd/lib/git.go

+	return res, nil
+}
+
+// CwgIsDirty returns whether the current directory is dirty.


nit: CwgIsDirty -> CWDIsDirty ?

jeromefroe · 2018-04-26T16:55:14Z

utilities/ggd/main.go

+	debug("changed packages: %v", packageChanges)
+
+	baseFullChangeSet := make(lib.ImportSet)
+	if !*includePrechanges {


What's the intention here? Why compute the graph and changed packages with the base reference?

The original thought was we'd need to consider both DAGs to answer the question of all affected changes but I'm not convinced that's right anymore. I need to draw some graphs to convince myself before I would be willing to delete the code. Will put down a TODO for now,

jeromefroe · 2018-04-26T17:04:24Z

utilities/ggd/main.go

+		debug("affected packages (including transitive changes): %v", baseFullChangeSet)
+	}
+
+	currentSha1, err := sha1Fn()


Should we try to checkout the current SHA again before we return?

also, super nit: currentSha1 -> currentSHA1

not sure what you mean, that we should retry or ensure we're on the right SHA1.

Re: retry - why? it'll only fail when the directory is dirty, at which point, retries buy you nothing.

Re: the right sha1 - we do that.

jeromefroe · 2018-04-26T17:11:01Z

utilities/ggd/lib/dag.go

+	if _, ok := visited[node]; ok {
+		return
+	}
+	visited[node] = struct{}{}


I don't know if we can unconditionally add the package. For example, if we delete a package in our branch then it will be a changed package even though it no longer exists and go test will complain that it cant find it.

That's a great point. Lemme add a test and ensure we handle the situation correctly.

Added testcase7 for the case you're describing, here's the mini-diff: f5b1fa0

richardartoul · 2018-04-26T21:48:35Z

utilities/ggd/README.md

+===============
+
+This is a tool to compute the packages affected by the difference between two Git
+revisions. It's primary goal is to help speed up CI jobs by figuring out the packages


"It was deSigned to speed up C.I jobs by determining which packages depend on packages that were changed in the target branch vs. the base branch" or something like that.

Also would be cool to link to Digital Ocean's blog post on the topic

Edit: nevermind you did link to it

richardartoul · 2018-04-26T21:49:11Z

utilities/ggd/README.md

+affected by git changes, and transitively looking through the Go Imports. It takes
+inspiration from `gta`, and the work done at [DigitalOcean].
+
+NB: debug mode (`-d`) allows users to inspect how the tool arrives at the decisions it does.


"allows users to inspect the tool's decision making process"

richardartoul · 2018-04-26T21:51:30Z

utilities/ggd/lib/dag.go

+	"golang.org/x/tools/go/buildutil"
+)
+
+// ImportGraph is a map from a package -> all the packages that import it.


nit: -> a set of all the packages that import (depend?) on it

richardartoul · 2018-04-26T21:51:52Z

utilities/ggd/lib/dag.go

+)
+
+// ImportGraph is a map from a package -> all the packages that import it.
+type ImportGraph map[string]map[string]struct{}


can you use the ImportSet as the value for this map?

richardartoul · 2018-04-26T21:53:01Z

utilities/ggd/lib/dag.go

+func (g ImportGraph) maybeAddEdge(
+	ctx *build.Context, buildPkg *build.Package, visited map[string]string, pkg, path string,
+) {
+	if path == "C" {


I'm confused, what is this?

CGO .. For context, similar code in go build: https://github.com/golang/go/blob/ae2a2d12f6d8cde35637a13f384f6de524112768/src/go/build/build.go#L860

jeromefroe · 2018-04-27T01:44:56Z

utilities/ggd/lib/go.go

+//
+// also filters any `vendor/` directory paths
+func filterChanges(input string) bool {
+	if strings.Contains(input, "/vendor/") {


Why filter the vendor? Can't we use the DAG to determine exactly what packages depend on them if we strip the vendor prefix?

We can, will make it optional.

I'm thinking of the case for the repos where we include vendor (internally) v the ones we don't (OSS/GH). To your point, it'll be helpful for the former, don't want to include it in the computation for the latter.

jeromefroe · 2018-04-27T01:52:24Z

utilities/ggd/lib/dag.go

+func (g ImportGraph) Closure(paths ...string) (ImportSet, error) {
+	// add all the paths we're starting at, as they are by definition
+	// reachable.
+	for _, p := range paths {


Instead of including the logic to determine if a package was deleted so we can add all packages here would it make sense to instead only add the package if it exists in the graph since otherwise we know it was deleted?

"only add the package if it exists in the graph since otherwise we know it was deleted"

I actually tried to do this the first. Unfortunately, it doesn't work in certain cases.

Can repro yourself, steps:

$ cd $GOPATH/src/github.com/m3db/build-tools $ git checkout prateek/ggd/demo-testdata-issue $ cd ./utilities/ggd $ ./test.sh # testcase6 will fail # can see output for the run in ./utilities/ggd/bin/testcase6.out

Hmm, that seems to fail because we unconditionally panic if we can't find a package, but what if we first strip the root packages which don't appear in the DAG before walking the graph? I'm thinking something like the following:

func (g ImportGraph) Closure(roots ...string) ImportSet { closure := make(ImportSet) for _, r := range roots { if _, ok := g[r]; !ok { log.Debugf("root package '%s' is not in the import graph, it has been removed", r) } g.walk(r, closure) } return closure } func (g ImportGraph) walk(node string, visited ImportSet) { if _, ok := visited[node]; ok { return } if _, ok := g[node]; !ok { panic(fmt.Sprintf("node (%s) doesn't exist in the graph", node)) } visited[node] = struct{}{} for to := range g[node] { g.walk(to, visited) } }

talked offline: if we detect changes in */testdata/*, run everything; otherwise - do ^^ some version of what Jerome's suggesting.

Done. There's one more edge case we didn't talk about: a package that's not imported by any other packages, and gets deleted (testcase7). It isn't present the import graph either. I throw an error for this case and run tests on all non-filtered packages.

jeromefroe · 2018-04-27T16:52:07Z

utilities/ggd/lib/go.go

+//
+// also filters any `vendor/` directory paths
+func filterChanges(input string, filterDirPatterns []string) bool {
+	for _, ptrn := range filterDirPatterns {


Won't this mean we won't run tests if we just change vendor?

It's an option passed in from the CLI, so up to the user's discretion.

We'd use it in the following modes:
(a) CI in our internal repo which has vendor/ checked in, we would not filter vendor
(b) CI in our OSS repos (where we do not checkin vendor/), we would filter vendor/`

Local development for both would mirror the choices made in the CI.

Wouldn't we then be relying on local CI to verify changes to vendor? I'm kinda of the opinion that if you don't vendor you're dependencies in your repo you need to run tests in CI each time when you pull them.

talked offline: we're going to the change the default to include vendor/. Standard usage for us is going to be run all tests on master, and if we see any glide.yaml/lock changes; otherwise rely on ggd.

jeromefroe · 2018-05-01T08:51:26Z

utilities/ggd/lib/dag.go

+	}
+	if _, ok := g[node]; !ok {
+		// NB(prateek): this happens in the case of un-used deleted packages. Look at testcase7.
+		return DanglingDeleteError{node}


Why return an error here?

need to indicate to main this happened so we can act on it

Yea, I guess my question is why do we need to act on it? At least, it doesn't seem like an error to me if I remove a package that wasn't imported.

It's not, but I'm being paranoid and rerun the entire test suite if this situation occurs.

I see, maybe worth just calling that out?

jeromefroe · 2018-05-01T08:59:29Z

utilities/ggd/main.go

+	defaultCatchallPatterns = []string{
+		"/testdata/", // go list ./... is unable to gauge impact of changes to this directory
+	}
+	defaultFilterPatterns = []string{"/vendor/"}


Should the default for this be empty?

No. Going to stick with this as the default because it's typically what people want when using the tool by hand. I prefer to optimise for that over what the CI invocation for this will be.

go test does this too so it's not unexpected behaviour.

Hmm, but if working locally and I update a vendored dependency wouldn't it be preferable to test the packages which depend on the changed vendor package?

Sure. But in that case, you've updated glide.yaml/glide.lock (added to catchall patterns) and we'd run all tests

or, they can alternatively run with ggd -f='' and get the same behaviour (w/o using glide changes as catch all)

jeromefroe · 2018-05-01T09:01:59Z

utilities/ggd/lib/misc.go

+func ChangedPackages(changedFiles []string) []string {
+	changedPkgsMap := make(map[string]struct{}, len(changedFiles))
+	for _, f := range changedFiles {
+		dir := filepath.Dir(f)


Is it ok to take the whole directory path? Was thinking we might need to potentially remove GOPATH if it's in the prefix so we can ensure we have the correct import path.

the changed files are relative paths already. Will add a comment to indicate.

Hmm, so then the packages in the import statements are relative as well? For example, if I do a git diff in statsdex the paths of files which have changed will be relative to the base of the repo (e.g. services/collector/etc), but if I were to import one of those packages they would use the full Git path (e.g. <hostname>/infra/statsdex/services/collector/etc).

if I were to import one of those packages they would use the full Git path (e.g. /infra/statsdex/services/collector/etc

I don't follow. Go imports are always GOPATH or repo/vendor/ relative, what situation are they not going to be?

jeromefroe

LGTM

prateek · 2018-05-12T16:14:10Z

Boo yah! Many thanks for the reviews @jeromefroe!

prateek force-pushed the prateek/gogitdelta branch from d4674f2 to 1d23755 Compare April 24, 2018 19:08

prateek changed the title ~~[WIP] gogitdelta~~ gogitdiff: incremental build tool Apr 24, 2018

prateek force-pushed the prateek/gogitdelta branch from b898026 to b4b5684 Compare April 24, 2018 22:26

prateek commented Apr 24, 2018

View reviewed changes

prateek force-pushed the prateek/gogitdelta branch 3 times, most recently from 26f78a2 to e22a67b Compare April 25, 2018 05:39

Add 'ggd' tool

0ab136c

prateek force-pushed the prateek/gogitdelta branch from 2bd9362 to 0ab136c Compare April 25, 2018 16:59

prateek requested review from robskillington, jeromefroe and richardartoul April 25, 2018 17:00

m3db deleted a comment from CLAassistant Apr 25, 2018

add another test case

a2cb197

jeromefroe mentioned this pull request Apr 26, 2018

[WIP] Add tool to find changed packages #17

Closed

Update README.md

38e2630

misc cleanup

jeromefroe reviewed Apr 26, 2018

View reviewed changes

richardartoul reviewed Apr 26, 2018

View reviewed changes

prateek added 2 commits April 26, 2018 20:21

address feedback

301ac7c

stash

d0d4f57

jeromefroe reviewed Apr 27, 2018

View reviewed changes

address nits

b8dca14

prateek force-pushed the prateek/gogitdelta branch from 9878563 to b8dca14 Compare April 30, 2018 20:09

jeromefroe reviewed May 1, 2018

View reviewed changes

prateek added 6 commits May 1, 2018 11:53

more comment nits

055eb84

more nits

8d9f2a9

comment nit

3d8f1de

stash

de4b15a

more cleanup

8dfd930

try this

07664e2

jeromefroe approved these changes May 12, 2018

View reviewed changes

prateek merged commit ec41198 into master May 12, 2018

prateek deleted the prateek/gogitdelta branch May 12, 2018 16:14


		visited := make(map[string]string)

		// ensure we add the pkg to the graph

gogitdiff: incremental build tool #21

gogitdiff: incremental build tool #21

Conversation

prateek commented Apr 24, 2018 • edited Loading

codecov bot commented Apr 24, 2018 • edited Loading

Codecov Report

robskillington commented Apr 24, 2018

Choose a reason for hiding this comment

prateek commented Apr 25, 2018

jeromefroe 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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek Apr 30, 2018 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromefroe left a comment

Choose a reason for hiding this comment

prateek commented May 12, 2018 • edited Loading

prateek commented Apr 24, 2018 •

edited

Loading

codecov bot commented Apr 24, 2018 •

edited

Loading

prateek Apr 30, 2018 •

edited

Loading

prateek commented May 12, 2018 •

edited

Loading