search

Wohlstand / libADLMIDI

A Software MIDI Synthesizer library with OPL3 (YMF262) emulator
GNU Lesser General Public License v3.0
170 stars 17 forks source link

CMake: proper config #263

Closed dbarkar closed 1 year ago

dbarkar commented 1 year ago

CMake will try to find package config in the following files: libADLMIDIConfig.cmake libadlmidi-config.cmake Currently libADLMIDI-config.cmake is generated, so it works only for case-insensitive filesystems. Oops. With this change it will work everywhere, tested on Linux.

dg0yt commented 1 year ago

I don't see that this change was needed. <Pkg>-config.cmake is matched case-insensitively by CMake (!), regardless of filesystem. <Pkg>Config.cmake is matched case-sensitively by CMake, but a case-insensitive filesystem may still give hits for wrong case. So AFAICS the change may have broken users which relied on portable case-insensitive match, and it may lead (Windows) users to rely non-portable case-insensitive match.

Wohlstand commented 1 year ago

This was a change done for the recent previous pull-request (that adds this config) to fix the made issue. This would be an issue if this was been done over the thing existed for a while. Am I right?

dg0yt commented 1 year ago

I don't know the project's history but I see a related pull request on vcpkg and wonder if it should be accepted outside a regular release. I don't see that it fixes any issue at all. You should make a deliberate decision about your desired CMake config interface (package name, namespace, target names) and implement it correctly.

dbarkar commented 1 year ago

<Pkg>-config.cmake is matched case-insensitively by CMake (!), regardless of filesystem.

This is not true. CMake is looking for <PackageName>Config.cmake or <lowercasePackageName>-config.cmake (see https://cmake.org/cmake/help/latest/command/find_package.html). So when I try to find package libADLMIDI on Linux, I get this error:

Could not find a package configuration file provided by "libADLMIDI" with
  any of the following names:

    libADLMIDIConfig.cmake
    libadlmidi-config.cmake

The patch is legit.

You should make a deliberate decision about your desired CMake config interface (package name, namespace, target names) and implement it correctly.

It is arleady decided.

dg0yt commented 1 year ago

My bad, it takes <lowercasePackageName>-config.cmake indeed for portable case-insensitive matching, and you opted for exact match. Sorry for the noise.