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

sp: CMakeLists.txt: don't require c++ #7254

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

ThomasDevoogdt
Copy link
Contributor

stream_processor is written in c, so only enforce a c compiler


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.
  • 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.

@leonardo-albertovich
Copy link
Collaborator

It seems like there is no downside to this but I wonder why do we need to do it for this particular sub directory. I mean, it's the only one that has a project call and according to the documentation I couldn't find a reason for it to have it.

I don't have anything against this change but do you know if there is a reason to add that rather than just removing the call? The project definition in the main CMakeLists.txt does specify C as the only language.

@ThomasDevoogdt
Copy link
Contributor Author

It seems like there is no downside to this but I wonder why do we need to do it for this particular sub directory. I mean, it's the only one that has a project call and according to the documentation I couldn't find a reason for it to have it.

I don't have anything against this change but do you know if there is a reason to add that rather than just removing the call? The project definition in the main CMakeLists.txt does specify C as the only language.

Multiple projects can have another language requirement, so the top level might be C, but that doesn't mean that this subproject is enforced to be C only.

This PR is part of a buildroot patch (buildroot/buildroot@ceb9c36) which I'm trying to upstream:

I made a series of other PRs to the upstream libs. If these all get merged, then I can create a PR here with also those.

@koleini
Copy link
Collaborator

koleini commented Apr 24, 2023

@leonardo-albertovich I leave the decision on the approval of this PR to you, since as you mentioned, only including the stream processor in the patch doesn't look consistent with the rest of the Fluent Bit source code.

@leonardo-albertovich
Copy link
Collaborator

I understand the functionality of that parameter, what I'm not entirely sure about is why do we need to include that call in stream_processor but not the other sub projects considering that it doesn't have any special requirements.

I understand that this is not something you included and I suspect that it might be just something that was added when the sub project was created but not technically necessary which is why I'm asking the question "do we need that line there?" because I assume that if that line is not there (I was able to configure and build the project) then cmake would assume the constraint is inherited from the parent project (the root CMakeLists.txt file does set the project name and language to C).

So what I'm asking is :

  1. Could you remove the line altogether and see if that fixes the error?
  2. Is there anyone around who knows what the right choice would be in case that works?

What I saw was that when @edsiper added the file in commit 6dd4468 he had already added the project call and I suspect that there might not be a strong reason behind it.

My opinion is that if removing the call fixes the issue and nobody has a strong reason to include it we should remove it.

@ThomasDevoogdt
Copy link
Contributor Author

Ah, now I understand the question. No project is not needed and PROJECT_NAME is not used.
So the line can be dropped. Will update the PR.

Copy link
Collaborator

@leonardo-albertovich leonardo-albertovich left a comment

Choose a reason for hiding this comment

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

I think that unless we want to establish setting the project name and language in every sub project this is the way to go.

I'll leave this open for @edsiper to make call but I agree with the change.

@ThomasDevoogdt
Copy link
Contributor Author

I think that unless we want to establish setting the project name and language in every sub project this is the way to go.

I'll leave this open for @edsiper to make call but I agree with the change.

While at it, can you also have a look at #7253? Which is similar. I can also drop the project name there.

@leonardo-albertovich
Copy link
Collaborator

Considering that lib/snappy is a stand alone project that's added as a dependency I think it's appropriate for it to have its own project name and language constraints so I approved the PR.

Copy link
Member

@edsiper edsiper left a comment

Choose a reason for hiding this comment

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

thanks for your contribution. I have added a comment with a request change.

note: commits should be prefixed with the component name and not other string.

src/stream_processor/CMakeLists.txt Show resolved Hide resolved
stream_processor is written in c, so only enforce a c compiler

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
@ThomasDevoogdt ThomasDevoogdt changed the title fix: sp: don't require c++ sp: CMakeLists.txt: don't require c++ Apr 25, 2023
@edsiper
Copy link
Member

edsiper commented Apr 25, 2023

general comment: we need to define when to have project(...) set or not for the whole project.

@edsiper
Copy link
Member

edsiper commented Apr 25, 2023

I understand the functionality of that parameter, what I'm not entirely sure about is why do we need to include that call in stream_processor but not the other sub projects considering that it doesn't have any special requirements.

I understand that this is not something you included and I suspect that it might be just something that was added when the sub project was created but not technically necessary which is why I'm asking the question "do we need that line there?" because I assume that if that line is not there (I was able to configure and build the project) then cmake would assume the constraint is inherited from the parent project (the root CMakeLists.txt file does set the project name and language to C).

So what I'm asking is :

  1. Could you remove the line altogether and see if that fixes the error?
  2. Is there anyone around who knows what the right choice would be in case that works?

What I saw was that when @edsiper added the file in commit 6dd4468 he had already added the project call and I suspect that there might not be a strong reason behind it.

My opinion is that if removing the call fixes the issue and nobody has a strong reason to include it we should remove it.

If I am not wrong, the reason at that time was because we needed to have the stream processor as a single functionality that could be called from the outside without any problems, not sure if that statement is still valid since we need to define the scope of that interface.

