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.

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

build: Fix release builds with enabled FLB_DEBUG #8058

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

Garfield96
Copy link
Contributor

@Garfield96 Garfield96 commented Oct 18, 2023

Enabling FLB_RELEASE should enable compiler optimizations. However, if FLB_DEBUG (enabled by default) was enabled at the same time, no optimization settings were applied. Now, FLB_RELEASE will always enable optimizations and includes debug information (CMAKE_BUILD_TYPE is RelWithDebInfo).

This issue affects all published Docker images and most likely severely degrades performance.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

Enabling FLB_RELEASE should enable compiler optimizations. However, if
FLB_DEBUG was enabled at the same time, no optimization settings were
applied. Now, FLB_RELEASE will always enable optimizations and includes
debug information (CMAKE_BUILD_TYPE is RelWithDebInfo).

Signed-off-by: Christian Norbert Menges <[email protected]>
@patrick-stephens
Copy link
Contributor

Probably want to target 2.1 branch as well @Garfield96 as this will go into 2.2 otherwise.

@Garfield96
Copy link
Contributor Author

Hi @patrick-stephens,
thanks for the fast review. I'll create a backport PR for the 2.1 branch (https://github.com/fluent/fluent-bit/tree/2.1).

@Garfield96
Copy link
Contributor Author

Hi @patrick-stephens,
the test failure looks unrelated. It would be great if this bug fix would be included in the next release. Could you please merge this PR and the backport PR (#8068). Thanks

@Garfield96
Copy link
Contributor Author

@niedbalski @patrick-stephens
Sorry for asking again, but is there anything preventing this and the backport PR from getting merged?

@patrick-stephens
Copy link
Contributor

Just the size of the review queue is probably the only blocker @Garfield96 , I'll ping @niedbalski on it

@Garfield96
Copy link
Contributor Author

@patrick-stephens @niedbalski I understand that there is a huge number of open PRs and it takes a considerable amount of time to review them. However, the long review times are slowly becoming a problem, because users are forced to build fluent-bit locally in order to have their features included, and people refrain from contributing. This PR, which changes 2 LOCs, is open for 3 months and my other PR #6087 is open for almost 1.5 years.
Can you provide some plan on how we want to proceed with these PRs?

@edsiper
Copy link
Member

edsiper commented Jan 15, 2024

my apologies for the delay on this, thanks @patrick-stephens for the heads up.

thanks for the fix, indeed the code is wrongly evaluating FLB_DEBUG first before FLB_RELEASE

@edsiper edsiper merged commit 71746b3 into fluent:master Jan 15, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants