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

[BitSerializer] Update to v0.50 #28564

Merged
merged 15 commits into from
Dec 30, 2022
Merged

Conversation

PavelKisliak
Copy link
Contributor

@PavelKisliak PavelKisliak commented Dec 26, 2022

Describe the pull request

  • What does your PR fix?

    Updates BitSerializer to version 0.50

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all (but only with static linkage), No

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

github-actions[bot]
github-actions bot previously approved these changes Dec 26, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Dec 27, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 27, 2022
github-actions[bot]
github-actions bot previously approved these changes Dec 27, 2022
@PavelKisliak PavelKisliak marked this pull request as ready for review December 27, 2022 07:05
Comment on lines 2 to 4
if (${BUILD_CSV_ARCHIVE})
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Flipping linkage by feature doesn't look right. If everything else is header-only, why use if at all?

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 also doubt how will be right.
Thanks, fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second sight, what's the problem with the CSV lib? If it is only DLL symbol export, dynamic linkage might work well for systems other than windows, and the statement might be wrapped into if(VCPKG_TARGET_IS_WINDOWS). This pattern exists in some other ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to export symbols on Windows, library doesn't have CMake code which support build shared library.
This is not big work, but as author, I don't see reasons to support shared linkage, the library is quite small.
If you know any cases where shared library will be useful even the library size is small, please let me know.

github-actions[bot]
github-actions bot previously approved these changes Dec 27, 2022
@PavelKisliak PavelKisliak requested a review from dg0yt December 27, 2022 09:10
@JonLiu1993 JonLiu1993 linked an issue Dec 27, 2022 that may be closed by this pull request
@JonLiu1993
Copy link
Member

All features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-static

@JonLiu1993
Copy link
Member

Tested usage successfully with BitSerializer:x64-windows triplet

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Dec 30, 2022
@vicroms vicroms merged commit edc91e1 into microsoft:master Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BitSerializer] update to 0.50
4 participants