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

Implement Carton::Setup (was: PERL_CARTON_EXEC) #82

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement Carton::Setup (was: PERL_CARTON_EXEC) #82

wants to merge 2 commits into from

Conversation

lestrrat
Copy link

@lestrrat lestrrat commented May 9, 2013

Scenario: You have a legacy app that is now carton-ized. You have a bunch of scripts that don't use carton, and you can't expect people in your teamto explicitly call "carton exec -- script.pl". So you want to re-execute these scripts via carton and set the right paths when somebody casually does "./bin/foo.pl".

This change allows sets $ENV{PERL_CARTON_EXEC} variable to true, so called scripts can detect if it has been dispatched via carton or not.

@miyagawa
Copy link
Contributor

miyagawa commented May 9, 2013

I do understand the motivation/use case but am not sure if that's the best way to handle this. Re-running carton exec inside your app can lead it to the infinite loop easily by a simple misconfiguration.

I think what you want to do can be done in two separate steps, one is to ensure that the script must be loaded from carton exec, like bundler's bundler.setup, which would raise exceptions if it is not.

The other is to provide an easy way to generate wrapper/shim scripts (like binstubs) so that the user of your code can run as a standalone script without prefixing them carton exec.

@lestrrat
Copy link
Author

lestrrat commented May 9, 2013

@miyagawa: gotcha. so I've now removed my previous commit, and replaced it with a new module, Carton::Setup. This is a hack atm, but can you check and let me know if I'm in the same wavelength as you?

https://github.com/lestrrat/carton/commit/b855ef191fd32d00c5b95fab5e95408f1c96e84a

@miyagawa
Copy link
Contributor

miyagawa commented May 9, 2013

Yeah, looks sane, unless @masaki disagrees.

@masaki
Copy link
Contributor

masaki commented May 9, 2013

I agree to Carton::Setup. I think it's a good change, will be easier to implement --binstubs.

@miyagawa
Copy link
Contributor

miyagawa commented May 9, 2013

I haven't looked at the details, and I'm not asking you @lestrrat to update the code (yet :D), but what happens if you run the script with carton exec ./foo.pl and then inside foo.pl you have use Carton::Setup? I think it should be a null operation (since it is already set up), but am not 100% sure if that's the correct behavior.

@lestrrat
Copy link
Author

lestrrat commented May 9, 2013

@miyagawa currently it does the setup twice. only PATH is really affected as the use of lib::core::only resets @inc, but it's I guess it's redundant. Now, to detect that we've already been setup.... I guess we're back to setting some sort of ENV var?

@miyagawa
Copy link
Contributor

miyagawa commented May 9, 2013

Yeah i guess you can de-duplicates the INC path, and setting that up twice is acceptable.

I plan to eliminate lib::core::only for the runtime, at least not by setting them to PERL5OPT since that will affect the subprocesses that are not meant to be run with carton.

* Changed functions to be called via $class->setup() et al, so juuuuuust in
  case somebody was crazy enough to want to change the semantics of
  Carton::Setup, they can.
* Added docs
@lestrrat
Copy link
Author

@miyagawa @masaki : I didn't know how exactly the plan to eliminate lib::core::only was going to be carried, so I punted it for now, and added more thorough docs. As noted in my previous message, only PATH gets a bit icky in case you do 'carton exec' (but shouldn't really affect anything), so I left it as is too.

If you have any other concerns, please let me know and I'll get on top of them. Otherwise, I'll wait for review/merge.

miyagawa added a commit that referenced this pull request Jun 2, 2013
…PERL5LIB. Re: #82 #70 #60

PERL5LIB isn't perfect since it doesn't eliminate site_perl from @inc
in the child process, which means carton exec could accidentally load
modules from site_perl even when they are not available in the
carton.lock.

We'll address this by either implementing a check-like logic in the
exec (which would add an overhead), or add some additional @inc hooks
in the development, which can detect such errors, a la App::FatPacker
@marcusramberg
Copy link

👍 on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants