build: pkg-config related cleanup #20201
Conversation
|
Concept ACK: nice cleanup! |
|
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. |
Guix builds
|
What makes these redundant? |
From commit message:
|
|
Thanks. Yes, clear. But otoh that seems pretty deeply buried. I think one advantage of specifying them explicitly is to have a better idea what gets exported from the |
|
|
Rebased ae92fb1 -> 58737de (pr20201.01 -> pr20201.02) due to the conflict with #20202. |
|
Rebased 58737de -> ab1fead (pr20201.02 -> pr20201.03) due to the conflict with #18077. |
|
Running Kali Linux. 2020.4 and getting the error after attempting |
|
@bitmastercoin What happens when you do the same on the master branch? |
|
@bitmastercoin I was able to successfully build and and run with Kali Linux 2020.4 and 2021.1 on master branch and PR branch. Can you give any update? |
There was a problem hiding this comment.
Concept ACK, I cannot ACK yet because I want to confirm the reproducibility of the stripped information with the bitcoin-maintainer-tools/build-for-compare.py script, but I'm currently running into the following issue:
>>> [do_build] Command failed: git apply /home/xyz/Bitcoin/Code/revie/test-reproducibility/bitcoin-maintainer-tools/patches/stripbuildinfo.patch
>>> [do_build] Could not apply patch to strip build info. Probably it needs to be updated
I tried running on the PR branch itself, and the PR branch rebased on the current master, but no luck. Obviously, I am not a maintainer, so any help would be appreciated 😊
Notes:
I wanted to try to document what is going on here and give my opinion.
-
- Commit 388135b states that it is removing a redundant check of
PKG_CHECK_MODULES. If we look at the pkg.m4 source code, and specifically lines 131-142, we can see that performing this check within our configure.ac script is in fact redundant.
- Commit 388135b states that it is removing a redundant check of
-
- From my understanding, the AC_SUBT macro is meant to provide output variables to automake. The description for this commit explains why performing this work ourselves is redundant; these variables will already be provided to automake thanks to
pkg.m4. This can be confirmed by looking at Lines 141 and 142 of thepkg.m4source code. As such, it is ok to remove what is being removed in this commit.
- From my understanding, the AC_SUBT macro is meant to provide output variables to automake. The description for this commit explains why performing this work ourselves is redundant; these variables will already be provided to automake thanks to
Should we do this?
I think it is great not to redo unnecessary work when it is unnecessary. As such, this PR is a nice simplification.
@laanwj brings up a good point that this is kind of buried, and you need to dig through the source code to confirm this. Luckily the pkg.m4 source code is not too large, and it's easy to refer to. In this case, I'd say it's not a big deal. But, I will defer to our build experts.
Not sure if you're aware, but https://github.com/pkgconf/pkgconf (which you've been linking to here), is not the same piece of software as https://www.freedesktop.org/wiki/Software/pkg-config/, which is what most builders would actually be using. |
|
@fanquake ah thanks for the clarification. Just trying to learn about our build system by reviewing. I've checked the source code for the pkg-config you've linked and have found the same relevant lines in its |
|
|
Comparing the current master (c3545a7) with |
See the update PR description. |
The correct reference is here: |
|
Rebased ab1fead -> 17c2cad (pr20201.03 -> pr20201.04) on top of the recent change in the build system (including Guix) and CI. |
Guix hashes: |
|
GUIX hashes, mine match @hebasto |
There was a problem hiding this comment.
ACK 17c2cad
As I've outlined here and here, this change is ok to do.
cannot ACK yet because I want to confirm the reproducibility of the stripped information
I have used the bitcoin-maintainer-tools/build-for-compare.py script to check for the reproducibility of the stripped information. The two branch heads tested were 17c2cad and c3545a7 and the built executables were bitcoind and bitcoin-qt. Running git diff -W --word-diff /tmp/compare/17c2cad /tmp/compare/c3545a7 returns a zero-diff.
shasums of the stripped info:
93c4fe1e65dffc43380d638f2fc72d8967bd6813b175d6363cfc46d27fcc5c24 /tmp/compare/bitcoind.17c2cad.stripped
93c4fe1e65dffc43380d638f2fc72d8967bd6813b175d6363cfc46d27fcc5c24 /tmp/compare/bitcoind.c3545a7.stripped
52e8e1375b7ea0e733c9e33eafd07c2f96731f96bebc65b72b2d156ac1faa37f /tmp/compare/bitcoin-qt.17c2cad.stripped
52e8e1375b7ea0e733c9e33eafd07c2f96731f96bebc65b72b2d156ac1faa37f /tmp/compare/bitcoin-qt.c3545a7.stripped
|
Rebased 17c2cad -> 362edc2 (pr20201.04 -> pr20201.05) due to the conflict with #22646. |
Guix builds: |
Variables that are declared with AC_ARG_VAR macro are substituted via
AC_SUBST macro.
PKG_CHECK_MODULES macro already has AC_ARG_VAR(${PACKAGE}_CFLAGS) and
AC_ARG_VAR(${PACKAGE}_LIBS).
|
Rebased 362edc2 -> c236f2e (pr20201.05 -> pr20201.06) due to the conflict with #23593. |
c236f2e build: Drop redundant AC_SUBST macros (Hennadii Stepanov) 9049812 build: Drop redundant check of PKG_CHECK_MODULES presence (Hennadii Stepanov) Pull request description: This PR: - does not change behavior - drops redundant `AC_SUBST` macros Also checks of PKG_CHECK_MODULES presence are removed because they are redundant due to the following code https://github.com/bitcoin/bitcoin/blob/ab1feadf4e6cd4f5f2c7e74cea1c7baad61458ba/configure.ac#L16-L20 ACKs for top commit: fanquake: ACK c236f2e - I see no difference in `config.log`s. Tree-SHA512: 7ebbfc37123d693e034b8eea4aecbd9ef52b0ea5706b90bd282474e2d61ab0fedc8bca38aa3c9aa88cf2938339b7a491231b11865d39c682d60ef362082f22c5
This PR:
AC_SUBSTmacrosAlso checks of PKG_CHECK_MODULES presence are removed because they are redundant due to the following code
bitcoin/configure.ac
Lines 16 to 20 in ab1fead