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

whipper not picking up all settings in whipper.conf #99

Closed
mokkurkalve opened this issue Jan 2, 2017 · 24 comments · Fixed by #240
Closed

whipper not picking up all settings in whipper.conf #99

mokkurkalve opened this issue Jan 2, 2017 · 24 comments · Fixed by #240
Assignees
Labels
Bug Generic bug: can be used together with more specific labels
Milestone

Comments

@mokkurkalve
Copy link

This used to work a month or so ago.
I have my config in $HOME/.config/whipper/whipper.conf
which should be the same as $XDG_CONFIG_HOME/whipper/whipper.conf on my system anyway.
The config looks like:

[main]
path_filter_fat = True
path_filter_special = False

[drive:TSSTcorp%3ACDDVDW%20TS-U633J%20%3ATM00]
vendor = TSSTcorp
model = CDDVDW TS-U633J
release = TM00
read_offset = 6
defeats_cache = True

[rip.cd.rip]
unknown = True
output_directory = /home/eivind/rippers/whipper
track_template = %%r/%%A/%%d (%%y)/[%%t] %%n
disc_template = %%r/%%A/%%d (%%y)/%%A - %%d (%%y)
profile = flac

The settings under [main] and [drive:*] seems to be picked up as before but the settings under [rip.cd.rip] are not and those values fall back to default. This is happening with current git checkout as of today.

@ArchangeGabriel
Copy link

Most likely because the command is not rip.cd.rip anymore, but whipper.cd.rip (right?). Blind guess, but could you try replacing with that one (don’t have the time to look how the change was done right now).

@mokkurkalve
Copy link
Author

Thanks but no difference. It still won't pick up those settings.

@ArchangeGabriel
Copy link

OK, so this will have to wait for someone being able to dig in the code to check when those settings are read and if something changed when the rip command was renamed.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jan 2, 2017

Thanks but no difference. It still won't pick up those settings.

Didn't check throughly but: have you tried changing [rip.cd.rip] to [command.cd.rip]?

@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Jan 2, 2017
@JoeLametta JoeLametta added this to the 1.0 milestone Jan 2, 2017
@45054
Copy link

45054 commented Jan 3, 2017

You could also try to remove the spaces before and afther the "=" so it would look like:

output_directory=/home/eivind/rippers/whipper
track_template=%%r/%%A/%%d (%%y)/[%%t] %%n
disc_template=%%r/%%A/%%d (%%y)/%%A - %%d (%%y)

@mokkurkalve
Copy link
Author

None of the proposed changes made any difference.

BTW. The command run are simply
whipper cd rip
And my OS are Arch Linux x86_64 current (as of today).

@ghost
Copy link

ghost commented Jan 6, 2017

Having a similar problem basically running the example config...
[rip.cd.rip]
unknown = True
output_directory = ~/Music
track_template = new/%%A/%%y - %%d/%%t - %%n
disc_template = %(track_template)s
profile = flac

The config file is ~/.config/whipper/whipper.conf

@Freso
Copy link
Member

Freso commented Jan 7, 2017

rip.cd.rip definitely won't work anymore, so it doesn't make sense trying with that. Is there some documentation that says rip.cd.rip specifically?

@ghost
Copy link

ghost commented Jan 7, 2017

Yes. That's the way it's listed on the github homepage

https://github.com/JoeLametta/whipper#configuration-file-documentation

It's also not working with [command.cd.rip]

@peaveyman
Copy link

I can confirm this isn't working for me either. Here is my whipper.conf

[drive:TSSTcorp%3ACD/DVDW%20SH-S183L%3ASB01]
vendor = TSSTcorp
model = CD/DVDW SH-S183L
release = SB01
defeats_cache = True
read_offset = 6

[whipper.cd.rip]
track_template = %%A - %%d/%%d - %%t - %%n
disc_tramplate = %%A - %%d/%%A - %%d
output_directory = /home/gary/whipper
working_directory = /home/gary/whipper/

@ArchangeGabriel
Copy link

I’m trying to track this. If anyone wants to take a look, here is a good start: ace8d5a#diff-a1778966d092b4c9aaf13b4e50fca475

@JoeLametta
Copy link
Collaborator

I’m trying to track this. If anyone wants to take a look, here is a good start

I'm not sure but I think the regression was introduced with d1ed80d.

@istathar
Copy link

istathar commented Mar 8, 2017

Just hit this as well. Anything we can to do help you diagnose?

@oliverhenshaw
Copy link

oliverhenshaw commented Apr 11, 2017

d1ed80d seems to regress config file parsing for me too. I see that this is a squash merge and tried to bisect through the individual commits from PR #92 but most of the commits would not run for me[1]. Perhaps someone with more familiarity with the code will have better luck - it is an easy bug to reproduce, after all.

[1] e.g. the first commit, b9cf34b "introduce logcommand.Lager, Whipper(); use argparse for whipper image commands, stub logging", installed locally via:

$ python2 ./setup.py install --home=~/tmp-scratch/tmp-rip/dist/

fails:

