[flake8] Automatically install/update flake8 package to correct version

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: ahal, Assigned: francesco.pischedda)

Tracking

unspecified
mozilla52

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
Currently when running ./mach lint -l flake8, it will simply print and error if you don't have flake8 installed. And if you have an older or newer version (from the one in CI), it'll just happily use that.

This is A) inconvenient, and B) can result in different errors being printed from what is seen in automation. This needs to be fixed.
Reporter

Updated

3 years ago
Duplicate of this bug: 1304204
Assignee

Comment 3

3 years ago
I can start to work on this one, is there some wiki page that describes how the mach script works?
Reporter

Comment 4

3 years ago
Actually, this one I think would be better to implement in the tools/lint/flake8.lint file. If you look at tools/lint/eslint.lint, you'll notice there is code to detect whether the proper version of eslint is installed. We want to do the same thing with flake8 (except we'll be using pip instead of npm).

There is a virtualenv that is created as part of the build, ideally we want to install pip in there. I'm not yet sure the best way to get access to that yet though. But try to start writing a patch for now, and we'll see how it goes and worry about the virtualenv later. Let me know if you need more clarification!
Assignee

Comment 5

3 years ago
Thank you for the info, I'll have a look at those files; thank you for your help as usual :)
Comment hidden (mozreview-request)
Assignee

Comment 7

3 years ago
Hi Andrew, this push to review is actually a request to validate the overall code layout, also I have some questions:
- the required flake8 version can be hardcoded or can it be taken from some other source? (for example https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/system-setup.sh#65)
- is this the correct way to get the installed version of flake8 ?
- is this the correct way to verify the intalled version against the target version?
- how can I write a unit test for all the new code?
Reporter

Updated

3 years ago
Assignee: nobody → francesco.pischedda
Status: NEW → ASSIGNED
Reporter

Comment 8

3 years ago
(In reply to Francesco Pischedda from comment #7)
> Hi Andrew, this push to review is actually a request to validate the overall
> code layout, also I have some questions:

Thanks, this is looking really good overall, just what I had in mind!


> - the required flake8 version can be hardcoded or can it be taken from some
> other source? (for example
> https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/system-
> setup.sh#65)

Yes, we should try to share the version of flake8 between these two places. Though I think the version should actually be stored somewhere in tools/lint, and system-setup.sh should be modified to grab the version from there. Maybe create a tools/lint/flake8/flake8_requirements.txt file that has the same contents of:
https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/system-setup.sh#65

Then in flake8.lint, do pip install -r flake8_requirements.txt.

For system-setup.sh, in-tree files can actually be added to the Dockerfile using this syntax:
https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/Dockerfile#14

So you could add the flake8_requirements.txt file that way, and then modify system-setup.sh to use that rather than writing it out manually. Make sense?

> - is this the correct way to get the installed version of flake8 ?

Yes, just some minor tweaks like using a requirements.txt file mentioned above. I'd like to use --require-hashes as well which means we need pip > 8.0. So we may also need to run `pip install --upgrade pip` before hand as well.

The main thing is that I think we should try to use the build system virtualenv (this gets created under <objdir>/_virtualenv). 

> - is this the correct way to verify the intalled version against the target
> version?

If we go with the requirements.txt approach above combined with the build system virtualenv.. We might just be able to run 'pip install -r requirements.txt' and let pip figure out if it needs to upgrade anything or not on its own. So we can either:

1) Always run 'pip install' no matter what, which might be a no-op if everything is installed
2) Detect versions and only run 'pip install' if we need to

I'm not sure which approach is better yet.. I guess it'll depend on whether a no-op pip install is faster than a --version check or not.

> - how can I write a unit test for all the new code?

Unfortunately there is not a good way to test mach commands. There are mozlint unittests, but they don't cover stuff that is specific to individual linters (like flake8). I do have some ideas on how to get tests up and running for mach (and other command line utilities) though, but that's another bug for another time. I know it's bad, but for now don't worry about the test.
Assignee

Comment 9

3 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> (In reply to Francesco Pischedda from comment #7)
> > Hi Andrew, this push to review is actually a request to validate the overall
> > code layout, also I have some questions:
> 
> Thanks, this is looking really good overall, just what I had in mind!

Good :)
 
> > - the required flake8 version can be hardcoded or can it be taken from some
> > other source? (for example
> > https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/system-
> > setup.sh#65)
> 
> Yes, we should try to share the version of flake8 between these two places.
> Though I think the version should actually be stored somewhere in
> tools/lint, and system-setup.sh should be modified to grab the version from
> there. Maybe create a tools/lint/flake8/flake8_requirements.txt file that
> has the same contents of:
> https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/system-
> setup.sh#65
> 
> Then in flake8.lint, do pip install -r flake8_requirements.txt.
> 
> For system-setup.sh, in-tree files can actually be added to the Dockerfile
> using this syntax:
> https://dxr.mozilla.org/mozilla-central/source/testing/docker/lint/
> Dockerfile#14
> 
> So you could add the flake8_requirements.txt file that way, and then modify
> system-setup.sh to use that rather than writing it out manually. Make sense?

It sounds good to me, it is really cleaner then any other solution that I was considering (such as parsing system-setup.sh)

> > - is this the correct way to get the installed version of flake8 ?
> 
> Yes, just some minor tweaks like using a requirements.txt file mentioned
> above. I'd like to use --require-hashes as well which means we need pip >
> 8.0. So we may also need to run `pip install --upgrade pip` before hand as
> well.
> 
> The main thing is that I think we should try to use the build system
> virtualenv (this gets created under <objdir>/_virtualenv).

is the virtualenv automatically activated before the mach script runs its commands or should it be activated inside flake8.lint?
 
> > - is this the correct way to verify the intalled version against the target
> > version?
> 
> If we go with the requirements.txt approach above combined with the build
> system virtualenv.. We might just be able to run 'pip install -r
> requirements.txt' and let pip figure out if it needs to upgrade anything or
> not on its own. So we can either:
> 
> 1) Always run 'pip install' no matter what, which might be a no-op if
> everything is installed
> 2) Detect versions and only run 'pip install' if we need to
> 
> I'm not sure which approach is better yet.. I guess it'll depend on whether
> a no-op pip install is faster than a --version check or not.

