Closed Bug 1146892 Opened 9 years ago Closed 9 years ago

Investigate fixing mozharness' gittool/hgtool to work without setting env

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mshal, Assigned: mshal)

References

Details

Attachments

(1 file, 1 obsolete file)

In bug 1132123 I had to jump through some hoops with the environment settings in order to get both hg and git checkouts working on Windows. They should probably work out of the box using the default environment. I think at least the following things are issues:

1) git.exe isn't in the Windows PATH
2) SYSTEMROOT needs to be in the environment for 'import random' to work (affects both hg and git IIRC)
3) If PROPERTIES_FILE is in the environment, it can try to use twig settings for non-mc checkouts, like this git checkout of gaia:

18:08:28     INFO -  2015-03-20 18:08:28,010 git init -q /builds/slave/map-linux32_g-0000000000000000/build/src/gaia
18:08:28     INFO -  2015-03-20 18:08:28,033 command: START
18:08:28     INFO -  2015-03-20 18:08:28,033 command: git fetch -q https://git.mozilla.org/releases/gaia.git +refs/heads/projects/maple:refs/remotes/origin/projects/maple
18:08:28     INFO -  2015-03-20 18:08:28,033 command: cwd: /builds/hg-shared/git.mozilla.org/releases%2Fgaia.git
18:08:28     INFO -  2015-03-20 18:08:28,033 command: output:
18:08:28     INFO -  fatal: Couldn't find remote ref refs/heads/projects/maple

It tries to add +refs/heads/projects/maple, which doesn't exist in gaia.git, so the command fails. The PROPERTIES_FILE logic should probably be smarter here.
Blocks: 1146894
git/hgtool assume that the stuff in $PROPERTIES_FILE is for the "primary" checkout, e.g. gecko in most cases. So yeah, there will be problems if this environment var is set for checkouts of other repos like gaia.
Attached patch bug1146892-git-hg (obsolete) — Splinter Review
This should fix both git and hg to work without setting the environment beforehand. If any of the required env settings are missing, it'll add them into the env before calling the tools scripts.

This doesn't fix the PROPERTIES_FILE issue (I'm not quite sure how it should be fixed), but since this fix means we don't have to pass in the build env anyway, I don't think it's a priority.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=531250a4e6c1

Note the failure summary in the b2g desktop windows builds appear to be bug 1119905, not related to this change.
Attachment #8590254 - Flags: review?(jlund)
Comment on attachment 8590254 [details] [diff] [review]
bug1146892-git-hg

Review of attachment 8590254 [details] [diff] [review]:
-----------------------------------------------------------------

looks good :)

it's too bad we are adding more env config items in the hg and gitool scripts rather than having things configured properly but as mentioned below, I don't think it's worth untangling the bag of snakes.

::: mozharness/base/vcs/gittool.py
@@ +61,5 @@
>          branch = c.get('branch')
>          clean = c.get('clean')
>          share_base = c.get('vcs_share_base', os.environ.get("GIT_SHARE_BASE_DIR", None))
>          env = {'PATH': os.environ.get('PATH')}
>          env.update(c.get('env', {}))

it be nice to set this correctly in every config like you are for configs/b2g/desktop_windows32.py. However I don't think it's worth blocking and testing everywhere.

@@ +62,5 @@
>          clean = c.get('clean')
>          share_base = c.get('vcs_share_base', os.environ.get("GIT_SHARE_BASE_DIR", None))
>          env = {'PATH': os.environ.get('PATH')}
>          env.update(c.get('env', {}))
> +        if sys.platform == 'win32':

can we use self._is_windows() ?

::: mozharness/base/vcs/hgtool.py
@@ +74,5 @@
>          if c.get('env'):
>              env.update(c['env'])
>          if share_base is not None:
>              env['HG_SHARE_BASE_DIR'] = share_base
> +        if sys.platform == 'win32':

_is_windows() here too
Attachment #8590254 - Flags: review?(jlund) → review+
(In reply to Jordan Lund (:jlund) from comment #3)
> @@ +62,5 @@
> >          clean = c.get('clean')
> >          share_base = c.get('vcs_share_base', os.environ.get("GIT_SHARE_BASE_DIR", None))
> >          env = {'PATH': os.environ.get('PATH')}
> >          env.update(c.get('env', {}))
> > +        if sys.platform == 'win32':
> 
> can we use self._is_windows() ?

Oh, I didn't know about that - it seems to work fine. Plus it gets rid of the 'import sys' addition.
Updated with _is_windows(); r+ carried forward.
Attachment #8590254 - Attachment is obsolete: true
Attachment #8591066 - Flags: review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: