-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 CMake build - missing hashtablez_sampler build #238
Fix CMake build - missing hashtablez_sampler build #238
Conversation
Newly added file `hashtablez_sampler.cc` is not being built by the cmake build and hence linking fails.
@@ -22,6 +22,7 @@ absl_cc_library( | |||
container | |||
SRCS | |||
"internal/raw_hash_set.cc" | |||
"internal/hashtablez_sampler.cc" |
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.
See the comment a few lines up on lines 17-19. This target is deprecated and doesn't do anything anyway. I'd rather just remove this target.
Yeah, I saw that but it's still the recommended way per the documentation
and I don't think one can expect people to read every single cmake file.
Personally, I also prefer having a single target but that's just me so I
would understand removing it but then the docs should be updated first.
I can do that as well though, if you want.
…On Sat, 22 Dec 2018, 20:28 Derek Mauro ***@***.*** wrote:
***@***.**** commented on this pull request.
------------------------------
In absl/container/CMakeLists.txt
<#238 (comment)>:
> @@ -22,6 +22,7 @@ absl_cc_library(
container
SRCS
"internal/raw_hash_set.cc"
+ "internal/hashtablez_sampler.cc"
See the comment a few lines up on lines 17-19. This target is deprecated
and doesn't do anything anyway. I'd rather just remove this target.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#238 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACxSkzANnrqgGjMx9qUgWX7AFQs9tGfBks5u7ofKgaJpZM4ZfLTM>
.
|
Which documentation are you referring to? |
… On Sat, 22 Dec 2018, 21:41 Derek Mauro ***@***.*** wrote:
Which documentation are you referring to?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#238 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACxSkwQm8V8QR9d_UCB6y19ZTlFiu5p-ks5u7pkLgaJpZM4ZfLTM>
.
|
That one is also missing `absl::hash` I think.
On Sat, 22 Dec 2018, 21:44 Stephan Dollberg <stephan.dollberg@gmail.com
wrote:
… This one https://github.com/abseil/abseil-cpp/blob/master/CMake/README.md
On Sat, 22 Dec 2018, 21:41 Derek Mauro ***@***.*** wrote:
> Which documentation are you referring to?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#238 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACxSkwQm8V8QR9d_UCB6y19ZTlFiu5p-ks5u7pkLgaJpZM4ZfLTM>
> .
>
|
We are doing to have to fix that documentation. We are planning on generating the CMakeLists from Bazel Build files so there is going to be a one-to-one correspondence between targets soon. |
Ok, just to be clear, you want me to do that now and remove the |
I'll consult with the team, but I think we are probably going to remove it. This probably won't be merged until Wednesday at the earliest due to the holidays. |
Newly added file
hashtablez_sampler.cc
is not being built by the cmakebuild and hence linking fails.