install tox in the lint image

RESOLVED FIXED in mozilla55

Status

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: dustin, Assigned: swapneshks, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55

Details

Attachments

(1 attachment, 1 obsolete attachment)

The mozharness unit tests run

https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/source-check/python-tests.yml
        command: >
            cd /home/worker/checkouts/gecko/testing/mozharness &&
            /usr/bin/pip2 install tox &&
            /home/worker/.local/bin/tox -e py27-hg3.7

which means that on every run of this task, we're hitting pypi.python.org and re-downloading tox (not to mention then hitting it again when tox generates the py27-hg3.7 virtualenv).

At the least, we should install tox in the docker image, as we do for flake8.

Better, make this a nice mach command.

Best, install the dependencies (coverage, nose, rednose -- mercurial's already installed) in the lint image and run the test command directly, rather than via tox.
There are a number of Python tests that could be extracted to their own task. There's also a number of existing Python-centric tasks (like Sphinx). It might be worth creating a "python" image or some such that comes pre-installed with everything we'd ever need.

Even better would be vendoring all the packages so we don't take the chance of downloading things from the internet or using varying package versions over time due to not pinning package versions.
This feels like it would be better under the Taskcluster product, as the work really needs to happen there I believe.
Component: Lint → Docker Images
Product: Testing → Taskcluster
+1 for vendoring tox.. I'd like to use it to set up the ability to run python-test with multiple versions of python so we at least have the ability to consider supporting python 3 in certain libraries.
Not really -- there's nothing to change in taskcluster here.  It's all in-tree changes, and I think most of those are to how the tests are invoked (at least / better / best options in the first comment).  While the TC team has some expertise in the generation of tasks for Gecko, we don't have much expertise in how test harnesses work, modifying mach commands, or in mozlint specifically.

That said, two experts have now spoken up, so if someone does take this bug as a good-first-bug, I know who to needinfo :)

If we vendor tox, it'd be good to *also* vendor (or at least include in the docker image) the stuff it installs when it runs, so that we're not hitting pypi.
(In reply to Dustin J. Mitchell [:dustin] from comment #4)
> If we vendor tox, it'd be good to *also* vendor (or at least include in the
> docker image) the stuff it installs when it runs, so that we're not hitting
> pypi.

Good point. Looks like it depends on 'py' (already vendored) and 'pluggy'. So it would just be the latter that's missing.

Also, if you want to add tox to the docker image to solve the mozharness issue in the meantime, that would be fine, and certainly easier.

Comment 6

2 years ago
I will take this bug

Comment 7

2 years ago
(In reply to omomuro.m from comment #6)
> I will take this bug

nevermind
Hi.. Can I help with this bug?
Which of the approaches mentioned in the description should I adopt - docker image, mach or lint?
Flags: needinfo?(dustin)
Let's do docker image.
Assignee: nobody → swapneshks
Flags: needinfo?(dustin)
I forgot to mention that I am a beginner and would be requiring some help to get started with the docker image approach. :)

I did some digging on my own though and it looks like something similar to http://searchfox.org/mozilla-central/source/taskcluster/docker/lint/Dockerfile#20 has to be done. But I am not very clear on where and how to do it..
Flags: needinfo?(dustin)
I think that's a good start.  Do you have access to push to try?  This will be pretty difficult if not!

