Revert "build: remove some no-longer-needed var unexporting from conf… #25660

hebasto · 2022-07-21T08:22:33Z

Building with depends implies exporting of the PKG_CONFIG_PATH and PKG_CONFIG_LIBDIR environment variables:

bitcoin/depends/config.site.in

Lines 87 to 93 in d1e4265

    
           # These two need to remain exported because pkg-config does not see them 
        
           # otherwise. That means they must be unexported at the end of configure.ac to 
        
           # avoid ruining the cache. Sigh. 
        
           export PKG_CONFIG_PATH="${depends_prefix}/share/pkgconfig:${depends_prefix}/lib/pkgconfig" 
        
           if test -z "@allow_host_packages@"; then 
        
             export PKG_CONFIG_LIBDIR="${depends_prefix}/lib/pkgconfig" 
        
           fi

To use --config-cache option, as we do in CI, requires that "they must be unexported at the end of configure.ac".

…igure" This reverts commit ee30bf7.

fanquake · 2022-07-21T08:40:27Z

If nothing is currently broken in our repository, why are we reverting this? If it needs reverting at the next subtree update, it could be done then. I can locally build a secp subtree update, using depends or not, without reverting anything, so I think it could be made more clear what the actual problem is, and where it occurs. Which change is it specifically in the secp repo since our last subtree update? Note that we already include bitcoin-core/secp256k1#1090 in our repo.

real-or-random · 2022-07-21T08:51:58Z

If nothing is currently broken in our repository, why are we reverting this?

I'd say it's a bug. Running ./configure isn't supposed to change the environment, at least not permanently.

But I think you're right, we should investigate further, I don't think we have understood what's actually going on here.

Which change is it specifically in the secp repo since our last subtree update? Note that we already include bitcoin-core/secp256k1#1090 in our repo.

Yes, we should figure this out. I thought it's bitcoin-core/secp256k1#1090 but you're right, that's already in the subtree here. @dhruv said it's bitcoin-core/secp256k1#1084 but I'm not sure if this is first PR that fails.

real-or-random · 2022-07-21T09:00:52Z

Oh I think I understand now:

Here we removed PKG_PROG_PKG_CONFIG in 1090:
bitcoin-core/secp256k1@21b2eba

And then the not rebased (!) merge of bitcoin-core/secp256k1@2be6ba0 in 1084 brought that back at another location.

So we should fix this in secp256k1. I'll open a PR there. (edit: bitcoin-core/secp256k1#1128)

But I still believe there's a bug here. If configure exports these variables, it should also unset them later? (And if not, we should adjust the comments in config.site.in.)

fanquake · 2022-07-21T09:20:34Z

And then the not rebased (!) merge of bitcoin-core/secp256k1@2be6ba0 in 1084 brought that back at another location.
So we should fix this in secp256k1. I'll open a PR there. (edit: bitcoin-core/secp256k1#1128)

Good to know that was the root cause.

But I still believe there's a bug here. If configure exports these variables, it should also unset them later? (And if not, we should adjust the comments in config.site.in.)

Yes I think we can follow up with that here.

hebasto · 2022-07-21T09:21:40Z

More details.

PKG_PROG_PKG_CONFIG uses the AC_ARG_VAR macro for PKG_CONFIG* variables that makes them "precious" and forces configure to check for consistency between two runs.

hebasto · 2022-07-21T09:28:19Z

As the secp256k1 has its own correct fix now, I've removed secp256k1 references from the PR description.

A CI build with the updated secp256k1 subtree -- https://cirrus-ci.com/build/6423701909405696.

real-or-random · 2022-07-21T09:51:01Z

Without this PR:

Depends ./configure misbehaves a little bit: config.site.in (only for depends builds) exports variables and they leak into the user's environment.

With this PR:

Non-depends ./configure misbehaves a little bit: If the user had set the variables in their environment, then we'll unset them...

That means either is wrong, because in either case we touch the user's environment. I'd say then we should go with the "solution" with fewer lines code, and this is just doing nothing. Though it may be a good idea to update the comment in config.site.in.

