755835 - Specify full path to buildprops.json

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[John Hopkins (:jhopkins)]

buildprops.json cannot be found when doing Thunderbird repacks because the file is in the parent of the working directory and we don't pass in any path details.

Rather than add another 'if thunderbird' condition, I'd rather just pass in the full path to the buildprops.json.

Comment 1

[John Hopkins (:jhopkins)]

Attachment: buildbotcustom patch

Comment 2

[Rail Aliiev [:rail]]

Comment on attachment 624455 [details] [diff] [review]
buildbotcustom patch

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

::: process/factory.py
@@ +8412,5 @@
>              value=lambda b: b.builder.botmaster.parent.buildbotURL
>          ))
> +        self.addStep(SetProperty(
> +            name='get_workdir',
> +            property='working_dir',

Could you use "basedir" for consistency here?

@@ +8413,5 @@
>          ))
> +        self.addStep(SetProperty(
> +            name='get_workdir',
> +            property='working_dir',
> +            command=['pwd'],

Please use the following, since pwd is a builtin command (windows may fail)
command=['bash', '-c', 'pwd']

@@ +8416,5 @@
> +            property='working_dir',
> +            command=['pwd'],
> +            workdir='.',
> +            alwaysRun=True,
> +            flunkOnFailure=True,

You can drop alwaysRun=True and flunkOnFailure=True

@@ +8418,5 @@
> +            workdir='.',
> +            alwaysRun=True,
> +            flunkOnFailure=True,
> +        ))
> +        self.env = {'PROPERTIES_FILE': WithProperties('%(working_dir)s/buildprops.json')}

%(basedir) for consistency, please.

@@ +8427,5 @@
>          ))
>          if extra_data:
>              self.addStep(JSONStringDownload(
>                  extra_data,
>                  name="download_extra",

Can you also add the same trick to self.env['EXTRA_DATA'] = 'data.json' to be safe?

Comment 3

[John Hopkins (:jhopkins)]

Attachment: buildbotcustom patch

addressed rail's review comments

Comment 4

[John Hopkins (:jhopkins)]

Comment on attachment 624471 [details] [diff] [review]
buildbotcustom patch

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

Landed in https://hg.mozilla.org/build/buildbotcustom/rev/7168d3acbcb4

Per IRC conversation, added haltOnFailure=True in case the 'pwd' call fails.

Comment 5

[Aki Sasaki (not active)]

Looks like this is causing bug 757708.
We need to fix or back out.

Comment 6

[John Hopkins (:jhopkins)]

Can you point me to a failed log?

Why isn't bash available on those systems?

Comment 7

[Aki Sasaki (not active)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=12156017&tree=Firefox&full=1

These are test boxes, not sure why it's not in the path.

Comment 8

[John Hopkins (:jhopkins)]

We may be able to work around this problem.  Pseudocode:

if platform is windows:
   cmd = ['cmd', '/C', 'pwd']
else:
   cmd = ['bash', '-c', 'pwd']

Comment 9

[Rail Aliiev [:rail]]

(In reply to John Hopkins (:jhopkins) from comment #8)
> We may be able to work around this problem.  Pseudocode:
> 
> if platform is windows:
>    cmd = ['cmd', '/C', 'pwd']
> else:
>    cmd = ['bash', '-c', 'pwd']

see http://hg.mozilla.org/build/buildbotcustom/file/4a25912f0f4b/process/factory.py#l935

Comment 10

[Aki Sasaki (not active)]

Something similar might fix bug 724221 too.

Comment 11

[John Hopkins (:jhopkins)]

Attachment: proposed (but untested) patch

Here's a proposed patch that could work but needs some testing.

Comment 12

[John Hopkins (:jhopkins)]

Attachment: proposed (but untested) patch

attaching the correct patch

Comment 13

[Aki Sasaki (not active)]

Comment on attachment 628036 [details] [diff] [review]
proposed (but untested) patch

>diff --git a/misc.py b/misc.py
>--- a/misc.py
>+++ b/misc.py
>@@ -411,18 +411,19 @@ def generateTestBuilder(config, branch_n
>         # suites is a dict!
>         extra_args = suites.get('extra_args', [])
>         factory = ScriptFactory(
>             interpreter=mozharness_python,
>             scriptRepo=suites['mozharness_repo'],
>             scriptName=suites['script_path'],
>             hg_bin=suites['hg_bin'],
>             extra_args=suites.get('extra_args', []),
>             reboot_command=suites.get('reboot_command'),
>+            platform=platform,

I think this is the only required change in misc.py.

>@@ -1759,18 +1762,19 @@ def generateBranchObjects(config, name, 
>     if config['enable_weekly_bundle']:
>         stageBasePath = '%s/%s' % (config['stage_base_path'],
>                                    pf['stage_product'])
>         bundle_factory = ScriptFactory(
>             config['hgurl'] + config['build_tools_repo_path'],
>             'scripts/bundle/hg-bundle.sh',
>             interpreter='bash',
>             script_timeout=3600,
>             script_maxtime=3600,
>+            platform=platform,

This one is actually problematic.
It comes *after* a |for platform in enabled_platforms:| loop ends.
So platform shouldn't be in scope, but it will be.  The value of |platform| here won't have anything to do with this ScriptFactory.

>>> for i in ('a', 'b', 'c'):
...     pass
... 
>>> i
'c'

>@@ -3052,18 +3058,19 @@ def generateCCBranchObjects(config, name
>     if config['enable_weekly_bundle']:
>         stageBasePath = '%s/%s' % (config['stage_base_path'],
>                                    pf['stage_product'])
>         bundle_factory = ScriptFactory(
>             config['hgurl'] + config['build_tools_repo_path'],
>             'scripts/bundle/hg-bundle.sh',
>             interpreter='bash',
>             script_timeout=3600,
>             script_maxtime=3600,
>+            platform=platform,

Here too.

I'm not 100% sure about the platform in the other locations, but they look right.

r- as is; r+ if we either remove those two lines or limit the misc.py changes to the first chunk.

Comment 14

[Aki Sasaki (not active)]

Comment on attachment 628036 [details] [diff] [review]
proposed (but untested) patch

>+        cmd = ['bash', '-c', 'for file in `ls -1`; do cat $file; done']
>+        if self.platform and 'win' in self.platform:
>+            # note: prefixing 'type' command with '@' to suppress extraneous output
>+            self.get_basedir_cmd = ['cmd', '/C', 'for', '%f', 'in', '(*)', 'do', '@type', '%f']
>+        

Also, could we get rid of all the trailing whitespace on the final line here?

Comment 15

[John Hopkins (:jhopkins)]

Attachment: updated patch

removed all but 1st hunk in misc.py

Comment 16

[John Hopkins (:jhopkins)]

Comment on attachment 628750 [details] [diff] [review]
updated patch

Landed in http://hg.mozilla.org/build/buildbotcustom/rev/a553eb666d62

Comment 17

[Chris Cooper [:coop] (he/him)]

(In reply to John Hopkins (:jhopkins) from comment #16) 
> Landed in http://hg.mozilla.org/build/buildbotcustom/rev/a553eb666d62

Reconfig for this just happened.

Comment 18

[Aki Sasaki (not active)]

...and I see a green win64 peptest run.
Thanks all!