Skip to content

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

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
Merged
hebasto merged 2 commits into from
Jun 28, 2024
Merged

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

hebasto 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.

@hebasto
fixup! cmake: Do not add the rpath information to compiled executables
aadee95 
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

cc:

@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
m3dwards
m3dwards reviewed Jun 27, 2024
View reviewed changes
theuni
theuni reviewed Jun 27, 2024
View reviewed changes
src/CMakeLists.txt
add_executable(bitcoin-chainstate
bitcoin-chainstate.cpp
)
# TODO: The `SKIP_BUILD_RPATH` property setting can be deleted
Copy link

@theuni theuni Jun 27, 2024

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 •
edited
Loading

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?

@hebasto
fixup! ci: Test CMake edge cases
f39a181 
Add a new "ubuntu-chainstate" job, which builds and runs the
`bitcoin-chainstate` binary with static and shared `libbitcoinkernel`.
@m3dwards
Copy link

m3dwards commented Jun 28, 2024

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.

theuni
theuni approved these changes Jun 28, 2024
View reviewed changes
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@m3dwards m3dwards m3dwards left review comments

@theuni theuni theuni approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

Ready for master

Development

Successfully merging this pull request may close these issues.

4 participants

@hebasto @m3dwards @theuni