$ ./dist/bin/whipper cd rip
Traceback (most recent call last):
  File "./dist/bin/whipper", line 9, in <module>
    load_entry_point('whipper==0.4.0', 'console_scripts', 'whipper')()
  File "/home/oliver/tmp-scratch/tmp-rip/dist/lib/python/whipper-0.4.0-py2.7.egg/morituri/rip/main.py", line 27, in main
    ret = Whipper(sys.argv[1:], prog=os.path.basename(sys.argv[0]))
  File "/home/oliver/tmp-scratch/tmp-rip/dist/lib/python/whipper-0.4.0-py2.7.egg/morituri/rip/main.py", line 95, in __init__
    opt.remainder[1:], prog=prog + " " + opt.remainder[0]
  File "/home/oliver/tmp-scratch/tmp-rip/dist/lib/python/whipper-0.4.0-py2.7.egg/morituri/common/logcommand.py", line 50, in __init__
    command.Command.__init__(self, parentCommand, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'prog'

@MasterofJOKers
Copy link

As I see it, that functionality was removed with "d1ed80d#diff-a1778966d092b4c9aaf13b4e50fca475L39" and I don't see it being added again. Especially, I don't see a way of the subcommand knowing its parent for building the dotted section name in the current implementation.

@ghost
Copy link

ghost commented Jul 15, 2017

So are we supposed to use the command line arguments for now?

@xmixahlx
Copy link

xmixahlx commented Jul 31, 2017

I have a script i execute instead of whipper itself:

#!/bin/sh
# run whipper with config because config file in git is broken
whipper cd rip \
--prompt \
--offset 48 \
--unknown \
--cdr \
--output-directory=/mnt/Data/Media/Music/Ripped/ \
--disc-template="%A/%A - %d/%A - %d" \
--track-template="%A/%A - %d/%t - %a - %n";
exit

@suuuehgi
Copy link

But not every option is available using command line arguments. Take profile = flac.
No idea how to set it to wav.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Aug 29, 2017

But not every option is available using command line arguments. Take profile = flac.
No idea how to set it to wav.

That isn't supported anymore since whipper v0.5.0: now it's FLAC only (rationale in #121).
I'll document it here.

@MerlijnWajer MerlijnWajer self-assigned this Jan 26, 2018
JoeLametta added a commit that referenced this issue Feb 18, 2018
Removed reference to unused "profile = flac" config option (issue #99)
@calumchisholm
Copy link
Contributor

@MerlijnWajer - are you currently working on this, or would you mind if I picked it up?

I have some WIP code, but it would still need some more work before it's ready to merge. I don't want to spend time on it if you've already made good progress though.

calumchisholm added a commit to calumchisholm/whipper that referenced this issue Feb 27, 2018
@RecursiveForest
Copy link
Contributor

By all means keep plugging away @calumchisholm -- I also have some unfinished code that works towards fixing this, but that shouldn't be a barrier towards actually fixing it if you can.

@calumchisholm
Copy link
Contributor

I'll certainly keep looking at it when I get some time - it would be nice to get this functional again.

@calumchisholm
Copy link
Contributor

calumchisholm commented Feb 28, 2018

While playing around in this area, I thought it might be a good idea to review the command-line arguments and config settings currently supported by whipper (or intended to be supported).

Most of these are quite well understood, and included only for the sake of completeness, however there are a few items (listed in the last section) that I would appreciate some input on though. Either their intended use is unclear, or I've identified them as possible targets for tidy-up, consolidation or moving elsewhere in the config hierarchy.

Feedback welcomed.

CLI Only

Mainly single-use or debug options. Generally no value to writing them to whipper.conf

  • whipper --version
  • whipper --help
  • whipper.cd --toc-pickle
  • whipper.cd --release-id
  • whipper.image.retag --cuefile
  • whipper.image.retag --release-id
  • whipper.image.verify --cuefile
  • whipper.offset.find --offsets
  • whipper.debug.resultcache --logger
  • whipper.debug.checksum --files
  • whipper.debug.encode --input
  • whipper.debug.encode --output
  • whipper.debug.tag --file
  • whipper.debug.musicbrainzngs --mbdiscid

whipper.conf only

No (directly) corresponding command-line argument, but not an issue.

  • [drive:] read_offset
    • Overridden by whipper.cd.rip --offset
  • [drive:] defeats_cache
  • [main] path_filter_fat
  • [main] path_filter_special
  • [musicbrainz] server

CLI & whipper.conf

Set either from CLI or whipper.conf. Well understood.

  • whipper --eject
  • whipper.cd.rip --offset
    • Overrides [drive:] read_offset
  • whipper.cd.rip --output-directory
  • whipper.cd.rip --working-directory
  • whipper.cd.rip --track-template
  • whipper.cd.rip --disc-template
  • whipper.cd.rip --unknown
  • whipper.cd.rip --cdr

Investigate

Possible candidates for tidy-up, or maybe just a lack of understanding on my part.

  • whipper --record
    • Description reads "record API requests for playback", however it's really only the MusicBrainz release data which is saved. Presumably it's at the top-level because it can be used with either the cd or image commands, but it feels out of place (and a bit 'debuggy'). At the very least, the description text should better reflect its purpose.
  • whipper.cd --prompt
  • whipper.image.retag --prompt
    • Both of these commands support a --prompt argument. Is there value in keeping these separate, or could they be combined and promoted to the top-level?
  • whipper.cd --country
  • whipper.image.retag --country
    • Again - could these be combined?
  • whipper.cd.rip --logger
    • Shouldn't this be a top-level argument and made available for commands other than 'rip'?
  • whipper.cd.rip --force-overread
    • Should this be stored in the config file on a per-device setting (similar to --offset) or does it apply regardless of the drive used (as with --cdr)?

@RecursiveForest
Copy link
Contributor

RecursiveForest commented Feb 28, 2018

This PR fixes the config issue, so I wouldn't spend any more time on it: #240

I have a proposal from some months ago in IRC for reworking the command line interface that I quite liked. If I remember when I get home I'll share it. I appreciate the writeup here, in particular calling attention to the infelicities under "investigate". :)

I'd appreciate user testing for the above PR. Note that configuration sections now begin with whipper instead of rip to correspond to their command line invocation. e.g., whipper cd rip options go into section [whipper.cd.rip], whipper drive analyze into [whipper.drive.analyze], etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Generic bug: can be used together with more specific labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.