depends: expat 2.4.8 & fix building with -flto#25697
Merged
fanquake merged 2 commits intobitcoin:masterfrom Jul 27, 2022
Merged
depends: expat 2.4.8 & fix building with -flto#25697fanquake merged 2 commits intobitcoin:masterfrom
fanquake merged 2 commits intobitcoin:masterfrom
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Contributor
|
GUIX hashes for lto_in_guix x86: arm64: |
hebasto
reviewed
Jul 25, 2022
Member
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
Currently, when building the expat package in depends, using -flto (LTO=1), inside Guix (and only inside Guix from what I've seen), the configure check fails...
It fails not only inside Guix, but for every cross-compiling, e.g.,:
$ make -C depends expat_configured LTO=1 HOST=arm-linux-gnueabihf
Considering that the _DEFAULT_SOURCE macro implies a wider feature set than the _POSIX_C_SOURCE one, is it safe to define the former for a dependency while the latter been used in
Line 45 in aa22009
?
This was referenced Jul 25, 2022
Member
Author
I don't really think that makes much of a difference here. _DEFAULT_SOURCE is still only used for the expat build. |
hebasto
approved these changes
Jul 26, 2022
Member
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
ACK 790ae82, I have reviewed the code and it looks OK, I agree it can be merged.
It's worth to mention that in the span from v2.4.1 to v2.4.8 a few CVEs have been fixed.
I've verified that no other adjustments of our depends build system required.
Successfully can cross-compile the expat package in depends with LTO=1.
fanquake
force-pushed
the
expat_2_4_8_with_lto
branch
from
790ae82 to
e838a98
Compare
Member
Author
|
Addressed the suggestion and updated the PR description. |
fanquake
changed the title
Member
Author
This doesn't fail for me. What system is this on? What compiler are you using etc? |
Member
|
Member
Author
|
I've opened an issue upstream: libexpat/libexpat#618. |
Member
Author
|
Not an expat bug, so I've submitted a report further upstream to autoconf: https://savannah.gnu.org/support/index.php?110687. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 27, 2022
e838a98 depends: re-enable using -flto when building expat (fanquake) 3044525 depends: expat 2.4.8 (fanquake) Pull request description: Currently, when building the expat package in depends, using `-flto` (`LTO=1`), the configure check can fail, because it cannot determine the system endianess: ```bash configure:18718: result: unknown configure:18733: error: unknown endianness presetting ac_cv_c_bigendian=no (or yes) will help ``` Fix that by defining `_DEFAULT_SOURCE`, which in turn defines `__USE_MISC` (`features.h`): ```c #if defined _DEFAULT_SOURCE # define __USE_MISC1 #endif ``` which exposes additional definitions in `endian.h`: ```c #include <features.h> /* Get the definitions of __*_ENDIAN, __BYTE_ORDER, and __FLOAT_WORD_ORDER. */ #include <bits/endian.h> #ifdef __USE_MISC # define LITTLE_ENDIAN__LITTLE_ENDIAN # define BIG_ENDIAN__BIG_ENDIAN # define PDP_ENDIAN__PDP_ENDIAN # define BYTE_ORDER__BYTE_ORDER #endif ``` and gives us a working configure. You could test building this change with Guix + LTO with [this branch](https://github.com/fanquake/bitcoin/tree/lto_in_guix). Note that that build may fail for other reasons (on x86_64), unrelated to this change. Some related upstream discussion: https://bugs.gentoo.org/757681 https://forums.gentoo.org/viewtopic-t-1013786.html ACKs for top commit: hebasto: re-ACK e838a98, only [suggested](bitcoin#25697 (comment)) changes since my recent [review](bitcoin#25697 (review)). jarolrod: code review ACK e838a98 Tree-SHA512: 9dbf64c9bd1fd995a4d1addc011ffeff83d50df736030012346c97605e63aed4b5bac390a81abe646c1be28ad6fd600f64560dcb26bbc2edf5d513ca3b180bfa
Member
Author
|
This has now also been fixed in autoconf upstream: https://git.savannah.gnu.org/cgit/autoconf.git/commit/?id=234fc6c86613ed3f366dd1d88996e4d5d85ee222. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, when building the expat package in depends, using
-flto(LTO=1), the configure check can fail, because it cannot determine the system endianess:configure:18718: result: unknown configure:18733: error: unknown endianness presetting ac_cv_c_bigendian=no (or yes) will helpFix that by defining
_DEFAULT_SOURCE, which in turn defines__USE_MISC(features.h):which exposes additional definitions in
endian.h:and gives us a working configure.
You could test building this change with Guix + LTO with this branch. Note that that build may fail for other reasons (on x86_64), unrelated to this change.
Some related upstream discussion:
https://bugs.gentoo.org/757681
https://forums.gentoo.org/viewtopic-t-1013786.html