Bug 277754
| Summary: | audio/sox: fixes to make options work properly | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Tatsuki Makino <tatsuki_makino> | ||||||
| Component: | Individual Port(s) | Assignee: | Zsolt Udvari <uzsolt> | ||||||
| Status: | In Progress --- | ||||||||
| Severity: | Affects Only Me | CC: | dnelson, dnelson_1901, uzsolt | ||||||
| Priority: | --- | Flags: | dnelson_1901:
maintainer-feedback+
|
||||||
| Version: | Latest | ||||||||
| Hardware: | Any | ||||||||
| OS: | Any | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 277803 | ||||||||
| Attachments: |
|
||||||||
These changes look good. Adding --disable-mp3 to CONFIGURE_ARGS and then adding --enable-mp3 later when needed to override it is interesting, but autoconf doesn't seem to complain, and I guess the alternative would be an .ifdef block. (In reply to Dan Nelson from comment #1) This method has a maximum sequence of the following arguments --disable-mp3 --with-lame --enable-mp3 --with-mad --enable-mp3 --with-twolame --enable-mp3 It then takes advantage of the fact that the last --{enable,disable}-mp3 is adopted from among them. If that method is not adopted, the following part would be needed .include <bsd.port.options.mk> .if ${PORT_OPTIONS:MLAME} || ${PORT_OPTIONS:MMAD} || ${PORT_OPTIONS:MTWOLAME} CONFIGURE_ARGS+= --enable-mp3 .else CONFIGURE_ARGS+= --disable-mp3 .endif These are better if the preferred method is adopted. Hi Tatsuki and Dan, I've a few suggestion: - FOO_CONFIGURE_ON=--enable-foo can replace to FOO_CONFIGURE_ENABLE=foo (see https://docs.freebsd.org/en/books/porters-handbook/book/#options-configure_enable or in your Makefile). In this case if FOO is off the port system add --disable-foo to CONFIGURE_ARGS. - The '--disable-mp3' is ambiguous (to a reader) in "general" CONFIGURE_ARGS - I'd suggest remove it (see next point) - MP3 group: if LAME, MAD and TWOLAME are off and --disable-lame, --disable-mad and --disable-twolame are in CONFIGURE_ARGS the --disable-mp3 is implied or not? (All mp3-format is disabled) If yes the '--disable-mp3' is unnecessary. If no you can test these options and if it needed can add '--disable-mp3'. Could you please test it? - LADSPA: why not LIB_DEPENDS (instead of BUILD and RUN_DEPENDS)? - maybe can remove the SYMLINK option because it doesn't require plus dependency. Could you please use 'portfmt Makefile'? This utility suggest many things about Makefile's formatting. The portfmt is part of ports-mgmt/portfmt port. Thanks for your work! (In reply to Zsolt Udvari from comment #3) Hello. I don't even remember why I did this :) I'll answer first, but it's up to Dan, the maintainer, to decide what to choose. > - FOO_CONFIGURE_ON=--enable-foo can replace to FOO_CONFIGURE_ENABLE=foo (see > - The '--disable-mp3' is ambiguous (to a reader) in "general" CONFIGURE_ARGS - I'd suggest remove it > - MP3 group: if LAME, MAD and TWOLAME are off and --disable-lame, --disable-mad and --disable- About these. create one option for --enable-mp3. create one option group for --with-{lame,mad,twolame}. members of that option group are imply to option for --enable-mp3. In this case, I think that we can select a combination from the options dialog where configure ends with an error. It is a combination of --enable-mp3 --without-lame --without-mad --without-twolame So, the patch method is hard to understand for those who read it, but I think it's kind to those who use it. Ambiguous --disable-mp3 is a bit tricky because if --disable-mp3 and --enable-mp3 exist at the same time, the latter will be used in preference. > - LADSPA: why not LIB_DEPENDS (instead of BUILD and RUN_DEPENDS)? It is how it was from the beginning. I'm kidding :) Perhaps libdepends can't detect it. It's in one of lib directory, and it doesn't have anything installed in libdata/ldconfig, and it's not named like libfoo.[0-9]. > - maybe can remove the SYMLINK option because it doesn't require plus dependency. The option is for conflict. But the conflicting audio/play no longer exists, does it? CONFLICTS=play can be removed, but I think the symlink option should be an option to avoid using filenames that tend to conflict with others. (In reply to Tatsuki Makino from comment #4) > It is a combination of --enable-mp3 --without-lame --without-mad --without-twolame Maybe I wasn't clear. Forget --enable-mp3 and --disable-mp3 in ports Makefile. My question is: if you've "--without-lame --without-mad --without-twolame" it causes "--disable-mp3" (without adding ./configure args)? And if you've at least one of --with-{lame,mad,twolame} it causes "--enable-mp3" (without adding ./configure args)? If yes don't need --{enable,disable}-mp3 hack in ports Makefile. > The option is for conflict. > But the conflicting audio/play no longer exists, does it? > CONFLICTS=play can be removed, but I think the symlink option should be an option to avoid using filenames that tend to conflict with others. Oh, okay. We can forget the CONFLICTS=play, indeed. I think you don't have to introduce SYMLINK option. Created attachment 254857 [details]
examples of why IMPLIES is not good
As we can see when we actually try like this, it simplifies both the Makefile and the configure options, but the default option for this is just the state where configure fails.
Also, if we do not explicitly specify --disable-* and --without-*, it may be automatically detected and used against the intention of the port.
I think SYMLINK should be an option for those who are already using sox with the play command, and for those who install all kinds of audio-related software to avoid the word play, which tends to conflict with others.
(In reply to Tatsuki Makino from comment #6) I run manually (--with-lame, without-{mad,twolame}, nothing --{enable,disable}-mp3): CFLAGS="-isystem /usr/local/include" LDFLAGS="-L/usr/local/lib/" ./configure --with-lame --without-mad --without-twolame --with-pkgconfigdir="/usr/local/libdata/pkgconfig" Summary (at end of ./configure's output): ... Optional libraries: id3tag yes lame yes libgsm yes libltdl yes libsndfile yes mad no magic yes opencore-amrnb no opencore-amrwb no png yes twolame no vo-amrwbenc no Optional formats: amrnb no amrwb no flac yes gsm yes lpc10 yes mp3 yes oggvorbis yes opus yes sndfile yes wavpack no Please note that mp3 is automatically "yes". Another run (without-{lame,mad,twolame}, nothing --{disable,enable}-mp3): CFLAGS="-isystem /usr/local/include" LDFLAGS="-L/usr/local/lib/" ./configure --without-lame --without-mad --without-twolame --with-pkgconfigdir="/usr/local/libdata/pkgconfig" Summary (at end of ./configure's output): Optional libraries: id3tag yes lame no libgsm yes libltdl yes libsndfile yes mad no magic yes opencore-amrnb no opencore-amrwb no png yes twolame no vo-amrwbenc no Optional formats: amrnb no amrwb no flac yes gsm yes lpc10 yes mp3 no oggvorbis yes opus yes sndfile yes wavpack no Please note that mp3 is "no"! Another run (without-{lame,mad,twolame}, --enable-mp3): CFLAGS="-isystem /usr/local/include" LDFLAGS="-L/usr/local/lib/" ./configure --without-lame --without-mad --without-twolame --with-pkgconfigdir="/usr/local/libdata/pkgconfig" --enable-mp3 ... configure: error: mp3 not available Disabled lame/mad/twolame but wanted mp3 - it's controversy. So you don't have to set --enable-mp3 or --disable-mp3 because it's set by ./configure automatically: if at least one of mad, lame, twolame is enabled, then --enable-mp3 is supposed by ./configure. If mad, lame and twolame are disabled then --disable-mp3 is supposed by ./configure (as you can see the latest sample ./configure). IMHO the --{enable,disable}-mp3 flags are unneeded it's automatically specified by ./configure. (In reply to Zsolt Udvari from comment #7) PS: lame, libmad installed. Friendly ping. |
Created attachment 249236 [details] proposed patch for audio/sox Changed AMR codecs to use opencore ones. I was going to fix just that, but the other options were not working correctly, so I fixed those as well. The following is a summary of the corrections. Apply patch to ${WRKSRC}/m4/sox.m4 to correct listing of unrecognized options. Removed options from CONFIGURE_ARGS that appeared to be unrecognized. Sort the order of occurrence of variables to the point where portclippy (ports-mgmt/portfmt) is quietest. LAME, MAD, and TWOLAME became MP3 groups. SYMLINK option was added for reducing the range of conflicts.