Closed Bug 943008 Opened 8 years ago Closed 8 years ago

Enable pinning of marionette_client package in setup.py

Categories

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mdas, Assigned: mdas)

Details

Attachments

(2 files, 1 obsolete file)

We currently do not pin the version of marionette_client used in gaiatest because we want to enable testing of changes to the mozilla-central in-tree marionette_client on TBPL. By not pinning a version, TBPL uses the intree one and runs the Gu tests. The problem with this is if we don't pin our version of marionette_client, if a significant change happens in the client, then even if a package pins its version of gaiatest, it will still see the marionette_client changes. If the client changes causes an older version of gaiatest to break in some way, it breaks the expectation that no change will be seen if packages that depend on gaiatest pin the gaiatest version.

We can fix this by adding a tbpl_setup.py which our mozharness scripts can use. This file will have an unpinned marionette_client version and will enable us to test the in-tree changes. We can then pin a version of marionette in the normal setup.py file.
Attached patch mozharness change (obsolete) — Splinter Review
We can pin the version of marionette_client easily if we use requirements.txt files instead of defining dependencies in the setup.py file. This way, there's the default file used by gaiatest is the requirements.txt file with the pinned marionette version, and the file used by tbpl will be tbpl_requirements.
Attachment #8339525 - Flags: review?(aki)
Attached file gaia-ui-tests changes
you can test the gaia change locally by doing the following in the gaia-ui-tests directory:

pip install -r tbpl_requirements.txt .

which is similar to what mozharness is doing. It will use the tbpl_requirements file even if the install_requires dependencies in setup.py point use those in  requirements.txt
Attachment #8339533 - Flags: review?(dave.hunt)
Comment on attachment 8339525 [details] [diff] [review]
mozharness change

>             self.register_virtualenv_module('gaia-ui-tests',
>-                url=self.query_abs_dirs()['abs_gaiatest_dir'], method='pip')
>+                url=os.path.join(self.query_abs_dirs()['abs_gaiatest_dir']), method='pip', requirements=[requirements])

self.query_abs_dirs() returns a dict of directory nickname keys to absolute path values, so self.query_abs_dirs()['abs_gaiatest_dir'] is a single absolute path value.
I think the os.path.join() on a single string that's already in directory path format is redundant, though harmless; r=me without it.
Attachment #8339525 - Flags: review?(aki) → review+
Comment on attachment 8339533 [details] [review]
gaia-ui-tests changes

If there was a way to maintain a single list of dependencies and only call out the marionette client version as different for TBPL then that would be preferable from a maintenance perspective. We are going to need to remember that any new dependencies or changes to the dependencies must be made to both requirements files. I'm not sure I have any suggestions for how to do this though.. does TBPL perhaps have an environment variable we could use to know we're running on it?

