Makefile rework and sharness test coverage #3504

Kubuxu · 2016-12-14T00:24:11Z

This PR introduces non-recursive Makefile infrastructure that replaces current Makefile infrastructure.
It also generally cleanups the Makefiles, separates them into nicer sub-modules and centralizes common operations into single definitions.

Why non-recursive Makefile?

It allows to depend on any target that is defined in the makefile, this means that for example gx install is called once when make build test_expensive_sharness is called instead of 4 or 5 times.

It also makes the dependencies much cleaner and allows for reuse of modules. For example sharness coverage collection uses sharness target with amended PATH, previously it might have been possible but not without wiring the coverage collection into sharness Makefile runner code.

Isn't non-recursive Makefile much more complex?

Yes, it is a bit more complex but not much more. There are few rules that have to be followed and few complexities added but IMHO it is worth it.

How to NR-make:

If make is to generate some file via a target, it MUST be defined in Rules.mk file in the directory of the target.
Rules.mk file MUST have include mk/header.mk statement as the first line and include mk/footer.mk statement as the last line (apart from project root Rules.mk).
It then MUST be included by the closest Rules.mk file up the directory tree.
Inside a Rules.mk special variable accessed as $(d) is defined. Its value is current directory, use it so if the Rules.mk file is moved in the tree it still works without a problem. Caution: this variable is not available in the recipe part and MUST NOT be used there. Use name of the target or prerequisite to extract it if you need it.
Make has only one global scope, this means that name conflicts are a thing. Names SHOULD follow VAR_NAME_$(d) convention. There are exceptions from this rule in form of well defined global variables. Examples: General lists TGT_BIN, CLEAN; General targets: TEST, COVERAGE; General variables: GOFLAGS, DEPS_GO.
Any rules, definitions or variables that fit some family SHOULD be defined in mk/$family.mk file and included from project root Rules.mk

