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

portmidi update #6

Open
rbdannenberg opened this issue Oct 1, 2021 · 30 comments
Open

portmidi update #6

rbdannenberg opened this issue Oct 1, 2021 · 30 comments

Comments

@rbdannenberg
Copy link

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

@sonoro1234
Copy link
Owner

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
Copy link
Owner

sonoro1234 commented Oct 2, 2021

@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.

@rbdannenberg
Copy link
Author

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: rbd@cs.cmu.edu. Thanks.

@sonoro1234
Copy link
Owner

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: rbd@cs.cmu.edu. Thanks.

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

@rbdannenberg
Copy link
Author

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.

@sonoro1234
Copy link
Owner

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
Copy link
Owner

Now waiting to cmake changes in your portmidi repo.

@rbdannenberg
Copy link
Author

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
Copy link
Author

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.

@sonoro1234
Copy link
Owner

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

@sonoro1234
Copy link
Owner

sonoro1234 commented Dec 21, 2021

@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

@rbdannenberg
Copy link
Author

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

@andy5995
Copy link

andy5995 commented Dec 21, 2021

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.

@rbdannenberg
Copy link
Author

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.

@sonoro1234
Copy link
Owner

sonoro1234 commented Dec 22, 2021

@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?

@andy5995
Copy link

@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
Copy link

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).

@sonoro1234
Copy link
Owner

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

@andy5995
Copy link

andy5995 commented Dec 22, 2021

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.

@sonoro1234
Copy link
Owner

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

@andy5995
Copy link

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!

@sonoro1234
Copy link
Owner

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

@rbdannenberg
Copy link
Author

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!

@sonoro1234
Copy link
Owner

Can you try this with Lua and see what happens?

Change is still not done!!!

@rbdannenberg
Copy link
Author

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.

@sonoro1234
Copy link
Owner

I think it wont work because packaging folder does not exist

@sonoro1234
Copy link
Owner

sonoro1234 commented Dec 23, 2021

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

@rbdannenberg
Copy link
Author

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.

@sonoro1234
Copy link
Owner

sonoro1234 commented Dec 24, 2021

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
Copy link
Owner

sonoro1234 commented Dec 24, 2021

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

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

No branches or pull requests

3 participants