search

rbarrois / mpdlcd

A small tool to display MPD status on a lcdproc server
MIT License
31 stars 10 forks source link

Two crashes #1

Closed ldolse closed 12 years ago

ldolse commented 12 years ago

Hi,

Just gave mpdlcd a shot, and immediately ran into two crashes:

I've resolved both of these in a branch I created: https://github.com/ldolse/mpdlcd/commits/master

As both fixes are quick hacks in my branch I figured it might be better just to open an issue than a pull request, but feel free to use them.

rbarrois commented 12 years ago

Hmm, good catch.

I've added fixes for both (829b30ee959fc0ffe7c789f68e255271ad29c826 and 0eb46e762404960e1b24f266a4ec2a6903d63e98).

Could you explain more in depth how you got the first crash ("Not 'all' the args at the cli"), so that I can reproduce them? The best would be to have the exact command line that crashed the server.

ldolse commented 12 years ago

Great, thanks! I'll be away from my home system for a bit, so won't be able to beat on these changes til next weekend.

Regarding the CLI commands, I was just typing 'mpdlcd' at the CLI and it crashed. I tried adding a few args and it still crashed. Looking at the code it seemed to be looping through this dict/list of all the possible args: ('lcdproc', 'mpd', 'lcdproc_screen', 'lcdd_debug', 'pattern', 'patterns', 'retry_attempts', 'retry_backoff', 'retry_wait')

So I was assuming I had to set all of them manually in order to avoid the Nonetype error (I didn't actually attempt it). I did see that there is a config file as well in the archive, but not sure if it was installed - that might have something to do with the crash. I installed by entering 'python setup.py install' from the archive directory, I didn't do anything else.

rbarrois commented 12 years ago

Thanks for the details!

I had indeed not tested that behavior, which is fixed in commit 2183dc2c0279300ae92469c003ca8ffbb0b18f93. The configuration file is not installed to /etc/ — it looks like Python does not provide helpers for proper configuration file installation.

You may still copy the provided configuration file to /etc/mpdlcd.conf, it provides some saner defaults for running mpdlcd as a system daemon (see also the initd/mpdlcd file).

What OS are you using? I plan to provide packages for most used ones (I currently only have a Gentoo ebuild)

ldolse commented 12 years ago

Cool, I suspected it was the .conf file, but I didn't see that in the readme and hadn't read through the source to see where it was expected. The warning makes that more clear. I've seen other python mpd clients just create the conf file with sane defaults when it didn't exist, something to think about.

I'm using Puppy Linux - I'm actually maintaining a livecd variant focused on mpd here, and also showcasing all my favorite embedded clients. I'm thinking to bundle mpdlcd into the next release, as it's a lot more capable than the client I'm using right now, and you're actually maintaining it, which is great :-). I don't think making a pet package is worth the effort - there isn't a well maintained centralized repository for that in the Puppy world. I did see your init script and borrowed some parts of that - I have a single init script both LCDd and it's client so I can toggle both daemons on and off with a remote control. For some reason I couldn't get your stop function to work, might be some difference in my start-stop-daemon vs. Gentoo.

rbarrois commented 12 years ago

Glad to see the soft is used by others :)

Regarding the init.d script, the behavior is:

But your version of start-stop-daemon might not have the --make-pidfile option…

I'm going to close this issue since reported problems have been fixed; please open new issues if you find other bugs or have ideas for new features!