Inherit appropiate environment variables in goma module for goma_utils.py

Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.
* rename GetGomaTmpDirectory -> GetGomaLogDirectory in goma_utils.py

BUG=544330
Committed: https://chromium.googlesource.com/chromium/tools/build/+/6da5a843e44836e7b7b084a6eca6c65eb4b4f16b

[tikuta, 2016-08-12 02:25:24]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 02:25:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 02:25:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 02:25:30]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[tikuta, 2016-08-12 02:27:42]

Description was changed from

==========
Add GOMA_TMP_DIR to env for pointing out the location of *.INFO

* inherit some env vars to upload with log in goma_utils

BUG=544330
==========

to

==========
Add GOMA_TMP_DIR to env for letting goma_utils.py know the location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon()

BUG=544330
==========

[tikuta, 2016-08-12 02:27:42]

tikuta@chromium.org changed reviewers:
+ phajdan.jr@chromium.org, shinyak@chromium.org, ukai@chromium.org,
yyanagisawa@chromium.org

[tikuta, 2016-08-12 02:27:42]

tikuta@chromium.org changed required reviewers:
+ phajdan.jr@chromium.org

[ukai, 2016-08-12 04:22:30]

https://codereview.chromium.org/2245553002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:87: self._goma_ctl_env['GOMA_TMP_DIR']
= self.m.path.mkdtemp('goma_tmp_dir')
can we make sure all gomacc invocation will have the same environment?

if it wants to change logging directory only, might be better to use
GLOG_log_dir?

[tikuta, 2016-08-12 04:52:54]

Description was changed from

==========
Add GOMA_TMP_DIR to env for letting goma_utils.py know the location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon()

BUG=544330
==========

to

==========
Add GLOG_log_dir to env for letting goma_utils.py know the location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon()
* add GLOG_log_dir as the candidate of INFO location in
goma_utils.GetLatestGlogInfoFile

BUG=544330
==========

[tikuta, 2016-08-12 04:53:20]

Description was changed from

==========
Add GLOG_log_dir to env for letting goma_utils.py know the location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon()
* add GLOG_log_dir as the candidate of INFO location in
goma_utils.GetLatestGlogInfoFile

BUG=544330
==========

to

==========
Add GLOG_log_dir to env for letting goma_utils.py know the location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon()
* add GLOG_log_dir as the candidate of INFO location in
goma_utils.GetLatestGlogInfoFile

BUG=544330
==========

[tikuta, 2016-08-12 04:55:27]

Thank you for review.

https://codereview.chromium.org/2245553002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/goma/api.py:87: self._goma_ctl_env['GOMA_TMP_DIR']
= self.m.path.mkdtemp('goma_tmp_dir')
On 2016/08/12 04:22:30, ukai wrote:
> can we make sure all gomacc invocation will have the same environment?
> 
> if it wants to change logging directory only, might be better to use
> GLOG_log_dir?

I changed to use GLOG_log_dir instead of GOMA_TMP_DIR and modified goma_utils
for it.

[ukai, 2016-08-15 06:40:45]

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/goma_util...
File scripts/slave/goma_utils.py (right):

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/goma_util...
scripts/slave/goma_utils.py:46: candidates = ['GOMA_TMP_DIR', 'TEST_TMPDIR',
'TMPDIR', 'TMP']
GOMA_TMP_DIR, GLOG_log_dir, ..
if it matches with client/mypath.cc GetTempDirectory?

maybe, client/goma_ctl.py has the same issue.

or client/mypath.cc's GetTempDirectory should be fixed to ignore GLOG_log_dir?

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:87: self._goma_ctl_env['GLOG_log_dir']
= self.m.path.mkdtemp('goma_tmp_dir')
I thought GLOG_log_dir won't affect goma temp directory, but it would, i.e., if
we set GLOG_log_dir (and not GOMA_TMP_DIR or so), goma.ipc will be created in
GLOG_log_dir.
That is, we need to make sure GLOG_log_dir to be used for all gomacc invocation,
too.  Then, GOMA_TMP_DIR might be better than GLOG_log_dir.

[tikuta, 2016-08-15 08:32:25]

Description was changed from

==========
Add GLOG_log_dir to env for letting goma_utils.py know the location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon()
* add GLOG_log_dir as the candidate of INFO location in
goma_utils.GetLatestGlogInfoFile

BUG=544330
==========

to

==========
Inherit some env vars from os.environ for letting goma_utils.py know the
location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.

BUG=544330
==========

