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

Set VERSION and SOVERSION as additional target properties #2403

Closed
wants to merge 6 commits into from

Conversation

glaubitz
Copy link
Contributor

Fixes #2402

Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Like where this is headed, but maybe more specifics on the numbers?
For my comment to make sense, line 2:

set(MAJORVER 0)
set(MINORVER 10)
set(PATCHVER 21)
project (s2n LANGUAGES C VERSION ${MAJORVER}.${MINORVER}.${PATCHVER})

@@ -173,6 +173,10 @@ file(GLOB S2N_SRC
add_library(${PROJECT_NAME} ${S2N_HEADERS} ${S2N_SRC})
set_target_properties(${PROJECT_NAME} PROPERTIES LINKER_LANGUAGE C)

# As long as the ABI isn't stable, set the SO version to 0unstable
set_target_properties(${CMAKE_PROJECT_NAME} PROPERTIES VERSION 1.0.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

As s2n is not yet at 1.0, maybe:

set_target_properties(${CMAKE_PROJECT_NAME} PROPERTIES VERSION ${MAJORVER}.${MINORVER})
set_target_properties(${CMAKE_PROJECT_NAME} PROPERTIES SOVERSION ${MAJORVER})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was basically doing what AWS is using in other projects like aws-c-common or aws-checksums. Both used version 1.0.0 and the SO version unstable0.

But, of course, I can change it anyway you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

What version would you suggest for a project that hasn't had a 1.0.0 release yet? unstable0 for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library version number is that important, so you can choose any value you want. But the SO version should have something that indicates whether the API is stable or not.

@dougch dougch requested a review from lrstewart November 19, 2020 21:32
@codecov-io
Copy link

Codecov Report

Merging #2403 (780b1f0) into main (d0b2c5e) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2403      +/-   ##
==========================================
- Coverage   81.81%   81.80%   -0.01%     
==========================================
  Files         268      268              
  Lines       18326    18326              
==========================================
- Hits        14993    14992       -1     
- Misses       3333     3334       +1     

Impacted file tree graph

@glaubitz
Copy link
Contributor Author

glaubitz commented Nov 24, 2020 via email

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/stale label Jun 26, 2021
@glaubitz
Copy link
Contributor Author

glaubitz commented Jun 26, 2021 via email

@stale stale bot removed the status/stale label Jun 26, 2021
@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added status/stale and removed status/stale labels Jan 8, 2022
@glaubitz
Copy link
Contributor Author

glaubitz commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Bump.

@dougch dougch self-requested a review January 18, 2022 20:11
@dougch
Copy link
Contributor

dougch commented Jan 18, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 22, 2022

So we're at version 1.3.7 now and the unstable can come off, this is worth a revisit.

@dougch
Copy link
Contributor

dougch commented Feb 22, 2022

Benchmark report
No change in performance detected.

@glaubitz
Copy link
Contributor Author

glaubitz commented Mar 2, 2022

Any input required from my side?

I would also be great if the AWS C++ packages could be changed to adhere to the common cmake paths, see for example: awslabs/aws-c-event-stream#15.

The canonical paths for cmake files is /usr/lib*/cmake/$package, not /usr/lib*/$package/cmake which is why distributions have to keep patching all AWS C++ packages.

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

I apologize, this fell through the cracks!

It looks good to me, except that s2n-tls has been promoted to 1.0 / stable since it was opened. Should we set a different SOVERSION?

@dougch
Copy link
Contributor

dougch commented Aug 3, 2022

superseded by #3407

@dougch dougch closed this Aug 3, 2022
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.

Shared library missing SO version
4 participants