TheBlueMatt
commented at 3:47 pm on April 16, 2018:
member
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.
Default to defining endian-conversion DECLs in compat w/o config
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.
150b2f0265
laanwj
commented at 5:09 pm on April 16, 2018:
member
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 -DHAVE_DECL_BE16TOH etc on the compiler command line, or ship with a pre-existing config.h?
laanwj added the label
Build system
on Apr 16, 2018
TheBlueMatt
commented at 6:36 pm on April 16, 2018:
member
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.
theuni
commented at 2:57 pm on April 18, 2018:
member
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):
rename all external calls to htobe16_int. I don’t think there should be (m)any of these outside of crypto/.
If HAVE_CONFIG_H, htobe16_int is defined as htobe16, essentially as it’s done now
If no HAVE_CONFIG_H and HAVE_DECL_HTOBE16, same as above, so that it can be overridden by other projects that know about their environment
If no HAVE_CONFIG_H and no HAVE_DECL_HTOBE16, htobe16_int is an explicit manual conversion function
TheBlueMatt
commented at 6:38 pm on April 18, 2018:
member
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.
theuni
commented at 5:26 pm on April 19, 2018:
member
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.
utACK150b2f0265f11590940d9c89560b5c067f4d2120
laanwj
commented at 8:19 am on April 21, 2018:
member
utACK - with the #ifdef htole16 condition it makes some sense, there’s no implicit assumption now.
I suppose there’s no way to shortcut this like
0#define HAVE_DECL_HTOBE16 defined(htobe16)
laanwj merged this
on Apr 22, 2018
laanwj closed this
on Apr 22, 2018
laanwj referenced this in commit
3e60b9cfa7
on Apr 22, 2018
fanquake added the label
Needs backport
on Apr 24, 2018
fanquake
commented at 3:17 pm on April 24, 2018:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-11-21 18:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
