Fix various issues in the test makefiles #1032

torarnv · 2015-04-07T15:50:45Z

Fix broken glob dependency from test bin targets
Ensure sharness test (clean, build, test, aggregate) is run sequentially
Ensure we don't remake binaries due to missing IPFS-BUILD-OPTIONS

jbenet · 2015-04-07T15:53:29Z

Makefile


-test_sharness_short:
-	cd test/sharness/ && make
+test_sharness_expensive: SHARNESS_MAKE_FLAGS += TEST_EXPENSIVE=1


i prefer explicitness in these targets. adding flags like that is errorprone in my opinion.

Fine by me, not married to 7acee02 and 086f038, those were just drive-by changes on the way to the other two. Personally I think duplicated code is more error prone, as things are changed one place but missed in another, and as a new contributor to a project I find logic easier to understand when I don't have to scan 4 instances of it to see if there's any difference in them.

torarnv · 2015-04-07T15:55:58Z

@chriscool 📟

jbenet · 2015-04-07T15:57:10Z

test/sharness/Makefile

@@ -14,6 +14,9 @@ IPFS_ROOT = ../..
 # User might want to override those on the command line
 GOFLAGS =

+# The targets in this makefile depend on being run sequentially
+.NOTPARALLEL:


👍 i would love to move towards being able to run them in parallel. wonder what git does + how it would manage the sharness output. cc @chriscool

We may get away with adding explicit deps in the makefile. I wasn't sure if the tests themselves could be run in parallell. The main problem that spawned this is that with make -j4 eg, the tests will start running before the clean and the deps targets have been run, which resulted in the missing ipfs binary error. Let me try that real quick.

To run tests in parallel I think we need something like:

all: make clean make deps make $(T) make aggregate

and then use make -j 4 for example.

b8bc885 adds the proper deps. I'm sure if the tests actually support the parallelism, so .NOTPARALLEL: should possibly be added either way.

@jbenet about Git I had a look and yeah some work has been done a long time ago so that the tests can run in parallel (see git/git@abc5d37).

And there are always some flags or options or other stuff in the IPFS-BUILD-OPTIONS like files, so that there is no problem with empty files.

jbenet · 2015-04-07T15:59:42Z

copying comments from the commit:

