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

defaulting to standard plackup if none is found in PATH #6

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
1 participant
@garu
Collaborator

garu commented Nov 28, 2015

Hi!

First of all, thank you so much for all the work you put onto this module! I was looking at a way to help and noticed some failing tests, and it looks like most of them are related to plackup not being found by File::Which.

This may be related to the fact that File::Which looks for it under the PATH environment variable, and I think in some cases (such as while smoke testing or an environment where installed perl binaries are simply not directly put in the PATH) it just doesn't find it, even with Plack installed.

A simple (ish) solution is to, instead of actually running the external 'plackup' command, we replace it by its internals, turning the command into:

use Plack::Runner; my $r = Plack::Runner->new; $r->parse_options(@ARGV); $r->run;

The problem with this approach is Ubic::Daemon's start_daemon expecting a binary, not a function (and as I understand you don't want to support function running anymore).

So we could fiddle with the UBIC_SERVICE_PLACKUP_BIN environment variable and have it turn the $plackup_command variable into something like [ 'perl', '-e', 'use Plack::Runner....'] and put the rest of the args from sub bin inside. But since that implied completely changing UBIC_SERVICE_PLACKUP_BIN I figured I should wait for your "ok" before sending any patches :)

What this PR does instead is simply ignore the cases where File::Which fails to find plackup and fallback to the default 'plackup' command. Please note that tests will still fail because this doesn't really fix anything, it just makes the error message clearer.

Even if you like this approach, please don't merge this just yet. There is another PR coming up with a different approach to the same issue, which I think you'll like more (and will most definitely be more useful).

Cheers!

@garu garu referenced this pull request Nov 28, 2015

Open

bailout on missing 'plackup' #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment