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

libcdio: fix crash reading CD TOC on macOS Ventura #22701

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

jmon12
Copy link
Contributor

@jmon12 jmon12 commented Feb 17, 2024

Remove the unnecessary byte added to the TOC buffer. See the upstream patch: https://git.savannah.gnu.org/cgit/libcdio.git/commit/?id=6f2426e8bf4dc5269ccbd9fbfa94340895f8be6e

Description

The test is rudimentary. I'm using the library only through freac which was crashing because of this bug, see for example this ticket. After the fix, it works.

This is my first contribution to macports, and I'd be happy to contribute more. Please feel free to add any "pedagogical comment"/recommendation.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 14.3.1 23D60 x86_64
Command Line Tools 15.1.0.0.1.1700200546

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

devel/libcdio/Portfile Outdated Show resolved Hide resolved
Copy link
Member

@pmetzger pmetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! A very small issue you might want to take care of.

devel/libcdio/Portfile Outdated Show resolved Hide resolved
@jmon12 jmon12 force-pushed the fix-libcdio branch 2 times, most recently from 46211e9 to 803f5de Compare February 18, 2024 20:30
# Fix the crash reading CD on macos Ventura
# Remove the unecessary addtional byte added to the TOC buffer
if {${os.platform} eq "darwin" && ${os.major} >= 22} {
patchfiles remove-additional-byte-TOC-buffer.diff
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, one more change needed and then I'll merge. You will need to use "patchfiles-append", otherwise you will remove the patchfile added a few lines above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh crap I'm sorry. I edit right away.

Copy link
Contributor

@chrstphrchvz chrstphrchvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change requires a rebuild, so the revision should be incremented, correct?

@reneeotten
Copy link
Contributor

This change requires a rebuild, so the revision should be incremented, correct?

Indeed!

@jmon12
Copy link
Contributor Author

jmon12 commented Feb 22, 2024

@chrstphrchvz Sorry for the beginner question, but what revision number should I increment? I don't see anything else than the libcdio version in the Portfile.

@chrstphrchvz
Copy link
Contributor

@chrstphrchvz Sorry for the beginner question, but what revision number should I increment? I don't see anything else than the libcdio version in the Portfile.

--- a/devel/libcdio/Portfile
+++ b/devel/libcdio/Portfile
@@ -4,6 +4,7 @@ PortSystem          1.0
 
 name                libcdio
 version             2.1.0
+revision            1
 
 categories          devel
 license             GPL-3+

There is no revision 0 already in this port because it is optional and the libcdio port has not been updated in a while, but going forward the guide now recommends that ports specify revision 0 rather than omit it: https://guide.macports.org/chunked/reference.html#reference.keywords

devel/libcdio/Portfile Outdated Show resolved Hide resolved
@reneeotten
Copy link
Contributor

@jmon12 please make the requested changes so that we can merge this PR

@jmon12
Copy link
Contributor Author

jmon12 commented Feb 27, 2024

OK, I just added the multiple changes requests and tested with port test libcdio:

  • Increment the revision of the Portfile
  • Remove the MacOS platform check
  • Use patchfiles-append instead of patchfiles

I hope everything is fine now. I'm sorry for the late answer/action.

@reneeotten reneeotten merged commit ff7dcd7 into macports:master Feb 28, 2024
4 checks passed
@jmon12 jmon12 deleted the fix-libcdio branch March 2, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants