Perl5-Alien / Alien-Base

Base classes for Alien:: modules (deprecated, see Alien-Build)
Other
35 stars 19 forks source link

interpolation of arbitrary perl at command time #129

Closed plicease closed 9 years ago

plicease commented 9 years ago

As discussed on IRC this allows the interpolation of arbitrary Perl at build and staged install time. It would simplify a number of the solutions in the FAQ branch, which I intend on updating if this is accepted. This handles nested { { } }. I've never used Text::Balanced before so if let me know if I'm doing something terribly wrongly.

shadowcat-mst commented 9 years ago

If you're wanting to be able to run arbitrary perl immediately before the command gets run, wouldn't it be better to allow a build command of a subref and just run that? (optionally, the subref could then return a build command if you're trying to build one dynamically).

Seems strange to use string eval when the configuration is already perl code ...

plicease commented 9 years ago

I keep wanting to do a subref, but unfortunately this is defined in Build.PL and stored by Module::Build which doesn't allow such things.

shadowcat-mst commented 9 years ago

B::Deparse! (that's a joke please don't actually implement that the spiders the spiders the spiders are coming for me)

shadowcat-mst commented 9 years ago

More seriously, this still seems a bit odd - are you trying to have the perl code itself have side effects? Or are you trying to generate a build command a more complex way? If the latter, why does that need to happen later on?

Assuming you just want to run the code, I wonder whether helper scripts would be a nicer approach? Or for small stuff, some way to say "this entire build command is actually perl code, just run it".

Basically, I can see the point of the feature but without use cases it's really hard to tell whether the API makes sense at all - so those need to come first, then we can figure out where to patch.

jberger commented 9 years ago

are you trying to generate a build command a more complex way? If the latter, why does that need to happen later on?

This is a good point, the Build.PL file could just run the commands and then the value be added to the alien_build_commands then.

# Build.PL

...

my $patch = get_patch_from_somewhere;

...

alien_build_commands => [ "$patch blah", ... ]

...
zmughal commented 9 years ago

The reason we were looking into this is so that certain kinds of expressions can be written inline to the build commands. It started with this call to patch.

On Windows systems, patch requires the --binary flag in order to handle newlines properly (it crashes otherwise), however we can't use that flag everywhere since *BSD systems don't using GNU's patch.

So something like: patch %{ $^O eq 'MSWin32' ? '--binary' : '' } ... might be helpful for dealing with these small expressions. It's not a huge issue, but it makes certain things easier. Right now helper scripts is how I accomplish this.

plicease commented 9 years ago

Also by delaying the logic to build / staged install time you can avoid dependencies that are only needed if you are doing a build from source code. Right now you have to make Alien::gmake or Alien::patch a configure_requires dependency if you are doing the logic in Build.PL.

shadowcat-mst commented 9 years ago

Um. @zmughal

[ 'patch', ($^O eq 'Win32' ? '--binary' : ()) ]

would already work, no?

plicease commented 9 years ago

I would like to make:

# Build.PL
 use Alien::Base::ModuleBuild;
 use Alien::gmake;
 my $gmake = Alien::gmake->exe;
 Alien::Base::ModuleBuild->new(
   ...
   configure_requires => {
     'Alien::gmake' => 0,
   },
   alien_bin_requires => [ 'Alien::gmake' ],
   alien_build_commands => [
     "$gmake",
   ],
   alien_install_commands => [
     "$gmake install",
   ],
   ...
 )->create_build_script;

Into this:

# Build.PL
 use Alien::Base::ModuleBuild;
 Alien::Base::ModuleBuild->new(
   ...
   alien_bin_requires => {
     'Alien::gmake' => 0,
   },
   alien_build_commands => [
     "%{ Alien::gmake->exe }",
   ],
   alien_install_commands => [
     "%{ Alien::gmake->exe } install",
   ],
   ...
 )->create_build_script;

Now Alien::gmake doesn't get installed unless it is actually needed. Alien::gmake is maybe not especially onerous, but I can see some dependencies are going to be.

zmughal commented 9 years ago

@shadowcat-mst, yeah it would. (I can't do that directly from Dist::Zilla though :-P Not a big problem.)

I think what we really want by a patch like this is a way to standardise the lessons learned from building different Alien modules. Being able to encapuslate those lessons in a function call allows for future extensibility and reduces cognitive load for other users and that's why we're playing with the idea of extended interpolation.

Something like %{helper_name} which always calls Alien::Base::Helper->alien_helper_name might be a worthwhile compromise. And perhaps another option that loads tools like Alien::gmake when doing the interpolation [Edit: bin_requires already does that and just using that makes sense].

shadowcat-mst commented 9 years ago

@plicease what if you said "scalar references are evaluated as perl code", so

alien_build_commands => [ \'Alien::gmake->exe' ],

and then the return value of that is interpolated normally? Seems like it'd provide basically the same functionality, deal with Module::Build being brain damaged, and avoid clever parsing.

I think either that or specific helpers would end up being a nicer approach in the long run - we don't want to accidentally a templating engine in the interpolate code and the current PR seems to be kinda going that direction.

plicease commented 9 years ago

@shadowcat-mst That might work for gmake

  alien_build_commands => [ [ \'Alien::gmake->exe', 'all' ] ],  # list ref if you need arguments

but for patch you need (at the moment) single arg version of system:

 alien_build_commands => [ '%{ Alien::patch->exe } -p1 < mypatch.diff' ]
plicease commented 9 years ago

I can rewrite this with the helper approach, I do feel this is a more general solution, that does not require adding a helper to the AB core each time we need something new.

shadowcat-mst commented 9 years ago

Um. I don't think anybody was assuming the helpers would be hard-coded - I was expecting you'd register them at Build.PL time, something like

alien_interpolate => { patch => 'Alien::Patch->exe' }
shadowcat-mst commented 9 years ago

The point of the exercise is to keep the interpolation bit and the running perl bit seperate - and I don't see why you can't support a single scalarref too.

But I think registering helpers is probably fine - I just fear that at some point somebody will try and give the helpers arguments and then that leads down a path that finds yourself wishing you'd done something sane and easy instead like, say, parsing HTML with regular expressions ...

plicease commented 9 years ago

closing in favor of #130