Page MenuHomePhabricator

Force php5 in mwscript
AbandonedPublic

Authored by thcipriani on Apr 9 2018, 3:13 PM.

Details

Reviewers
demon
mmodell
hashar
bd808
Group Reviewers
Release-Engineering-Team
Patch without arc
git checkout -b D1025 && curl -L https://phabricator.wikimedia.org/D1025?download=true | git apply
Summary

Since we are now not forcing mwscript to php5 per
Ic3e0c7ee79a52cc33b31f091c6961f9bac23d9bf we'll need to set an
environment variable so that scap uses php5 instead of hhvm. Set this to
a configuration value so that it can be overridden on individual
machines for future testing.

Diff Detail

Event Timeline

I disagree with making it a config variable. We should support the env variable, yes, but otherwise we should expect the system we're on to do the right thing. Configuring this feels like it's just gonna bite us later.

In D1025#20520, @demon wrote:

I disagree with making it a config variable. We should support the env variable, yes, but otherwise we should expect the system we're on to do the right thing. Configuring this feels like it's just gonna bite us later.

Good point. Too many knobs, etc.

In D1025#20520, @demon wrote:

I disagree with making it a config variable. We should support the env variable, yes, but otherwise we should expect the system we're on to do the right thing. Configuring this feels like it's just gonna bite us later.

I think having the ability to change the default via a config variable is useful in certain situations (for instance, when migrating scap to a new PHP version while we don't want to migrate the rest of the system scripts).

What I'd like to see, though, is a saner logic, that is:

  • Use "php" as the default. This will make the system pick the php interpreter and should generally be the right thing to do (unless you're in the middle of a migration, like right now)
  • If the env variable is set, it should override the config value
  • We can then set the interpreter in the scap config via puppet, if we want to override it for now. We could even create a second alternative called php-scap if we prefer to abstract things away from the configuration.

See my comments - I like the general idea but I think the behaviour of the code should be changed.

scap/cli.py
201

I would do
if not os.environ.get('PHP'):

php_version = self.config.get('php_version')
if php_version is not None:
  os.environ['PHP'] = php_version
scap/config.py
74

default to 'php'.