Provide better error message when ./mach robocop is run w/no MOZ_HOST_BIN

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: capella, Assigned: gioyik, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 39
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=python][good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

If a new dev tries to fire off a robocop test using |./mach robocop| or |./mach robocop testFoo| without first exporting their MOZ_HOST_BIN value, they receive this somewhat cryptic message:

TypeError: coercing to Unicode: need string or buffer, NoneType found

This might could be friendlier :-)
And maybe update the wiki page? Not sure how you want that worded... I still execute manually ( the "deprecated" way :) )
The old make targets do a good job of checking MOZ_HOST_BIN. Something very similar might be good for mach:

http://hg.mozilla.org/mozilla-central/annotate/95c76c3b0172/testing/testsuite-targets.mk#l90
Hi,

I want to work on this issue. Could you tell me on which file I need to make the changes?

Thanks
Flags: needinfo?(markcapella)
Guess I should have posted the rest of that:

  File "/home/master/mozilla-central/testing/mochitest/mach_commands.py", line 907, in run_robocop
    '--xre-path=' + os.environ.get('MOZ_HOST_BIN'),

So basically it fails here:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py?rev=f397b3559467&mark=907-907#887
Flags: needinfo?(markcapella)
Duplicate of this bug: 1082980
Could this be a solution:

  if MOZ_HOST_BIN == '':
      print('environment variable MOZ_HOST_BIN must be set')

I ask before attach a possible patch. Let me know if I am correct.
Flags: needinfo?(markcapella)
Hi Giovanny!

   While I filed this bug, it's being mentored by nalexander. I know he's got some additional background plans in the works here, so I'll let him respond from now on.
