libcdio-paranoia 10.2+0.94+2 (new formula) #27347

marivera · 2018-05-02T02:26:12Z

libcdio-paranoia.rb

This commit add the formula to support libcdio-paranoia on homebrew. The source
page is https://www.gnu.org/software/libcdio/ for further information and
documentation on this package.

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

…isting libcdui.rb This commit add the formula to support libcdio-paranoia on homebrew. The source page is https://www.gnu.org/software/libcdio/ for further information and documentation on this package.

Changes to fix problems and coply with the standards required by homebrew.

fxcoudert

Could you explain what is the difference between this and our existing cdparanoia and libcdio formulas?

fxcoudert · 2018-05-02T15:07:25Z

Formula/libcdio-paranoia.rb

+  mirror "https://ftpmirror.gnu.org/libcdio/libcdio-paranoia-10.2+0.94+2.tar.gz"
+  sha256 "d60f82ece97eeb92407a9ee03f3499c8983206672c28ae5e4e22179063c81941"
+
+  depends_on "libcdio" => :build


=> :build should be removed

This is my first time to submit a formula. I will change this.

fxcoudert · 2018-05-02T15:07:44Z

Formula/libcdio-paranoia.rb

@@ -0,0 +1,20 @@
+class LibcdioParanoia < Formula
+  desc "The cd-paranoia audio-cd ripper based on libcdio"


Audio CD ripper based on libcdio would be shorter

Changed the description.

fxcoudert · 2018-05-02T15:08:02Z

Formula/libcdio-paranoia.rb

+  sha256 "d60f82ece97eeb92407a9ee03f3499c8983206672c28ae5e4e22179063c81941"
+
+  depends_on "libcdio" => :build
+  depends_on "pkg-config" => :build


Move build dep (pkg-config) above runtime dep (libcdio)

I made this change per your request.

fxcoudert · 2018-05-02T15:08:11Z

Formula/libcdio-paranoia.rb

+  end
+
+  test do
+    assert_match /cdparanoia III release 10.2/, shell_output("#{bin}/cd-paranoia -V", 0)


We need a test that exercises the some of the functionality of the app.

Testing the functionality is going to be near impossible, but testing that the code executes is achievable.

fxcoudert · 2018-05-02T15:08:23Z

Formula/libcdio-paranoia.rb

+  depends_on "pkg-config" => :build
+
+  def install
+    system "./configure", "--without-versioned-libs",


Why --without-versioned-libs?

Also: does configure support --disable-dependency-tracking and/or --disable-silent-rules? If so, include them.

The linker in macOS do not support it. It won't build without it.

I added --disable-dependency-tracking and/or --disable-silent-rules per your suggestion. Thank you for the feedback as this is my first formula.

marivera

All changes were performed.

fxcoudert · 2018-05-02T16:46:24Z

Could you explain what is the difference between this and our existing cdparanoia and libcdio formulas?

fxcoudert · 2018-05-02T16:46:53Z

Formula/libcdio-paranoia.rb

+class LibcdioParanoia < Formula
+  desc "Audio CD ripper based on libcdio"
+  homepage "https://www.gnu.org/software/libcdio/"
+  url "https://ftp.gnu.org/gnu/libcdio/libcdio-paranoia-10.2+0.94+2.tar.gz"


What is the version number here? How does it work?

I am not the author of the program. I am only providing a formula for the package to build under home brew. I am not sure how their versioning work.

I did some sleuthing, and it looks like 10.2 refers to the CDDA Paranoia version, 0.94 refers to the libcdio version, and +2 is some kind of revision number.

fxcoudert · 2018-05-02T16:47:24Z

Formula/libcdio-paranoia.rb

+  end
+
+  test do
+    assert_match /cdparanoia/, shell_output("#{bin}/cd-paranoia -V 2>&1", 0).partition(" ").first


We are going to need to test some operation. If nothing is possible, at least try to do something and check that we get the correct error (exit code and error message).

This check is similar to the one from the cdparanoia formula. This library and binary are functionally the same as cdparanoia, but the main difference is that this is being actively maintain and it is supported in multiple platforms as it uses libcdio, which already exists in homebrew, to abstract all cd operations.