Otherwise, r=me.
Attachment #8339533 - Flags: review?(dave.hunt) → review+
Thanks, I've updated the patch and tested it locally. Moving r+ forward.
Attachment #8339525 - Attachment is obsolete: true
Attachment #8339980 - Flags: review+
(In reply to Dave Hunt (:davehunt) from comment #4)
> Comment on attachment 8339533 [details] [review]
> gaia-ui-tests changes
> 
> If there was a way to maintain a single list of dependencies and only call
> out the marionette client version as different for TBPL then that would be
> preferable from a maintenance perspective. We are going to need to remember
> that any new dependencies or changes to the dependencies must be made to
> both requirements files. I'm not sure I have any suggestions for how to do
> this though.. does TBPL perhaps have an environment variable we could use to
> know we're running on it?
> 
> Otherwise, r=me.

The environment variable way seems a bit roundabout, but I just tested it with a minimal tbpl_requirements.txt with just marionette_client listed and it works this way. You can now specify the other deps just once in the requirements.txt file. I've updated the PR.
Comment on attachment 8339533 [details] [review]
gaia-ui-tests changes

What do think of this approach?
Attachment #8339533 - Flags: review+ → review?(dave.hunt)
Comment on attachment 8339533 [details] [review]
gaia-ui-tests changes

I didn't realise it would be that simple! Looks great!
Attachment #8339533 - Flags: review?(dave.hunt) → review+
(In reply to Dave Hunt (:davehunt) from comment #8)
> Comment on attachment 8339533 [details] [review]
> gaia-ui-tests changes
> 
> I didn't realise it would be that simple! Looks great!

Dave, mind merging this into gaia?
(In reply to Malini Das [:mdas] from comment #9)
> (In reply to Dave Hunt (:davehunt) from comment #8)
> > Comment on attachment 8339533 [details] [review]
> > gaia-ui-tests changes
> > 
> > I didn't realise it would be that simple! Looks great!
> 
> Dave, mind merging this into gaia?

Landed in:
https://github.com/mozilla-b2g/gaia/commit/fc51878943c362f21629967c05bbc457d51d7ebe (master)
https://github.com/mozilla-b2g/gaia/commit/ab6c92069cc9a32c0563141447ee54da44a2d4be (v1.2)
Landed mozharness change in  https://hg.mozilla.org/build/mozharness/rev/735c70159fa1

So we need to wait for this to migrate to production. In any case, tip marionette-client will be used as long as we don't change the tip marionette client's version number
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
I merged this to production just now.
I think this broke all gaia-ui-tests: https://tbpl.mozilla.org/php/getParsedLog.php?id=31266528&tree=Mozilla-Inbound&full=1

09:25:29     INFO - Installing gaia-ui-tests into virtualenv /builds/slave/test/build/venv
09:25:29     INFO - Running command: ['/builds/slave/test/build/venv/bin/pip', 'install', '--download-cache', '/builds/slave/test/build/venv/cache', '-r', '/builds/slave/test/gaia/tests/python/gaia-ui-tests/tbpl_requirements.txt', '--no-index', '--find-links', 'http://pypi.pvt.build.mozilla.org/pub', '--find-links', 'http://pypi.pub.build.mozilla.org/pub', '/builds/slave/test/gaia/tests/python/gaia-ui-tests'] in /builds/slave/test/build
09:25:29     INFO - Copy/paste: /builds/slave/test/build/venv/bin/pip install --download-cache /builds/slave/test/build/venv/cache -r /builds/slave/test/gaia/tests/python/gaia-ui-tests/tbpl_requirements.txt --no-index --find-links http://pypi.pvt.build.mozilla.org/pub --find-links http://pypi.pub.build.mozilla.org/pub /builds/slave/test/gaia/tests/python/gaia-ui-tests
09:25:29     INFO -  Ignoring indexes: http://pypi.python.org/simple/
09:25:29     INFO -  Could not open requirements file: [Errno 2] No such file or directory: '/builds/slave/test/gaia/tests/python/gaia-ui-tests/tbpl_requirements.txt'
09:25:29     INFO -  Storing complete log in /home/cltbld/.pip/pip.log
09:25:29    ERROR - Return code: 1
09:25:29    FATAL - Unable to install /builds/slave/test/gaia/tests/python/gaia-ui-tests!
09:25:29    FATAL - Running post_fatal callback...
09:25:29    FATAL - Exiting -1

I backed it out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Ben Hearsum [:bhearsum] from comment #13)
> I backed it out.

Which patch was backed out? The mozharness one or the gaia one?
https://hg.mozilla.org/mozilla-central/rev/90d84177ff3d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
I don't think this is resolved due to the backout, presumably of the mozharness patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Dave Hunt (:davehunt) from comment #16)
> I don't think this is resolved due to the backout, presumably of the
> mozharness patch.

Yeah, sorry for the confusion. It was the mozharness one backed out.
oh, a new version of gaiatest has to be released for the tbpl_requirements.txt file to be available. Let me coordinate with davehunt
Ah, davehunt pointed out that it's using tip gaia to install gaiatest, so I jumped the gun. I now think it's because the changes to gaiatest didn't make it into the gaia blob we pull in. I'm testing it on ash to see if that's the case now.
Great, it seems the problem was that the change didn't migrate in time for mozharness to get it. The try attempt passed: https://tbpl.mozilla.org/php/getParsedLog.php?id=31325385&tree=Ash

Bhearsum, do you think you can re-land this please?
pushed to mozharness as https://hg.mozilla.org/build/mozharness/rev/b8168d737be3 and will be moved to production soon.
Seems to run fine in production!
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.