fluent-bit icon indicating copy to clipboard operation
fluent-bit copied to clipboard

in_blob: fix glob function for win32

Open zshuang0316 opened this issue 1 month ago β€’ 7 comments

Set context glob path size correctly.


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
  • [x] Debug log output from testing the change [2025/11/15 17:48:33] [debug] [input:blob:blob.0] scanning path C:\data\Logs\local*.log [2025/11/15 17:48:33] [ info] [input:blob:blob.0] DEBUG: recursive_file_search called with path='NULL' pattern='C:\data\Logs\local*.log' [2025/11/15 17:48:33] [ info] [input:blob:blob.0] DEBUG: calling glob with local_pattern='C:\data\Logs\local*.log' [2025/11/15 17:48:33] [ info] [input:blob:blob.0] DEBUG: glob returned 0, found 14 matches
  • [ ] 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.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue on Windows where the reported count of results from glob-style file searches could be incorrect; the displayed number now accurately reflects the actual entries returned, improving consistency and reliability of file-matching operations and reducing confusion when reviewing search results.

✏️ Tip: You can customize this high-level summary in your review settings.

zshuang0316 avatar Nov 15 '25 10:11 zshuang0316

Walkthrough

Replaces direct inclusion of a Windows source with a guarded Windows glob header, adds a Windows-specific glob header and tests, refactors Windows glob implementation to expose a POSIX-like API (glob, globfree, is_directory), and updates build/test CMake lists to include Windows sources and tests.

Changes

Cohort / File(s) Summary
Windows Glob API Header
include/fluent-bit/flb_glob_win32.h
New header providing Windows-specific glob API: flag and error macros, structures (flb_file_glob_inner_entry, flb_file_glob_inner_context, flb_file_glob_context), glob_t typedef, and declarations for glob, globfree, and is_directory.
Windows Glob Implementation
src/win32/flb_glob.c
Refactored implementation: removed internal scaffolding and guards, elevated globfree and is_directory to external linkage, adjusted internal call sites and glob result handling, and fixed directory-check precedence.
Build Configuration Updates
src/CMakeLists.txt, tests/internal/CMakeLists.txt
Added win32/flb_glob.c to Windows build sources; added tests/internal/win32_glob.c to Windows unit-test sources.
Plugin consumer change
plugins/in_blob/blob.c
Replaced direct inclusion of win32_glob.c with a guarded include of <fluent-bit/flb_glob_win32.h> under FLB_SYSTEM_WINDOWS.
Windows Unit Tests
tests/internal/win32_glob.c
New Windows-only test file adding test_glob_basic, test_glob_nomatch, test_glob_wildcard and a Windows-guarded TEST_LIST registration.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review API surface in include/fluent-bit/flb_glob_win32.h for ABI/visibility and naming consistency.
  • Verify globfree and is_directory visibility changes and callers across the tree.
  • Confirm src/win32/flb_glob.c refactor preserved behavior (path counts, error mapping).
  • Validate new Windows tests exercise expected error/success paths and cleanup.

Suggested labels

backport to v4.0.x

Suggested reviewers

  • edsiper
  • niedbalski
  • patrick-stephens
  • celalettin1286

Poem

