search

konrad-kruczynski / elfsharp

Pure managed C# library for reading ELF, UImage, Mach-O binaries.
https://elfsharp.it
Other
150 stars 56 forks source link

Add support for Mach-O dylib load commands #73

Closed 0xced closed 3 years ago

0xced commented 3 years ago
konrad-kruczynski commented 3 years ago

Hi! Thanks for your PR. It looks good and I plan to merge it. A few things need to be corrected though:

  1. Can you provide test binaries as a separate pull request to this repository. This will be merged first.
  2. Please update README, adding yourself to the end of the contributors list.
  3. Please update ELFSharp.csproj, adding yourself to authors and bumping version to 2.12 as well as writing a short summary in the PackageReleaseNotes tag.
0xced commented 3 years ago

Thanks for your reply, Konrad.

  1. I have re-used an existing binary (3dengine_libretro_ios.dylib) which is enough for testing the dylib load commands, there is no need for an additional test binary file.
  2. Done in f8a5ecfff72f45b7e7831f9f6596328a8dc520b1
  3. I'd suggest to do this in a separate pull request and also maybe create a CHANGELOG file with release notes for all versions then use the url to this new CHANGELOG file in the PackageReleaseNotes. Some NuGet package do this (for example Moq) and from the perspective of a NuGet package consumer, this is really nice.
konrad-kruczynski commented 3 years ago
  1. Indeed, otherwise tests would not have passed on CI... 🤦
  2. Great.
  3. It can be a separate PR, would you like to issue it? I like the CHANGELOG idea, however I'd probably also put changes description for the given version in this tag directly (as well as the URL).
konrad-kruczynski commented 3 years ago

Anyway I'm merging it now.

konrad-kruczynski commented 3 years ago

@0xced Do you plan to issue such an additional PR in the nearest days? Just asking, if not, then I'll probably make one myself.

0xced commented 3 years ago

I have no plan to do so, please go ahead. 😉