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

plugins: in_exec: enable on win32 #7664

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

ringerc
Copy link
Contributor

@ringerc ringerc commented Jul 7, 2023

Work done as part of #7207 made the in_exec plugin build-able on Windows, but did not enable it as part of the build.

Enable its compliation on Windows per request of @patrick-stephens .

Please also merge related Windows PR #7463


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

master is totally valgrind-unclean so please remove this from the PR template^^

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

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

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • [N/A] 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.

@ringerc
Copy link
Contributor Author

ringerc commented Jul 7, 2023

I don't have any idea how to add test cover for this and I've torn down my Windows dev-env after completion of work on #7207.

Feel free to merge this change if you wish to do so based on the github test runs etc.

Work done as part of fluent#7207 made the in_exec plugin build-able on
Windows, but did not enable it as part of the build.

Enable its compliation on Windows.

Signed-off-by: Craig Ringer <[email protected]>
@ringerc
Copy link
Contributor Author

ringerc commented Jul 7, 2023

@patrick-stephens probably needs ok-package-test label?

@ringerc
Copy link
Contributor Author

ringerc commented Jul 7, 2023

Please also merge #7463

@ringerc
Copy link
Contributor Author

ringerc commented Jul 7, 2023

I've kicked off a local Windows build via github actions as https://github.com/ringerc/fluent-bit/actions/runs/5481401003

Presumably this won't run any relevant tests for the functionality, since otherwise it'd have failed on in_exec when it wasn't compiled. I have no idea how the tests for windows work (if they exist) and no interest in Windows; I'm just raising this PR because it's low-hanging fruit on top of the work I did on the in_exec plugin earlier.

The build succeeded anyway.

@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Jul 7, 2023
@patrick-stephens
Copy link
Contributor

I've triggered the build, otherwise looks good to me.

@ringerc
Copy link
Contributor Author

ringerc commented Jul 9, 2023

@patrick-stephens Failed tests look like the usual ones I've seen failing on prior PRs.

Got a Windows machine you can do a quick sanity check on? I tried it with Wine, but it didn't want to behave with Wayland and I gave up. Don't really want to spin up another Windows VM right now.

BTW, would you consider removing the valgrind section from the PR template since Valgrind is known to be unclean on fluent-bit's master, without a usable suppressions file.

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.

@edsiper edsiper merged commit a8e2270 into fluent:master Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required ok-package-test Run PR packaging tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants