Bug 22 - Expansion of $REDIR quoted on MacOSX breaks cp
Summary: Expansion of $REDIR quoted on MacOSX breaks cp
Status: RESOLVED FIXED
Alias: None
Product: abcde
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Macintosh All
: Normal normal
Assignee: Andrew Strong
URL: http://git.einval.com/cgi-bin/gitweb....
Depends on:
Blocks:
 
Reported: 2015-11-22 21:43 GMT by Von Welch
Modified: 2015-12-15 23:36 GMT (History)
1 user (show)

See Also:


Attachments
Debug output from ./abcde -d /dev/disk2 -n -D >& /tmp/abcde-debug.txt (20.38 KB, text/plain)
2015-11-22 21:43 GMT, Von Welch
Details
Fix for redir issue on MacOS (3.39 KB, patch)
2015-12-12 09:09 GMT, Andrew Strong
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Von Welch 2015-11-22 21:43:01 GMT
Created attachment 8 [details]
Debug output from ./abcde -d /dev/disk2 -n -D >& /tmp/abcde-debug.txt

On MacOSX 10.11.1 with abcde 2.7.1 and bash 3.2.57, the expansion of $REDIR is quoted, which causes bash to pass it to cp as a argument instead of treating it as a redirection, which causes cp to throw an error.

Snippet of debug output with full log attached:

+ '[' -e '/Volumes/Audio CD/1 Audio Track.aiff' ']'
+ nice -n 10 cp -f '/Volumes/Audio CD/1 Audio Track.aiff' /Volumes/music/wav/abcde.ffffffffbeeef50c/track01.wav '>&2'
usage: cp [-R [-H | -L | -P]] [-fi | -n] [-apvX] source_file target_file
       cp [-R [-H | -L | -P]] [-fi | -n] [-apvX] source_file ... target_directory
+ RETURN=64
Comment 1 Andrew Strong 2015-12-11 23:21:48 GMT
I confess that it was me that re-broke this for MacOS :(. There was an old attempt back in 2009 to correct this issue which had the unwanted side effect of breaking usepipes for everybody. This was the original fix:

http://git.einval.com/cgi-bin/gitweb.cgi?p=abcde.git;a=commit;h=900a8efed

That I reversed to re-enable usepipes. I see that I left myself a note about the broken MacOS issue:

http://git.einval.com/cgi-bin/gitweb.cgi?p=abcde.git;a=commit;h=d823c1de

which I did not return to, my apologies!

So it remains broken in current git and it remains something I would dearly like to see a proper resolution for. Time is a big issue for me now and for the next few months so I would welcome any patches / ideas on this issue.
Comment 2 Andrew Strong 2015-12-12 06:08:38 GMT
Hmmmm.... looks like some variation of the y n syntax should work, I will investigate. Pity I don't have a mac to experiment on :(.
Comment 3 Andrew Strong 2015-12-12 09:09:05 GMT
Created attachment 12 [details]
Fix for redir issue on MacOS

I had trouble wrapping my head around the 'redir' syntax so eventually I remade it: it is a little clunkier but I can understand it better and hopefully others will as well. Patch is attached against current git, hopefully this is not too difficult to get under MacOS?

I have tested with dagrab (yes it still works!), cdda2wav and cdparanoia and all work as expected. If cdda2wav works I presume icedax does as well, I don't run debian...

Can you test on MacOS with whatever ripper you use? I would be especialy keen to hear if cddafs works as this MacOS only. If all works well I would be more than happy to produce a working patch against the release version 2.7.1 if this would make your life easier :)
Comment 4 Von Welch 2015-12-12 21:40:15 GMT
Andrew - Thanks for the patch. I grabbed git head and applied your patch without a problem. The first time I ran it, it worked great - ripped a whole CD and created MP3s. Ever since then however, I get the following error about cddb-choice. I've tried same CD and other CDs with no change. I can't figure out how it could be related, but has me baffled.

% ./abcde
Volume Audio CD on disk2 unmounted
Volume Audio CD on disk2 mounted
Grabbing entire CD - tracks: 01 02 03 04 05 06 07 08 09 10 11 12
abcde: internal error: cddb-choice not recorded.
Status: 1
% ./abcde  # to get temp dir
Volume Audio CD on disk2 unmounted
Volume Audio CD on disk2 mounted
Grabbing entire CD - tracks: 01 02 03 04 05 06 07 08 09 10 11 12
abcde: attempting to resume from /Volumes/music/wav/abcde.ffffffffbeeef50c..
abcde: internal error: cddb-choice not recorded.
Status: 1
% cat /Volumes/music/wav/abcde.ffffffffbeeef50c/status
abcde-version=2.7.2-UNRELEASED
cddbmethod=cddb
cddb-statcomplete
cddb-querycomplete
cddb-readcomplete
%
Comment 5 Andrew Strong 2015-12-12 22:46:07 GMT
I am hoping that the issue with cddb-choice is unrelated to the changes in the patch, and indeed I suspect this is the case. Could you post your abcde.conf file here?

Also make sure that your abcde temp directory has been deleted, you have a 'resume' in there. What abcde is saying is that you have not recorded a choice from the cddb options presented to you, so meta data will not be added and abcde has exit 1.

Good to hear that your initial run was trouble free, hopefully we can get to the bottom of this other odd issue and then move on :)
Comment 6 Von Welch 2015-12-13 16:32:33 GMT
I've confirmed I see the cddb-choice issue with both git head and with your patch, so it seems unrelated to your patch besides the fact it's letting me fully test it out.

I see the cddb-choice issue with a minimal ~/.abcde.conf with just "OUTPUTTYPE=mp3" and without doing a resume (i.e. after doing a 'rm -rf of the abcde.<id> directory). I'll keep debugging, but as far a I can tell from 'abcde -D' cddb-choice is never written to status before abcde expects to find it. Looks like there are lot of places in the script it could be written, so I'm not sure what logic path I'm following that leads me to this problematic state.
Comment 7 Von Welch 2015-12-13 17:30:33 GMT
I figured out that things work fine if I specify '-n' or "NOCDDBQUERY=y" (i.e. set CDDBAVAIL=n). My guess is something with regards to my environment for CDDB is messed up and abcde isn't catching until sanity check at the end of do_cddbedit().

The good news is that I can now confirm your patch for the original REDIR issue seems to be working well and consistently.
Comment 8 Von Welch 2015-12-13 17:57:30 GMT
FYI, for anyone landing here searching for the cddb-choice issue, I've opened bug 26 regarding that.
Comment 9 Andrew Strong 2015-12-13 19:25:04 GMT
Good news that the original redir issue seems adequately addressed. I will tidy things up a little more, test a little more, write the changelog etc and hopefully push this weekend. Woulod be sooner but work, family etc etc...

And thanks for the bug report as well as testing the fix!
Comment 10 Andrew Strong 2015-12-15 23:36:49 GMT
I have pushed the changes, thanks Von Welch for the bug report and the testing. It is good to see that abcde continues to have an active use on MacOSX!