Perl5-Alien / Alien-Base

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

add alien_env property #152

Closed plicease closed 8 years ago

plicease commented 8 years ago

Add a property for use in overriding environment variables used by alien_do_system. For the rationale, see

https://github.com/salva/p5-Alien-OpenSSL/issues/1

and cpantesters failure:

http://cpantesters.org/cpan/report/42a41d60-6fdd-1014-8c40-57d8de5a8c74

plicease commented 8 years ago

/cc: @salva

plicease commented 8 years ago

reminder to @plicease, need to update the dzil plugin when this is accepted.

zmughal commented 8 years ago

Code is fine. Just pointed out a few typos. Once those are fixed, I'm all for merging.

Merge: Aye

plicease commented 8 years ago

@zmughal++ for pointing out the typos.

salva commented 8 years ago

How about adding also some specific option to transform environment variables containing paths from Windows style to UNIX (i.e. s|//|/|g)? It would be much easier for authors to use if that is the issue.

I have faced issues with Windows style paths confusing MSYS tools in other cases, though IIRC, they didn't involve environment variables. So, I don't know how common this specific problem may be.

See the following patches:

https://github.com/salva/p5-Alien-Base/commit/ddc65f69867b2711fcca4da57c1812cc158216c7 https://github.com/salva/p5-Alien-OpenSSL/commit/a9939fad02bc1f8e639f5b64c7ad60a6000c08f4

jberger commented 8 years ago

I think you are going to have some precedence concerns. Some users will want their environment to win out while dist authors might want to assert that their choices are correct. I wonder if you could make the configuration indicate that?

alien_env => {
  SOME_VAR => '?value_unless_set',
  ANOTHER_VAR => 'value_that_warn_and_overrides',
  YET_ANOTHER => '!value_that_dies_if_conflicting',
}

or some other setup. Perhaps the indicator goes on the key rather than the value? Or maybe you think this is YAGNI?

plicease commented 8 years ago

@jberger I envision this being used sparingly, and I lean toward YAGNI. If we did go down this route then we'd also need to allow for AB devs specifying a literal '?' or '!' at the start of their environment variable.

plicease commented 8 years ago

@jberger Although there is a precedent for this, we set CONFIG_SITE only if the user hasn't set it him/herself.

jberger commented 8 years ago

If it is intended to be used sparingly I would strongly urge it to be documented as such.

plicease commented 8 years ago

I can also document alternative:

alien_helper => { foo => '$ENV{FOO}//"my value"' }, alien_env => { FOO => '%{foo}' },

as it will be useful in some circumstances.

zmughal commented 8 years ago

The additional documentation looks good to me.

zmughal commented 8 years ago

++ for the change regarding empty environment variables.