[tikuta, 2016-08-15 08:35:28]

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/goma_util...
File scripts/slave/goma_utils.py (right):

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/goma_util...
scripts/slave/goma_utils.py:46: candidates = ['GOMA_TMP_DIR', 'TEST_TMPDIR',
'TMPDIR', 'TMP']
On 2016/08/15 06:40:45, ukai wrote:
> GOMA_TMP_DIR, GLOG_log_dir, ..
> if it matches with client/mypath.cc GetTempDirectory?
> 
> maybe, client/goma_ctl.py has the same issue.
> 
> or client/mypath.cc's GetTempDirectory should be fixed to ignore GLOG_log_dir?

Removed GOMA_TMP_DIR that is not used for *.INFO files.

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:87: self._goma_ctl_env['GLOG_log_dir']
= self.m.path.mkdtemp('goma_tmp_dir')
On 2016/08/15 06:40:45, ukai wrote:
> I thought GLOG_log_dir won't affect goma temp directory, but it would, i.e.,
if
> we set GLOG_log_dir (and not GOMA_TMP_DIR or so), goma.ipc will be created in
> GLOG_log_dir.
> That is, we need to make sure GLOG_log_dir to be used for all gomacc
invocation,
> too.  Then, GOMA_TMP_DIR might be better than GLOG_log_dir.

In offline chat, we decided to inherit env vars from goma module users and set
assertion for GLOG_log_dir.

[Yoshisato Yanagisawa, 2016-08-16 03:54:23]

lgtm w/ comments.

https://codereview.chromium.org/2245553002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:27: 'BUILDBOT_CLOBBER',
I doubt you need to remember above four because these envs are automatically set
when recipe runs and might not be modified during recipe run, but maybe ok.

https://codereview.chromium.org/2245553002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:28: 'GOMA_TMP_DIR',
you might not need to remember GOMA_TMP_DIR because, if my memory is correct,
the reason you need to remember following args is for glog.  GOMA_TMP_DIR is not
used for the logging.

[tikuta, 2016-08-16 04:09:04]

https://codereview.chromium.org/2245553002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:27: 'BUILDBOT_CLOBBER',
On 2016/08/16 03:54:23, Yoshisato Yanagisawa wrote:
> I doubt you need to remember above four because these envs are automatically
set
> when recipe runs and might not be modified during recipe run, but maybe ok.

I set these variables because these variables will be used in
upload_goma_logs.py by setting self._goma_ctl_env as env.
Is it better to keep these variables to send as a part of log?

https://codereview.chromium.org/2245553002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/goma/api.py:28: 'GOMA_TMP_DIR',
On 2016/08/16 03:54:23, Yoshisato Yanagisawa wrote:
> you might not need to remember GOMA_TMP_DIR because, if my memory is correct,
> the reason you need to remember following args is for glog.  GOMA_TMP_DIR is
not
> used for the logging.

Done.

[shinyak, 2016-08-16 04:17:31]

lgtm

Could you elaborate the commit log so that why this change is necessary?

[tikuta, 2016-08-16 04:27:30]

Description was changed from

==========
Inherit some env vars from os.environ for letting goma_utils.py know the
location of *.INFO

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.

BUG=544330
==========

to

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.

BUG=544330
==========

[tikuta, 2016-08-16 04:27:59]

On 2016/08/16 04:17:31, shinyak wrote:
> lgtm
> 
> Could you elaborate the commit log so that why this change is necessary?

Done.

[tikuta, 2016-08-16 04:28:10]

Description was changed from

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.

BUG=544330
==========

to

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.

BUG=544330
==========

[tikuta, 2016-08-16 04:29:01]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 04:29:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 04:29:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 04:29:04]

Dry run: All required reviewers (with asterisk prefixes) have not yet approved
this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[ukai, 2016-08-16 05:39:47]

lgtm

https://codereview.chromium.org/2245553002/diff/60001/scripts/slave/goma_util...
File scripts/slave/goma_utils.py (right):

https://codereview.chromium.org/2245553002/diff/60001/scripts/slave/goma_util...
scripts/slave/goma_utils.py:44: def GetGomaTmpDirectory():
GetGomaLogDirectory()?

[tikuta, 2016-08-16 05:42:45]

Description was changed from

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.

BUG=544330
==========

to

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.
* rename GetGomaTmpDirectory -> GetGomaLogDirectory in goma_utils.py

BUG=544330
==========

[tikuta, 2016-08-16 05:46:01]

Now, I got lgtm from all goma developers.

But, nobody of goma member is in OWNER of files in this CL.
Paweł, could you review this CL?

https://codereview.chromium.org/2245553002/diff/60001/scripts/slave/goma_util...
File scripts/slave/goma_utils.py (right):

https://codereview.chromium.org/2245553002/diff/60001/scripts/slave/goma_util...
scripts/slave/goma_utils.py:44: def GetGomaTmpDirectory():
On 2016/08/16 05:39:47, ukai wrote:
> GetGomaLogDirectory()?

