perezmeyer commented 5 years ago

Note: this is still WIP, as an install target must be defined.

Define the current version in the main CMakeLists.txt, so it can be used within the project.
Do not pass STATIC to add_library. By default CMake will create a static library but a user can define BUILD_SHARED_LIBS=1 to create a shared library.
Define VERSION, SOVERSION and PUBLIC_HEADER as target properties.
Add a simple note in README.md to let users know how to build it as a shared library.

codecov[bot] commented 5 years ago

Codecov Report

Merging #49 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   92.72%   92.72%           
=======================================
  Files           1        1           
  Lines        2544     2544           
=======================================
  Hits         2359     2359           
  Misses        185      185

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 a505f69...4bc2ef1. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #49 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   92.66%   92.66%           
=======================================
  Files           1        1           
  Lines        2548     2548           
=======================================
  Hits         2361     2361           
  Misses        187      187

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 c340e78...4efced6. Read the comment docs.

perezmeyer commented 5 years ago

I still have to solve the static lib issue when installing, but I'll defer it after the API review.

perezmeyer commented 5 years ago

I still need to fix the static build and probably add a CMake export file too.

perezmeyer commented 5 years ago

Sorry if the history went wrong, if you want I can close this MR and create a new one. Apart from that this should be ready.

perezmeyer commented 5 years ago

The appveyor test suite fails because it seems to not know the default install target. I guess this is due to the way windows works.

perezmeyer commented 5 years ago

Better wait some more, I want to fix some other stuff.

perezmeyer commented 5 years ago

@mity from my side this should be ready. I do not have Windows at hand (haven't used it in years), so I really don't know how to solve the appveyor check. I'll see if I can find someone who knows :-/

perezmeyer commented 5 years ago

Finally, there it is. On Windows one has to declare which symbols need to be exported or export them all. For this CMake >= 3.4 is needed.

@mity At this point it works but maybe history is too messy. Please tell me if you want me to squash all commits into a new MR. If this is just fine, then it's ready for being merged.

mity commented 5 years ago

@mity At this point it works but maybe history is too messy. Please tell me if you want me to squash all commits into a new MR. If this is just fine, then it's ready for being merged.

Given, the complete PR changes < 70 lines, I'd squash everything into a single commit. GitHub knows how to do that. Are you ok with that?

perezmeyer commented 5 years ago

@mity At this point it works but maybe history is too messy. Please tell me if you want me to squash all commits into a new MR. If this is just fine, then it's ready for being merged.

Given, the complete PR changes < 70 lines, I'd squash everything into a single commit. GitHub knows how to do that. Are you ok with that?

Totally ok!

mity commented 5 years ago

@perezmeyer Thanks for all the work on it.

perezmeyer commented 5 years ago

My pleasure!

On Thu, 7 Feb 2019 at 15:54, Martin Mitáš notifications@github.com wrote:

@perezmeyer https://github.com/perezmeyer Thanks for all the work on it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mity/md4c/pull/49#issuecomment-461551902, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI6TJQigobh5MocjTCHU_GkuR_VrQFeks5vLHZwgaJpZM4agNmM .

-- Lisandro Damián Nicanor Pérez Meyer http://perezmeyer.com.ar/ http://perezmeyer.blogspot.com/

mity / md4c

Be able to build md4c as a shared library. #49

Codecov Report

Codecov Report