-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Makefile: build precomputed_ecmult generators using build-system toolchain to support cross-compiling #1159
Conversation
When cross-compiling libsecp256k1, if the `precomputed_ecmult*.c` source files need to be regenerated, then the generators need to be built for the *build* system, not for the *host* system. Autoconf supports this fairly cleanly via the `AX_PROG_CC_FOR_BUILD` macro (from Autoconf Archive), but Automake requires some hackery. When building the generators, we override the `CC`, `CFLAGS`, `CPPFLAGS`, and `LDFLAGS` variables to their build-system counterparts, whose names are suffixed with `_FOR_BUILD` and whose values are populated by the aforementioned Autoconf macro and may be overridden on the `make` command line. Since Automake lacks support for overriding `EXEEXT` on a per-program basis, we define a recipe that builds the generator binaries with names suffixed with `$(EXEEXT)` and then renames them suffixed with `$(BUILD_EXEEXT)`.
Note: This PR introduces a dependency on Autoconf Archive. If this is undesirable, then the Addendum: It looks like the automatic tests are failing because the build environment lacks Autoconf Archive. I don't know enough about Cirrus to fix that. Help? |
The precomputed_* files don't need to be regenerated. They're independent of the host or target build environment, so they can be created on any system, and compiled on any system. |
@sipa: The precomputed files need to be regenerated if you specify non-default values for |
An example illustrating the failure:
The same example after applying this PR:
|
This has a bit of a history. We used to support The current resolution (as of #988) is to have the precomputed files in the repo. There should be only two reasons to rebuild them:
See the docs:
I see in your example that you want a SIZE of 2. And I see that you run (And maybe try a SIZE of 4. It should be much faster than 2 but still need only a negligible amount of memory.) (And even if you want > 15, just delete |
@real-or-random: Thank you for the history review. It's a shame that a few edge cases had to ruin it for everyone. I do have to wonder if the issues you mentioned were due to Autotools not being used correctly, but I guess that ship has sailed. My interest comes not as a user but as a packager. Gentoo takes a purist approach to open-source software: anything than can be rebuilt from sources ought to be. Standard operating procedure is to discard pregenerated assets and rebuild them from sources wherever possible. This is what the ebuilds for libsecp256k1 do with the precomputed ecmult tables. We have had to employ a hack to support cross-compiling, whereas most software packages that use Autotools and that compile tools to be executed during the build apparently have no trouble using I'll withdraw my request for proper cross-compilation support and continue carrying my patch downstream. Thank you for your time and explanation. |
Thanks for your understanding. And thanks for reaching out! We don't typically get feedback from packagers, and I see people applying (sometimes weird) patches without having talked to us.
I see. I was a Gentoo user in the past, but that's maybe a little bit too purist for my taste (though there's apparently a "wherever possible" exception). I'm not sure if this helps you, but maybe a simpler patch is to invoke a compiler on the build system manually. We try to make sure that the library is very easy to build manually, even though that's currently not documented. I had a look at your ebuild at https://gitlab.com/bitcoin/gentoo/-/blob/d783d4d95cc1e7befeecec30a2d1f9e0d3a5ad72/dev-libs/libsecp256k1/libsecp256k1-0.1.0_pre20220803.ebuild#L73-74
This should really be
(We should really just have a And The defaults are 15 and 4, and they've been selected using careful benchmarks for desktop machines. Maybe you can squeeze out a few percents on your specific machine with something like (14,4) or (16,4) or similar if you really do the benchmarks. But (24,8) is insanely high and has no benefits. |
@real-or-random: Thanks. I'll make those changes. Those flags originated before I got involved. |
|
And change the ecmult window size for USE="lowmem" from 2 to 4. Suggested-by: Tim Ruffing <[email protected]> See: bitcoin-core/secp256k1#1159 (comment)
And change the ecmult window size for USE="lowmem" from 2 to 4. Suggested-by: Tim Ruffing <[email protected]> See: bitcoin-core/secp256k1#1159 (comment) See: https://gitlab.com/bitcoin/gentoo/-/commit/6e39601a748f3465f66a38e7989e7414a4a1d9c0 Signed-off-by: Matt Whitlock <[email protected]>
See: bitcoin-core/secp256k1#1159 Signed-off-by: Matt Whitlock <[email protected]>
And change the ecmult window size for USE="lowmem" from 2 to 4. Suggested-by: Tim Ruffing <[email protected]> See: bitcoin-core/secp256k1#1159 (comment) See: https://gitlab.com/bitcoin/gentoo/-/commit/6e39601a748f3465f66a38e7989e7414a4a1d9c0 Signed-off-by: Matt Whitlock <[email protected]>
See: bitcoin-core/secp256k1#1159 Signed-off-by: Matt Whitlock <[email protected]>
And change the ecmult window size for USE="lowmem" from 2 to 4. Suggested-by: Tim Ruffing <[email protected]> See: bitcoin-core/secp256k1#1159 (comment) See: https://gitlab.com/bitcoin/gentoo/-/commit/6e39601a748f3465f66a38e7989e7414a4a1d9c0 Signed-off-by: Matt Whitlock <[email protected]>
See: bitcoin-core/secp256k1#1159 Signed-off-by: Matt Whitlock <[email protected]>
And change the ecmult window size for USE="lowmem" from 2 to 4. Suggested-by: Tim Ruffing <[email protected]> See: bitcoin-core/secp256k1#1159 (comment) See: https://gitlab.com/bitcoin/gentoo/-/commit/6e39601a748f3465f66a38e7989e7414a4a1d9c0 Signed-off-by: Matt Whitlock <[email protected]> Signed-off-by: Sam James <[email protected]>
See: bitcoin-core/secp256k1#1159 Signed-off-by: Matt Whitlock <[email protected]> Closes: #28990 Signed-off-by: Sam James <[email protected]>
When cross-compiling libsecp256k1, if the
precomputed_ecmult*.c
source files need to be regenerated, then the generators need to be built for the build system, not for the host system. Autoconf supports this fairly cleanly via theAX_PROG_CC_FOR_BUILD
macro (from Autoconf Archive), but Automake requires some hackery. When building the generators, we override theCC
,CFLAGS
,CPPFLAGS
, andLDFLAGS
variables to their build-system counterparts, whose names are suffixed with_FOR_BUILD
and whose values are populated by the aforementioned Autoconf macro and may be overridden on themake
command line. Since Automake lacks support for overridingEXEEXT
on a per-program basis, we define a recipe that builds the generator binaries with names suffixed with$(EXEEXT)
and then renames them suffixed with$(BUILD_EXEEXT)
.