Skip to content
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

build: enable in_kafka and out_kafka by default #5204

Merged
merged 2 commits into from
Jun 1, 2023
Merged

build: enable in_kafka and out_kafka by default #5204

merged 2 commits into from
Jun 1, 2023

Conversation

tarruda
Copy link

@tarruda tarruda commented Mar 28, 2022

close #5189

Signed-off-by: Thiago Padilha [email protected]

@tarruda tarruda force-pushed the enable-kafka-plugins-by-default branch from 7de7a96 to 696ccd8 Compare April 1, 2022 19:09
@tarruda
Copy link
Author

tarruda commented Apr 1, 2022

@agup006 I've enabled kafka plugins by default, but not on Windows since it causes a build error on CI

@patrick-stephens
Copy link
Contributor

What's the build error?

@patrick-stephens patrick-stephens temporarily deployed to integration April 5, 2022 22:25 Inactive
@patrick-stephens patrick-stephens temporarily deployed to integration April 5, 2022 22:31 Inactive
@tarruda tarruda temporarily deployed to integration April 6, 2022 18:39 Inactive
@tarruda tarruda temporarily deployed to integration April 6, 2022 18:45 Inactive
@patrick-stephens
Copy link
Contributor

What's the build error?

Looks like it doesn't like some of the void* usage: https://ci.appveyor.com/project/fluent/fluent-bit-2e87g/builds/43155542/job/yyye570bjrb67uvg#L2827

@tarruda tarruda force-pushed the enable-kafka-plugins-by-default branch from 39e9e22 to 049ef77 Compare April 6, 2022 20:28
@tarruda tarruda force-pushed the enable-kafka-plugins-by-default branch from 049ef77 to f77011b Compare April 6, 2022 20:33
@tarruda tarruda temporarily deployed to integration April 6, 2022 20:34 Inactive
@tarruda tarruda temporarily deployed to integration April 6, 2022 20:38 Inactive
@tarruda
Copy link
Author

tarruda commented Apr 6, 2022

Fixed the issue with windows build, just replacing offsetof by container_of macro took care of windows incompatibility

patrick-stephens
patrick-stephens previously approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@patrick-stephens patrick-stephens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @niedbalski

@edsiper
Copy link
Member

edsiper commented May 12, 2022

FYI: this is blocked until we fix threaded input interface

@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Aug 10, 2022
@github-actions
Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:18 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr-package-test April 26, 2023 15:19 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to integration April 26, 2023 15:23 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr-package-test April 26, 2023 15:34 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to pr April 26, 2023 15:39 — with GitHub Actions Inactive
@tarruda
Copy link
Author

tarruda commented Apr 26, 2023

@patrick-stephens rebased and fixed the builds. Couldn't enable by default on windows because the build fails on missing libcrypto, which I assume is a librdkafka dependency.

leonardo-albertovich
leonardo-albertovich requested changes May 16, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@leonardo-albertovich
Copy link
Collaborator

These changes are a hard requirement for this PR to be merged.

@tarruda tarruda force-pushed the enable-kafka-plugins-by-default branch from 5163388 to b786be3 Compare May 16, 2023 14:56
@tarruda tarruda temporarily deployed to integration May 16, 2023 14:56 — with GitHub Actions Inactive
@tarruda tarruda temporarily deployed to integration May 16, 2023 15:02 — with GitHub Actions Inactive
@tarruda
Copy link
Author

tarruda commented May 16, 2023

@leonardo-albertovich it seems the windows-specific cmake is not being applied during CI since appveyor tried to build it. Do you know what I might have missed?

@tarruda tarruda force-pushed the enable-kafka-plugins-by-default branch from b786be3 to 70069f0 Compare May 16, 2023 16:36
@tarruda tarruda temporarily deployed to integration May 16, 2023 16:36 — with GitHub Actions Inactive
@tarruda
Copy link
Author

tarruda commented May 16, 2023

@leonardo-albertovich nevermind, it was a typo in windows-setup.cmake. Can you approve again?

@tarruda tarruda temporarily deployed to integration May 16, 2023 16:44 — with GitHub Actions Inactive
@edsiper edsiper merged commit 3fdd42c into fluent:master Jun 1, 2023
@prince-chrismc prince-chrismc mentioned this pull request Jul 29, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonardo-albertovich leonardo-albertovich leonardo-albertovich approved these changes

@patrick-stephens patrick-stephens patrick-stephens left review comments

@fujimotos fujimotos Awaiting requested review from fujimotos fujimotos is a code owner

@niedbalski niedbalski Awaiting requested review from niedbalski niedbalski is a code owner

@edsiper edsiper Awaiting requested review from edsiper

@koleini koleini Awaiting requested review from koleini

Assignees
No one assigned
Labels
ci/integration-docker-ok ci/integration-test-gcp-ok ci/integration-test-ok ok-package-test Run PR packaging tests ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tarruda @patrick-stephens @edsiper @Boojapho @leonardo-albertovich @tchrono