PortMidi / portmidi

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

reorganize repository and rewrite build system #3

Closed Be-ing closed 2 years ago

Be-ing commented 2 years ago
Be-ing commented 2 years ago

New directory structure:

portmidi on  build_rewrite_new [$] via △ v3.22.0 
❯ lsd --tree
 .
├──  bindings
│  └──  java
│     ├──  CMakeLists.txt
│     ├──  jportmidi
│     │  ├──  JPortMidi.java
│     │  ├──  JPortMidiApi.java
│     │  └──  JPortMidiException.java
│     ├──  make.bat
│     ├──  pmdefaults
│     │  ├──  manifest.txt
│     │  ├──  pmdefaults
│     │  ├──  pmdefaults-icon.bmp
│     │  ├──  pmdefaults-icon.gif
│     │  ├──  pmdefaults-icon.png
│     │  ├──  pmdefaults-icon.xcf
│     │  ├──  pmdefaults-license.txt
│     │  ├──  pmdefaults.icns
│     │  ├──  pmdefaults.ico
│     │  ├──  PmDefaults.java
│     │  ├──  PmDefaultsFrame.java
│     │  ├──  portmusic_logo.png
│     │  └──  README.txt
│     ├──  pmjni
│     │  ├──  jportmidi_JportMidiApi.h
│     │  ├──  pmjni.c
│     │  └──  pmjni.rc
│     └──  README.txt
├──  CHANGELOG.txt
├──  CMakeLists.txt
├──  Doxyfile
├──  include
│  ├──  pmutil.h
│  ├──  portmidi.h
│  └──  porttime.h
├──  license.txt
├──  packaging
│  ├──  portmidi.pc.in
│  └──  PortMidiConfig.cmake.in
├──  portmusic_logo.png
├──  README.md
├──  README.txt
├──  src
│  ├──  portmidi
│  │  ├──  common
│  │  │  ├──  CMakeLists.txt
│  │  │  ├──  pminternal.h
│  │  │  ├──  pmutil.c
│  │  │  └──  portmidi.c
│  │  ├──  linux
│  │  │  ├──  finddefault.c
│  │  │  ├──  finddefault.h
│  │  │  ├──  pmlinux.c
│  │  │  ├──  pmlinux.h
│  │  │  ├──  pmlinuxalsa.c
│  │  │  ├──  pmlinuxalsa.h
│  │  │  └──  README_LINUX.txt
│  │  ├──  mac
│  │  │  ├──  finddefault.c
│  │  │  ├──  Makefile.osx
│  │  │  ├──  pmmac.c
│  │  │  ├──  pmmac.h
│  │  │  ├──  pmmacosxcm.c
│  │  │  ├──  pmmacosxcm.h
│  │  │  ├──  readbinaryplist.c
│  │  │  ├──  readbinaryplist.h
│  │  │  └──  README_MAC.txt
│  │  └──  windows
│  │     ├──  debugging_dlls.txt
│  │     ├──  pmwin.c
│  │     ├──  pmwinmm.c
│  │     ├──  pmwinmm.h
│  │     └──  README_WIN.txt
│  └──  porttime
│     ├──  porttime.c
│     ├──  ptlinux.c
│     ├──  ptmacosx_cf.c
│     ├──  ptmacosx_mach.c
│     └──  ptwinmm.c
└──  test
   ├──  CMakeLists.txt
   ├──  fast.c
   ├──  fastrcv.c
   ├──  latency.c
   ├──  midiclock.c
   ├──  midithread.c
   ├──  midithru.c
   ├──  mm.c
   ├──  multivirtual.c
   ├──  qtest.c
   ├──  README.txt
   ├──  recvvirtual.c
   ├──  sendvirtual.c
   ├──  sysex.c
   ├──  testio.c
   ├──  txdata.syx
   └──  virttest.c
Be-ing commented 2 years ago

GitHub Actions results won't show up on pull requests until you merge this with the workflow file. Until then, you can see results on my fork: https://github.com/Be-ing/portmidi-2/actions