A no-op pip install seem fairly fast, taking a fraction of a second to complete so this may be the right solution; I even tryed to run it without an internet connection (to simulate offline working) and it worked properly

> > - how can I write a unit test for all the new code?
> 
> Unfortunately there is not a good way to test mach commands. There are
> mozlint unittests, but they don't cover stuff that is specific to individual
> linters (like flake8). I do have some ideas on how to get tests up and
> running for mach (and other command line utilities) though, but that's
> another bug for another time. I know it's bad, but for now don't worry about
> the test.

Ok, good

So to recap:
- create tools/lint/flake8/flake8_requirements.txt with proper content
- add this file to docker
- change system-setup.sh to run pip install -r flake8_requirements.txt
- be sure to user the setup virtualenv while running mach script
- before the linter starts doing its stuff run pip install -U pip && pip install -Ur flake8_requirements.txt

Missing something?
Reporter

Comment 10

3 years ago
(In reply to Francesco Pischedda from comment #9)
> is the virtualenv automatically activated before the mach script runs its
> commands or should it be activated inside flake8.lint?

Good question, normally mach commands have access to a 'self.virtualenv_manager' object that handles stuff like this, but linters are run in a new subprocess, so they won't be able to get it. So we'll need to active it inside flake8.lint, but I'm not sure how best to find where it is there. I'll have to look into it and get back to you.
 
> So to recap:
> - create tools/lint/flake8/flake8_requirements.txt with proper content
> - add this file to docker
> - change system-setup.sh to run pip install -r flake8_requirements.txt
> - be sure to user the setup virtualenv while running mach script
> - before the linter starts doing its stuff run pip install -U pip && pip
> install -Ur flake8_requirements.txt
> 
> Missing something?

Yep, that sounds like a good summary. I think we should avoid doing pip install -U pip unless we get an error about --require-hashes not working.
Reporter

Comment 11

3 years ago
Actually the virtualenv problem is easier than I thought. You can just call self._activate_virtualenv() in the tools/lint/mach_commands.py, and then it'll automatically get activated and persist across multiprocesses. I believe calling a pip subprocess should automatically install to there too, but we should test this further.
Assignee

Comment 12

3 years ago
it seems that multiprocessing.Process propagates the environment variables to child processes, try this example in a virtualenv:

import multiprocessing
import os


def func():

    for e in os.environ.items():
        print(e)

if __name__ == '__main__':
    p = multiprocessing.Process(target=func)
    p.start()
    p.join()

virtualenv just plays with env vars to do its magic so this should be sufficient for our needs, correct me if I'm wrong
Reporter

Comment 13

3 years ago
You are correct, multiprocessing does propagate environment, so it should just work. You can verify this by looking for flake8 in <objdir>/_virtualenv/lib/python2.7/site-packages after installation.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 16

3 years ago
Hi Andrew,

the last review request contains these changes:
- system-setup.sh installs flake8 and its dependencies from flake8_requirements.txt; also updates pip (this was not mentioned before, I'll remove it if it could case some truble)
- added flake8_requirements.txt to Dockerfile
- in flake8.lint install/update flake8 using testing/lint/flake8_requirements.txt
- instead of checking if pip install supports --require-hashes try to update pip before addressing flake8, it seems faster then parsing the update of pip help install to check the presence of --require-hashes
Reporter

Comment 17

3 years ago
mozreview-review
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review85570

Thanks, looking good! Don't forget to call `self._activate_virtualenv()` in the tools/lint/mach_commands.py file though!

::: tools/lint/flake8.lint:19
(Diff revisions 1 - 3)
>  
>  from mozlint import result
>  
>  
>  FLAKE8_NOT_FOUND = """
> -Could not find flake8! Install flake8 and try again.
> +Could not find flake! Install flake8 and try again.

Typo: flake should be flake8

::: tools/lint/flake8.lint:161
(Diff revisions 1 - 3)
> -    if not check_flake8_version(current_version, target_version):
> -        print(FLAKE8_WRONG_VERSION.format(current_version,
> -                                          target_version))
> -        if not install_flake8_at_version(target_version):
> -            print(FLAKE8_INSTALL_ERROR.format(target_version))
> -            return []
> +        return []

Return 1 here

::: testing/docker/lint/Dockerfile:17
(Diff revision 3)
>  ADD topsrcdir/testing/mozharness/external_tools/robustcheckout.py /usr/local/mercurial/robustcheckout.py
>  
>  # %include testing/docker/recipes/install-mercurial.sh
>  ADD topsrcdir/testing/docker/recipes/install-mercurial.sh /build/install-mercurial.sh
>  ADD system-setup.sh /tmp/system-setup.sh
> +ADD flak8_requirements.txt /tmp/flake8_requirements.txt

Typo: flake8_requirements.txt

::: testing/docker/lint/flake8_requirements.txt:1
(Diff revision 3)
> +flake8==2.5.4 --hash=sha256:fb5a67af4024622287a76abf6b7fe4fb3cfacf765a790976ce64f52c44c88e4a
> +mccabe==0.4.0 --hash=sha256:cbc2938f6c01061bc6d21d0c838c2489664755cb18676f0734d7617f4577d09e
> +pep8==1.7.0 --hash=sha256:4fc2e478addcf17016657dff30b2d8d611e8341fac19ccf2768802f6635d7b8a
> +pyflakes==1.2.3 --hash=sha256:e87bac26c62ea5b45067cc89e4a12f56e1483f1f2cda17e7c9b375b9fd2f40da

Please put this file under tools/lint/flake8/flake8_requirements.txt

In the docker file you can include it with the magic "# %include" syntax. This is a special Dockerfile syntax that works in mozilla-central.

::: testing/docker/lint/system-setup.sh:65
(Diff revision 3)
> -pip install --require-hashes -r requirements.txt
> +# update pip at its latest version before installing flake8
> +pip install -U pip

I don't think it's necessary to do this here, pip should already be recent enough. Plus we don't want to hit pypi from automation without using hashes.

::: tools/lint/flake8.lint:121
(Diff revision 3)
> +def update_pip():
> +    """
> +    Try to update pip if it is not already up to date
> +    """
> +    try:
> +        subprocess.check_call(['pip', 'install', '-U', 'pip'])
> +        return True
> +    except subprocess.CalledProcessError as e:
> +        print("Error updating pip: {}".format(e))
> +        return False
> +
> +
> +def reinstall_flake8():
> +    """
> +    Try to install flake8 at the target version, returns True on success
> +    otherwise prints the otuput of the pip command and returns False
> +    """
> +    try:
> +        subprocess.check_call(['pip', 'install', '-U',
> +                               '--require-hashes', '-r'
> +                               FLAKE8_REQUIREMENTS_PATH])
> +        return True
> +    except subprocess.CalledProcessError as e:
> +        print("Error installing flake8 {0}".format(e.output))
> +        return False

You can probably share some of the code here in a `_run_pip()` method that takes the pip command as an argument.
Attachment #8801466 - Flags: review?(ahalberstadt)
Comment hidden (mozreview-request)
Assignee

Comment 19

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review85570

hope to have placed the call in the right place, mach_commands.py:42 inside the lint method, before the call to cli.run
Reporter

Comment 20

3 years ago
mozreview-review
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86042

Thanks for making the changes so quickly yet again, I really appreciate it! For next time, please test out your changes with e.g `mach lint -l flake8 testing/mozbase`.

::: tools/lint/flake8.lint:25
(Diff revision 4)
> +FLAKE8_WRONG_VERSION = """
> +Wrong version of flake8 found, please reinstall it with:
> +
> +    $ pip install -U --require-hashes -r flake8_requirements.txt
> +""".strip()

Unused, please remove.

::: tools/lint/flake8.lint:42
(Diff revision 4)
> +
> +PIP_VERSION_ERROR = """
> +It seems that pip is not working properly, please reinstall it
> +"""
> +
> +FLAKE8_REQUIREMENTS_PATH = 'testing/docker/link/flake8/flake8_requirements.txt'

This needs to be updated. A convention we often use is to define a `here` variable like so:

    here = os.path.abspath(os.path.dirname(__file__))
    FLAKE8_REQUIREMENTS_PATH = os.path.join(here, 'flake8', 'flake8_requirements.txt')

::: tools/lint/flake8.lint:126
(Diff revision 4)
> -        print(FLAKE8_NOT_FOUND)
> +def _run_pip(*args):
> +    """
> +    Helper function that runs pip with subprocess
> +    """
> +    try:
> +        subprocess.check_call(['pip'].extend(args))

extend() returns None, so this is an Exception. You can use ['pip'] + args instead.

::: tools/lint/flake8.lint:126
(Diff revision 4)
> +        subprocess.check_call(['pip'].extend(args))
> +        return True
> +    except subprocess.CalledProcessError as e:
> +        print("Error running pip: {}".format(e))
> +        return False

I think we should avoid printing stdout unless the command fails. You can use `subprocess.check_output` to accomplish this (it'll return output as a string).

::: tools/lint/flake8.lint:137
(Diff revision 4)
> +    if not _run_pip('install', '-U', 'pip'):
> +        return True
> +    else:
> +        print("Error updating pip")
> +        return False

You have your True/False mixed up here. `_run_pip` returns True on success. I might actually consider getting rid of `update_pip` and `reinstall_flake8` as they don't really do much. You can just inline them into the main function.

::: tools/lint/flake8.lint:159
(Diff revision 4)
> +    if not update_pip():
> +        print(PIP_VERSION_ERROR)
> +        return []

I think it would be better to only run this if we need to. I.e only if installing flake8 fails. Then we could try again.

::: tools/lint/flake8.lint:165
(Diff revision 4)
> +        print(PIP_VERSION_ERROR)
> +        return []
> +
> +    if not reinstall_flake8():
> +        print(FLAKE8_INSTALL_ERROR)
>          return []

Return 1 here as well.

::: tools/lint/flake8.lint:168
(Diff revision 4)
> +    if binary is None:
> +        # binary not found, probably the installation failed for some reasons
> +        print(FLAKE8_NOT_FOUND)
> +        return 1

Theoretically this should never happen, right? Either the install is successful and we know flake8 is here, or else we've already returned. I think you can omit this check.
Assignee

Comment 21

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86042

> I think we should avoid printing stdout unless the command fails. You can use `subprocess.check_output` to accomplish this (it'll return output as a string).

check_call should check the exit status of the executable and since pip install returns 0 even if the required package is already installed (tested with pip install <something-already-there> & echo $?) I think that print will happen only if pip had some problems reporting an exit status != 0
Assignee

Comment 22

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86042

> You have your True/False mixed up here. `_run_pip` returns True on success. I might actually consider getting rid of `update_pip` and `reinstall_flake8` as they don't really do much. You can just inline them into the main function.

even if those functions are trivial I'd like to keep them in order to make the code more clear and to show its intents, let me know if you want to delete those functions anyway
Comment hidden (mozreview-request)
Reporter

Comment 24

3 years ago
mozreview-review
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86998

::: tools/lint/flake8.lint:120
(Diff revision 5)
> +        subprocess.check_call(['pip'] + list(args))
> +        return True
> +    except subprocess.CalledProcessError as e:
> +        print("Error running pip: {}".format(e))
> +        return False

This still needs to use check_output, and only print it to stdout if something went wrong.

::: tools/lint/flake8.lint:127
(Diff revision 5)
> +def update_pip():
> +    """
> +    Try to update pip if it is not already up to date
> +    """
> +    if _run_pip('install', '-U', 'pip'):
> +        return True
> +    else:
> +        print("Error updating pip")
> +        return False

I'm really sorry to back track here, but I've changed my mind about automatically upgrading pip. I know I told you to do this earlier..

But I think going ahead and upgrading automatically might be out of scope for running the flake8 linter. I think it might be better to just print an error message about pip > 8.0 being required. The error message should instruct the user how to upgrade too.
Assignee

Comment 25

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86998

> This still needs to use check_output, and only print it to stdout if something went wrong.

Now I understand what you mean, will be fixed in the next push.

> I'm really sorry to back track here, but I've changed my mind about automatically upgrading pip. I know I told you to do this earlier..
> 
> But I think going ahead and upgrading automatically might be out of scope for running the flake8 linter. I think it might be better to just print an error message about pip > 8.0 being required. The error message should instruct the user how to upgrade too.

Ok, not a problem :) how do you think this should be implemented?
- check pip version and report an error message; and in the message should I instruct the user how to activate the virtualenv created at setup time?
- run pip install --require-hash and see if this fails? how can I distinguish between a "non supported parameter error" and every other error? (for example the network is down)

Thanks for your help
Reporter

Comment 26

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86998

> Ok, not a problem :) how do you think this should be implemented?
> - check pip version and report an error message; and in the message should I instruct the user how to activate the virtualenv created at setup time?
> - run pip install --require-hash and see if this fails? how can I distinguish between a "non supported parameter error" and every other error? (for example the network is down)
> 
> Thanks for your help

I just tested it out and it looks like pip will print "no such option: --require-hashes" if it's missing:

    $ pip install --foobar
    
    Usage:   
      pip install [options] <requirement specifier> [package-index-options] ...
      pip install [options] -r <requirements file> [package-index-options] ...
      pip install [options] [-e] <vcs project url> ...
      pip install [options] [-e] <local project path> ...
      pip install [options] <archive url/path> ...

    no such option: --foobar
    
So I think we can either,

A) Search for "no such option: --require-hashes" in the output, or
B) Just print the error above and not worry about the outdated pip case (which shouldn't happen in mozilla-central anyway)

What do you think is better?
Assignee

Comment 27

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review86998

> I just tested it out and it looks like pip will print "no such option: --require-hashes" if it's missing:
> 
>     $ pip install --foobar
>     
>     Usage:   
>       pip install [options] <requirement specifier> [package-index-options] ...
>       pip install [options] -r <requirements file> [package-index-options] ...
>       pip install [options] [-e] <vcs project url> ...
>       pip install [options] [-e] <local project path> ...
>       pip install [options] <archive url/path> ...
> 
>     no such option: --foobar
>     
> So I think we can either,
> 
> A) Search for "no such option: --require-hashes" in the output, or
> B) Just print the error above and not worry about the outdated pip case (which shouldn't happen in mozilla-central anyway)
> 
> What do you think is better?

Since we are activating the virtualenv created at setup time I think that we are good to go with option B; in this case I think that we can now completely remove the update_pip function as requested before
Comment hidden (mozreview-request)
Reporter

Comment 29

3 years ago
mozreview-review
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review87092

Thanks, this looks great! Just fix the last couple comments, and when I see a green task in the try run I'll land it :).

::: tools/lint/flake8.lint:132
(Diff revisions 5 - 6)
>      """
>      if _run_pip('install', '-U',
>                  '--require-hashes', '-r',
>                  FLAKE8_REQUIREMENTS_PATH):
>          return True
>      else:

nit: remove else statement

::: tools/lint/flake8.lint:28
(Diff revision 6)
>  
>  
> +FLAKE8_INSTALL_ERROR = """
> +Unable to install correct version of flake8
> +Try to install it manually with:
> +    $ pip install -U --require-hashes -r flake8/flake8_requirements.txt

Should probably print the full `FLAKE8_REQUIREMENTS_PATH` here. Users probably aren't running the command from "tools/lint" and this way we don't have to modify the error message if the file changes locations.

::: tools/lint/flake8.lint:116
(Diff revision 6)
> +        out = subprocess.check_output(['pip'] + list(args))
> +        return True
> +    except subprocess.CalledProcessError as e:
> +        print(out)
> +        return False

If subprocess raises here, out will not get set. It looks like the exception object should have a .output attribute though according to:
https://docs.python.org/2/library/subprocess.html#subprocess.check_output

Also, please add stderr=subprocess.STDOUT here just so stderr gets mingled in with stdout where it's supposed to.
Attachment #8801466 - Flags: review?(ahalberstadt) → review+
Comment hidden (mozreview-request)
Reporter

Comment 31

3 years ago
mozreview-review
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review87440

::: tools/lint/flake8.lint:116
(Diff revisions 6 - 7)
>  def _run_pip(*args):
>      """
>      Helper function that runs pip with subprocess
>      """
>      try:
> -        out = subprocess.check_output(['pip'] + list(args))
> +        out = subprocess.check_output(['pip'] + list(args),

nit: this will be a lint error I think, unused variable.
Comment hidden (mozreview-request)
Assignee

Comment 33

3 years ago
mozreview-review-reply
Comment on attachment 8801466 [details]
Bug 1285555 - [flake8] Automatically install/update flake8 package to correct version

https://reviewboard.mozilla.org/r/86216/#review87440

> nit: this will be a lint error I think, unused variable.

You are right, thanks for reporting this :)

Comment 34

3 years ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ced81cfb02f1
[flake8] Automatically install/update flake8 package to correct version r=ahal
Reporter

Comment 35

3 years ago
Thanks again for your contribution! You'll get another kudos at our next team meeting ;) [1]. Are you interested in finding another bug? If so send me an email (ahalberstadt@mozilla.com) and we can figure out a good next bug for you. Cheers!

[1] https://wiki.mozilla.org/EngineeringProductivity/Meetings/2016-10-31

Comment 36

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ced81cfb02f1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

a year ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.