A clean solution would not touch the environment at all, but this will require larger and uglier hacks (e.g., creating a wrapper script for pkg-config on the fly) and it's totally not worth the hassle.)

fanquake · 2022-07-21T09:55:42Z

A clean solution would not touch the environment at all,

@real-or-random you might also be interested in #25465, where I'm trying to remove at least some of the additional unsetting.

DrahtBot · 2022-07-21T18:32:10Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25465 (build: remove boost library detection by fanquake)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#23561 (BIP324: Handshake prerequisites by dhruv)
#23432 (BIP324: CPubKey encode/decode to elligator-swift by dhruv)

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.

real-or-random · 2022-07-22T08:36:57Z

A clean solution would not touch the environment at all, but this will require larger and uglier hacks (e.g., creating a wrapper script for pkg-config on the fly) and it's totally not worth the hassle.)

Well, we could "just" store the values of PKG_CONFIG_PATH and PKG_CONFIG_LIBDIR (with correct handling of unset) and then reset them at the end... Certainly a bit uglier than what we have now but probably not too bad for a solution that behaves cleanly. What do you think?

DrahtBot · 2022-07-23T03:02:08Z

Guix builds

File	commit `b8067cd` (master)	commit `535d5f7` (master and this pull)
SHA256SUMS.part	`7673e37bbedece31...`	`d6ca5fd50412a22c...`
*-aarch64-linux-gnu-debug.tar.gz	`c486104e468488a9...`	`341f72a420d00851...`
*-aarch64-linux-gnu.tar.gz	`48ca2d94d32933b6...`	`4c6e33e7d8830f00...`
*-arm-linux-gnueabihf-debug.tar.gz	`6f12b7ffa097cb7e...`	`d9f227cd137ba762...`
*-arm-linux-gnueabihf.tar.gz	`20767b2d1f4a39cb...`	`da425e7ca8a339fd...`
*-arm64-apple-darwin-unsigned.dmg	`29d530af8ec47d2e...`	`acbf6a063dd33262...`
*-arm64-apple-darwin-unsigned.tar.gz	`ca230509ae416e7f...`	`5735633b0b4afc0d...`
*-arm64-apple-darwin.tar.gz	`a9d9c77ddd805106...`	`1d53c6243a1d1d94...`
*-powerpc64-linux-gnu-debug.tar.gz	`ae6271d5ec82f24c...`	`f79cb756bfb6a90a...`
*-powerpc64-linux-gnu.tar.gz	`1fe33ac54838cedf...`	`2fb09c8ee7b3a139...`
*-powerpc64le-linux-gnu-debug.tar.gz	`3e143018180eac96...`	`452ccc9059038b63...`
*-powerpc64le-linux-gnu.tar.gz	`daaac1e1b4fa9bae...`	`fb9752184752178c...`
*-riscv64-linux-gnu-debug.tar.gz	`82e1b018a36ce42c...`	`87baf751fb1fe07e...`
*-riscv64-linux-gnu.tar.gz	`b45b60a2c94f9c9c...`	`5b100d12d22353f3...`
*-win64-debug.zip	`b15c357ea6ed18b9...`	`3cabe3e4ca2546dc...`
*-win64-setup-unsigned.exe	`2235eddc26bce3ce...`	`bda6339351171caa...`
*-win64-unsigned.tar.gz	`ea461339fe0f9a10...`	`45a54fac95c3311a...`
*-win64.zip	`18a8c3fcc4497640...`	`81759a0b4246d766...`
*-x86_64-apple-darwin-unsigned.dmg	`7573e0223f7e99b7...`	`c8f4853e4dd7e8eb...`
*-x86_64-apple-darwin-unsigned.tar.gz	`5864bc67d5faf71e...`	`ec07243b40b973f4...`
*-x86_64-apple-darwin.tar.gz	`49c22339666fa5ad...`	`4ce86873b992ce22...`
*-x86_64-linux-gnu-debug.tar.gz	`f68331562688d03f...`	`783e6cb102779dbd...`
*-x86_64-linux-gnu.tar.gz	`77a191a8d3368661...`	`576c8a266e4c0575...`
*.tar.gz	`5f0b8042d7e9a13f...`	`8252479811fcd53b...`
guix_build.log	`8f9948d7f8cb8b77...`	`42a221445de43931...`
guix_build.log.diff		`f3a2211c0ac1ca2f...`

