Build $_ENV from CLI scripts
Needs RevisionPublic
Actions

Authored by epriestley on Jul 15 2014, 9:06 PM.

Details

Reviewers

joshuaspence
btrahan

Maniphest Tasks

T5554: arc fails with svn/git/hg non english locales

Summary

Ref T5554. Currently, when a system is in a non-English language, some things sometimes do not work because we parse output for English error messages but the messages are actually in French (or whatever).

To remedy this, D9922 attempts to pass LC_ALL or LANG or some mixture of things explicitly (I haven't figured out exactly what we have to pass, yet). However, a side effect of doing this is that we wipe out the rest of the environment, because $_ENV is not (always? ever?) populated from the CLI.

Since $_ENV is empty, we end up generating a subprocess with only the overridden variables when building the subprocess enviroment with $overrides + $_ENV. For example, some of my recent commits have a bogus author string because HOME (I think?) gets wiped, so git doesn't know where to look for ~/.gitconfig.

Instead, make sure $_ENV is always populated. The only way I could see to do this is capture phpinfo(), which is very gross, but should work OK.

Test Plan

Added a var_dump($_ENV) and saw it populated where previously it was not populated.

Diff Detail

Repository

rPHU libphutil

Branch

buildenv

Lint

Lint Errors

Severity	Location	Code	Message
Error	scripts/__init_script__.php:106	XHP45	PHP Compatibility

Unit

No Test Coverage

Build Status

Buildable 1701
Build 1702: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 23843.Jul 15 2014, 9:06 PM

epriestley retitled this revision from to Build $_ENV from CLI scripts.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: joshuaspence, btrahan.

epriestley added a task: T5554: arc fails with svn/git/hg non english locales.

Herald added a subscriber: epriestley. · View Herald TranscriptJul 15 2014, 9:06 PM

Harbormaster completed remote builds in B1701: Diff 23843.Jul 15 2014, 9:07 PM

btrahan accepted this revision.Jul 17 2014, 10:26 PM

btrahan edited edge metadata.

btrahan added inline comments.

scripts/__init_script__.php
75	so no environment variable can be false eh? (Seems like the right code looking at getenv; just checking. :D )

This revision is now accepted and ready to land.Jul 17 2014, 10:26 PM

epriestley added inline comments.Jul 17 2014, 10:26 PM

scripts/__init_script__.php
75	Yeah -- as far as I know, they always have to be strings.

I'm not convinced that this is a good idea. FWIW, I was looking at the PHP document on $_ENV and found this comment.

I haven't got a better idea at the moment, but wanted to raise my concerns before this gets landed.

This revision now requires changes to proceed.Jul 20 2014, 9:50 PM

I'm not sure that this would work in all cases, but (on my machine) $_ENV is empty whereas $_SERVER contains all of my environment variables.

Another possibility here is to whitelist a list of environment variables that we do care about, and retrieve them via getenv():

<?php

$env = $_ENV;

if (!$env) {
  // PHP `variables_order` does not contain "E"
  $env_vars = array(
    'HOME',
    ...
  );

  foreach ($env_vars as $env_var) {
    $env[$env_var] = getenv($env_var);
  }
}

For getenv() to work, php.ini variables_order must contain 'E'.

This doesn't hold on my system:

$ php -r 'echo ini_get("variables_order")."\n"; echo getenv("HOME")."\n";'
GPCS
/Users/epriestley

I think the author may be confused between $_ENV (which will not be populated if "E" is missing) and getenv() (which, AFAIK, does not care).

In D9938#15, @epriestley wrote:
For getenv() to work, php.ini variables_order must contain 'E'.

This doesn't hold on my system:
$ php -r 'echo ini_get("variables_order")."\n"; echo getenv("HOME")."\n";'
GPCS
/Users/epriestley
I think the author may be confused between $_ENV (which will not be populated if "E" is missing) and getenv() (which, AFAIK, does not care).