Lua2SC icon indicating copy to clipboard operation
Lua2SC copied to clipboard

portmidi update

Open rbdannenberg opened this issue 2 years ago • 30 comments

Hi sonoro1234,

I'm the creator of PortMidi. I was a bit surprised to find how many portmidi versions there are on Github. I've finally gotten around to making a number of updates that are mostly minor but long overdue.

I also decided it would be wise to host a copy of PortMidi on Github.

I went through every copy of PortMidi I could find, looking at 4583 files and running diff 7961 times, but who's counting? :-)

I picked up a few fixes from your collective works, although with this many files and changes, I'm sure I missed something.

Your repo appears to serve as a source of bindings for Lua. I hope you can add support for the new Pm_CreateVirtualInput() and Pm_CreateVirtualOutput() functions for Linux and macOS. Let me know any problems. If you tell me you updated and things work, I'll recommend your repo in rbdannenberg/portmidi/README.md. Note that you can also unbundle the bindings from the base portmidi implementation in C, and just refer to a particular version at rbdannenberg/portmidi. (Although rbdannenberg/portmidi has some old bindings for python, C#, and Java, I'm not planning to maintain or even keep them since they have been siphoned off and maintained in independent efforts. So I'm unbundling bindings from base code too.)

Thanks, Roger

rbdannenberg avatar Oct 01 '21 16:10 rbdannenberg

Hi Roger,

Thanks for your interest. All my work on this is very old, so I must first understand what I did. It seems that I only did modifications on CMakeLists but I should do more investigation to use rbdannenberg/portmidi I will keep you informed about progress

Victor

sonoro1234 avatar Oct 01 '21 17:10 sonoro1234

@rbdannenberg I like to have all necessary repos as submodules to ease the building. The problem for me with your github repository is that it is including other projects besides portmidi. Also: I should be able to just use add_subdirectory(portmidi_directory) perhaps with some cmake variables defined to build portmidi_static library, as it is done now in Lua2SC repository. But I had to change CMakeLists.txt for achieving that.

sonoro1234 avatar Oct 02 '21 09:10 sonoro1234

Did you get my reply via email? It seems to me that GitHub's notifications are not reliable for me, but I don't know how it is supposed to work. Email is most reliable: [email protected]. Thanks.

rbdannenberg avatar Oct 04 '21 16:10 rbdannenberg

Did you get my reply via email? It seems to me that GitHub's notifications are not reliable for me, but I don't know how it is supposed to work. Email is most reliable: [email protected]. Thanks.

No, I did not get it!! But I did get your github notification, so it seems to me to be a reliable channel.

sonoro1234 avatar Oct 04 '21 16:10 sonoro1234

Interesting! This does seem to be reliable -- working on GitHub, but there are about 15 other social media platforms that I'd have to poll to receive timely messages, and I'm not sure GitHub always notifies me by email. When they do, they say I can reply by email, but that didn't work. So I'm learning...

Here's what I tried to send days ago by email:

Thanks for the comments. I was trying to keep things simple and consistent with sourceforge, but it seems that splitting even these small libraries is expected, so I'll work on that. I like the idea of add_subdirectory(portmidi_directory), although that then means there is an implicit CMake interface -- cmake is so full of global variables and build system are so full of options, there seem to be endless ways the main program and the subdirectory can be incompatible. But I'll give it a shot.

-Roger

PS I just acquired the github/PortMidi organization, and I plan to move sources there. Haskell bindings are already there. If you'd like to move Lua bindings there, that would be fine, or else I can put a link in the main readme.md to point to your repo.

rbdannenberg avatar Oct 04 '21 17:10 rbdannenberg

Just checked a build with your repo as portmidi source on windows with mingw. It seems to build correctly. I dont have a midi interface right now to perform tests. I should also check build and tests on Linux.

I will let you know when I am done.

sonoro1234 avatar Oct 12 '21 08:10 sonoro1234

Now waiting to cmake changes in your portmidi repo.

sonoro1234 avatar Oct 18 '21 09:10 sonoro1234

Right, I just got virtual devices working as planned (and learned much more than I wanted to know about CoreMIDI, like some things assume Apple's CF event loop is running, but that's not documented anywhere). Anyway, work is still underway.

rbdannenberg avatar Oct 27 '21 21:10 rbdannenberg

The latest portmidi is at portmidi repo in PortMidi project. Virtual devices are working and code was tested on Win, macOS & Linux, but I'm sure there will be some tweaks to CMake and other details based on feedback.

rbdannenberg avatar Dec 20 '21 01:12 rbdannenberg

Seems to compile correctly. Still couldnt try library due to lack of midi device.

sonoro1234 avatar Dec 20 '21 10:12 sonoro1234