hebasto · 2022-07-24T00:07:08Z

@real-or-random

A clean solution would not touch the environment at all

Done in #25687.

hebasto · 2022-07-24T00:07:18Z

Closing in favor of #25687.

…IBDIR}` variables b9f06bf build: Do not export `PKG_CONFIG_{PATH|LIBDIR}` variables (Hennadii Stepanov) Pull request description: This is an alternative to bitcoin/bitcoin#25660 with no [drawbacks](bitcoin/bitcoin#25660 (comment)). Guix builds on `x86_64`: ``` 8ac4c1164512d8aa1c4c3e379b5e12d7e91f196dc32decea422ab1edcb51461f guix-build-b9f06bf05b67/output/aarch64-linux-gnu/SHA256SUMS.part 0e91431387030b7d2a6aba9368fab7fcf15931477b17c06350101bcb32a49217 guix-build-b9f06bf05b67/output/aarch64-linux-gnu/bitcoin-b9f06bf05b67-aarch64-linux-gnu-debug.tar.gz d36ef4c9230d73d73760bf1533535aa8fd325584b11adb9101cff2097f548b88 guix-build-b9f06bf05b67/output/aarch64-linux-gnu/bitcoin-b9f06bf05b67-aarch64-linux-gnu.tar.gz 280d3c31e755b0e8e58cdcf184435fa6b7b69cf3446651ebfe76f9a632827094 guix-build-b9f06bf05b67/output/arm-linux-gnueabihf/SHA256SUMS.part c6e7869ca390a8693c0d569ec89ffdcb128692e0e7cae89332adc0bd0663d0f3 guix-build-b9f06bf05b67/output/arm-linux-gnueabihf/bitcoin-b9f06bf05b67-arm-linux-gnueabihf-debug.tar.gz 2b0046e12d675c64a157265e16d014bd476be1c6f487f239cbdb151543790eb9 guix-build-b9f06bf05b67/output/arm-linux-gnueabihf/bitcoin-b9f06bf05b67-arm-linux-gnueabihf.tar.gz 92abf22c6c7e6a72d3018a836a2d3e16d2051af14a0c6add749eca268ddad470 guix-build-b9f06bf05b67/output/arm64-apple-darwin/SHA256SUMS.part 4cb47c5b5a302f0156ff0e998d0cb8103418f5e0f85b8d47d395771187cd8fda guix-build-b9f06bf05b67/output/arm64-apple-darwin/bitcoin-b9f06bf05b67-arm64-apple-darwin-unsigned.dmg 660dab4a573b60a034f06f95a48563e9ea7d96632818140e578cd3ae972eb640 guix-build-b9f06bf05b67/output/arm64-apple-darwin/bitcoin-b9f06bf05b67-arm64-apple-darwin-unsigned.tar.gz 39ac1ecdce5a848aaca91f9f9dcc2a4436c1d257b27608191af45d4d29054990 guix-build-b9f06bf05b67/output/arm64-apple-darwin/bitcoin-b9f06bf05b67-arm64-apple-darwin.tar.gz 5afa45e1c9c2e31d97148e868415f6bbaf51def45aeaa32bb13b8a092284139e guix-build-b9f06bf05b67/output/dist-archive/bitcoin-b9f06bf05b67.tar.gz 2aff14d389202d87266b93e1c17aa0ebbd9cde787349127f1a891dfddc41b675 guix-build-b9f06bf05b67/output/powerpc64-linux-gnu/SHA256SUMS.part 650c555c9d3d5b2ae18353d621b51cbdfbe5f2ebce31e7add47887adfd9b0283 guix-build-b9f06bf05b67/output/powerpc64-linux-gnu/bitcoin-b9f06bf05b67-powerpc64-linux-gnu-debug.tar.gz 38b33f13f2ac03ea2d864a02d5088b34441567bb6af04df9dc0c3aa4a9068cb2 guix-build-b9f06bf05b67/output/powerpc64-linux-gnu/bitcoin-b9f06bf05b67-powerpc64-linux-gnu.tar.gz 3b435cd35afe0990a6badb3d4f2a5e120c89644b566581db882241e82d5b94ca guix-build-b9f06bf05b67/output/powerpc64le-linux-gnu/SHA256SUMS.part 3a3259a8c489e522a1763e4178f4d8b6b49cff927d5ebe9918c853f4d04547b7 guix-build-b9f06bf05b67/output/powerpc64le-linux-gnu/bitcoin-b9f06bf05b67-powerpc64le-linux-gnu-debug.tar.gz 006df723180d8260112e26089062f2a1ca4742099bf2d9455acd6be1b5395939 guix-build-b9f06bf05b67/output/powerpc64le-linux-gnu/bitcoin-b9f06bf05b67-powerpc64le-linux-gnu.tar.gz d13f9c8e9396c46496e06cf6bfbaffae3980e6305024a1e447f73346e66e48a5 guix-build-b9f06bf05b67/output/riscv64-linux-gnu/SHA256SUMS.part 9b18dfafd51a6d249ed74d884c4a8d1b2cf320133cea8008742bc93583cff19e guix-build-b9f06bf05b67/output/riscv64-linux-gnu/bitcoin-b9f06bf05b67-riscv64-linux-gnu-debug.tar.gz 76d29d553f06c7098c67e8fc95f83c45619860988668567f37946efd63668cef guix-build-b9f06bf05b67/output/riscv64-linux-gnu/bitcoin-b9f06bf05b67-riscv64-linux-gnu.tar.gz 6661426b6180c8bb908b05f1ea4e8fe81acc02a443784e0ca042feebb1c8770c guix-build-b9f06bf05b67/output/x86_64-apple-darwin/SHA256SUMS.part 8310942fddcbf991d3162f94b9e0f2f9f413f10089b6ac31d5c3a73039c3e987 guix-build-b9f06bf05b67/output/x86_64-apple-darwin/bitcoin-b9f06bf05b67-x86_64-apple-darwin-unsigned.dmg cdcbb08d7596a3f9a0b3816b113e7e4afd435fa82ae20d2d6750e30ccb13d820 guix-build-b9f06bf05b67/output/x86_64-apple-darwin/bitcoin-b9f06bf05b67-x86_64-apple-darwin-unsigned.tar.gz ac02894454dcee5c822304ab83165e500a882f7b5dd4d5f3645ac526652eb707 guix-build-b9f06bf05b67/output/x86_64-apple-darwin/bitcoin-b9f06bf05b67-x86_64-apple-darwin.tar.gz fa55686ae7c977ee9ec0213cad8f4021e81153a6de60a5b9f74fb840f173fdfa guix-build-b9f06bf05b67/output/x86_64-linux-gnu/SHA256SUMS.part cf40ec54ea736876a0fa5060ecad41d9215762b6d9b89fb2716cf073729097b9 guix-build-b9f06bf05b67/output/x86_64-linux-gnu/bitcoin-b9f06bf05b67-x86_64-linux-gnu-debug.tar.gz 4479fe5dc29925d7b51ed20faa44a87b68aa40b1fef979f1061240325892373f guix-build-b9f06bf05b67/output/x86_64-linux-gnu/bitcoin-b9f06bf05b67-x86_64-linux-gnu.tar.gz bef988880e6dbb7be90c4b2b56d5d9a68b91dceb64b2fa38e4d67e8c8cc5a78a guix-build-b9f06bf05b67/output/x86_64-w64-mingw32/SHA256SUMS.part 0040f79968d8ebb507358ee86797880a019f9730b92576af125b778bcf5ee233 guix-build-b9f06bf05b67/output/x86_64-w64-mingw32/bitcoin-b9f06bf05b67-win64-debug.zip ac63bf2dbf78361133043db7fa24be51c25fc5ddbbe19ea4a1c78e0843054757 guix-build-b9f06bf05b67/output/x86_64-w64-mingw32/bitcoin-b9f06bf05b67-win64-setup-unsigned.exe 0d9e317a95a613eb2e9216c4c9f5b0046ff52e3b11af80b8de9ac89209f33ab7 guix-build-b9f06bf05b67/output/x86_64-w64-mingw32/bitcoin-b9f06bf05b67-win64-unsigned.tar.gz 1a47e56d06207f3c86310c6eaec66f2c7693ca810de27ab2f97e67086239d396 guix-build-b9f06bf05b67/output/x86_64-w64-mingw32/bitcoin-b9f06bf05b67-win64.zip ``` ACKs for top commit: real-or-random: utACK b9f06bf Tree-SHA512: b7dc4aa6edd4d3291034b5a00dcf205d56e4a1133058cdc32faafb95eb050377937fa9336820b5ad0fe8550431fcd5f1ed3c7f3da27486bd022a36140c5499ba

Revert "build: remove some no-longer-needed var unexporting from conf…

df7268c

…igure" This reverts commit ee30bf7.

maflcko added Build system DrahtBot Guix build requested labels Jul 21, 2022

This was referenced Jul 21, 2022

build: remove boost library detection #25465

Merged

[Draft / POC] Silent Payments #24897

Closed

BIP324: Handshake prerequisites #23561

Closed

BIP324: CKey encode/decode to elligator-swift #23432

Closed

DrahtBot removed the DrahtBot Guix build requested label Jul 23, 2022

hebasto mentioned this pull request Jul 23, 2022

build: Do not export PKG_CONFIG_{PATH|LIBDIR} variables #25687

Merged

hebasto closed this Jul 24, 2022

hebasto deleted the 220721-pkgconfig branch July 27, 2022 18:50

bitcoin locked and limited conversation to collaborators Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "build: remove some no-longer-needed var unexporting from conf… #25660

Revert "build: remove some no-longer-needed var unexporting from conf… #25660

hebasto commented Jul 21, 2022 •

edited

fanquake commented Jul 21, 2022 •

edited

real-or-random commented Jul 21, 2022

real-or-random commented Jul 21, 2022 •

edited

fanquake commented Jul 21, 2022

hebasto commented Jul 21, 2022

hebasto commented Jul 21, 2022

real-or-random commented Jul 21, 2022

fanquake commented Jul 21, 2022

DrahtBot commented Jul 21, 2022

real-or-random commented Jul 22, 2022

DrahtBot commented Jul 23, 2022

hebasto commented Jul 24, 2022

hebasto commented Jul 24, 2022

	# These two need to remain exported because pkg-config does not see them
	# otherwise. That means they must be unexported at the end of configure.ac to
	# avoid ruining the cache. Sigh.
	export PKG_CONFIG_PATH="${depends_prefix}/share/pkgconfig:${depends_prefix}/lib/pkgconfig"
	if test -z "@allow_host_packages@"; then
	export PKG_CONFIG_LIBDIR="${depends_prefix}/lib/pkgconfig"
	fi

Revert "build: remove some no-longer-needed var unexporting from conf… #25660

Revert "build: remove some no-longer-needed var unexporting from conf… #25660

Conversation

hebasto commented Jul 21, 2022 • edited

fanquake commented Jul 21, 2022 • edited

real-or-random commented Jul 21, 2022

real-or-random commented Jul 21, 2022 • edited

fanquake commented Jul 21, 2022

hebasto commented Jul 21, 2022

hebasto commented Jul 21, 2022

real-or-random commented Jul 21, 2022

Without this PR:

With this PR:

fanquake commented Jul 21, 2022

DrahtBot commented Jul 21, 2022

Conflicts

real-or-random commented Jul 22, 2022

DrahtBot commented Jul 23, 2022

Guix builds

hebasto commented Jul 24, 2022

hebasto commented Jul 24, 2022

hebasto commented Jul 21, 2022 •

edited

fanquake commented Jul 21, 2022 •

edited

real-or-random commented Jul 21, 2022 •

edited