πŸ‡ I hopped from source into a header bright,
Windows paths now gambol in morning light.
glob, globfree, is_directory β€” now in view,
Tests trot along to prove what they do. πŸ₯•

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
βœ… Passed checks (2 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title accurately summarizes the main changes: fixing the glob function and extracting it as a Windows utility file, which aligns with the core modifications across multiple files.
✨ Finishing touches
  • [ ] πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 15 '25 10:11 coderabbitai[bot]

@coderabbitai help could you guide me how to re-run the failed checks? I think they should not relate to my change.

zshuang0316 avatar Nov 28 '25 08:11 zshuang0316

[!TIP] For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the status of checks/CI runs on this PR
gh pr view 11165 --repo fluent/fluent-bit --json statusCheckRollup,commits | jq '.'

Length of output: 22891


🏁 Script executed:

#!/bin/bash
# Get more details about the PR including check runs
gh api repos/fluent/fluent-bit/pulls/11165/commits | jq '.[-1].sha' -r | xargs -I {} gh api repos/fluent/fluent-bit/commits/{}/check-runs | jq '.check_runs[] | {name: .name, status: .status, conclusion: .conclusion, html_url: .html_url}'

Length of output: 6507


🏁 Script executed:

#!/bin/bash
# Check if there are any workflow runs for this PR
gh run list --repo fluent/fluent-bit --branch patch-4 --limit 10 --json databaseId,status,conclusion,name,event,createdAt,workflowName,url

Length of output: 2445


@zshuang0316, you're correct that the failed check is unrelated to your Windows-specific changes. Let me explain what happened and how to re-run the checks.

What Failed

The only failing check is:

This is a Linux unit test with the undefined behavior sanitizer enabled. Your changes are in plugins/in_blob/win32_glob.c, which is Windows-specific code, so this failure is definitely unrelated to your PR.

Good News

All Windows-related checks passed successfully:

  • βœ… Windows 32bit build and tests
  • βœ… Windows 64bit build and tests
  • βœ… Windows 64bit (Arm64) build and tests

How to Re-run Failed Checks

You have a few options:

Option 1: Ask a Maintainer

Since you've already tagged maintainers as reviewers (@edsiper, @cosmo0920), you can ask them to re-run the failed checks. Maintainers can click the "Re-run failed jobs" button in the Actions tab.

Option 2: Push an Empty Commit (if you have write access)

git commit --allow-empty -m "chore: trigger CI re-run"
git push

Option 3: Wait for Maintainer Review

Often maintainers will recognize flaky tests and either re-run them or merge the PR knowing the failure is unrelated.

Recommendation

I suggest commenting on your PR mentioning the maintainers and noting that the failing test is a Linux sanitizer test unrelated to your Windows-specific fix, and all Windows checks passed. They can then re-run the failed job or proceed with the merge.

Would you like me to help you draft a comment for the maintainers?

coderabbitai[bot] avatar Nov 28 '25 08:11 coderabbitai[bot]

Hello @cosmo0920, could you please help on the failed linux check which should not related to the code change? Thanks!

zshuang0316 avatar Nov 28 '25 08:11 zshuang0316

Hello @cosmo0920, could you please help on the failed linux check which should not related to the code change? Thanks!

This is not related to your changes. But this type of errors are already eliminated in the current master. So, could you rebase off master? Then, this type of error will be gone.

cosmo0920 avatar Dec 01 '25 05:12 cosmo0920

I'd like to see a unit test for it though ideally.

patrick-stephens avatar Dec 01 '25 10:12 patrick-stephens

I'd like to see a unit test for it though ideally.

Sure, added. image

zshuang0316 avatar Dec 03 '25 06:12 zshuang0316

@patrick-stephens Any other concerns?

zshuang0316 avatar Dec 12 '25 06:12 zshuang0316

I supposed that this unit test contaminates for unit testing layer. Instead, extracting as win32 utility file for glob and unit testing with internal test would be fine to follow the layers of testing and implementing structures.

@cosmo0920 I have the code updated, could you please take a look to see if there is any misunderstanding?

zshuang0316 avatar Dec 15 '25 01:12 zshuang0316

Oh, our commit linter complains prefix issue in your commit: ❌ Commit 59cd7bbdd3 failed: Subject prefix 'in_blob:' does not match files changed. Expected one of: CMakeLists.txt:, build:, glob:, in_blob:, tests: Commit prefix validation failed.

cosmo0920 avatar Dec 15 '25 03:12 cosmo0920

Oh, our commit linter complains prefix issue in your commit: ❌ Commit 59cd7bb failed: Subject prefix 'in_blob:' does not match files changed. Expected one of: CMakeLists.txt:, build:, glob:, in_blob:, tests: Commit prefix validation failed.

@cosmo0920 The file name is updated. I updated the commit message with "glob:" but the check still failed, any suggestions?

zshuang0316 avatar Dec 15 '25 10:12 zshuang0316