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

thcipriani created this revision.Apr 9 2018, 3:13 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptApr 9 2018, 3:13 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
thcipriani requested review of this revision.Apr 9 2018, 3:14 PM
demon added a comment.Apr 9 2018, 6:56 PM

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.

thcipriani abandoned this revision.Apr 9 2018, 10:18 PM
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.

Joe added a comment.Apr 10 2018, 8:39 AM
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.
Joe added a comment.Apr 10 2018, 8:44 AM

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'.