-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Userspace RCU recipe. #11595
Add Userspace RCU recipe. #11595
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
👋 thanks for opening this PR and contributing this recipe
I had two small nits and two questsions, I am not familiar with this project so hopefully you can fill in the gaps
self.copy("*{}.so*".format(lib_name), dst="lib", keep_path=False) | ||
self.copy("*{}.dylib".format(lib_name), dst="lib", keep_path=False) | ||
else: | ||
self.copy("*{}.a".format(lib_name), dst="lib", keep_path=False) |
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.
Is there as reason that only on lib at a time is linked again but all of them are copied?
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.
I'm not sure I follow; this is the package() function so while all library variants are built, only the one selected with the model
option is copied into the package (and thus added to the cpp_info linker flags). Am I missing something?
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.
We try to respect the original projects layout as closely as possible this way existing users of the project can continue what they are doing and new consumers can just follow the upstream docs without Conan getting in the way.
Looking of the project what we'd ideally have is the the projects install and model the pkg_config
targets https://github.com/urcu/userspace-rcu/blob/7c40d7f5b2139ec9151daf8410b6b934234220d5/configure.ac#L310-L316
This was, as the original author intended, consumers can pick which libs they link against
My suggestion is
- deleting the
model
option - call the upstream install method (noted Add Userspace RCU recipe. #11595 (comment))
- add components (should be a one to one with the libs) how to docs
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.
Ah, I see. Thanks for the clarification!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
I added some suggestions, but they are just suggestions.
Please do not force push 🙏 GitHub forces us to restart the review which is not fun! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All green in build 24 (
|
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.
Looks really good!
@@ -0,0 +1,214 @@ | |||
/* |
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.
would be nice to reduce it, calling one function (e.g. rcu_register_thread
) is enough to test we can compile and link against the library
Specify library name and version: userspace-rcu/0.11.4
Userspace RCU implements a library around the Linux Kernel RCU interface. It is used by benchmarking and metrics gathering libraries at eBay Inc. and a dependency for many of our Open Source projects.