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

wildmidi: add new recipe, version 0.4.5 #15345

Merged
merged 1 commit into from
Jan 28, 2023

Conversation

dbarkar
Copy link
Contributor

@dbarkar dbarkar commented Jan 18, 2023

Specify library name and version: wildmidi/0.4.5

WildMIDI is a simple software midi player which has a core softsynth library that can be used with other applications.

Unlike other MIDI libraries here, it supports Redbook's extended MIDI files (XMI) that are used by many old games.

There are few issues that I'd like to resolve, I don't know where else I can ask for help.

  • On Windows, CMAKE_BUILD_TYPE is not set automatically from build_type, so I had to set it explicitly. Why is this happening?
  • CMake's find_library can't find "libWildMidi" without ".a"/."so" extension on Linux, so I had to add it in conanfile.py. Why? Why is it working for other recipes?
  • Sometimes I get this error
    [HOOK - conan-center.py] post_package_info(): ERROR: [LIBRARY DOES NOT EXIST (KB-H054)] Component wildmidi::wildmidi library 'libwildmidi' is listed in the recipe, but not found installed at self.cpp_info.libdirs. Make sure you compiled the library correctly. If so, then the library name should probably be fixed. Otherwise, then the component should be removed. (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H054)
    Even though the library is in correct location, test_package and my app works well. Is it ok?
  • There are files in lib/cmake describing WildMidi::libwildmidi target but this lines produce vague errors
    self.cpp_info.set_property("cmake_file_name", "wildmidi")
    self.cpp_info.set_property("cmake_target_name", "libwildmidi")
    How to debug this issue?

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2023

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@jwillikers jwillikers left a comment

Choose a reason for hiding this comment

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

Looks really good so far!

recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@jwillikers
Copy link
Contributor

jwillikers commented Jan 24, 2023

  • On Windows, CMAKE_BUILD_TYPE is not set automatically from build_type, so I had to set it explicitly. Why is this happening?

On Windows, Conan Center's CI is building with the Visual Studio generator, which is a multi-configuration generator. Multiconfiguration generators do not set or use CMAKE_BUILD_TYPE. WildMIDI's top-level CMakeLists.txt needs to take this into account when setting things based on the build type. Generator expressions are used for the purpose of setting options based on the build type since they work for multi-configuration and single-configuration generators.
Also, WildMIDI sets the CMAKE_CONFIGURATION_TYPES variable, which is not a safe thing to do. Your workaround is probably sufficient for the Conan Center package here, though.

  • CMake's find_library can't find "libWildMidi" without ".a"/."so" extension on Linux, so I had to add it in conanfile.py. Why? Why is it working for other recipes?

It probably adds the lib prefix along with the possible suffixes. Maybe try setting the library name to WildMidi instead.

  • Sometimes I get this error
    [HOOK - conan-center.py] post_package_info(): ERROR: [LIBRARY DOES NOT EXIST (KB-H054)] Component wildmidi::wildmidi library 'libwildmidi' is listed in the recipe, but not found installed at self.cpp_info.libdirs. Make sure you compiled the library correctly. If so, then the library name should probably be fixed. Otherwise, then the component should be removed. (https://github.com/conan-io/conan-center-index/blob/master/docs/error_knowledge_base.md#KB-H054)
    Even though the library is in correct location, test_package and my app works well. Is it ok?

Try switching the library name to WildMidi and that might fix this.

  • There are files in lib/cmake describing WildMidi::libwildmidi target but this lines produce vague errors
    self.cpp_info.set_property("cmake_file_name", "wildmidi")
    self.cpp_info.set_property("cmake_target_name", "libwildmidi")
    How to debug this issue?

I suspect that these will work if you rename the library, but the cmake_target_name should be the full target name with the ::. In the new CMakeDeps generator, the file name corresponds to the name used to find the library via find_package, i.e. WildMidi in this case since the file name is WildMidiConfig.cmake.

self.cpp_info.set_property("cmake_file_name", "WildMidi")
self.cpp_info.set_property("cmake_target_name", "WildMidi::libwildmidi")

@conan-center-bot

This comment has been minimized.

@dbarkar dbarkar force-pushed the wildmidi branch 2 times, most recently from ee079f2 to e1d0f99 Compare January 25, 2023 11:40
@dbarkar
Copy link
Contributor Author

dbarkar commented Jan 25, 2023

Thanks! All issues are resolved now :)
Changing library name to WildMidi worked! It still has to be libWildMidi for MSVC, though, the code is much cleaner now.
This code seemed to work:

