Default to defining endian-conversion DECLs in compat w/o config #12998
Conversation
While this isn't a supported build configuration, some build systems need to build without going through our autotools steps, so defaulting to something sane may make it easier to build. Specifically, this fixes the inability to build rust-bitcoinconsensus on some non-x86 platforms. It needs to build without our autotools/configure steps to ensure correct compile args are passed from the rust build system to gcc. Converting the args from the rust build system to gcc would be a lot of unmaintainable work.
|
I'm not sure making this assumption makes sense - the environment might or might not have those functions, without configure (or similar) that's impossible to know. You could also define the |
|
In my use-case (rust-bitcoinconsensus) we don't know anything more about the environment than non-configure Bitcoin. Re-implementing just enough of Bitcoin Core's configure to define the appropriate symbols seems like the wrong way to go (and would be entirely unmaintainable), actually running autogen/configure would also be a pain as we'd have to figure out what environment we're targeting and appropriately convert gcc flags to configure flags (as well as probably reimplement parts of the "compile a C dependency" rust module that is well-maintained by others). Not sure of another easy solution aside from this, but if we don't take it we should almost certainly add a #ifndef HAVE_CONFIG_H #error "You need to run configure first" in a bunch of places. |
|
Agree with @laanwj. Also agree with @TheBlueMatt about this being annoying, though. I've run into this exact issue as well, usually when I try to copy/paste sha256.cpp into a quick project. As these are non-standard, I don't think there's a default that makes more sense than any other. For example, I believe that these aren't present on macOS. And I don't think we can assume that they're always defines when they do exist. Imo what makes the most sense is (using htobe16 as an example):
|
|
I mean I'm open to doing the big refactor and proxying everything through inline "int" functions, but I figured that would be a bigger deal than just skipping compile of the conversion functions in the case where they're already defined (ie its super obvious we dont need them). Its not supposed to be a foolproof solution, just a simple fix that happens to be sufficient in some cases, and creates ever-so-slightly more robust defaults. |
|
I was a bit too hasty in my review of this. I see where you're coming from now. This won't fix all non-autotools builds, but it should be strictly an improvement in cases where these defines exist. I'd still prefer _int functions since we only use these in crypto/ and serialize.h, but barring that, I see now that it's hard to argue against this. utACK 150b2f0 |
|
utACK - with the |
…/o config 150b2f0 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo) Pull request description: While this isn't a supported build configuration, some build systems need to build without going through our autotools steps, so defaulting to something sane may make it easier to build. Specifically, this fixes the inability to build rust-bitcoinconsensus on some non-x86 platforms. It needs to build without our autotools/configure steps to ensure correct compile args are passed from the rust build system to gcc. Converting the args from the rust build system to gcc would be a lot of unmaintainable work. Tree-SHA512: 776fdb8c91b66f421fc751cb281c99c53c47e496f46b26c9f49bb6fdb6da3d2dda5dcc1c5bf413206bdd9ff3cbe5cef2823455900462519a4944631d9c48b54c
|
I can include this in #12967. |
|
@TheBlueMatt Is this for backport? |
While this isn't a supported build configuration, some build systems need to build without going through our autotools steps, so defaulting to something sane may make it easier to build. Specifically, this fixes the inability to build rust-bitcoinconsensus on some non-x86 platforms. It needs to build without our autotools/configure steps to ensure correct compile args are passed from the rust build system to gcc. Converting the args from the rust build system to gcc would be a lot of unmaintainable work. GitHub-Pull: bitcoin#12998 Rebased-From: 150b2f0
|
I'd prefer it, as indicated at https://botbot.me/freenode/bitcoin-core-dev/2018-04-24/?msg=99324321&page=2 |
|
@TheBlueMatt It is included in #12967 (you are very welcome to review it) |
8fca086 List support for BIP173 in bips.md (Pieter Wuille) 9645aa6 Remove blockmaxsize option from init.cpp (fanquake) 7847b92 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo) 1720eb3 qt:Show the entire Window when double clicking on taskbar (Chun Kuan Lee) e055bc0 depends: Fix Qt build with XCode 9.3 (fanquake) 0684cf9 Avoid launching as admin when NSIS installer ends. (JeremyRand) e802c22 [config] Remove blockmaxsize option (John Newbery) f118a7a Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251) f60e84d Limit the number of IPs we use from each DNS seeder (e0) Pull request description: Backports: - #12626 Limit the number of IPs addrman learns from each DNS seeder - #12650 gui: Fix issue: "default port not shown correctly in settings dialog" - #12756 [config] Remove blockmaxsize option - #12985 Windows: Avoid launching as admin when NSIS installer ends. - #12946 depends: Fix Qt build with XCode 9.3 - #12998 Default to defining endian-conversion DECLs in compat w/o config - #12999 qt: Show the Window when double clicking the taskbar icon - #13064 List support for BIP173 in bips.md to the 0.16 branch. Tree-SHA512: 3e6b47c54b2cd2bdd81fbc6176cb31e46423f6e05988984d3a09b3535e3cee101ffb071cf753a4beff3c9f0521eb5de4b7c0424a3e97da801d56b4015847ac0f
While this isn't a supported build configuration, some build systems need to build without going through our autotools steps, so defaulting to something sane may make it easier to build. Specifically, this fixes the inability to build rust-bitcoinconsensus on some non-x86 platforms. It needs to build without our autotools/configure steps to ensure correct compile args are passed from the rust build system to gcc. Converting the args from the rust build system to gcc would be a lot of unmaintainable work. GitHub-Pull: bitcoin#12998 Rebased-From: 150b2f0
Summary: Backport of Bitcoin Core PR12998 bitcoin/bitcoin#12998 Test Plan: ``` make check ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3966
Summary: Backport of Bitcoin Core PR12998 bitcoin/bitcoin#12998 Test Plan: ``` make check ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3966
Summary: Backport of Bitcoin Core PR12998 bitcoin/bitcoin#12998 Test Plan: ``` make check ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3966
…ompat w/o config 150b2f0 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo) Pull request description: While this isn't a supported build configuration, some build systems need to build without going through our autotools steps, so defaulting to something sane may make it easier to build. Specifically, this fixes the inability to build rust-bitcoinconsensus on some non-x86 platforms. It needs to build without our autotools/configure steps to ensure correct compile args are passed from the rust build system to gcc. Converting the args from the rust build system to gcc would be a lot of unmaintainable work. Tree-SHA512: 776fdb8c91b66f421fc751cb281c99c53c47e496f46b26c9f49bb6fdb6da3d2dda5dcc1c5bf413206bdd9ff3cbe5cef2823455900462519a4944631d9c48b54c
Reduce the dependencies of libzcashconsensus This is the first of two PRs that rework the `libzcashconsensus` library into `libzcash_script`, and enable it to be wrapped by the `zcash_script` crate: https://github.com/ZcashFoundation/zcash_script Includes code cherry-picked from bitcoin/bitcoin#12998.
Reduce the dependencies of libzcashconsensus This is the first of two PRs that rework the `libzcashconsensus` library into `libzcash_script`, and enable it to be wrapped by the `zcash_script` crate: https://github.com/ZcashFoundation/zcash_script Includes code cherry-picked from bitcoin/bitcoin#12998.
While this isn't a supported build configuration, some build
systems need to build without going through our autotools steps,
so defaulting to something sane may make it easier to build.
Specifically, this fixes the inability to build
rust-bitcoinconsensus on some non-x86 platforms. It needs to build
without our autotools/configure steps to ensure correct compile
args are passed from the rust build system to gcc. Converting the
args from the rust build system to gcc would be a lot of
unmaintainable work.