Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bitcoin Core configure fails after #1084 was merged #1127

Closed
dhruv opened this issue Jul 20, 2022 · 6 comments · Fixed by #1128
Closed

Bitcoin Core configure fails after #1084 was merged #1127

dhruv opened this issue Jul 20, 2022 · 6 comments · Fixed by #1128

Comments

@dhruv
Copy link

dhruv commented Jul 20, 2022

Bitcoin Core's secp subtree is currently at 44c2452fd387f7ca604ab42d73746e7d3a44d8a2. Updating the subtree to cd470333351bd6d90296352f2a957f38bbdaf014 (merging #1084) causes Bitcoin Core CI failure as it cannot run the config scripts anymore as seen here

I don't know much about build systems, but am happy to learn and help with guidance.
@real-or-random
Copy link
Contributor

real-or-random commented Jul 20, 2022 •
edited
Loading

Hmmm.... So apparently Core's CI sets some explicit cache file that caches some configuration values:
https://cirrus-ci.com/task/6310546097045504?logs=ci#L478-L485

I have never really used explicit cache files, but the error message tells us that the file is outdated. So I believe this is not an issue in our repo. Let me ask the Core build system experts on IRC.

@real-or-random
Copy link
Contributor

Hmmm.... So apparently Core's CI sets some explicit cache file that caches some configuration values:

Hm no, so this done automatically by autoconf when running AC_CONFIG_SUBDIRS. And IIUC then it complains that the environment has changed in between the two configure runs (core and secp256k1), namely

configure: error: `PKG_CONFIG_PATH' was not set in the previous run

@dhruv Can you test #1090? I suspect that to be the issue instead of #1084 but I could be wrong.

@real-or-random
Copy link
Contributor

Okay, so I believe this is the reason:

https://github.com/bitcoin/bitcoin/blob/948f5ba6363fcc64f95fed3f04dbda3d50d61827/depends/config.site.in#L87-L93

These were added in the same PR as this commit which implemented the unexport.
bitcoin/bitcoin@0dc8613
But the code added by this commit was removed apparently, probably because it "just worked" as long as secp256k1 also used pkgconf... (can't find the commit right now...) I believe it should be brought back...

"sigh", indeed.

cc @theuni

@real-or-random
Copy link
Contributor

real-or-random commented Jul 20, 2022 •
edited
Loading

This is the bad commit:
bitcoin/bitcoin@ee30bf7

@dhruv Can you try if CI passes if you revert this? (I guess a local depends build will do the job too, if this is easier than running CI).

edit: ok, sorry, this would work. But since only secp256k1 was using this stuff, the proper fix is just to remove those lines here entirely: https://github.com/bitcoin/bitcoin/blob/948f5ba6363fcc64f95fed3f04dbda3d50d61827/depends/config.site.in#L85-L94

real-or-random added a commit to real-or-random/bitcoin that referenced this issue Jul 20, 2022
@real-or-random
build: remove left-over pkgconfig vars in config.site.in
e404fdf 
This were overlooked in ee30bf7,
and they will create problems when updating the secp256k1 subtree,
see bitcoin-core/secp256k1#1127.
real-or-random added a commit to real-or-random/bitcoin that referenced this issue Jul 20, 2022
@real-or-random
build: remove left-over pkgconfig vars in config.site.in
36286ff 
These were overlooked in ee30bf7,
and they will create problems when updating the secp256k1 subtree,
see bitcoin-core/secp256k1#1127.
@real-or-random real-or-random mentioned this issue Jul 20, 2022
Closed
@real-or-random
Copy link
Contributor

edit: ok, sorry, this would work. But since only secp256k1 was using this stuff, the proper fix is just to remove those lines here entirely: bitcoin/bitcoin@948f5ba/depends/config.site.in#L85-L94

bitcoin/bitcoin#25655 taught me I'm wrong, so I believe the right fix is to revert bitcoin/bitcoin@ee30bf7. @dhruv Could you try if this makes the subtree update work?

@hebasto hebasto mentioned this issue Jul 21, 2022
Closed
@real-or-random real-or-random mentioned this issue Jul 21, 2022
Merged
real-or-random added a commit that referenced this issue Jul 21, 2022
@real-or-random
Merge #1128: configure: Remove pkgconfig macros again (reintroduced b…
9f8a13d 
…y mismerge)

cabe085 configure: Remove pkgconfig macros again (reintroduced by mismerge) (Tim Ruffing)

Pull request description:

  We had removed `PKG_PROG_PKG_CONFIG` in 21b2eba
  (#1090). But then then the not rebased (!) merge of 2be6ba0
  (#1084) brought that macro back at another location, without git
  complaining about a conflict.

  Fixes #1127.

ACKs for top commit:
  fanquake:
    ACK cabe085
  hebasto:
    ACK cabe085
  jonasnick:
    ACK cabe085

Tree-SHA512: ba497503db3a11e631b15c4fe875e62d892971c2c708d90b2f6be684e85d164043ea97c13af0452831eef41f3cf8230cd8a9eafa332dc5b5ae18e118b87c3828
@dhruv
Copy link
Author

dhruv commented Jul 21, 2022

Verified that this works now: https://github.com/dhruv/bitcoin-core-ci/pull/14/checks

Thanks, @real-or-random !

dderjoel pushed a commit to dderjoel/secp256k1 that referenced this issue May 23, 2023
@real-or-random @dderjoel
configure: Remove pkgconfig macros again (reintroduced by mismerge)
3768744 
We had removed `PKG_PROG_PKG_CONFIG` in 21b2eba
(bitcoin-core#1090). But then then the not rebased (!) merge of 2be6ba0
(bitcoin-core#1084) brought that macro back at another location, without git
complaining about a conflict.

Fixes bitcoin-core#1127.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

configure: Remove pkgconfig macros again (reintroduced by mismerge) real-or-random/secp256k1
2 participants
@dhruv @real-or-random