Done.

[ukai, 2016-08-16 07:21:56]

lgtm

https://codereview.chromium.org/2245553002/diff/80001/scripts/slave/goma_util...
File scripts/slave/goma_utils.py (right):

https://codereview.chromium.org/2245553002/diff/80001/scripts/slave/goma_util...
scripts/slave/goma_utils.py:45: """Get goma's temp directory."""
fix doc string as well

[tikuta, 2016-08-16 07:25:06]

https://codereview.chromium.org/2245553002/diff/80001/scripts/slave/goma_util...
File scripts/slave/goma_utils.py (right):

https://codereview.chromium.org/2245553002/diff/80001/scripts/slave/goma_util...
scripts/slave/goma_utils.py:45: """Get goma's temp directory."""
On 2016/08/16 07:21:56, ukai wrote:
> fix doc string as well

Done.

[ukai, 2016-08-17 00:58:35]

lgtm

[Paweł Hajdan Jr., 2016-08-17 06:31:06]

LGTM w/comments

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:19: 
nit: Remove redundant empty line (in a class the horizontal spacing is one
line).

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:20: def goma_ctl_env_init(self):
Just checking: did you intend to make this part of the public API? I'm still
thinking this should be internal, i.e. prefixed with an underscore.

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:23: # inherit some env vars used in
goma_utils.SendGomaTsMon
nit: Start with capital letter (i -> I), end the sentence with a dot.

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:24: for key in ['BUILDBOD_BUILDERNAME',
nit: typo BUILDBOD -> BUILDBOT

[tikuta, 2016-08-17 06:57:27]

Thank you for review!

I'll commit this after passing dry run.

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/goma/api.py (right):

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:19: 
On 2016/08/17 06:31:05, Paweł Hajdan Jr. wrote:
> nit: Remove redundant empty line (in a class the horizontal spacing is one
> line).

Done.

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:20: def goma_ctl_env_init(self):
On 2016/08/17 06:31:06, Paweł Hajdan Jr. wrote:
> Just checking: did you intend to make this part of the public API? I'm still
> thinking this should be internal, i.e. prefixed with an underscore.

Done.

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:23: # inherit some env vars used in
goma_utils.SendGomaTsMon
On 2016/08/17 06:31:05, Paweł Hajdan Jr. wrote:
> nit: Start with capital letter (i -> I), end the sentence with a dot.

Done.

https://codereview.chromium.org/2245553002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/goma/api.py:24: for key in ['BUILDBOD_BUILDERNAME',
On 2016/08/17 06:31:05, Paweł Hajdan Jr. wrote:
> nit: typo BUILDBOD -> BUILDBOT

Done.

[tikuta, 2016-08-17 06:57:34]

The CQ bit was checked by tikuta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 06:57:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 07:01:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 07:01:54]

Dry run: This issue passed the CQ dry run.

[tikuta, 2016-08-17 07:11:40]

The CQ bit was checked by tikuta@chromium.org

[tikuta, 2016-08-17 07:11:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
yyanagisawa@chromium.org, shinyak@chromium.org, phajdan.jr@chromium.org,
ukai@chromium.org
Link to the patchset: https://codereview.chromium.org/2245553002/#ps120001
(title: "fix typo and etc.")

[commit-bot: I haz the power, 2016-08-17 07:11:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 07:16:01]

Description was changed from

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.
* rename GetGomaTmpDirectory -> GetGomaLogDirectory in goma_utils.py

BUG=544330
==========

to

==========
Inherit appropiate environment variables in goma module for goma_utils.py

goma_utils.py finds a compiler_proxy.INFO from temporary dir set in env.
But, there is not consistency with glog temporary dir.
glog uses GLOG_log_dir if it is set in env.
By inheriting some env variables and forbidding GLOG_log_dir,
goma_util.py can find the INFO file from correct location.

* inherit some env vars to upload with log in goma_utils.SendGomaTsMon().
* if GLOG_log_dir is set, assertion failed.
* stop to use GOMA_TMP_DIR as candidate directory for glog *.INFO files.
* rename GetGomaTmpDirectory -> GetGomaLogDirectory in goma_utils.py

BUG=544330

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/6da5a843e44836e7b7b0...
==========

[commit-bot: I haz the power, 2016-08-17 07:16:03]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/tools/build/+/6da5a843e44836e7b7b0...

[Paweł Hajdan Jr., 2016-08-17 08:32:38]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2250973002/ by phajdan.jr@chromium.org.

The reason for reverting is: This interacts badly with the recipe autoroller,
see e.g. https://goto.google.com/tezblb ..