Refactor includes (attempt #2, with absolute paths)

aleksejspopovs commented 7 years ago

This pull request replaces and closes #77.

This pull request:

makes the paths in all local includes (i.e. those including files from within libsnark) directory-qualified (see pull request 8 in libff for justification)
converts all libff/libfqfft includes from local to system-style (e.g. #include "common/utils.hpp" → #include <libff/common/utils.hpp>) to make it explicit and unambiguous when files from an external library are being included.

This pull request (as well as the accompanying ones in libff and libfqfft) was largely generated using a Python script.

tromer commented 7 years ago

Looks good to me. However, I'm now worried about the cost when merging with other contributors (I know of several ongoing ones): I think automatic merging will completely fail due to the renames. We could use symlinks (libsnark --> src etc.) instead of renaming. Will this work on modern Windows and Mac?

aleksejspopovs commented 7 years ago

Although you are correct that automatic merging will fail, git rebase works perfectly --- I just tried locally rebasing my branch multiexp-perf on top of absolute-includes in libff, and I only had one little conflict, which was due to me adding an extra include in one file in multiexp-perf (so this would've been a conflict regardless of the strategy we chose for this refactoring, and rightly so). After resolving that, the rest of the rebase worked just fine and the resulting branch had no conflicts with absolute-includes.

Pull requests should usually be rebased before submission anyway to ensure that they are following the latest version of the original source code.

I've never seen a symlink in a git repository. A Google search suggests that they work more or less the way we'd expect them to on filesystems other than FAT, but this solution still seems kind of hackish and confusion-inducing to me (e.g., if libsnark is a symlink to src, you can't do git add libsnark/something.cpp).

madars commented 7 years ago

I agree with @popoffka: pull requests should be rebased and adding a symlink would just obscure the changes. With a symllink simple examples will continue to work, but once you've added/removed/modified at least one include the code won't merge anymore.

tromer commented 7 years ago

OK, then I'm fine with merging this. @howardwu, OK with you?

howardwu commented 7 years ago

SGTM in general.

@popoffka Can you confirm your PR builds as expected in your client? I see the following compilation error (at the referenced commit hash below):

[ 78%] Building CXX object libsnark/CMakeFiles/snark.dir/common/data_structures/set_commitment.cpp.o
In file included from /home/vagrant/libsnark-c1df6b5a027762545b821921bf0eec17c7bda0ec/./libsnark/common/data_structures/set_commitment.hpp:15:0,
                 from /home/vagrant/libsnark-c1df6b5a027762545b821921bf0eec17c7bda0ec/libsnark/common/data_structures/set_commitment.cpp:8:
/usr/local/include/libff/common/utils.hpp:61:122: fatal error: common/utils.tcc: No such file or directory
 #include "common/utils.tcc" /* note that utils has a templatized part (utils.tcc) and non-templatized part (utils.cpp) */
                                                                                                                          ^
compilation terminated.
make[2]: *** [libsnark/CMakeFiles/snark.dir/common/data_structures/set_commitment.cpp.o] Error 1
make[1]: *** [libsnark/CMakeFiles/snark.dir/all] Error 2
make: *** [all] Error 2

howardwu commented 7 years ago

@tromer I plan to turn on continuous integration for candidate-master soon, so we won't have to manually check these things in PRs. I'll start an email thread on this.

aleksejspopovs commented 7 years ago

@howardwu that's expected — you need to also apply the changes from the corresponding pull request in libff. Sorry I didn't make that clear.

howardwu commented 7 years ago

@popoffka thanks and wonderful work! This should make development easier and IDE integration more fluid.

LGTM. Builds and passes tests - make check output:

Test project /home/vagrant/libsnark-c1df6b5a027762545b821921bf0eec17c7bda0ec/build
      Start  1: common_routing_algorithms_test
 1/24 Test  #1: common_routing_algorithms_test ...................   Passed    2.53 sec
      Start  2: gadgetlib1_simple_test
 2/24 Test  #2: gadgetlib1_simple_test ...........................   Passed    0.30 sec
      Start  3: gadgetlib1_r1cs_ppzksnark_verifier_gadget_test
 3/24 Test  #3: gadgetlib1_r1cs_ppzksnark_verifier_gadget_test ...   Passed    4.79 sec
      Start  4: gadgetlib1_fooram_test
 4/24 Test  #4: gadgetlib1_fooram_test ...........................   Passed  484.88 sec
      Start  5: gadgetlib2_adapters_test
 5/24 Test  #5: gadgetlib2_adapters_test .........................   Passed    0.03 sec
      Start  6: gadgetlib2_constraint_test
 6/24 Test  #6: gadgetlib2_constraint_test .......................   Passed    0.03 sec
      Start  7: gadgetlib2_gadget_test
 7/24 Test  #7: gadgetlib2_gadget_test ...........................   Passed    0.05 sec
      Start  8: gadgetlib2_integration_test
 8/24 Test  #8: gadgetlib2_integration_test ......................   Passed    0.12 sec
      Start  9: gadgetlib2_protoboard_test
 9/24 Test  #9: gadgetlib2_protoboard_test .......................   Passed    0.03 sec
      Start 10: gadgetlib2_variable_test
10/24 Test #10: gadgetlib2_variable_test .........................   Passed    0.05 sec
      Start 11: relations_qap_test
11/24 Test #11: relations_qap_test ...............................   Passed   32.99 sec
      Start 12: relations_ssp_test
12/24 Test #12: relations_ssp_test ...............................   Passed   28.95 sec
      Start 13: zk_proof_systems_bacs_ppzksnark_test
13/24 Test #13: zk_proof_systems_bacs_ppzksnark_test .............   Passed    0.12 sec
      Start 14: zk_proof_systems_r1cs_ppzksnark_test
14/24 Test #14: zk_proof_systems_r1cs_ppzksnark_test .............   Passed    0.41 sec
      Start 15: zk_proof_systems_r1cs_gg_ppzksnark_test
15/24 Test #15: zk_proof_systems_r1cs_gg_ppzksnark_test ..........   Passed    0.32 sec
      Start 16: zk_proof_systems_r1cs_sp_ppzkpcd_test
16/24 Test #16: zk_proof_systems_r1cs_sp_ppzkpcd_test ............   Passed  443.34 sec
      Start 17: zk_proof_systems_ram_ppzksnark_test
17/24 Test #17: zk_proof_systems_ram_ppzksnark_test ..............   Passed   46.24 sec
      Start 18: zk_proof_systems_ram_zksnark_test
18/24 Test #18: zk_proof_systems_ram_zksnark_test ................   Passed  485.05 sec
      Start 19: zk_proof_systems_tbcs_ppzksnark_test
19/24 Test #19: zk_proof_systems_tbcs_ppzksnark_test .............   Passed    0.08 sec
      Start 20: zk_proof_systems_uscs_ppzksnark_test
20/24 Test #20: zk_proof_systems_uscs_ppzksnark_test .............   Passed    0.35 sec
      Start 21: test_knapsack_gadget
21/24 Test #21: test_knapsack_gadget .............................   Passed    0.04 sec
      Start 22: test_merkle_tree_gadgets
22/24 Test #22: test_merkle_tree_gadgets .........................   Passed   17.53 sec
      Start 23: test_set_commitment_gadget
23/24 Test #23: test_set_commitment_gadget .......................   Passed   17.47 sec
      Start 24: test_sha256_gadget
24/24 Test #24: test_sha256_gadget ...............................   Passed    0.10 sec

100% tests passed, 0 tests failed out of 24

Total Test time (real) = 1565.96 sec

scipr-lab / libsnark

Refactor includes (attempt #2, with absolute paths) #78