(Based on https://evbergen.home.xs4all.nl/nonrecursive-make.html)

Resolves: #3488

Kubuxu · 2016-12-16T18:00:57Z

It finally works again. Teamcity OSX and js-ipfs-api test failures are unrelated.

67 commits bye-bye.

Kubuxu · 2016-12-16T18:08:35Z

The sharness test with coverage is about 4 to 6 min + 2min for coverage report merge procedure.
The unit test with coverage takes about 8 to 10 min.

Kubuxu · 2016-12-16T18:53:38Z

@lgierth @whyrusleeping this should be ready for CR

Kubuxu · 2016-12-19T17:53:56Z

I have figured out smart way to do cross package coverage. The cross package unit test coverage increased the test time only by 10% over normal coverage unit tests.

kevina · 2016-12-19T20:09:49Z

I have some experience with Makefiles, I am also going to have a look.

Kubuxu · 2016-12-19T20:14:28Z

cmd/ipfs/Rules.mk

+# DEPS_OO_$(d) += pin/internal/pb/header.pb.go unixfs/pb/unixfs.pb.go
+
+# uses second expansion to collect all $(DEPS_GO)
+$(IPFS_BIN_$(d)): $(d) $$(DEPS_GO) ALWAYS #| $(DEPS_OO_$(d))


I missed the ldflags for the current hash.

kevina · 2016-12-19T20:21:51Z

Also, this looks like it is going to disrupt my workflow so I want a chance to try it. Added myself as a reviewer. Should be able to look at it this week.

kevina · 2016-12-19T22:18:11Z

The makefile is a bit slow. For example:

$ /usr/bin/time make
...
0.92user 0.16system 0:00.96elapsed 114%CPU (0avgtext+0avgdata 18244maxresident)k
8inputs+64outputs (0major+23775minor)pagefaults 0swaps

That is around a second to do nothing.

kevina · 2016-12-19T22:25:45Z

@Kubuxu would it be okay if I add some dummy Makefiles in some key sub-directories that just call the top level Makefile? For example I often do:

make -j4 > log

in the shareness directory. I also sometimes do

make deps

within the shareness directory and then run a failing test manually. Now I have to type something a lot longer to get the same effect.

kevina

From what I can tell it looks good. I left two general comments but neither of them are critical.

whyrusleeping

This PR conflates a lot of real changes with refactor. I would really appreciate breaking this apart into isolated changes as much as we can.

whyrusleeping · 2016-12-20T18:26:44Z

bin/Rules.mk

+
+dist_root_$(d)=/ipfs/QmNZL8wNsvAGdVYr8uGeUE9aGfHjFpHegAWywQFEdSaJbp
+
+$(d)/gx: $(d)/gx-v0.9.0


See point number 4 in the PR comment.

whyrusleeping · 2016-12-20T18:27:34Z

bin/gen-make-dep-chain

@@ -0,0 +1,9 @@
+#!/bin/sh
+(for p ; do


what is going on here? where is p coming from? i'm so confused. what is tac?

This script is not used right now, committed as I thought I would use it, tac is cat that reverses lines, for p; is equivalent to for p in "@";

whyrusleeping · 2016-12-20T18:29:04Z

circle.yml

@@ -35,6 +35,6 @@ dependencies:

 test:
  override:
-    - case $CIRCLE_NODE_INDEX in 0) make coverage ;; 1) make test_sharness_expensive ;; esac:
+    - case $CIRCLE_NODE_INDEX in 0) make -j 1 coverage/unit_tests.coverprofile && bash <(curl -s https://codecov.io/bash) -cF unittests -X search -f coverage/unit_tests.coverprofile;; 1) make -j 1 coverage/sharness_tests.coverprofile &&  bash <(curl -s https://codecov.io/bash) -cF sharness -X search -f coverage/sharness_tests.coverprofile;; esac:


does this really need to be a one-liner? Its very... uh... Kubuxuy :P

I can make it separate script, AFAIK yaml doesn't support multiline keys.

Ah, makes sense. Having it be a separate script (with comments explaining its existence) might help readability a bit

whyrusleeping · 2016-12-20T18:38:35Z

cmd/ipfs/Rules.mk

+$(COVER_BIN_$(d)): $(d) $$(DEPS_GO) ALWAYS
+	$(eval TMP_PKGS := $(shell go list -f '{{range .Deps}}{{.}} {{end}}' $(go-flags-with-tags) ./cmd/ipfs | sed 's/ /\n/g' | grep ipfs/go-ipfs | grep -v ipfs/go-ipfs/Godeps) $(call go-pkg-name,$<))
+	$(eval TMP_LIST := $(call join-with,$(comma),$(TMP_PKGS)))
+	@echo go test $@ -c -covermode atomic -coverpkg ... $(go-flags-with-tags) ./$(@D) # for info


what is @D ?

It is short form of $(dir $@)

whyrusleeping · 2016-12-20T18:40:08Z

cmd/ipfs/runmain_test.go

+	os.Args = append([]string{os.Args[0]}, args...)
+	ret := mainRet()
+
+	p := os.Getenv("IPFS_COVER_RET_FILE")


what is this used for?

I can't return value from test binary, it has to exit naturally, so I save the to be return value to a file and return it from wrapper program, coverage/main/main.go

whyrusleeping · 2016-12-20T18:42:38Z

exchange/bitswap/message/pb/Rules.mk

@@ -0,0 +1,8 @@
+include mk/header.mk


is this include relative to the root dir? That feels really weird...

Yes, the working dir is constant and it is the root dir, the current working dir is under $(d).

whyrusleeping · 2016-12-20T18:44:06Z

pin/pin.go

-		os.Exit(1)
+		msg := "failed to decode empty key constant"
+		log.Error(msg)
+		panic(msg)


whats the reason for this change? I'd prefer not the have the user see an ugly panic stack trace.

I didn't want to exit over there as it would change the coverage, but as it shouldn't happen here I will switch back to Exit.

Kubuxu · 2016-12-20T20:01:33Z

@kevina 3d84db9 should resolve the running make in sharness dir and 6b38da2 cuts down the flat time by half by not running coverage Rules.mk file unless coverage category or clean targets are used.

whyrusleeping · 2017-02-12T00:15:45Z

@Kubuxu Is this ready? It seems to be failing on circleCI

This commit introduces non-recursive Makefile infrastructure that replaces current Makefile infrastructure. It also generally cleanups the Makefiles, separates them into nicer sub-modules and centralizes common operations into single definitions. It allows to depend on any target that is defined in the makefile, this means that for example `gx install` is called once when `make build test_expensive_sharness` is called instead of 4 or 5 times. It also makes the dependencies much cleaner and allows for reuse of modules. For example sharness coverage collection (WIP) uses sharness target with amended PATH, previously it might have been possible but not without wiring in the coverage collection into sharness make runner code. Yes, it is more complex but not much more. There are few rules that have to be followed and few complexities added but IMHO it is worth it. How to NR-make: 1. If make is to generate some file via a target, it MUST be defined in Rules.mk file in the directory of the target. 2. `Rules.mk` file MUST have `include mk/header.mk` statement as the first line and `include mk/footer.mk` statement as the last line (apart from project root `Rules.mk`). 3. It then MUST be included by the closest `Rules.mk` file up the directory tree. 4. Inside a `Rules.mk` special variable accessed as `$(d)` is defined. Its value is current directory, use it so if the `Rules.mk` file is moved in the tree it still works without a problem. Caution: this variable is not available in the recipe part and MUST NOT be used. Use name of the target or prerequisite to extract it if you need it. 5. Make has only one global scope, this means that name conflicts are a thing. Names SHOULD follow `VAR_NAME_$(d)` convention. There are exceptions from this rule in form of well defined global variables. Examples: General lists `TGT_BIN`, `CLEAN`; General targets: `TEST`, `COVERAGE`; General variables: `GOFLAGS`, `DEPS_GO`. 3. Any rules, definitions or variables that fit some family SHOULD be defined in `mk/$family.mk` file and included from project root `Rules.mk` License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>