Building with depends implies exporting of the PKG_CONFIG_PATH and PKG_CONFIG_LIBDIR environment variables: https://github.com/bitcoin/bitcoin/blob/d1e42659bbdd8da170542d8c638242cd94f71a7d/depends/config.site.in#L87-L93
To use --config-cache option, as we do in CI, requires that "they must be unexported at the end of configure.ac".
This reverts commit ee30bf7c01922938bcf861a57cdf2249edb36256.
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 https://github.com/bitcoin-core/secp256k1/pull/1090 in our repo.
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.
Oh I think I understand now:
Here we removed PKG_PROG_PKG_CONFIG in 1090:
https://github.com/bitcoin-core/secp256k1/pull/1090/commits/21b2ebaf74222017f85123deb6f30a33c7678513
And then the not rebased (!) merge of https://github.com/bitcoin-core/secp256k1/pull/1084/commits/2be6ba0fedd0d2d62ba6f346d7ced7abde0d66e4 in 1084 brought that back at another location.
So we should fix this in secp256k1. I'll open a PR there. (edit: https://github.com/bitcoin-core/secp256k1/pull/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.)
And then the not rebased (!) merge of https://github.com/bitcoin-core/secp256k1/commit/2be6ba0fedd0d2d62ba6f346d7ced7abde0d66e4 in 1084 brought that back at another location. So we should fix this in secp256k1. I'll open a PR there. (edit: https://github.com/bitcoin-core/secp256k1/pull/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.
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.
As the secp256k1 has its own correct fix now, I've removed secp256k1 references from the PR description.
A CI build with the updated
secp256k1subtree -- https://cirrus-ci.com/build/6423701909405696.
Depends ./configure misbehaves a little bit: config.site.in (only for depends builds) exports variables and they leak into the user's environment.
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.)
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
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?
<!--9cd9c72976c961c55c7acef8f6ba82cd-->