@rbdannenberg

My bad: I was compiling rbdannenberg/portmidi instead of PortMidi/portmidi

There is this CMake error: CMake Error: File C:/supercolliderrepos/Lua2SC/portmidi/Lua2SC.pc.in does not exist.

CMAKE_CURRENT_SOURCE_DIR should be used instead of CMAKE_PROJECT_NAME:

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/portmidi.pc.in ${CMAKE_CURRENT_SOURCE_DIR}/portmidi.pc @ONLY)
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/portmidi.pc DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)

or even simpler:

configure_file(portmidi.pc.in portmidi.pc @ONLY)

While building only this errors appeared:

C:\supercolliderrepos\Lua2SC\portmidi\pm_win\pmwinmm.c:256:8: warning: return type defaults to 'int' [-Wimplicit-int]
 static improve_winerr(int pm_hosterror, char *message)
        ^~~~~~~~~~~~~~
C:\supercolliderrepos\Lua2SC\portmidi\pm_win\pmwinmm.c: In function 'winmm_write_byte':
C:\supercolliderrepos\Lua2SC\portmidi\pm_win\pmwinmm.c:991:31: warning: assignment to 'uint32_t *' {aka 'unsigned int *'} from incompatible pointer ty
pe 'DWORD *' {aka 'long unsigned int *'} [-Wincompatible-pointer-types]
         midi->fill_offset_ptr = &(hdr->dwBytesRecorded);

Also, I find risky to use BUILD_SHARED_LIBS global cmake variable because it is seen by all subprojects

sonoro1234 avatar Dec 21 '21 11:12 sonoro1234

@sonoro1234 Thanks for the report -- this discussion continues on PortMidi/portmidi#2.

rbdannenberg avatar Dec 21 '21 16:12 rbdannenberg

or even simpler:

configure_file(portmidi.pc.in portmidi.pc @ONLY)

I don't necessarily think that's simpler but it depends on how people like to use variables. In this case though, it looks like the variable in the portmidi CMakeList.txt is getting overridden by a variable of the main project.

I was reading a bit about using external projects with cmake.

SuperTux uses several external projects. You can see for example on L339 how they're using the cmake function ExternalProject_Add

A little extra discussion about it on this Stackoverflow post, though it may be a little outdated.

andy5995 avatar Dec 21 '21 20:12 andy5995

Can you guys figure out the "right" CMake code for configure_file? It sounds like currently it's fine for @andy5995 who prefers not to change it, and broken for @sonoro1234 who wants to change it. I have no idea what the best practice is and never use configure.

rbdannenberg avatar Dec 21 '21 20:12 rbdannenberg

@rbdannenberg @andy5995

To state it in a simpler way:

Using add_subdirectory with PortMidi/portmidi from Lua2SC (as done with all other subprojects) gives the error CMake Error: File C:/supercolliderrepos/Lua2SC/portmidi/Lua2SC.pc.in does not exist. Which is the solution to this issue?

sonoro1234 avatar Dec 22 '21 08:12 sonoro1234

@sonoro1234 CMAKE_PROJECT_NAME in the portmidi cmake file is getting overwritten with the variable from your /CMakeLists.txt. Instead of using add_subdirectory(), I believe you should be using ExternalProject_Add().

andy5995 avatar Dec 22 '21 19:12 andy5995

I'm not sure really... I'm reading more about add_subdirectory() and it seems like maybe that should be ok? Maybe if EXCLUDE_FROM_ALL is used? I'm not really sure why the variable from the portmidi cmake is getting overwritten... Maybe @sonoro1234 you should push your changes to a branch so I can see what you have exactly and maybe we can figure it out by trial and error. I could pull your branch and try stuff too probably (more likely after the holidays though).

andy5995 avatar Dec 22 '21 19:12 andy5995

ExternalProject_Add is much more complicated stuff than add_subdirectory, sometimes needed for bad behaved repositories. add_subdirectory is used with succes in Lua2SC for all the other subprojects except LuaJIT.

Just changing this line: configure_file(portmidi.pc.in portmidi.pc @ONLY) makes it work for me

sonoro1234 avatar Dec 22 '21 19:12 sonoro1234

Just changing this line: configure_file(portmidi.pc.in portmidi.pc @ONLY) makes it work for me

Yes, but it's usually a bad sign when variables get overwritten. A bug in cmake maybe?

I get your point though. That small change would be simple and quick. But it doesn't really address that a variable is getting overwritten, which could just as easily happen again later as other changes are made to the portmidi cmake in the future.

andy5995 avatar Dec 22 '21 19:12 andy5995

Yes, but it's usually a bad sign when variables get overwritten. A bug in cmake maybe?

