search

PortMidi / portmidi

portmidi is a cross-platform MIDI input/output library
Other
116 stars 31 forks source link

Add option for MSVC_RUNTIME_LIBRARY #18

Closed nidefawl closed 2 years ago

nidefawl commented 2 years ago

In my project I use a static portmidi library. The library is linked into a binary that uses the MSVC DLL runtime.

With this pull request a new option is added PM_USE_STATIC_RUNTIME that allows disabling the use of the static runtime. PM_USE_STATIC_RUNTIME defaults to ON, but is only relevant when BUILD_SHARED_LIBS is OFF.

This pull request fixes 2 issues:

  1. Missing option to disable use of static runtime (I want static lib + DLL runtime for my project).
  2. The release runtime being used in debug builds.
OlivierSohn commented 2 years ago

@rbdannenberg would you mind reviewing? I am not an expert in cmake.

nidefawl commented 2 years ago

I never liked this pull-request + comments way of working. If you'd like, I'll try to add a PM_USE_STATIC_RUNTIME flag, which might be easier. But for now:

Why lower the CMAKE version? This was tested with 3.21, and I don't know if 3.20 works or not , and it's very hard to test all options.

The cmake version change isn't part of this pull request. Please ignore it.

This request removes helpful comments reminding me what all the MVC flags do (lines 126-127.) I need the hints.

This removes including ../pm_win/static.cmake, without which static builds generate warnings from MVC about inconsistent runtime models. CMAKE does not seem to support building a static application (or maybe there's a way to do it now and I haven't figured it out). This static.cmake file make brute force changes to create a consistent project.

Your fix overrides the debug runtime with the release one. This pull request fixes it.

Building a static library doesn't imply the use of a static runtime. I want a .lib that links against the debug dll visual studio runtime, and currently there is no way to override the hardcoded behavior. So I added a switch to turn it off.

You can read more about it in the cmake docs: https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html

rbdannenberg commented 2 years ago

How do I ignore portions of a pull request? Are you suggesting I apply the pull request locally, undo some changes, push to the main repo and then not accept the request? Sorry, I really don't know how this works.

"Your fix overrides the debug runtime with the release one. This pull request fixes it." - I think it's more correct to say this pull establishes a different convention for linking which will break normal uses of static libraries. I don't mind adding an option to support another convention, but I don't want to break things that already work and I find useful.

Your goal makes sense -- static library, dynamic runtime -- I'm all for a switch to enable this as long as it doesn't prevent static library, static runtime.

nidefawl commented 2 years ago

How do I ignore portions of a pull request? Are you suggesting I apply the pull request locally, undo some changes, push to the main repo and then not accept the request? Sorry, I really don't know how this works.

"Your fix overrides the debug runtime with the release one. This pull request fixes it." - I think it's more correct to say this pull establishes a different convention for linking which will break normal uses of static libraries. I don't mind adding an option to support another convention, but I don't want to break things that already work and I find useful.

Your goal makes sense -- static library, dynamic runtime -- I'm all for a switch to enable this as long as it doesn't prevent static library, static runtime.

I will make a new PR. I just wanted to make sure I don't waste my time if doesn't get merged

nidefawl commented 2 years ago

I made a new more minimal PR: https://github.com/PortMidi/portmidi/pull/19

Btw: My change was inspired by the great portaudio library. Apparently you think their use of static libraries is also broken then.