Barrier is higher for tests on new formulas, than existing tests (which we're trying to improve over time, but we have lots of formulas).

There is nothing noted in the documentation about this and there is not a lot of guidance on what are the requirements either.

Try to perform some operation, and check that it fails. See for example formulas like fabio, gist, unravel, etc.

marivera · 2018-05-02T18:25:49Z

Please let me know what are the next steps as this is a new process for me.

fxcoudert · 2018-05-02T21:56:55Z

We are going to need to test some operation. If nothing is possible, at least try to do something and check that we get the correct error (exit code and error message).

marivera · 2018-05-03T17:42:01Z

The changes you requested are done.

fxcoudert · 2018-05-03T23:15:18Z

Formula/libcdio-paranoia.rb

@@ -0,0 +1,26 @@
+require "English"


Remove this.

Could you please point me to an example where they are checking the return code without using $?. I tried and the audit failed and suggested that I use the English module.

@marivera Try taking a look at https://www.rubydoc.info/github/Homebrew/brew/master/Homebrew/Assertions
(though I am not sure how helpful it will prove...)

marivera · 2018-05-07T11:58:46Z

fxcoudert, I added checking for the exit code. Could you please review? Thanks!

marivera · 2018-05-09T22:04:31Z

Is anyone looking at this at all?

JCount

Currently the version on this is broken, and will probably need to be set explicitly. (The version currently being detected is 2, which is clearly incorrect.)

…omatically detect the version out of the tar file name.

marivera · 2018-05-11T12:56:14Z

All changes requested have been done.

marivera · 2018-05-12T17:22:18Z

I think the kind of punitive, arbitrary and undocumented requirements that fxcoudert imposes to be able to submit a formula, will discourage people like me to invest time into making homebrew better. Sorry about wasting your time. Good luck.

woodruffw · 2018-05-12T17:45:39Z

If it's the testing requirement you're talking about, it's documented here:

We want tests that don't require any user input and test the basic functionality of the application. For example foo build-foo input.foo is a good test and (despite their widespread use) foo --version and foo --help are bad tests. However, a bad test is better than no test at all.
See cmake for an example of a formula with a good test. The formula writes a basic CMakeLists.txt file into the test directory then calls CMake to generate Makefiles. This test checks that CMake doesn't e.g. segfault during basic operation. Another good example is tinyxml2, which writes a small C++ source file into the test directory, compiles and links it against the tinyxml2 library and finally checks that the resulting program runs successfully.

Our requirements aren't punitive or arbitrary, although they are sometimes less-than-perfectly documented (we all work on this in our spare time). Please don't make it personal by singling maintainers out.

I'd like to help you get this formula merged, if you're still interested. It looks pretty close to me -- all it really needs is a more substantive test. In this case, testing the output and failure of cd-paranoia -vQd /dev/null should do the trick.

marivera · 2018-05-21T21:49:44Z

What I was referring to is the fact that this formula does not differ too much from the cdparanoia formula which is already approved. I tried to do the same to make sure I come as close as possible to an already approved formula, but that is not good enough anymore. The requirement I am talking about is the fact that there is no guideline of what is good enough given the fact that many formulas are very minimalistic of their tests. I was not slinging the maintainer, I was bringing to his attention the lack of objectivity in the process. BTW, I am also contributing my time to support this as well. I am not affiliated with libcdio or GNU or homebrew.

chenrui333 · 2025-01-16T02:09:24Z

#204300

marivera and others added 3 commits May 1, 2018 21:18

Adding a new formula libcdio-paranoia.rb to complement the already ex…

ec6ce8a

…isting libcdui.rb This commit add the formula to support libcdio-paranoia on homebrew. The source page is https://www.gnu.org/software/libcdio/ for further information and documentation on this package.

Fix problems with the initial submission.

ead3b98

Changes to fix problems and coply with the standards required by homebrew.

Remove full stop from description.

32738ef

fxcoudert requested changes May 2, 2018

View reviewed changes

fxcoudert added the new formula PR adds a new formula to Homebrew/homebrew-core label May 2, 2018

fxcoudert changed the title ~~Adding a new formula libcdio-paranoia.rb to complement the already ex…~~ libcdio-paranoia 10.2+0.94+2 (new formula) May 2, 2018

marivera added 2 commits May 2, 2018 10:23

Fix test code.

db61940

Additional changes done per fxcoudert suggestions.

625ad8a

marivera commented May 2, 2018

View reviewed changes

fxcoudert reviewed May 2, 2018

View reviewed changes

marivera added 3 commits May 3, 2018 10:54

Add test to check the return code after the binary execution.

7b3266d

Add the english module to use $CHILD_STATUS instead of $?.

9c44f0d

Fix return code result.

19aa136

fxcoudert reviewed May 3, 2018

View reviewed changes

Fix code to check for exit code.

e76f096

JCount reviewed May 9, 2018

View reviewed changes

marivera added 2 commits May 10, 2018 07:05

Add hardcoded version in the formula as the algorithm used cannot aut…

e79ab58

…omatically detect the version out of the tar file name.

Fix version directive ordering.

75918b0

marivera closed this May 12, 2018

marivera deleted the libcdio-paranoia branch May 12, 2018 17:19

lock bot added the outdated PR was locked due to age label Jun 20, 2018

lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018

		@@ -0,0 +1,20 @@
		class LibcdioParanoia < Formula
		desc "The cd-paranoia audio-cd ripper based on libcdio"

libcdio-paranoia 10.2+0.94+2 (new formula) #27347

libcdio-paranoia 10.2+0.94+2 (new formula) #27347

Conversation

marivera commented May 2, 2018 • edited Loading

fxcoudert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marivera left a comment

Choose a reason for hiding this comment

fxcoudert commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marivera May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marivera commented May 2, 2018

fxcoudert commented May 2, 2018

marivera commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JCount May 4, 2018 • edited Loading

Choose a reason for hiding this comment

marivera commented May 7, 2018

marivera commented May 9, 2018

JCount left a comment

Choose a reason for hiding this comment

marivera commented May 11, 2018

marivera commented May 12, 2018

woodruffw commented May 12, 2018

marivera commented May 21, 2018

chenrui333 commented Jan 16, 2025

marivera commented May 2, 2018 •

edited

Loading

marivera May 2, 2018 •

edited

Loading

JCount May 4, 2018 •

edited

Loading