rbdannenberg commented 2 years ago

This is quite a change. If you'd like to discuss goals and design first and implement later, I'd be in favor.

First, I've been keeping the "pm*" naming and general organization to be consistent with old releases (and with PortAudio, which used "pa*"), but even PortAudio has given up on that, and I agree it's cumbersome.

Second, I see DEBUGMESSAGES added to CMakeLists -- probably a good idea, but it's inconsistent -- DEBUG is used for mac, MMDEBUG is used for Win, and it's not clear any of these are actually useful. I'd say keep the debugging message code, but enable it with specific defines like MMDEBUG, CM_DEBUG, etc., and don't even offer them as CMake options -- just edit the code if you want to turn them on.

This brings up PM_CHECK_ERRORS -- this option checks return values and exits if there's an error, allowing simple command line programs to simply write, say, Pm_OpenOutput() without any error checking, yet no risk of proceeding or mysteriously crashing in the event that the call fails. This should be a CMake option -- not for a general library release, but handy for writing simple programs or for student projects.

Third, the big issue, is default MIDI devices. This is based on PortAudio, but nowadays, OS's do not have default MIDI devices. This is the reason for PmDefaults -- it's a way to set default devices through a simple GUI. My guess is few if any use this, but it makes no sense to relegate it to "bindings/java", not include it as a build option (which further discourages its use) and still have mac/readbinaryplist.c, mac/finddefault.c, linux/finddefault.h, linux/finddefault.c, and a page or so of code to search patterns in the Registry in pmwin.c. These are all parsing/reading standard Java preferences and currently, the only way to make use of Pm_GetDefaultInputDeviceID()/Pm_GetDefaultOutputDeviceID().

I'm prepared to change all this to something that still makes sense, but I'm not sure what that is.

Comments?

Be-ing commented 2 years ago

Second, I see DEBUGMESSAGES added to CMakeLists -- probably a good idea, but it's inconsistent -- DEBUG is used for mac, MMDEBUG is used for Win, and it's not clear any of these are actually useful. I'd say keep the debugging message code, but enable it with specific defines like MMDEBUG, CM_DEBUG, etc., and don't even offer them as CMake options -- just edit the code if you want to turn them on.

This doesn't make any sense to me. Why would you want different options for different OSes? And why should developers have to hack the build system to use these instead of just supporting them as an option in the upstream build system?

This brings up PM_CHECK_ERRORS -- this option checks return values and exits if there's an error, allowing simple command line programs to simply write, say, Pm_OpenOutput() without any error checking, yet no risk of proceeding or mysteriously crashing in the event that the call fails. This should be a CMake option -- not for a general library release, but handy for writing simple programs or for student projects.

It is a CMake option already: https://github.com/PortMidi/portmidi/blob/f70cc0c1d11d907bce369509993ee740dd703077/CMakeLists.txt#L42

Third, the big issue, is default MIDI devices... it makes no sense to relegate it to "bindings/java", not include it as a build option (which further discourages its use) and still have mac/readbinaryplist.c, mac/finddefault.c, linux/finddefault.h, linux/finddefault.c, and a page or so of code to search patterns in the Registry in pmwin.c.

I agree, but as far as I can tell there isn't any C code in bindings/java besides the little bit needed for JNI. Writing new C code is beyond the scope of this PR.

Another would be to be more old-school and put preferences as text in ~/.portmidi (or the Windows equivalent). But would anyone use it?

I think that's really unlikely... I expect applications using PortMidi to be maintaining their own preferences. As an application developer, I wouldn't expect or want a library to manipulate state on the filesystem.

rbdannenberg commented 2 years ago

Be wrote "Why would you want different options for different OSes? And why should developers have to hack the build system to use these instead of just supporting them as an option in the upstream build system?" -- I think these are mostly useful for debugging PortMidi, and turning on every debugging message with one global switch is never something I would do. If I'm looking at the code, working with the code, editing the code, isn't it easier to selectively enable print statements in the code instead of changing CMake options? I wouldn't hack the build system at all -- I just change the defines (temporarily) in the code itself.