Not at all, CMAKE_PROJECT_NAME is Lua2SC as it is the project to be built. portmidi is just a subproject used by Lua2SC with add_subdirectory

sonoro1234 avatar Dec 22 '21 20:12 sonoro1234

Yes, for you portmidi is a subproject, but the portmidi project is also a project by itself and should be able to use that variable within its own cmake file.

But ultimately it's up to you two. I don't have a lot of cmake experience because I've tried to use it and ran into a lot of trouble, and now I use @mesonbuild for all my projects. I'll let you and @rbdannenberg continue the discussion. He can just make the change in the portmidi repo if he thinks it's appropriate. Good luck with your projects!

andy5995 avatar Dec 22 '21 20:12 andy5995

Yes, for you portmidi is a subproject, but the portmidi project is also a project by itself and should be able to use that variable within its own cmake file.

It is the same happening with CMAKE_SOURCE_DIR and CMAKE_CURRENT_SOURCE_DIR. If a project wants to be able to be used as a subproject it must use the second one because the first is overridden by the superproject

sonoro1234 avatar Dec 22 '21 20:12 sonoro1234

I've been integrating CMake changes from be-portmidi's pull request. Be obviously knows a lot more about CMake than I do -- in fact after some reading, I realized CMake had a major conceptual overhaul with v3.0 but I guess they had to retain backwards compatibility -- no wonder it's so confusing! I hate to introduce many more changes, but I think the CMake overhaul in portmidi will prevent others from turning up their noses at my old style and very ugly CMake.

Anyway, one of the changes is that the configure code looks like:

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/packaging/portmidi.pc.in
    ${CMAKE_CURRENT_BINARY_DIR}/packaging/portmidi.pc @ONLY)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/packaging/portmidi.pc
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)

I think this at least fixes the package name problem directly by not using a variable, and it uses CURRENT variables for locations. Can you try this with Lua and see what happens? If it fails, I should soon be able to push all the changes for Linux, and perhaps pulling the complete set of changes will work. And if that fails, we'll find a solution. Thanks!

rbdannenberg avatar Dec 22 '21 21:12 rbdannenberg

Can you try this with Lua and see what happens?

Change is still not done!!!

sonoro1234 avatar Dec 23 '21 08:12 sonoro1234

Change is still not done!!!

I think my message was confusing. I was suggesting that as I wrap up some testing of a new CMake, maybe you could copy the 4 lines of configure_file() code above into your working code and see if it works. If it doesn't work, maybe I could try something different in my local working code before I test it and push it to github. I have a few more things to look at but hope to push some new code today, but there's a lot of holiday-related stuff going on too.

rbdannenberg avatar Dec 23 '21 16:12 rbdannenberg

I think it wont work because packaging folder does not exist

sonoro1234 avatar Dec 23 '21 16:12 sonoro1234

Perhaps you mean

configure_file(${CMAKE_CURRENT_SOURCE_DIR}/portmidi.pc.in
    ${CMAKE_CURRENT_BINARY_DIR}/portmidi.pc @ONLY)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/portmidi.pc
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)

Just tried: It works with this lines

sonoro1234 avatar Dec 23 '21 16:12 sonoro1234

The top-level directory is getting pretty cluttered with meta-data files, so I adopted be's approach of putting config files in packaging subdirectory. I think when portmidi.pc.in is moved to packaging, the suggested changes will work with a packaging subdirectory. I'm going to assume that and proceed. Great to know this will fix your problem (modulo using the packaging subdir, so we'll see how that goes.) Thanks.

rbdannenberg avatar Dec 23 '21 21:12 rbdannenberg

just tried but:

CMake Error: INSTALL(EXPORT) given unknown export "PortMidiTargets"

I have cmake 3.16.1, may be I should try cmake 3.19 The same happens also with cmake 3.22.1

sonoro1234 avatar Dec 24 '21 11:12 sonoro1234

I have never used install(EXPORT ~but may be PortMidiTargets should be portmidi?~ ~Or perhaps it is missing as first line TARGETS portmidi~

I did try

install(
  TARGETS portmidi
  EXPORT PortMidiTargets
  DESTINATION "${PORTMIDI_INSTALL_CMAKEDIR}"
)

with success

Also tried

install(
  TARGETS portmidi
  EXPORT PortMidiTargets
  DESTINATION "${PORTMIDI_INSTALL_CMAKEDIR}"
)
install(
  EXPORT PortMidiTargets
  FILE PortMidiTargets.cmake
  NAMESPACE PortMidi::
  DESTINATION "${PORTMIDI_INSTALL_CMAKEDIR}"
)

with success

sonoro1234 avatar Dec 24 '21 17:12 sonoro1234