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

Fix shared build #148

Merged
merged 1 commit into from
Aug 30, 2020
Merged

Fix shared build #148

merged 1 commit into from
Aug 30, 2020

Conversation

intelligide
Copy link
Contributor

Currently, it always build both static and shared libs with the same name, which leads to name collision on Windows. The static library cbor.a replaces the symbol archive cbor.a.

This PR fix this problem:

  • Build shared lib or static lib, not both at once. Use BUILD_SHARED_LIBS to create the shared library.
  • Export symbols with cbor_export.h header (generated by CMake) and CBOR_EXPORT macros.

Fix conan-io/conan-center-index#2578

@codecov-commenter
Copy link

Codecov Report

Merging #148 into master will decrease coverage by 1.72%.
The diff coverage is 91.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
- Coverage   90.16%   88.44%   -1.73%     
==========================================
  Files          52       53       +1     
  Lines        3528     3548      +20     
  Branches      195      198       +3     
==========================================
- Hits         3181     3138      -43     
- Misses        169      332     +163     
+ Partials      178       78     -100     
Impacted Files Coverage Δ
src/allocators.c 100.00% <ø> (ø)
src/cbor/callbacks.c 100.00% <ø> (ø)
test/memory_allocation_test.c 83.00% <ø> (ø)
src/cbor/internal/builder_callbacks.c 90.08% <86.66%> (+3.68%) ⬆️
src/cbor/internal/stack.c 93.75% <100.00%> (+0.41%) ⬆️
test/stack_over_limit_test.c 100.00% <100.00%> (ø)
test/assertions.c 0.00% <0.00%> (-100.00%) ⬇️
test/stream_expectations.c 0.00% <0.00%> (-100.00%) ⬇️
src/cbor/strings.c 96.00% <0.00%> (+1.33%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c78f437...e30bdc0. Read the comment docs.

@PJK
Copy link
Owner

PJK commented Aug 25, 2020

Hi @intelligide, thank you very much for the PR, I think this makes sense.

Quick question: What is the potential to cause breakages/incompatibilities for clients that rely on the current behavior? I.e. do we need a new version?

@intelligide
Copy link
Contributor Author

There is no breaking change inside code. And if clients currently use the static lib, all should be fine.
A minor version be good.

@PJK PJK merged commit 344fe90 into PJK:master Aug 30, 2020
PJK added a commit that referenced this pull request Aug 30, 2020
@PJK
Copy link
Owner

PJK commented Aug 30, 2020

Thank you, I'll push a release next week.

One more question: Is it possible to write a "linkage test" that would prevent future regressions (e.g. someone forgets to export a new function)?

@intelligide
Copy link
Contributor Author

intelligide commented Sep 1, 2020

Just write tests for each function and run them twice with BUILD_SHARED_LIBS as ON and OFF. The build will fail if an used function isn't exported.

PJK added a commit that referenced this pull request Sep 2, 2020
Drop superfluous import, add changelog for #148
@PJK PJK mentioned this pull request Sep 20, 2020
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.

[package] libcbor/0.7.0: "Shared" recipe option seems to be broken
3 participants