"as far as I can tell there isn't any C code in bindings/java besides the little bit needed for JNI" -- no, look for BUILD_PMDEFAULTS there.

"I think that's really unlikely [preferences as text in ~/.portmidi]... I expect applications using PortMidi to be maintaining their own preferences. -- That's certainly one use case. Another is making it simple to write command line programs with minimal interface requirements. Adding code to select a device or store preferences is awkward compared to using default devices (if only defaults were easy to set).

"As an application developer, I wouldn't expect or want a library to manipulate state on the filesystem." -- PortMidi does not write preferences, but it reads them currently.

Be-ing commented 2 years ago

turning on every debugging message with one global switch is never something I would do. If I'm looking at the code, working with the code, editing the code, isn't it easier to selectively enable print statements in the code instead of changing CMake options?

Ideally that would be selectable at runtime by setting the logging level or logging categories rather than selecting it at build time. Unfortunately doing simple stuff like this in a cross platform way familiar to developers is not so trivial in the C/C++ world... which is one reason I'm putting my energy into Rust now, but I digress.

Looking at the existing code, there seems to be one global option per OS already:

portmidi on  build_rewrite_new [$] via △ v3.22.0 took 3s 
❯ rg DEBUG
CMakeLists.txt
37:option(DEBUGMESSAGES "Print debugging messages (not implemented on Linux)" OFF)
38:if(DEBUGMESSAGES)
39:  target_compile_definitions(PortMidi PRIVATE DEBUG)

include/portmidi.h
65: *    DEBUG assumes stdio and a console. Use this if you want
68: *        console, DEBUG probably should be undefined.

README.txt
68:DEBUGGING
83:The Windows version (and perhaps others) also offers a DEBUG

