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

Add option to turn on/off all example modules #4668

Merged

Conversation

HofiOne
Copy link
Collaborator

@HofiOne HofiOne commented Oct 11, 2023

This is a bit more generic version of #4660 that allows switching on/off all the example modules both in cmake and autotools builds

  • Removed autotools, and example_destination example only compile switch ENABLE_EXAMPLE_DESTINATION
  • Added example modules wide common option ENABLE_EXAMPLE_MODULES both to autotools and cmake builds

Signed-off-by: Hofi hofione@gmail.com

@HofiOne HofiOne marked this pull request as draft October 11, 2023 10:15
@HofiOne HofiOne force-pushed the add-option-to-turn-off-example-modules branch from b957bef to db474e9 Compare October 11, 2023 10:20
@HofiOne HofiOne changed the title Add option to turn on/off example modules Add option to turn on/off all example modules Oct 11, 2023
@HofiOne HofiOne marked this pull request as ready for review October 11, 2023 10:46
@bazsi
Copy link
Collaborator

bazsi commented Oct 16, 2023

Just pushed a small fixup patch to make it work on automake for me.

@HofiOne HofiOne force-pushed the add-option-to-turn-off-example-modules branch from 737e720 to 8799422 Compare October 16, 2023 15:04
@HofiOne
Copy link
Collaborator Author

HofiOne commented Oct 16, 2023

Just pushed a small fixup patch to make it work on automake for me.

oh, thanks for the fixes!

MrAnno
MrAnno previously approved these changes Oct 17, 2023
configure.ac Outdated
@@ -2332,6 +2336,7 @@ echo " JSON support : $with_jsonc"
echo " Build options:"
echo " Generate manual pages : ${enable_manpages:=no}"
echo " Install manual pages : ${enable_manpages_install:=no}"
echo " Example modules : ${enable_example_modules:=yes}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I just noticed:
Can we move this under the modules section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -39,4 +40,6 @@ EXTRA_DIST += modules/examples/CMakeLists.txt

modules/examples modules/examples/ mod-examples: modules/examples/libexamples.la

endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we have a line like this in an else block?

modules/examples modules/examples/ mod-examples:

Copy link
Collaborator Author

@HofiOne HofiOne Oct 18, 2023

Choose a reason for hiding this comment

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

sorry, surely my bad, but i do not really get this
this endif belongs to the starting if ENABLE_EXAMPLE_MODULES that tries to exclude everything in this makefile if the example modules are disabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, you meant like this

...
modules/grpc/loki modules/grpc/loki/ mod-loki: modules/grpc/loki/libloki.la
else
modules/grpc/loki modules/grpc/loki/ mod-loki:
endif

I do not know what the empty module definition does here, could you explain please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually have this for our modules (e.g.: https://github.com/syslog-ng/syslog-ng/blob/master/modules/http/Makefile.am#L44)

My understanding was that this is needed so make mod-examples does not fail if we configured --disable-example-modules, but it seems like it works either way.

@MrAnno do you know why we add empty targets for our modules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just to be consistent with other modules I've added the else branch with the empty module, but still curious about what it does

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are there to make per-module compilation work.

Copy link
Collaborator Author

@HofiOne HofiOne Oct 24, 2023

Choose a reason for hiding this comment

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

They are there to make per-module compilation work.

my problem is that it is absolutely stochastic if it's presented or not, and not a strict habit/requirement/rule, so, it's hard to decide if it's really needed or not.

…LE_EXAMPLE_DESTINATION

Signed-off-by: Hofi <hofione@gmail.com>
@HofiOne HofiOne force-pushed the add-option-to-turn-off-example-modules branch from 8799422 to 7524a5f Compare October 18, 2023 07:48
alltilla
alltilla previously approved these changes Oct 18, 2023
@HofiOne HofiOne dismissed stale reviews from alltilla and MrAnno via 05a5797 October 18, 2023 09:03
@HofiOne HofiOne force-pushed the add-option-to-turn-off-example-modules branch from 7524a5f to 05a5797 Compare October 18, 2023 09:03
…ULES both to autotools and cmake builds

Signed-off-by: Hofi <hofione@gmail.com>
@HofiOne HofiOne force-pushed the add-option-to-turn-off-example-modules branch from 05a5797 to dc64f0b Compare October 18, 2023 09:27
@HofiOne HofiOne requested a review from alltilla October 18, 2023 09:30
@HofiOne HofiOne merged commit 80f7897 into syslog-ng:master Oct 18, 2023
18 of 19 checks passed
@HofiOne HofiOne deleted the add-option-to-turn-off-example-modules branch October 18, 2023 09:45
@bazsi
Copy link
Collaborator

bazsi commented Oct 24, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants