Skip to content

[librdkafka] add new feature librdkafka[sasl] #39894

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
BillyONeal merged 2 commits into from
Aug 2, 2024
Merged

[librdkafka] add new feature librdkafka[sasl] #39894

BillyONeal merged 2 commits into from
Aug 2, 2024

Conversation

@timkpaine
Copy link
Contributor

@timkpaine timkpaine commented Jul 12, 2024 •
edited by Cheney-W
Loading

Needs: #39892

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@timkpaine timkpaine mentioned this pull request Jul 12, 2024
Closed
3 tasks
@timkpaine timkpaine force-pushed the tkp/rdkafkasasl branch 3 times, most recently from adda9ad to 367c03d Compare July 12, 2024 21:59
@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jul 15, 2024
Cheney-W
Cheney-W reviewed Jul 15, 2024
View reviewed changes
@timkpaine timkpaine force-pushed the tkp/rdkafkasasl branch 3 times, most recently from 0c2aa3b to b805e33 Compare July 25, 2024 20:47
@timkpaine timkpaine marked this pull request as ready for review July 25, 2024 22:44
@timkpaine timkpaine requested a review from Cheney-W July 26, 2024 00:49
timkpaine
timkpaine commented Jul 26, 2024
View reviewed changes
timkpaine
timkpaine commented Jul 26, 2024
View reviewed changes
@Cheney-W
Copy link
Contributor

Cheney-W commented Jul 26, 2024 •
edited
Loading

@timkpaine
I tried to test this new feature, but I got an error with the new port cyrus-sasl, could you please help take a look at it? Thank you!
issue_body.md

@Cheney-W Cheney-W marked this pull request as draft July 26, 2024 10:32
@timkpaine
Copy link
Contributor Author

timkpaine commented Jul 26, 2024

@Cheney-W should be fixed, I accepted the review comments for the prior PR but they dont work at all, so I've undone them.

@timkpaine timkpaine force-pushed the tkp/rdkafkasasl branch 2 times, most recently from b6163c2 to 330a188 Compare July 26, 2024 13:35
dg0yt
dg0yt reviewed Jul 26, 2024
View reviewed changes
@timkpaine timkpaine marked this pull request as ready for review July 26, 2024 18:04
@timkpaine timkpaine marked this pull request as draft July 26, 2024 18:07
timkpaine
timkpaine commented Jul 26, 2024
View reviewed changes
@timkpaine timkpaine mentioned this pull request Jul 26, 2024
Merged
@timkpaine timkpaine force-pushed the tkp/rdkafkasasl branch 2 times, most recently from 8cf258f to c89530d Compare July 26, 2024 18:32
@timkpaine timkpaine marked this pull request as ready for review July 26, 2024 18:32
@timkpaine timkpaine requested a review from dg0yt July 26, 2024 18:39
dg0yt
dg0yt reviewed Jul 27, 2024
View reviewed changes
@timkpaine timkpaine marked this pull request as draft July 28, 2024 14:09
@timkpaine timkpaine marked this pull request as ready for review August 1, 2024 14:46
@timkpaine
Copy link
Contributor Author

timkpaine commented Aug 1, 2024

I removed snappy because WITH_SNAPPY is not used by librdkafka anymore it seems

@timkpaine timkpaine requested a review from dg0yt August 1, 2024 14:55
@dg0yt
Copy link
Contributor

dg0yt commented Aug 1, 2024

The PR is looks good, but I notice that the pc file isn't complete for static builds because they set PKG_CONFIG_REQUIRES_PRIVATE only for shared builds (where it isn't needed). This doesn't just affect sasl but also all other deps.

This should be fixed and upstreamed by moving
https://github.com/confluentinc/librdkafka/blob/95a542c87c61d2c45b445f91c73dd5442eb04f3c/src/CMakeLists.txt#L295-L297
behind the PKG_CONFIG_REQUIRES_PRIVATE setup.

(I really can't believe that upstream contributors happily improved dead PKG_CONFIG_REQUIRES_PRIVATE cmake code. Is there something which I don't see? Except that everybody uses shared libs and doesn't need it.)

@timkpaine
Copy link
Contributor Author

timkpaine commented Aug 1, 2024

@dg0yt I think you answered your own question 😆 Except that everybody uses shared libs and doesn't need it

@Cheney-W
Copy link
Contributor

Cheney-W commented Aug 2, 2024

New feature test passed with x64-windows triplet.

@Cheney-W Cheney-W added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Aug 2, 2024
@BillyONeal
Copy link
Member

BillyONeal commented Aug 2, 2024

Can you explain why you removed the snappy feature?

@dg0yt
Copy link
Contributor

dg0yt commented Aug 2, 2024

Snappy is always on, and symbols are renamed.

@BillyONeal BillyONeal merged commit 906d928 into microsoft:master Aug 2, 2024
@BillyONeal
Copy link
Member

BillyONeal commented Aug 2, 2024

Thanks for the feature!

@BillyONeal
Copy link
Member

BillyONeal commented Aug 2, 2024

Snappy is always on, and symbols are renamed.

https://github.com/confluentinc/librdkafka/blob/6eaf89fb124c421b66b43b195879d458a3a31f86/packaging/cmake/config.h.in#L35

Ah, I see. Thanks!

@timkpaine timkpaine deleted the tkp/rdkafkasasl branch August 2, 2024 18:33
@timkpaine
Copy link
Contributor Author

timkpaine commented Aug 2, 2024

thanks @BillyONeal

@mypasswordisqwerty
Copy link

mypasswordisqwerty commented Feb 19, 2025

Snappy is always on, and symbols are renamed.

Snappy is off in windows build. WITH_SNAPPY not defined anywhere.

@BillyONeal
Copy link
Member

BillyONeal commented Feb 19, 2025

Snappy is off in windows build. WITH_SNAPPY not defined anywhere.

It seems defined where I linked to?

@mypasswordisqwerty
Copy link

mypasswordisqwerty commented Feb 22, 2025

Snappy is off in windows build. WITH_SNAPPY not defined anywhere.

It seems defined where I linked to?

Did you try it? My build raises .unsupported error when setting compression.type=snappy
https://github.com/confluentinc/librdkafka/blob/93877617709eb071a0f4ec7038c54e2764abefc9/src/rdkafka_conf.c#L177

I think windows build includes win32_config.h instead of cmake generated config.h:
https://github.com/confluentinc/librdkafka/blob/93877617709eb071a0f4ec7038c54e2764abefc9/src/rd.h#L62

win32_config.h defines WINTH_SNAPPY
https://github.com/confluentinc/librdkafka/blob/93877617709eb071a0f4ec7038c54e2764abefc9/src/win32_config.h#L38
but only when WITHOUT_WIN32_CONFIG is OFF
https://github.com/confluentinc/librdkafka/blob/93877617709eb071a0f4ec7038c54e2764abefc9/src/win32_config.h#L35

It seems that WITHOUT_WIN32_CONFIG is ON in cmake build:
https://github.com/confluentinc/librdkafka/blob/93877617709eb071a0f4ec7038c54e2764abefc9/CMakeLists.txt#L186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dg0yt dg0yt Awaiting requested review from dg0yt

1 more reviewer

@Cheney-W Cheney-W Cheney-W approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Cheney-W Cheney-W

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@timkpaine @Cheney-W @dg0yt @BillyONeal @mypasswordisqwerty