src/portmidi/windows/README_WIN.txt
27:out the compile-time definition for DEBUG. (But leave _DEBUG, which I

src/portmidi/windows/pmwin.c
20:#ifdef DEBUG
27:   If DEBUG is on, we prompt for input to avoid losing error messages.
46:#ifdef DEBUG

src/portmidi/common/CMakeLists.txt
13:#  if(NOT DEFAULT_DEBUG_FLAGS)
14:#    set(DEFAULT_DEBUG_FLAGS ${CMAKE_C_FLAGS_DEBUG} CACHE 
18:#  else(NOT DEFAULT_DEBUG_FLAGS)
19:#    message(STATUS "DEFAULT_DEBUG_FLAGS not nil: " ${DEFAULT_DEBUG_FLAGS})
20:#  endif(NOT DEFAULT_DEBUG_FLAGS)

src/portmidi/common/pmutil.c
15:// #define QUEUE_DEBUG 1
16:#ifdef QUEUE_DEBUG

src/portmidi/windows/pmwinmm.c
22:#ifdef MMDEBUG
329:#ifdef MMDEBUG
953:#ifdef DEBUG_PRINT_BEFORE_SENDING_SYSEX
954:        /* DEBUG CODE: */
1146:#ifdef MMDEBUG
1156:#ifdef MMDEBUG
1175:#ifdef MMDEBUG

bindings/java/CMakeLists.txt
11:  # SOME DEBUGGING INFORMATION IS COMMENTED OUT HERE...

src/portmidi/mac/pmmacosxcm.c
71:#define CM_DEBUG if (0)
72:/* #define CM_DEBUG if (1) */
305:#ifdef DEBUG
343:#ifdef DEBUG
386:    CM_DEBUG printf("read_callback: numPackets %d: ", newPackets->numPackets);
411:    CM_DEBUG printf("read_callback packet @ %lld status %x length %d\n",
416:        CM_DEBUG printf("    packet->timeStamp %lld nanos %lld\n",
429:        CM_DEBUG printf("        len %d stat %x\n", packet->length, status);
472:                CM_DEBUG printf("got close request packet\n");
633:        CM_DEBUG printf("midi_out_open isIACdevice %d\n", info->isIACdevice);
757:    CM_DEBUG printf("add %d to packet %p len %d timestamp %lld @ %lld\n",
994:            CM_DEBUG printf("driver %s\n", s);

look for BUILD_PMDEFAULTS there.

That looks like it's a build option for the Java bindings which turns on building some Java code:

portmidi on  build_rewrite_new [$] via △ v3.22.0 took 29m52s 
❯ rg BUILD_PMDEFAULTS
bindings/java/README.txt
37:     BUILD_PMDEFAULTS

bindings/java/CMakeLists.txt
3:if(BUILD_PMDEFAULTS)
107:endif(BUILD_PMDEFAULTS)
rbdannenberg commented 2 years ago

Another issue: long ago, porttime was a separate library, but it's rarely used independently and it's very small, so it was merged into the portmidi library. If we're reorganizing all the files, and adding an extra "src" level, I'd much prefer to merge src/portmidi and src/porttime, putting porttime.c in src/common, ptlinux.c in src/linux/, etc.

Be-ing commented 2 years ago

I think it is better to keep the portmidi and porttime directories separate so long as they are built as separate libraries. If you want to build them all together as one library, then I agree that merging the subdirectories of src would be a good idea.

rbdannenberg commented 2 years ago

I think it is better to keep the portmidi and porttime directories separate so long as they are built as separate libraries. If you want to build them all together as one library, then I agree that merging the subdirectories of src would be a good idea.

Yes, portmidi and porttime are built as one library, so I think we agree.

rbdannenberg commented 2 years ago

there seems to be one global option per OS already

Well, more or less. So my proposal is take debugging switches out of CMakeLists, where they are not helpful (also not documented, not maintained, not expected to produce consistent behavior).

it's a build option for the Java bindings which turns on building some Java code:

"Java bindings" is your term. I would say that it's a build option to create PmDefaults, the PortMidi application for setting default devices, and as a side-effect, a Java API for portmidi is created.

Be-ing commented 2 years ago

portmidi and porttime are built as one library

Oh, I didn't realize the old build system did that. I just updated this branch to do so and merged src/portmidi and src/porttime into src. Now the src directory has this structure:

├──  src
│  ├──  common
│  │  ├──  CMakeLists.txt
│  │  ├──  pminternal.h
│  │  ├──  pmutil.c
│  │  ├──  portmidi.c
│  │  └──  porttime.c
│  ├──  linux
│  │  ├──  finddefault.c
│  │  ├──  finddefault.h
│  │  ├──  pmlinux.c
│  │  ├──  pmlinux.h
│  │  ├──  pmlinuxalsa.c
│  │  ├──  pmlinuxalsa.h
│  │  ├──  ptlinux.c
│  │  └──  README_LINUX.txt
│  ├──  mac
│  │  ├──  finddefault.c
│  │  ├──  Makefile.osx
│  │  ├──  pmmac.c
│  │  ├──  pmmac.h
│  │  ├──  pmmacosxcm.c
│  │  ├──  pmmacosxcm.h
│  │  ├──  ptmacosx_cf.c
│  │  ├──  ptmacosx_mach.c
│  │  ├──  readbinaryplist.c
│  │  ├──  readbinaryplist.h
│  │  └──  README_MAC.txt
│  └──  windows
│     ├──  debugging_dlls.txt
│     ├──  pmwin.c
│     ├──  pmwinmm.c
│     ├──  pmwinmm.h
│     ├──  ptwinmm.c
│     └──  README_WIN.txt
Be-ing commented 2 years ago

My opinion is that code which cannot be built by the build system should be removed because it is likely to break over time. If you insist, I can remove the DEBUGMESSAGES options.

rbdannenberg commented 2 years ago

OK, I insist, but I'll explain more... I don't think anyone is particularly testing PortMidi, so just because you can enable something from the build system doesn't mean it gets tested; the more options there are, the less complete testing will be; I don't mind if debugging code is not well tested -- it's only there for reference and because every once in awhile it's useful, e.g., recently adding virtual ports and strange things were happening. And if it's broken, well, it only impacts someone working inside the code and already fixing bugs, so they should be prepared for it.

I'm more interested in your thoughts on preferences. For me, it's much easier to just leave things in -- whether or not people use Pm_Get*Default() functions, they're not that big, even including a little Java code to make them usable (but defaulting to not building any Java code for people that don't have Java installed). On the other hand, if unifying these repos means taking them out, I think the best alternative would be to provide sample code in C to read preferences from $HOME/.portmidi, making small changes to PmDefaults to write them, and moving all the Java stuff to another repo.

rbdannenberg commented 2 years ago

A new question. With all these changes, would it make sense to reset the version to, say, 2.0? I used SourceForge SVN version numbers because it was simple and obvious, but it's pretty strange out of that context. On the other hand, going backward from 200+ to 2.0 might break something.

Be-ing commented 2 years ago

I have already had to re-do this work 3 times. Now, as we were actively discussing this branch, you pushed commits to the master branch creating merge conflicts without discussing this in advance. This is awfully rude and frankly one of the most frustrating experiences I've had attempting to contribute to an open source project. If you do this again, I'm just going to give up and close this pull request.

Be-ing commented 2 years ago

OK, I insist

Okay, I removed the DEBUGMESSAGES option.

Be-ing commented 2 years ago

I'm more interested in your thoughts on preferences.

My thoughts are that it is generally inappropriate for a library to have its own preferences and this is no exception. For command line applications using PortMidi, I suggest automatically using whatever MIDI device PortMidi finds if there is only 1. If there are multiple MIDI devices, prompt the user to pick one interactively and/or provide a command line option to do so.

Be-ing commented 2 years ago

If you do this again, I'm just going to give up and close this pull request.

You did this again. I'm not spending anymore time on this. Bye.

rbdannenberg commented 2 years ago

I had no idea this was a problem or considered rude. I thought git was good at managing and merging changes, but I really have little experience with the whole pull request thing on github. No offense was intended, and I'm perfectly happy to merge things even manually. I guess I'll plan to do that to a large extent. I'm still interested in coming up with a single library we can both live with, so let me know if you think that's possible.

Be-ing commented 2 years ago

Git is good at merging changes but it isn't magic. Git can't decide for you what changes are desired when two commits separately modify the same lines in different ways. Please read the chapter 3 of the Git book on branching or I don't know if other people are going to have the patience to contribute to your repositories.

You're welcome to merge this code on your own if you want, but I'm done continually solving merge conflicts after I discover you made conflicting changes that weren't discussed in advance. I've gone out of my way multiple times now to work with you but you have made it difficult every step of the way.

andy5995 commented 2 years ago

I had no idea this was a problem or considered rude. I thought git was good at managing and merging changes, but I really have little experience with the whole pull request thing on github. No offense was intended, and I'm perfectly happy to merge things even manually. I guess I'll plan to do that to a large extent. I'm still interested in coming up with a single library we can both live with, so let me know if you think that's possible.

@rbdannenberg if I may offer a piece of friendly advice, with such a big change in this PR, it's typically best for two people to work from the same branch. When so many files are being affected, the possibilities of merge conflicts are pretty high.

But essentially, only one person should really work on a branch like this at a time. For example, if I'm collaborating with someone on a big change, I'll make sure he's not making any changes at the same time, and that he's pushed all his local changes, and won't be working on anything if I want to work on it for a few hours or a day or two. After that, he or she can then pull my changes.

For maintainers, another good option in some cases is to use the insert suggestion feature on PRs.

Basically what happens if a maintainers alters files on master while someone is working on a PR, the submitter will have to resolve merge conflicts if the maintainer changes code near or at where the submitter is changing code. And merge conflicts can be quite time-consuming to resolve.

Experience helps a lot though! :) I hope my tips were helpful...

Be-ing commented 2 years ago

But essentially, only one person should really work on a branch like this at a time.

Or you merge the branch promptly then make modifications later. Or add your own commits on top of my branch and make a pull request for my fork.