Regenerate bindings and update core-foundation-sys

[jasongrlicky]

Hi there! This PR:

• Regenerates the bindings with bindgen 0.53.2, which seems to have generated slightly different bindings, formatted the output, and added Clone derives for structs
• Regenerates the bindings based on the macOS 10.14 SDK
• Updates core-foundation-sys to 0.7 to take advantage of improvements since 2015

Something worth noting is that the new bindgen adds packed(4) to MIDIPacket, which will bump the minimum rust version to 1.33 at least. It seems like it would fix #6, though!

One thing that confused me is that bindgen did not add packed(4) to MidiPacketList, despite it being in the #pragma in MIDIServices.h. I am not sure why this is, and am honestly not familiar enough with the intricacies of type layout to trust that it's doing the right thing. I followed @Boddlnagg 's message in the existing bindings and added it manually, but I would appreciate another set of eyes to make sure that it's right. In fact, another set of eyes on the whole set of changes to the bindings would be really handy.

At least because of the core-foundation-sys version bump, I believe this would be a breaking change.

Thanks so much! 🙏

[Boddlnagg]

This looks good to me! It's a bit hard to see the actual changes given that the formatting changed a lot, but it seems like everything is correct.

The fact that bindgen did add repr(packed(4)) to MIDIPacket, but not to MIDIPacketList is also not entirely clear to me. It might be because the natural alignment of MIDIPacketList is already 4 when MIDIPacket has an alignment of 4, so it's not needed to specify it manually.

Did you run the tests? Because bindgen also autogenerates these layout tests, e.g. checking that both ::std::mem::align_of::<MIDIPacket>() and ::std::mem::align_of::<MIDIPacketList>() are actually 4. If these tests pass even without the manual specification of packed(4) for MIDIPacketList, then it means that you don't need to add the additional annotation.

[jonas-k]

Thank you for your PR.

Also looks mostly good to me. But we'd have to increase the major version number because of the minimum rust version required.

Why was it necessary to add the absolute paths to the generation command? The frameworks should be the same as the ones installed in the system (If you're using Mojave). Also within the framework there's usually a symlink to /version/A/. So you could leave that out. If you want a specific SDK version, you could point to the specific SDK in XCode.

The packed(4) doesn't seem to be necessary. Since there is only the numPackets field which has a size of 4 before the packet array which is aligned to 4 already, the compiler would have to manually add a padding between the fields to make MIDIPacketList not being aligned to 4.

[jasongrlicky]

Thank you so much for the thoughtful reviews!

Good point about the packed(4) on MIDIPacketList being unnecessary - I removed the commit that manually added that. After doing a bunch of reading on the way that Rust handles type layout this morning, that makes sense to me too finally! 😁

And I did run the tests, so we should be good to go there.

I also revised the commit that updated the README to not include /version/A/ in the path and bumped the version number as you suggested - good catch on these, @jonas-k .

As far as the changed path for the generation command in the README goes, at least on my machine (macOS 10.14.6 and Xcode 11.3.1), the path that was in the README (the /System/Library/Frameworks/... one) doesn't exist, since it seems that Apple may have recently switched to having frameworks without headers in /System/Library/Frameworks.

When I was looking around for what to feed to bindgen instead, I did find other copies of MIDIServices.h in addition to the one I picked:

• /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/CoreMIDI.framework/Headers/ - this seems to have been installed by the Xcode Command Line Tools
• Versions targeting iOS instead of the macOS SDK at /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/System/Library/Frameworks/CoreMIDI.framework/Headers

I just went with the one in Xcode.app because it seemed like the path of the latest macOS SDK, which is the meaning of the path in the instructions before, I believe. But please do let me know if I'm wrong, we definitely don't want to be generating bindings for the wrong thing! 😅 I also split this out as a separate commit so it would be easy to remove if you'd rather just keep the current instructions.

Thanks again!

[jonas-k]

Thank you again for your changes. I just double checked on Mojave and it really appears that Apple deemed the headers are not necessary for regular users, so we really need the full XCode path.

[jasongrlicky]

Hey @jonas-k - I was just looking over some of the changes in the regenerated bindings again this morning, and I have some concerns about implementing Copy and Clone on MIDIPacket and MIDIPacketList, since they both have flexible-array members. I need to look over this issue in more detail, but I get the feeling that the FAMs not being defined as zero-length may misled bindgen's logic in this case.

I'll be looking more into this today, but if it's the case that we should leave them out, it seems like the fix would be really straightforward, so there may be another pull request coming from me shortly.

[Boddlnagg]

@jasongrlicky I think you're right, copying a MIDIPacket or MIDIPacketList cannot work, because it's dynamically sized. Rust would generate wrong code for it. However, even a move would be incorrect, which is why coremidi-rs has this comment:

At runtime this type must only be used behind immutable references that point to valid instances of MIDIPacket (mutable references would allow mem::swap).
This type must NOT implement Copy!

We cannot prevent the type from being movable, but the best we can do is to prevent it from being copyable.

@jonas-k You should definitely wait for this to be resolved before you upload version 3.0.0 to crates.io!