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

CMakeLists.txt: Miniz: compile as a static lib #6723

Conversation

ThomasDevoogdt
Copy link
Contributor

#6711

Buildroot enforces globally wide that all packages are dynamically linked (if BR2_SHARED_LIBS), so that means that these options slept through the subdirectories. The libminiz.so is not installed by cmake, causing the fluent-bit to crash. This patch forces libminiz to be compiled statically.

Note that this option doesn't have a dedicated prefix, adding one would require a change in libminiz. That is not the scope of this PR, if someone can take this up, feel free to do so.

Signed-off-by: Thomas Devoogdt thomas.devoogdt@barco.com

fluent#6711

Buildroot enforces globally wide that all packages are
dynamically linked (if BR2_SHARED_LIBS), so that means
that these options slept through the subdirectories.
The libminiz.so is not installed by cmake, causing the
fluent-bit to crash. This patch forces libminiz to be
compiled statically.

Note that this option doesn't have a dedicated prefix,
adding one would require a change in libminiz. That is
not the scope of this PR, if someone can take this up,
feel free to do so.

Signed-off-by: Thomas Devoogdt <thomas.devoogdt@barco.com>
@ThomasDevoogdt ThomasDevoogdt changed the title CMakeLists.txt: Miniz: compile as a header only lib CMakeLists.txt: Miniz: compile as a static lib Jan 23, 2023
@github-actions
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.

@github-actions github-actions bot added the Stale label Apr 28, 2023
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 the best would be for us to comply and install the SO but I doubt that's something we can tackle at the moment.

The only question I have is if overriding that in the root cmakelists.txt at that stage would be the right thing to do or if it could lead to side effects in other use cases.

I took a look at the cmakelists.txt file included in the library and it seems that since they don't use cmake we wrote a small one which combined with the fact that we are using a fairly old version of miniz makes me think that maybe we should make that override there on a localized way to avoid causing issues in the future when someone else adds something to the root cmakelists.txt file.

What do you think?

@ThomasDevoogdt
Copy link
Contributor Author

I think the best would be for us to comply and install the SO but I doubt that's something we can tackle at the moment.

The only question I have is if overriding that in the root cmakelists.txt at that stage would be the right thing to do or if it could lead to side effects in other use cases.

I took a look at the cmakelists.txt file included in the library and it seems that since they don't use cmake we wrote a small one which combined with the fact that we are using a fairly old version of miniz makes me think that maybe we should make that override there on a localized way to avoid causing issues in the future when someone else adds something to the root cmakelists.txt file.

What do you think?

I didn't know at the time that the CMakeLists.txt file in the lib was custom, so adapting makes sense.

For other libs, this is done:

  option(JANSSON_BUILD_SHARED_LIBS  OFF)

So I would add the MINIZ_BUILD_SHARED_LIBS option and turn it off in the root CMakeLists.txt.

@leonardo-albertovich
Copy link
Collaborator

Please excuse me, I mistakenly verified the wrong branch, the current branch seems to be using miniz 3 which does use cmake.

In that case, do you think we could do something like saving the current value of BUILD_SHARED_LIBS before line 513 and restore it after line 514 in order to limit the forced behavior?

@github-actions github-actions bot removed the Stale label Apr 29, 2023
@github-actions
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.

@github-actions github-actions bot added the Stale label Jul 29, 2023
@ThomasDevoogdt
Copy link
Contributor Author

Closed this PR as Miniz has disabled the shared library build by default in commit richgel999/miniz@0ce345c, which got bumped in commit 47fd29a.

@ThomasDevoogdt ThomasDevoogdt deleted the bugfix/miniz-linking-buildroot branch June 21, 2024 06:30
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

2 participants