autotools: Clean up after adding Wycheproof #1277

pull real-or-random wants to merge 6 commits into bitcoin-core:master from real-or-random:202304-makefile-wycheproof changing 1 files +25 −22
  1. real-or-random commented at 6:09 am on April 14, 2023: contributor

    Follow-up to #1245.

    This builds on top of #1276. Let’s only merge #1276 as a hotfix for the Core build.

  2. autotools: Move Wycheproof header from EXTRA_DIST to noinst_HEADERS 529b54d922
  3. in Makefile.am:232 in 3111cc2785 outdated 
    232+
233+src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h:
234+	mkdir -p $(@D)
235+	python3 tools/tests_wycheproof_generate.py src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > $@
236+
237+testvectors: $(TESTVECTORS)
    hebasto commented at 11:09 am on April 14, 2023: 
    0.PHONY: testvectors
1testvectors: $(TESTVECTORS)
    hebasto commented at 2:38 pm on April 22, 2023:
    nm, ignore this ^
  4. hebasto commented at 11:09 am on April 14, 2023: member

    In 3955f8f9940f35a37faa01c08d17795adf8ceb43

    autotools: Rename “pregenerated” clean targets and make them .PHONY

    This follows the automake conventions, see: https://www.gnu.org/software/automake/manual/html_node/Clean.html

    The clean-precomp was .PHONY: https://github.com/bitcoin-core/secp256k1/blob/3bab71cf0534fd5948386de2b6ac31a7044ad76b/Makefile.am#L1

    A comment needs to be updated: https://github.com/bitcoin-core/secp256k1/blob/3bab71cf0534fd5948386de2b6ac31a7044ad76b/Makefile.am#L205

    Why not just MAINTAINERCLEANFILES = $(PRECOMP) $(TESTVECTORS)?

  5. hebasto commented at 11:11 am on April 14, 2023: member 
     0--- a/ci/cirrus.sh
 1+++ b/ci/cirrus.sh
 2@@ -109,7 +109,7 @@ fi
 3 # Rebuild precomputed files (if not cross-compiling).
 4 if [ -z "$HOST" ]
 5 then
 6-    make clean-precomp clean-testvectors
 7+    make maintainer-clean
 8     make precomp testvectors
 9 fi
10

    to fix CI?

  6. real-or-random commented at 11:50 am on April 14, 2023: contributor

    Why not just MAINTAINERCLEANFILES = $(PRECOMP) $(TESTVECTORS)?

    Yeah, that would work, too. But I think it’s nice to have the ability to clean precomp or testvectors separately. I’ll address the other comments.

    (Apparently it was a good idea that I opened a separate PR for the first commit.)

  7. autotools: Move code around to tidy Makefile 08f4b1632d
  8. real-or-random force-pushed on Apr 19, 2023
  9. real-or-random force-pushed on Apr 19, 2023
  10. real-or-random commented at 2:15 pm on April 19, 2023: contributor

    I changed my mind and removed the renaming. It’s not clear that a “maintainer”-prefix is better… Sorry for the back and forth.

    Independently, make maintainer-clean in the Cirrus script won’t work because that target also removes the Makefile.

  11. real-or-random cross-referenced this on Apr 19, 2023 from issue build: Move generation of prebuilt files out of build system by real-or-random
  12. in Makefile.am:229 in 2dd672a287 outdated 
    226+### Pregenerated test vectors
227+### (see the comments in the previous section for detailed rationale)
228+TESTVECTORS = src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h
229+
230+src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h:
231+	mkdir -p $(@D)
    hebasto commented at 2:30 pm on April 22, 2023:

    For more robustness, could consider

    0	$(MKDIR_P) $(@D)

    with the AC_PROG_MKDIR_P macro in the configure.ac?

    real-or-random commented at 10:37 am on April 25, 2023:

    As I understand the history of this, the reason why this macro exists is that some implementations of mkdir -p on some old systems are not thread safe. And that’s a problem in a parallel make, where multiple make processes try to create the same directory (see https://lists.gnu.org/r/automake/2006-04/msg00084.html).

    I don’t see how this could be a problem here, given that there’s only a single recipe in the Makefile that creates this directory. That’s why I think it’s better to stick to mkdir -p here for simplicity.

  13. in Makefile.am:230 in 2dd672a287 outdated 
    227+### (see the comments in the previous section for detailed rationale)
228+TESTVECTORS = src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h
229+
230+src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.h:
231+	mkdir -p $(@D)
232+	python3 tools/tests_wycheproof_generate.py src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > $@
    hebasto commented at 2:32 pm on April 22, 2023:

    2dd672a287a0c9c4382ffbf56f690727f2e74696

    To make it work in a VPATH build, the diff required as follows:

    0	python3 $(top_srcdir)/tools/tests_wycheproof_generate.py $(top_srcdir)/src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json > $@
    real-or-random commented at 3:10 pm on April 25, 2023:
    added a commit
  14. in Makefile.am:204 in 2dd672a287 outdated 
    200@@ -202,7 +201,7 @@ precompute_ecmult_gen_LDADD = $(COMMON_LIB)
201 # otherwise make's decision whether to rebuild them (even in the first
202 # build by a normal user) depends on mtimes, and thus is very fragile.
203 # This means that rebuilds of the prebuilt files always need to be
204-# forced by deleting them, e.g., by invoking `make clean-precomp`.
205+# forced by deleting them, e.g., using `make maintainer-clean`.
    hebasto commented at 2:35 pm on April 22, 2023:
    Considering that make maintainer-clean removes Makefile, could this comment be confusing if someone follows it blindly?
    real-or-random commented at 3:09 pm on April 25, 2023:
    I just removed that part.
  15. autotools: Use same conventions for all pregenerated files e1b9ce8811
  16. autotools: Make all "pregenerated" targets .PHONY 
    This follows the automake conventions more, see:
https://www.gnu.org/software/automake/manual/html_node/Clean.html
    8764034ed5
  17. autotools: Create src/wycheproof dir before creating file in it 
    This directory may not exist in a VPATH build,
see https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1502994264 .
    2418d3260a
  18. autotools: Take VPATH builds into account when generating testvectors 7e977b3c50
  19. real-or-random force-pushed on Apr 25, 2023
  20. hebasto approved
  21. hebasto commented at 9:31 pm on April 25, 2023: member
    ACK 7e977b3c5071fc17575ff88ebbc9db7b17c70497
  22. real-or-random merged this on Apr 27, 2023
  23. real-or-random closed this on Apr 27, 2023
  24. azuchi cross-referenced this on May 8, 2023 from issue Port bitcoin secp256k1 release 0.3.1 fixes by Naviabheeman
  25. sipa referenced this in commit b4eb644b6c on May 12, 2023
  26. hebasto referenced this in commit 49c52ea2b1 on May 13, 2023
  27. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  28. sipa referenced this in commit 901336eee7 on Jun 21, 2023
  29. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  30. jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick


real-or-random hebasto

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 19:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me