So, yes, write up a requirements.txt for tox and install and run it the same as the flake8 file (or maybe combine the two?).  Then find where the command in comment 0 is defined (somewhere under taskcluster/ci) and modify it to use the tox installed in the image, rather than installing it directly.
Flags: needinfo?(dustin)
(In reply to Dustin J. Mitchell [:dustin] from comment #11)
> I think that's a good start.  Do you have access to push to try?  This will
> be pretty difficult if not!

Unfortunately, I don't have access to push to try, as I am very new here.. Could you vouch for me as you have reviewed my code once earlier? (https://bugzilla.mozilla.org/show_bug.cgi?id=1357802) If yes, then I'll file the required bug.
Flags: needinfo?(dustin)
I can -- please file the bug requesting access and I'll comment there.
Flags: needinfo?(dustin)
Let me know if the changes are appropriate and also if any other files need to be updated.
Attachment #8867352 - Flags: review?(dustin)
Comment on attachment 8867352 [details] [diff] [review]
Install tox in the docker image

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

Looks good (sorry for the delay.. I try to rest on weekends..)

I'll run the patch in a try job.  Now that you have level-1 access, can you try pushing this to mozreview?
  https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview-user.html
Comment on attachment 8867352 [details] [diff] [review]
Install tox in the docker image

Swapnesh, also take a look at that try job -- it didn't work :(
Attachment #8867352 - Flags: review?(dustin) → review-
(In reply to Dustin J. Mitchell [:dustin] from comment #17)
> Comment on attachment 8867352 [details] [diff] [review]
> Install tox in the docker image
> 
> Swapnesh, also take a look at that try job -- it didn't work :(

Looks like the try job didn't pick up the file tools/lint/tox/tox_requirements.txt:
https://hg.mozilla.org/try/rev/42f8c5fab1ac9a7e1abb384292fc0095389e21a8
Did I miss something while making the commit? (As the file tox_requirements.txt was an add)
Flags: needinfo?(dustin)
Oh, that's my fault, I didn't `hg add` it!

Now's your chance to try your own try job!  The message I used was "Bug 1302773: try: -b o -p lint -t none -u none"
Flags: needinfo?(dustin)
Comment hidden (mozreview-request)
Attachment #8867352 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed26a63092def4498d29ba3ee921c0a7a2c788f7&selectedJob=99211879

This is the try job that I tried. (I found out from IRC that I should have added the try:block in the first line)

I've also tried to push to mozreview.

As this was my first try job and first push to mozreview, do let me know if I went wrong anywhere...
I made a try run with the right syntax from your mozreview request (which looks fine).
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=07d4af954631
Ergh, the mozharness job didn't run there.  I've tried again.
Greg, I can't figure out how to make this job run in try.  It is getting optimized away.  Halp?
Flags: needinfo?(gps)
:swapnesh, sorry this is taking so long.  I'm going to make a fake change to mozharness to trick the job into running.

One thing to note: please don't use hg's "branch" feature -- the better feature to use is "bookmarks"
  http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/bookmarks.html
Flags: needinfo?(gps)
Great success!!
Reporter

Comment 28

2 years ago
mozreview-review
Comment on attachment 8867791 [details]
Bug 1302773 - Install tox in docker image in MozReview;

https://reviewboard.mozilla.org/r/139322/#review143600
Attachment #8867791 - Flags: review?(dustin) → review-
Reporter

Comment 29

2 years ago
mozreview-review
Comment on attachment 8867791 [details]
Bug 1302773 - Install tox in docker image in MozReview;

https://reviewboard.mozilla.org/r/139322/#review143602

Sorry, clicked wrong
Attachment #8867791 - Flags: review- → review+

Comment 30

2 years ago
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e66555bcd52f
Install tox in docker image in MozReview; r=dustin
(In reply to Dustin J. Mitchell [:dustin] from comment #25)

Sorry.. I meant to reply earlier but was having internet connectivity issues.

> One thing to note: please don't use hg's "branch" feature -- the better
> feature to use is "bookmarks"
>  
> http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/
> bookmarks.html

Ohh.. I didn't know that. Will keep it in mind in future. :)

Also, great to see the tests worked!

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e66555bcd52f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thanks Swapnesh, and sorry that took me so long to land!

What's next?
(In reply to Dustin J. Mitchell [:dustin] from comment #33)
> Thanks Swapnesh, 

My pleasure!

> and sorry that took me so long to land!

No worries at all. :)

> What's next?

I'm currently working on some other moz bugs, that should not take long to fix.. So, once those are done, if there are any bugs from your side that I can work on, I'll be more than willing to help. :)

Updated

11 months ago
Product: Taskcluster → Taskcluster Graveyard
You need to log in before you can comment on or make changes to this bug.