-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix debian "libatomic not found" error in downstream builds #133
base: master
Are you sure you want to change the base?
Conversation
This is an extension of the workaround implemented previously in chaincodelabs#119. That workaround let the libmultiprocess cmake build work with the debian capnproto package. This change extends the workaround to apply to downstream cmake builds that call find_package(Libmultiprocess), like Bitcoin Core. Fixes chaincodelabs#132
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 67afc23, tested on Ubuntu 24.04.
@@ -17,6 +17,9 @@ if ("Bin" IN_LIST ${CMAKE_FIND_PACKAGE_NAME}_FIND_COMPONENTS) | |||
endif() | |||
|
|||
if ("Lib" IN_LIST ${CMAKE_FIND_PACKAGE_NAME}_FIND_COMPONENTS) | |||
# Setting FOUND_LIBATOMIC is needed on debian & ubuntu systems to work around bug in | |||
# their capnproto packages. See compat_find.cmake for a more complete explanation. | |||
set(FOUND_LIBATOMIC TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a local fixup, this variable shouldn't pollute the global namespace.
Maybe unset(FOUND_LIBATOMIC)
after find_dependency(CapnProto)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #133 (comment)
As a local fixup, this variable shouldn't pollute the global namespace.
Maybe
unset(FOUND_LIBATOMIC)
afterfind_dependency(CapnProto)
?
You would know more than me, but it seems like that change could make things worse. If we just unset the variable, wouldn't that just be overwriting it as unset instead of true and polluting the namespace in a different way?
I think you are right that there may be a less invasive way to work around the debian bug though. Maybe checking to see if the variable is set before setting it, and restoring the previous value after finding capnproto. I would have to refresh my memory of cmake variable scopes before doing this though, and I would want he other code in compat_find.cmake from #119 to be consistent so both workarounds are identical.
Could be a good followup but I think current change is probably the most direct fix for the current issue.
This is an extension of the workaround implemented previously in #119. That workaround let the libmultiprocess cmake build work with the debian capnproto package.
This change extends the workaround to apply to downstream cmake builds that call find_package(Libmultiprocess), like Bitcoin Core.
Fixes #132