Flags: needinfo?(markcapella) → needinfo?(nalexander)
(In reply to Mark Capella [:capella] from comment #7)
> Hi Giovanny!
> 
>    While I filed this bug, it's being mentored by nalexander. I know he's
> got some additional background plans in the works here, so I'll let him
> respond from now on.

Giovanny: yes, something like this.  I think we should implement exactly the same checks as https://bugzilla.mozilla.org/show_bug.cgi?id=1126057#c2 does, but in Python at the relevant point.  Flag me for feedback/review on a patch.  Thanks!
Flags: needinfo?(nalexander)
I am working on this but I have questions about the use of "! -d" and "! -f". Could you tell me how can be ported to python?

This is the work I had done before create a patch for review:

if MOZ_HOST_BIN == '':
    print('environment variable MOZ_HOST_BIN must be set to a directory containing host xpcshell')
elif ! MOZ_HOST_BIN:
    print('MOZ_HOST_BIN does not specify a directory')
elif ! MOZ_HOST_BIN/xpcshell:
    print('xpcshell not found in MOZ_HOST_BIN')
elif TEST_DEVICE == '' & DM_TRANS != 'adb':
    print('please prepare your host with the environment variable TEST_DEVICE')
else:
    RUN_MOCHITEST_REMOTE
Flags: needinfo?(nalexander)
(In reply to Giovanny Gongora [:gioyik] from comment #9)
> I am working on this but I have questions about the use of "! -d" and "!
> -f". Could you tell me how can be ported to python?
> 
> This is the work I had done before create a patch for review:

You'll need to have

MOZ_HOST_BIN = os.environ.get('MOZ_HOST_BIN')

> if MOZ_HOST_BIN == '':
>     print('environment variable MOZ_HOST_BIN must be set to a directory
> containing host xpcshell')

if not MOZ_HOST_BIN:

> elif ! MOZ_HOST_BIN:
>     print('MOZ_HOST_BIN does not specify a directory')

Use os.path.isdir: http://devdocs.io/python2/library/os.path#os.path.isdir

> elif ! MOZ_HOST_BIN/xpcshell:
>     print('xpcshell not found in MOZ_HOST_BIN')

Use os.path.exists: http://devdocs.io/python2/library/os.path#os.path.exists and maybe use the helper function at

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/executables.py#57

to test for executable.

> elif TEST_DEVICE == '' & DM_TRANS != 'adb':
>     print('please prepare your host with the environment variable
> TEST_DEVICE')
> else:
>     RUN_MOCHITEST_REMOTE

This doesn't make sense here.
Flags: needinfo?(nalexander)
Thanks for the quickly feedback. I made changes and this is a second possible solution:

MOZ_HOST_BIN = os.environment.get('MOZ_HOST_BIN')
RUN_MOCHITEST_REMOTE = os.environment.get('RUN_MOCHITEST_REMOTE')

if not MOZ_HOST_BIN:
	print('environment variable MOZ_HOST_BIN must be set to a directory containing host xpcshell')
elif os.path.isdir(MOZ_HOST_BIN):
    print('MOZ_HOST_BIN does not specify a directory')
elif is_executable(MOZ_HOST_BIN + '/xpcshell'):
    print('xpcshell not found in MOZ_HOST_BIN')
elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':
	print('please prepare your host with the environment variable TEST_DEVICE')
else:
	open(RUN_MOCHITEST_REMOTE)

I have a question with the "-a" option in the last part. I think is an "AND" operator but I could be wrong.
Flags: needinfo?(nalexander)
Sorry I made a change in the first lines:

MOZ_HOST_BIN = os.environment.get('MOZ_HOST_BIN')
RUN_MOCHITEST_REMOTE = os.environment.get('RUN_MOCHITEST_REMOTE')

if MOZ_HOST_BIN == '':
	print('environment variable MOZ_HOST_BIN must be set to a directory containing host xpcshell')
elif not os.path.isdir(MOZ_HOST_BIN):
    print('MOZ_HOST_BIN does not specify a directory')
elif is_executable(MOZ_HOST_BIN + '/xpcshell'):
    print('xpcshell not found in MOZ_HOST_BIN')
elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':
	print('please prepare your host with the environment variable TEST_DEVICE')
else:
	open(RUN_MOCHITEST_REMOTE)
Sorry for the delayed reply.

Throughout, prefer os.path.join to +.

(In reply to Giovanny Gongora [:gioyik] from comment #12)
> Sorry I made a change in the first lines:
> 
> MOZ_HOST_BIN = os.environment.get('MOZ_HOST_BIN')
> RUN_MOCHITEST_REMOTE = os.environment.get('RUN_MOCHITEST_REMOTE')

You don't need RUN_MOCHITEST_REMOTE.

> if MOZ_HOST_BIN == '':

if not MOZ_HOST_BIN:

> 	print('environment variable MOZ_HOST_BIN must be set to a directory
> containing host xpcshell')
> elif not os.path.isdir(MOZ_HOST_BIN):
>     print('MOZ_HOST_BIN does not specify a directory')

add

elif not os.path.isfile(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
    print('xpcshell not found in MOZ_HOST_BIN')

> elif is_executable(MOZ_HOST_BIN + '/xpcshell'):

elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):

We want to be able to run this!

>     print('xpcshell not found in MOZ_HOST_BIN')

print('xpcshell in MOZ_HOST_BIN is not executable')

> elif os.environment.get('TEST_DEVICE') == '' &&
> os.environment.get('DM_TRANS') not 'adb':
> 	print('please prepare your host with the environment variable TEST_DEVICE')
> else:
> 	open(RUN_MOCHITEST_REMOTE)

Get rid of this stuff.  Once you have the tests above, we're ready to go!  Can I see a patch?  Where are you importing is_executable from?
Flags: needinfo?(nalexander)
Posted patch 1126057.patch (obsolete) — Splinter Review
Attachment #8567727 - Flags: review?(nalexander)
Sorry for the late answer. I was organizing my schedule with my University classes. I attached a first patch, please, could you review it? Also, could you tell me the computed syntax you could use for this bug to try it on treeherder? I have commit access level 1, so I can use try server with this.
Comment on attachment 8567727 [details] [diff] [review]
1126057.patch

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

Sorry for the extremely delayed feedback :/  Feel free to ping if I'm lagging.

::: testing/mochitest/mach_commands.py
@@ +1130,5 @@
> +        elif not os.path.isdir(MOZ_HOST_BIN):
> +            print('MOZ_HOST_BIN does not specify a directory')
> +        elif not os.path.isfile(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell not found in MOZ_HOST_BIN')
> +        elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):

I don't think this will work, since the function won't be found.  You might want

mozpack.executables.is_executable

@@ +1132,5 @@
> +        elif not os.path.isfile(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell not found in MOZ_HOST_BIN')
> +        elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell in MOZ_HOST_BIN is not executable')
> +        elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':

kill this clause.

@@ +1134,5 @@
> +        elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell in MOZ_HOST_BIN is not executable')
> +        elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':
> +            print('please prepare your host with the environment variable TEST_DEVICE')
> +        else:

and this clause.  At this point, we're good to go!
Attachment #8567727 - Flags: review?(nalexander) → feedback+
(In reply to Giovanny Gongora [:gioyik] from comment #15)
> Sorry for the late answer. I was organizing my schedule with my University
> classes. I attached a first patch, please, could you review it? Also, could
> you tell me the computed syntax you could use for this bug to try it on
> treeherder? I have commit access level 1, so I can use try server with this.

This code won't be exercised on the try server -- you'll have to test locally.  (I'll test locally too.)
Patch v2
Attachment #8567727 - Attachment is obsolete: true
Attachment #8570012 - Flags: review?(nalexander)
Nick don't worry about the time. :)

I attached a patch with the changes you mention. Let me know if there's something to change and fix.
Comment on attachment 8570012 [details] [diff] [review]
1126057-v2.patch

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

I'll land a slightly modified version of this.  Thanks, Giovanny!
Attachment #8570012 - Flags: review?(nalexander) → review+
Assignee: nobody → gioyik
Status: NEW → ASSIGNED
Sounds good, Thank you!
https://hg.mozilla.org/mozilla-central/rev/fa4247a2db1b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.