Sorry about the confusion, since we are cutting the release now I will just merge so then we need to decide about "when" to have these project(..) definitions (use case)

@edsiper edsiper merged commit b45c5a3 into fluent:master Apr 25, 2023
31 of 34 checks passed
@ThomasDevoogdt ThomasDevoogdt deleted the bugfix/no-c++-2 branch April 25, 2023 17:51
ThomasDevoogdt added a commit to ThomasDevoogdt/buildroot that referenced this pull request May 17, 2023
ThomasDevoogdt added a commit to ThomasDevoogdt/buildroot that referenced this pull request May 17, 2023
Fluent Bit v2.1 is the start of the new stable series of the project.

- Release Notes:

    https://fluentbit.io/announcements/v2.1.0/
    https://fluentbit.io/announcements/v2.1.1/
    https://fluentbit.io/announcements/v2.1.2/

No release notes yet for 2.1.3, but contains this
list of upstreamed patches:

    fluent/fluent-bit#7266
    fluent/fluent-bit#7254
    fluent/fluent-bit#7253

So the old patches can be dropped.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
ThomasDevoogdt added a commit to ThomasDevoogdt/buildroot that referenced this pull request May 22, 2023
Fluent Bit v2.1 is the start of the new stable series of the project.

Release Notes:

  https://fluentbit.io/announcements/v2.1.0/
  https://fluentbit.io/announcements/v2.1.1/
  https://fluentbit.io/announcements/v2.1.2/
  https://fluentbit.io/announcements/v2.1.3/

Fluent Bit v2.1.3 contains this list of upstreamed patches:

  fluent/fluent-bit#7266
  fluent/fluent-bit#7254
  fluent/fluent-bit#7253

So the old patches can be dropped.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
ThomasDevoogdt added a commit to ThomasDevoogdt/buildroot that referenced this pull request Jun 10, 2023
Fluent Bit v2.1 is the start of the new stable series of the project.

Release Notes:

  https://fluentbit.io/announcements/v2.1.0/
  https://fluentbit.io/announcements/v2.1.1/
  https://fluentbit.io/announcements/v2.1.2/
  https://fluentbit.io/announcements/v2.1.3/
  https://fluentbit.io/announcements/v2.1.4/

Fluent Bit v2.1.4 contains this list of upstreamed patches:

  fluent/fluent-bit#7266
  fluent/fluent-bit#7254
  fluent/fluent-bit#7253

So the old patches can be dropped.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
ThomasDevoogdt added a commit to ThomasDevoogdt/buildroot that referenced this pull request Jul 23, 2023
Fluent Bit v2.1 is the start of the new stable series of the project.

Release Notes:

  https://fluentbit.io/announcements/v2.1.0/
  https://fluentbit.io/announcements/v2.1.1/
  https://fluentbit.io/announcements/v2.1.2/
  https://fluentbit.io/announcements/v2.1.3/
  https://fluentbit.io/announcements/v2.1.4/

Fluent Bit v2.1.4 contains this list of upstreamed patches:

  fluent/fluent-bit#7266
  fluent/fluent-bit#7254
  fluent/fluent-bit#7253

So the old patches can be dropped.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
ThomasDevoogdt added a commit to ThomasDevoogdt/buildroot that referenced this pull request Jul 23, 2023
arnout pushed a commit to buildroot/buildroot that referenced this pull request Jul 23, 2023
Fluent Bit v2.1 is the start of the new stable series of the project.

Release Notes:

  https://fluentbit.io/announcements/v2.1.0/
  https://fluentbit.io/announcements/v2.1.1/
  https://fluentbit.io/announcements/v2.1.2/
  https://fluentbit.io/announcements/v2.1.3/
  https://fluentbit.io/announcements/v2.1.4/
  https://fluentbit.io/announcements/v2.1.5/
  https://fluentbit.io/announcements/v2.1.6/
  https://fluentbit.io/announcements/v2.1.7/

Fluent Bit v2.1.7 contains this list of upstreamed patches:

  fluent/fluent-bit#7266
  fluent/fluent-bit#7254
  fluent/fluent-bit#7253

So the old patches can be dropped.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
citral23 pushed a commit to citral23/buildroot that referenced this pull request Sep 18, 2023
Fluent Bit v2.1 is the start of the new stable series of the project.

Release Notes:

  https://fluentbit.io/announcements/v2.1.0/
  https://fluentbit.io/announcements/v2.1.1/
  https://fluentbit.io/announcements/v2.1.2/
  https://fluentbit.io/announcements/v2.1.3/
  https://fluentbit.io/announcements/v2.1.4/
  https://fluentbit.io/announcements/v2.1.5/
  https://fluentbit.io/announcements/v2.1.6/
  https://fluentbit.io/announcements/v2.1.7/

Fluent Bit v2.1.7 contains this list of upstreamed patches:

  fluent/fluent-bit#7266
  fluent/fluent-bit#7254
  fluent/fluent-bit#7253

So the old patches can be dropped.

Signed-off-by: Thomas Devoogdt <thomas@devoogdt.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
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.

None yet

4 participants