We dont want to have to rebuild on every test run-- this is unnecessarily slow.
if **/*.go doesnt work with gnu make, maybe we can use something like
$(shell find $(IPFS_ROOT) | egrep ".go$")
(not sure if this works)

jbenet · 2015-04-07T16:00:42Z

i dont feel strongly about most comments above. only the rebuilds.

let's hear @chriscool's opinions?

jbenet · 2015-04-07T16:02:49Z

Also, good find on both the parallel and gnu make bugs.

torarnv · 2015-04-07T16:41:13Z

@chriscool

the IPFS-BUILD-OPTIONS target depends on FORCE but that doesn't mean that we are re-building the binaries on every invocation of make.

Actually, as long as $(GOFLAGS) is never set, we never write IPFS-BUILD-OPTIONS, so the dependency will always fail, and we'll re-make the binaries every time:

{vegas:~/dev/ipfs/src/github.com/ipfs/go-ipfs/test} $ rm -f IPFS-BUILD-OPTIONS
{vegas:~/dev/ipfs/src/github.com/ipfs/go-ipfs/test} $ make bin/ipfs --debug
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i386-apple-darwin11.3.0
Reading makefiles...
Updating goal targets....
   File `IPFS-BUILD-OPTIONS' does not exist.
     File `FORCE' does not exist.
    Must remake target `FORCE'.
    Successfully remade target file `FORCE'.
  Must remake target `IPFS-BUILD-OPTIONS'.
 Prerequisite `IPFS-BUILD-OPTIONS' of target `bin/ipfs' does not exist.
Must remake target `bin/ipfs'.
*** installing bin/ipfs ***
go build  -o bin/ipfs ../cmd/ipfs
{vegas:~/dev/ipfs/src/github.com/ipfs/go-ipfs/test} $ make bin/ipfs --debug
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i386-apple-darwin11.3.0
Reading makefiles...
Updating goal targets....
   File `IPFS-BUILD-OPTIONS' does not exist.
     File `FORCE' does not exist.
    Must remake target `FORCE'.
    Successfully remade target file `FORCE'.
  Must remake target `IPFS-BUILD-OPTIONS'.
 Prerequisite `IPFS-BUILD-OPTIONS' of target `bin/ipfs' does not exist.
Must remake target `bin/ipfs'.
*** installing bin/ipfs ***
go build  -o bin/ipfs ../cmd/ipfs

I'll fix that in a separate commit.

torarnv · 2015-04-07T18:13:51Z

Okey, ready for review now, removed the subjective refactoring and focused on fixing the issues.

If the file doesn't exist, make will conclude that the missing prerequisite should trigger a rebuild of the binaries.

Running make -jN would result in the tests starting to execute before the tests binaries were built, resulting in the error: "Cannot find the tests' local ipfs tool" Each test now depends on the deps. They also depend on a new target for cleaning the test results, so that the tests can write new clean results. The aggregate target also needs to depend on the same test results clean target, as well as the tests themselves, so that the aggregation happens when all tests have finished running. By introducing a separate target for cleaning test results we also ensure that we don't end up removing and rebuilding the binary on each test run. The result is that the tests *can* be run with with -jN > 1, but individual tests may still not supports this, so to get stable test results it's still recommended to run them in sequence.

GNU Make's wildcard function does not recurse into subdirectories when passed the '**' glob, which results in adding a dependency only to .go files in the first level of subdirectories under the source root. We shell out to 'find' instead, which catches all .go files in the given directory.

chriscool · 2015-04-07T19:34:21Z

Ok, it LGTM, thanks @torarnv !

torarnv · 2015-04-07T19:44:04Z

Thanks! Happy to help! 😄

jbenet · 2015-04-08T00:51:36Z

Thanks @torarnv !

Fix various issues in the test makefiles

whyrusleeping added the ready label Apr 7, 2015

jbenet added backlog and removed ready labels Apr 7, 2015

jbenet reviewed Apr 7, 2015
View reviewed changes

torarnv force-pushed the cleanup-makefiles branch from ffc37de to 80271a9 Compare April 7, 2015 17:00

torarnv changed the title ~~Clean up test makefiles~~ Fix various issues in the test makefiles Apr 7, 2015

torarnv added 3 commits April 7, 2015 21:09

Ensure IPFS-BUILD-OPTIONS build dependency is created

d36a9ee

If the file doesn't exist, make will conclude that the missing prerequisite should trigger a rebuild of the binaries.

torarnv force-pushed the cleanup-makefiles branch from 7358b82 to f2dd060 Compare April 7, 2015 19:22

jbenet added a commit that referenced this pull request Apr 8, 2015

Merge pull request #1032 from torarnv/cleanup-makefiles

20dbea5

Fix various issues in the test makefiles

jbenet merged commit 20dbea5 into ipfs:master Apr 8, 2015

jbenet removed the backlog label Apr 8, 2015

torarnv deleted the cleanup-makefiles branch April 8, 2015 01:00

aschmahmann mentioned this pull request May 14, 2021

Release v0.9.0 #8058

Closed

71 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various issues in the test makefiles #1032

Fix various issues in the test makefiles #1032

torarnv commented Apr 7, 2015

jbenet Apr 7, 2015

torarnv Apr 7, 2015

torarnv commented Apr 7, 2015

jbenet Apr 7, 2015

torarnv Apr 7, 2015

chriscool Apr 7, 2015

torarnv Apr 7, 2015

chriscool Apr 7, 2015

jbenet commented Apr 7, 2015

jbenet commented Apr 7, 2015

jbenet commented Apr 7, 2015

torarnv commented Apr 7, 2015

torarnv commented Apr 7, 2015

chriscool commented Apr 7, 2015

torarnv commented Apr 7, 2015

jbenet commented Apr 8, 2015

Fix various issues in the test makefiles #1032

Fix various issues in the test makefiles #1032

Conversation

torarnv commented Apr 7, 2015

jbenet Apr 7, 2015

Choose a reason for hiding this comment

torarnv Apr 7, 2015

Choose a reason for hiding this comment

torarnv commented Apr 7, 2015

jbenet Apr 7, 2015

Choose a reason for hiding this comment

torarnv Apr 7, 2015

Choose a reason for hiding this comment

chriscool Apr 7, 2015

Choose a reason for hiding this comment

torarnv Apr 7, 2015

Choose a reason for hiding this comment

chriscool Apr 7, 2015

Choose a reason for hiding this comment

jbenet commented Apr 7, 2015

jbenet commented Apr 7, 2015

jbenet commented Apr 7, 2015

torarnv commented Apr 7, 2015

torarnv commented Apr 7, 2015

chriscool commented Apr 7, 2015

torarnv commented Apr 7, 2015

jbenet commented Apr 8, 2015