Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit.
Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes it. For example, setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
I’d be interested to know what the motivation for this is, are you planning to use this library from multiple binaries, or is it just organizational?
Yes, that is the motivation that I mentioned in the readme.
Also, we compile each file in the library (as of today) multiple times (for each target), so that should be fixed as well.
Also, we compile each file in the library (as of today) multiple times (for each target), so that should be fixed as well.
Can you give an example?
Can you give an example?
setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui)
src/test/util.h and src/test/util.cpp into the library as well (possibly into specific modules, much of it seems to be block and address-related)?
setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui)
Whoa. Yes, then it’s quite a serious issue!
How about moving the content of src/test/util.h and src/test/util.cpp into the library as well (possibly into specific modules, much of it seems to be block and address-related)?
Good point. I think I will leave this for later, so that this changeset can be kept a move-only scripted-diff
0@@ -0,0 +1,11 @@
1+# Test library
2+
3+This contains all files for the test library, which is used by all test binaries (unit tests, benchmarks, fuzzers, gui
4+tests)
0@@ -0,0 +1,11 @@
1+# Test library
2+
3+This contains all files for the test library, which is used by all test binaries (unit tests, benchmarks, fuzzers, gui
4+tests)
5+
6+Generally, the files in this folder should be well separated modules. New code should be added to existing modules or
0@@ -0,0 +1,11 @@
1+# Test library
2+
3+This contains all files for the test library, which is used by all test binaries (unit tests, benchmarks, fuzzers, gui
4+tests)
5+
6+Generally, the files in this folder should be well separated modules. New code should be added to existing modules or
7+(when in doubt) a new module should be created.
8+
9+The utilities in here are compiled into a library, which does not hold any state. However, the main file `setup_common`
setup_common.cpp, setup_common.h, or both?
Added a dot at the end of the sentence, among other minor doc fixups as requested by @jonatack
No code changes, so the previous ci builds are still valid.
Code review ACK fa6fb57837f7d0db90eb4bdf3b644ba540db1805. Seems good to separate test utilities from unit tests, since it’s natural for things like benchmarks, fuzzing, or simulation tools to depend on test utilities, but you wouldn’t want those things depending on unit tests.
Feel free to ignore this suggestion, but I think it’d be better to rename src/test/lib directory back to src/test/util. It’s normal to see lib prefixes in library paths (especially since they get added automatically by the linker), but not really normal to see lib/ in #include strings, and not something we have for any of our other libraries.
-BEGIN VERIFY SCRIPT-
# Move files
for f in $(git ls-files src/test/lib/); do git mv $f src/test/util/; done
git mv src/test/setup_common.cpp src/test/util/
git mv src/test/setup_common.h src/test/util/
# Replace Windows paths
sed -i -e 's|\\setup_common|\\util\\setup_common|g' $(git grep -l '\\setup_common')
sed -i -e 's|src\\test\\lib\\|src\\test\\util\\|g' build_msvc/test_bitcoin/test_bitcoin.vcxproj
# Everything else
sed -i -e 's|/setup_common|/util/setup_common|g' $(git grep -l 'setup_common')
sed -i -e 's|test/lib/|test/util/|g' $(git grep -l 'test/lib/')
# Fix include guard
sed -i -e 's|BITCOIN_TEST_SETUP_COMMON_H|BITCOIN_TEST_UTIL_SETUP_COMMON_H|g' ./src/test/util/setup_common.h
sed -i -e 's|BITCOIN_TEST_LIB_|BITCOIN_TEST_UTIL_|g' $(git grep -l 'BITCOIN_TEST_LIB_')
-END VERIFY SCRIPT-
lib was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two.
Both travis and appveyor passes (so this fixes the appveyor build). Anything left to do here?
This hasn’t had a ton of review, but it seems fairly safe to merge as it only affects test code.
I agree. I am concept ACK on both organizing all shared functions in one place, and on eventually turning this into a library. The code changes look good to me, but I didn’t test.
Just noting that the added documentation in src/test/util/README.md describes the goal but not the reality after this PR, but I think fixing the Appveyor problem is more important than that.
Also, we compile each file in the library (as of today) multiple times (for each target), so that should be fixed as well.
And are those always compiled with the same flags etc?