self.cpp_info.set_property("cmake_file_name", "wildmidi")
self.cpp_info.set_property("cmake_target_name", "wildmidi::wildmidi")

I not a CMake expert, I don't really know what I'm doing, please check if it's ok :)

Copy link
Contributor

@jwillikers jwillikers left a comment

Choose a reason for hiding this comment

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

Just need to tweak the CMake target names and config filename to match those provided by the upstream project.

recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
recipes/wildmidi/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
recipes/wildmidi/all/test_package/CMakeLists.txt Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@dbarkar
Copy link
Contributor Author

dbarkar commented Jan 25, 2023

So with latest changes, I have build errors:

wildmidi/0.4.5 (test package): CMake command: cmake -G "Unix Makefiles" -DCMAKE_TOOLCHAIN_FILE="/home/conan/project/test_package/build/Release/generators/conan_toolchain.cmake" -DCMAKE_POLICY_DEFAULT_CMP0091="NEW" -DCMAKE_BUILD_TYPE="Release" "/home/conan/project/test_package/."
-- Using Conan toolchain: /home/conan/project/test_package/build/Release/generators/conan_toolchain.cmake
-- Conan: Target declared 'WildMidi::libwildmidi'
-- Configuring done
-- Generating done
-- Build files have been written to: /home/conan/project/test_package/build/Release
wildmidi/0.4.5 (test package): CMake command: cmake --build "/home/conan/project/test_package/build/Release" '--' '-j28'
Scanning dependencies of target test_package
[ 50%] Building C object CMakeFiles/test_package.dir/test_package.c.o
/home/conan/project/test_package/test_package.c:4:10: fatal error: wildmidi_lib.h: No such file or directory
    4 | #include <wildmidi_lib.h>
      |          ^~~~~~~~~~~~~~~~
compilation terminated.

Test package can't find include file.
Specified in /home/conan/project/test_package/build/Release/generators/conan_toolchain.cmake:

list(PREPEND CMAKE_INCLUDE_PATH "/home/conan/.conan/data/wildmidi/0.4.5/_/_/package/dfbe50feef7f3c6223a476cd5aeadb687084a646/include")

I checked that dir and it's right there.
Also there is another error from CI.
I wonder why is it happening? I only changed the name.
Windows build works great.

Copy link
Contributor

@jwillikers jwillikers left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks for the contribution!

recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
@dbarkar dbarkar force-pushed the wildmidi branch 3 times, most recently from d2930da to d79bc91 Compare January 26, 2023 12:06
@conan-center-bot

This comment has been minimized.

@dbarkar
Copy link
Contributor Author

dbarkar commented Jan 26, 2023

Thanks for help!

jwillikers
jwillikers previously approved these changes Jan 26, 2023
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Looking good, just a small comment.

recipes/wildmidi/all/conanfile.py Outdated Show resolved Hide resolved
@prince-chrismc
Copy link
Contributor

Please do not force push 🙏 GitHub forces us to restart the review which is not fun!


def generate(self):
tc = CMakeToolchain(self)
if self.settings.os == "Windows":
Copy link
Contributor

Choose a reason for hiding this comment

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

@jcar87 the behavior of CMakeToolchain is a little wonky here if you can take a look please

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is more a problem with the project than CMakeToolchain. The project seems to incorrectly handle multi-configuration generators, like the Visual Studio generator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely agreed but I am curious if this is the right solution of perhaps patches might be more robust... maybe fixing upstream could be an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the right thing to do is to add a proper support for multi-config. I will try to make a change in upstream and come back with new PR when new version comes out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely think upstream could be better but I also think this is an edge case the new Conan generators weren't tested with so it would be nice to at least have test code to make sure this is intended

So I want to be clear, this PR is good :)

cmake.build()

def package(self):
copy(self, pattern="docs/license/LGPLv3.txt", dst=os.path.join(
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs says there's also a GPL licenseny reason why is that not copied?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm I found its only the demo app

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 19 (d15bc3b5074cb54bb5f1c6cc616846eb43be61b3):

  • wildmidi/0.4.5@:
    All packages built successfully! (All logs)

Conan v2 pipeline (informative, not required for merge) ✔️

Note: Conan v2 builds are informative and they are not required for the PR to be merged.

All green in build 20 (d15bc3b5074cb54bb5f1c6cc616846eb43be61b3):

  • wildmidi/0.4.5@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 3c5f838 into conan-io:master Jan 28, 2023
StellaSmith pushed a commit to StellaSmith/conan-center-index that referenced this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants