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.

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

cmake: Keep RPATH for the bitcoin-chainstate target in the build tree #242

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jun 26, 2024

This is a follow-up of bitcoin#30312 and #236.

On staging branch @ 8b80c1a:

$ cmake -B build -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_SHARED_LIBS=ON
$ cmake --build build -t bitcoin-chainstate
$ ./build/src/bitcoin-chainstate
./build/src/bitcoin-chainstate: error while loading shared libraries: libbitcoinkernel.so: cannot open shared object file: No such file or directory

This PR fixes this issue and adds a new CI job to check it.

1. Keep RPATH for the `bitcoin-chainstate` target in the build tree,
which is useful for the shared `libbitcoinkernel`.

2. Document future improvements.
@hebasto
Copy link
Owner Author

hebasto commented Jun 26, 2024

@hebasto hebasto added the bug Something isn't working label Jun 26, 2024
@hebasto hebasto added this to the Ready for master milestone Jun 26, 2024
@@ -355,6 +355,15 @@ if(BUILD_UTIL_CHAINSTATE)
add_executable(bitcoin-chainstate
bitcoin-chainstate.cpp
)
# TODO: The `SKIP_BUILD_RPATH` property setting can be deleted
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. Don't we always want the BUILD_RPATH for bitcoin-chainstate so that it works with a shared kernel in the build tree? Then it's stripped for the INSTALL_RPATH binary and everyone's happy?

Copy link
Owner Author

@hebasto hebasto Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we always want the BUILD_RPATH for bitcoin-chainstate so that it works with a shared kernel in the build tree?

We do, of course.

I mean, in the future, this code:

bitcoin/CMakeLists.txt

Lines 606 to 607 in 02d9f7f

set(CMAKE_SKIP_BUILD_RPATH TRUE)
set(CMAKE_SKIP_INSTALL_RPATH TRUE)
can be just:

set(CMAKE_SKIP_INSTALL_RPATH TRUE) 

which effectively makes set_target_properties(bitcoin-chainstate PROPERTIES SKIP_BUILD_RPATH OFF) redundant.

Or do you mean always keep it for the purpose of explicitness?

Add a new "ubuntu-chainstate" job, which builds and runs the
`bitcoin-chainstate` binary with static and shared `libbitcoinkernel`.
@m3dwards
Copy link

ACK f39a181

@theuni
Copy link

theuni commented Jun 28, 2024

I guess I'm just having trouble understanding the goal here at a high-level.

I've verified (locally) that the runpath now exists in the build-tree, and thus bitcon-chainstate works. But doesn't that mean that the runpath check (in master) will now fail? Don't we need to fix that upstream first?

@hebasto
Copy link
Owner Author

hebasto commented Jun 28, 2024

@theuni

I guess I'm just having trouble understanding the goal here at a high-level.

I've verified (locally) that the runpath now exists in the build-tree, and thus bitcon-chainstate works. But doesn't that mean that the runpath check (in master) will now fail? Don't we need to fix that upstream first?

But the bitcon-chainstate binary is not a subject of the runpath check at this moment, right?

@theuni
Copy link

theuni commented Jun 28, 2024

@theuni

I guess I'm just having trouble understanding the goal here at a high-level.
I've verified (locally) that the runpath now exists in the build-tree, and thus bitcon-chainstate works. But doesn't that mean that the runpath check (in master) will now fail? Don't we need to fix that upstream first?

But the bitcon-chainstate binary is not a subject of the runpath check at this moment, right?

Ah, ok, that's the part I was missing. Thanks. Seems it should be added, as part of the upstream guix changes to check the install tree.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f39a181

@hebasto
Copy link
Owner Author

hebasto commented Jun 28, 2024

@theuni

I guess I'm just having trouble understanding the goal here at a high-level.
I've verified (locally) that the runpath now exists in the build-tree, and thus bitcon-chainstate works. But doesn't that mean that the runpath check (in master) will now fail? Don't we need to fix that upstream first?

But the bitcon-chainstate binary is not a subject of the runpath check at this moment, right?

Ah, ok, that's the part I was missing. Thanks. Seems it should be added, as part of the upstream guix changes to check the install tree.

Sorry, but I have to clarify. The bitcon-chainstate binary is not configured for build in the Guix script.

@hebasto hebasto merged commit 3b65982 into cmake-staging Jun 28, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants