marionette-js-runner needs pip (and virtualenv) for buildbot job

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gaye, Assigned: gaye)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
We're trying to use the python gecko crash reporting stuff in marionette-js-runner but we need pip and virtualenv (installed locally by this shell script https://github.com/mozilla-b2g/marionette-js-runner/blob/master/scripts/setup_python.sh#L53). This fails on ci currently since we don't have sudo access, but if pip and virtualenv were available we wouldn't need to try to install them. :malini wrote that virtualenv install script but is on pto right now.
(Assignee)

Comment 1

3 years ago
Pinging people who know about python <> buildbot!
Flags: needinfo?(jgriffin)
Flags: needinfo?(ahalberstadt)
Unless you have a user python installation, this is a bit tricky. It looks like you could download the virtualenv.py script and run it directly with python:
http://opensourcehacker.com/2012/09/16/recommended-way-for-sudo-free-installation-of-python-software-with-virtualenv/
Flags: needinfo?(jgriffin)
Flags: needinfo?(ahalberstadt)
If this is for a buildbot job, virtualenv and pip are already available.  Pip should just work, and the path to virtualenv (on linux) is shown here:

http://hg.mozilla.org/build/mozharness/file/b0309005fc24/configs/marionette/prod_config.py#l15
(Assignee)

Comment 4

3 years ago
Created attachment 8571579 [details] [diff] [review]
bug-1137884.patch

Full disclosure my python is pretty rusty and I'm not sure how to test this without trying it on buildbot :). Thanks for your help :jgriffin!
Attachment #8571579 - Flags: review?(jgriffin)
Comment on attachment 8571579 [details] [diff] [review]
bug-1137884.patch

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

Hmm, so this won't actually work.  The format of self.config['exes']['virtualenv'] is something like:

   'virtualenv': ['/tools/buildbot/bin/python', '/tools/misc-python/virtualenv.py']

Your code would update $PATH so it just includes these paths, which are paths of executables, and not directories, so calling 'virtualenv' after munging the path like that would still fail.

On your own system, calling 'virtualenv' at a console actually invokes a shell script, which is probably in /usr/bin, that does this:

    #! /usr/bin/python
    import virtualenv
    virtualenv.main()

But, this shell script apparently doesn't exist on the buildbot slaves.

One way you could solve this is to modify the mozharness script to add a $VIRTUALENV_PATH env variable, and then modify your shell script that runs the tests to check for this env variable and use it if present.

*However*, your setup_python.sh shell script will still run into trouble, even once you're past the virtualenv hurdle, because calling 'python setup.py develop' will cause a bunch of dependencies to be installed, which by default will come from the pypi index on the internet, and thus occasionally cause intermittent oranges that the sheriffs will be unhappy about.

On buildbot slaves, there is already a virtualenv that contains all the necessary dependencies; all you would need to do is add your gaia-runner-service package to it, and then pass the location of this virtualenv to your script.  You could do this by calling:

    self.register_virtualenv_module('gaia-runner-service', '/path/containing/setup.py')

in gaia_integration.py's run_tests() method.

You can access the path to this virtualenv using:

     self.query_virtualenv_path()

...and you could store this in an env variable, and your script could access it if present.
Attachment #8571579 - Flags: review?(jgriffin) → review-
(Assignee)

Comment 6

3 years ago
Thanks for the thoroughness of response! What do you think about alternatively adding a bin/ directory to mozharness and shoving the missing virtualenv script in there and then adding /path/to/mozharness/bin to $PATH? If we roll with that hack then I guess we would also have to do the $VIRTUALENV_PATH bit so that we could sys.path.append(os.environ['VIRTUALENV_PATH']) in mozharness/bin/virtualenv? Thoughts?
Flags: needinfo?(jgriffin)
So doing that won't prevent the problem of you installing Python package dependencies from the internet, and it introduces a bunch of other problems: it will never work on Windows, it will likely fail because 'import virtualenv' is unlikely to work the way the buildbot slaves are setup (virtualenv isn't installed in the same way there as it is on a typical user's machine), and it fragments the way we deal with virtualenvs in our infrastructure so that future improvements and changes will not be integrated here.

If you go the self.register_virtualenv_module() and self.query_virtualenv_path() route, you should get what you want very simply...is there a concern with this approach?

self.query_virtualenv_path() returns the path of the virtualenv that will be pre-populated with everything you need, not the virtualenv executable.  So, you could:

    env['VIRTUALENV_PATH'] = self.query_virtulenv_path()

or even

    env['PYTHON_PATH'] = self.query_python_path()

which will return the path to the Python executable _inside_ the virtualenv that's arleady setup, so you could just call

    $PYTHON_PATH foo.py --cli-args

in your automation, and you never need to call your shell script to set the virtualenv up at all.
Flags: needinfo?(jgriffin)
(Assignee)

Comment 8

3 years ago
Right I was thinking about the mozharness/bin/virtualenv bit just for helping marionette-js-runner find virtualenv and rolling with your idea for putting 'gaia-runner-service' in buildbot's virtualenv with register_virtualenv_module(). I was mostly hoping that we could achieve everything in mozharness without making changes to gaia or marionette-js-runner (like supporting more environmental variables). Not that I think that there's anything canonical or more correct about the way that the setup_python.sh script discovers this stuff...
(Assignee)

Comment 9

3 years ago
Created attachment 8572117 [details] [diff] [review]
bug-1137884.patch

Second attempt! This time, we run make node_modules before running make test-integration so that $GAIA/node_modules/marionette-js-runner/host/python/runner-service (the python package we need to shove into virtualenv) is available before we run the tests. Then we add that to the virtualenv with install_module() and set env['PYTHON_BINARY'] to a python that's inside the virtualenv. My hope is that I can then go ahead and update the script in marionette-js-runner that calls python to respect $PYTHON_BINARY and use that if it's set (rather than the python we're already picking up).
Attachment #8571579 - Attachment is obsolete: true
Attachment #8572117 - Flags: review?(jgriffin)
(Assignee)

Comment 10

3 years ago
Created attachment 8572129 [details] [diff] [review]
bug-1137884.patch

Actually would still probably be useful to consumer to get VIRTUALENV_PATH in there too
Attachment #8572129 - Flags: review?(jgriffin)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8572117 [details] [diff] [review]
bug-1137884.patch

Obsolete
Attachment #8572117 - Flags: review?(jgriffin)
(Assignee)

Comment 12

3 years ago
Created attachment 8572137 [details] [review]
marionette-js-runner part

marionette-js-runner updates to check for python and virtualenv environment variables
Attachment #8572117 - Attachment is obsolete: true
Attachment #8572137 - Flags: review?(jlal)
Attachment #8572137 - Flags: review?(jgriffin)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8572137 [details] [review]
marionette-js-runner part

:lightsofapollo gave me a (possibly unenthused : ) r+ on irc for the marionette-js-runner parts but will still wait on :jgriffin reviews
Attachment #8572137 - Flags: review?(jlal) → review+
Comment on attachment 8572129 [details] [diff] [review]
bug-1137884.patch

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

So, with this change, there is no need for your automation to call setup_python.sh at all, since it already does everything that script does.  But I'm not sure where in your automation that virtualenv is actually used; that's where you might need to refer to e.g., $PYTHON_BINARY.

::: scripts/gaia_integration.py
@@ +43,5 @@
> +        # living in $GAIA/node_modules needed for integration tests can be
> +        # installed in virtualenv before running test suite
> +        self.run_command(['make', 'node_modules'], cwd=dirs['abs_gaia_dir'],
> +            env=env,
> +            output_timeout=330)

There's no need for this; the call to node_setup() earlier handles this already.

@@ -44,4 @@
>          cmd = [
>              'make',
>              'test-integration',
> -            'NPM_REGISTRY=' + self.config.get('npm_registry'),

Are you sure removing this is harmless?  It wasn't last time I tried, but that was many months ago.
Attachment #8572129 - Flags: review?(jgriffin) → review+
Comment on attachment 8572137 [details] [review]
marionette-js-runner part

As noted in the other patch review, this shouldn't be necessary at all.  If you want to do it for some reason, I'll re-review, since $VIRTUALENV_PATH isn't being used correctly.
Attachment #8572137 - Flags: review?(jgriffin) → review-
(Assignee)

Comment 16

3 years ago
Created attachment 8572258 [details] [diff] [review]
bug-1137884.patch

Third time's the charm! This one adds an environment variable to the make node_modules invocation to signal to marionette-js-runner that we can bail from scripts/setup_python.sh without doing anything. It also only exposes VIRTUALENV_PATH since the way that we call python in marionette-js-runner currently is by adding $VENV/bin to $PATH and then calling the python binary gaia-integration (which should exist in $VENV/bin) directly.
Attachment #8572129 - Attachment is obsolete: true
Attachment #8572258 - Flags: review?(jgriffin)
(Assignee)

Comment 17

3 years ago
Comment on attachment 8572137 [details] [review]
marionette-js-runner part

Reflagging for review. This changeset now makes marionette-js-runner bail from virtualenv setup if the env var VIRTUALENV_EXISTS is set and also allows $VIRTUALENV_PATH to override the normal/local venv location marionette-js-runner/venv.
Attachment #8572137 - Flags: review- → review?(jgriffin)
Attachment #8572137 - Flags: review?(jgriffin) → review+
Comment on attachment 8572258 [details] [diff] [review]
bug-1137884.patch

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

lgtm, thanks!
Attachment #8572258 - Flags: review?(jgriffin) → review+
ping me if you need help landing the mozharness part
http://hg.mozilla.org/build/mozharness/rev/a8fbffe4c044

Waiting for a green run on cedar before promoting to production.
(In reply to Jonathan Griffin (:jgriffin) from comment #20)
> http://hg.mozilla.org/build/mozharness/rev/a8fbffe4c044
> 
> Waiting for a green run on cedar before promoting to production.

merged to production: https://hg.mozilla.org/build/mozharness/rev/14da0434f169
Gareth,

Is this bug closable now?

Thanks,
Pete
Flags: needinfo?(gaye)
(Assignee)

Comment 23

3 years ago
Yep!
Assignee: nobody → gaye
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(gaye)
Resolution: --- → FIXED
Depends on: 1146077
Depends on: 1150205
QA Contact: pmoore → mshal
You need to log in before you can comment on or make changes to this bug.