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 INSTALL_STATIC_LIBS buid option #2071

Closed
wants to merge 1 commit into from

Conversation

remicollet
Copy link
Contributor

to allow to not install the static library

As RPM packaging Guidelines disallow static libraries
See https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries

Packages including libraries SHOULD exclude static libs as far as possible (e.g., by configuring with --disable-static). Applications linking against libraries SHOULD link against shared libraries not static versions.

to allow to not install the static library
@maxirmx
Copy link
Member

maxirmx commented May 3, 2023

@remicollet
Thank you for the issue. I do not think we need an additional option, it is a bug in rnp packaging that needs to be fixed.

@remicollet
Copy link
Contributor Author

remicollet commented May 3, 2023

This has to be managed upstream in the CMakeLists else the installed /usr/lib64/cmake/rnp/ files will have reference to the static library and will be unusable.

Downstream (packaging) can remove the *.a file (a common practice), but cmake will have to be fixed, so this new option is really the simple way, have cmake generate the proper file.

Of course, this patch can be used "only" for packaging, but as it has no impact in standard build... and other distros may need them... and I hate having not upstreamable patch...

@maxirmx
Copy link
Member

maxirmx commented May 3, 2023

It was incorrect optimization.

Existing build script always creates static library. If BUILD_SHARED_LIBS is ON it repackages static library into shared library.
So if someone needs both shared and static the same build procedure is run twice

I will rollback it so that BUILD_SHARED_LIBS does not install static library

@maxirmx
Copy link
Member

maxirmx commented May 3, 2023

You may be right though ... An INSTALL may be better approach

@remicollet
Copy link
Contributor Author

I will rollback it so that BUILD_SHARED_LIBS does not install static library

Thanks (and indeed version 0.16.x was OK)

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7e63ae6) 83.48% compared to head (1f5a82a) 83.49%.

❗ Current head 1f5a82a differs from pull request most recent head d1c59d9. Consider uploading reports for the commit d1c59d9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2071   +/-   ##
=======================================
  Coverage   83.48%   83.49%           
=======================================
  Files         159      159           
  Lines       31957    31957           
=======================================
+ Hits        26679    26681    +2     
+ Misses       5278     5276    -2     

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ronaldtse
Copy link
Contributor

@maxirmx can you help approve if good to go? Thanks.

@maxirmx
Copy link
Member

maxirmx commented May 3, 2023

@ronaldtse please give me a day to think. @remicollet made a perfectly valid point about packaging. There are three options how to fix it. We shall consider both downstream requirements and CI performance. We are now running ~110 test workflows and adding a flag can bring it up to 150.
I am sorry for this delay.

@maxirmx maxirmx self-requested a review May 3, 2023 20:13
Copy link
Member

@maxirmx maxirmx left a comment

Choose a reason for hiding this comment

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

I propose to rollback to the previous algorithm -- #2076
It means double run for installation/packaging if both libraries are needed but it loooks like there are no complains

@remicollet remicollet closed this May 9, 2023
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