Status

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: rbartlensky, Assigned: rbartlensky)

Tracking

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

We would like to have infer as an artifact, just like in the case of clang-tidy.
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review262270

Integrating infer into static analysis seems like a good idea and this patch is a good first start. From a high level, this patch looks good. There's just a bunch of paper cuts (a lot of them Python). You can feel free to ignore many of the points of feedback if you don't think it is worth your time to address them. Obviously the thing that matters most is that this works in CI. And I'm pretty sure this patch does work in CI. So all the paper cuts are minor technical debt.

Also, could you please point me at the logs of an infer build? I'd like to spot check it for various things (like it pulling in other 3rd party dependencies).

::: build/build-infer/build-infer.py:1
(Diff revision 1)
> +#!/usr/bin/python2.7

Ideally all new Python code we write for mozilla-central would be Python 3. The exception to this is code that requires dependencies that don't yet work on Python 3. This code has no dependencies outside the standard library. So it should be Python 3.

We have Python 3.5 installed on these build images. And `/usr/bin/python3` points to it. If you port this to Python 3 (I'll leave inline comments about what is needed), the shebang should `/usr/bin/env python3`.

::: build/build-infer/build-infer.py:7
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import os
> +import os.path

Nit: you do not need to import `os.path` if you import `os`.

::: build/build-infer/build-infer.py:16
(Diff revision 1)
> +from contextlib import contextmanager
> +import sys
> +
> +
> +def check_run(args):
> +    print >> sys.stderr, ' '.join(args)

This needs to be `print(' '.join(args), file=sys.stderr)` on Python 3. And the `>>` syntax should be avoided on Python 2 because it isn't forward compatible. On Python 2, you can use `print()`. But unless you use `from __future__ import print_function`, `print()` is limited and doesn't support some arguments.

This issue with `print` occurs multiple times in this file.

::: build/build-infer/build-infer.py:17
(Diff revision 1)
> +    r = subprocess.call(args)
> +    assert r == 0

There exists a `subprcess.check_call()` that raises if the exit code is non-0. In addition, Python 3 has a `subprocess.run()` with a `check=True` argument that does something similar. On Python 3, `subprocess.run()` should probably be used.

::: build/build-infer/build-infer.py:21
(Diff revision 1)
> +def run_in(path, args):
> +    '''
> +    Runs the given commands in the directory specified by <path>.
> +    '''
> +    d = os.getcwd()
> +    print >> sys.stderr, 'cd "%s"' % path
> +    os.chdir(path)
> +    check_run(args)
> +    print >> sys.stderr, 'cd "%s"' % d
> +    os.chdir(d)

`subprocess` functions take a `cwd=<path>` argument that executes a process in a specific directory. Please use that instead of `os.chdir()`.

::: build/build-infer/build-infer.py:62
(Diff revision 1)
> +    updated_env(('NO_CMAKE_STRIP', '1'))
> +    run_in(src_dir, ['./build-infer.sh'])
> +    updated_env(('', ''))

`updated_env()` is decorated as a context manager. So this should be:

```
with updated_env({'NO_CMAKE_STRIP', '1'}):
    run_in(...)
```

That being said, I'm not sure if you need the context manager at all. The `subprocess` functions take an `env` argument that defines the environment variables for the process. You can then do something like:

```
def run(..., extra_env=None):
   env = dict(os.environ)
   env.update(extra_env or {})
   
   subprocess.call(..., env=env)
```
```

::: build/build-infer/build-infer.py:76
(Diff revision 1)
> +
> +if __name__ == '__main__':
> +    # The directories end up in the debug info, so the easy way of getting
> +    # a reproducible build is to run it in a know absolute directory.
> +    # We use a directory that is registered as a volume in the Docker image.
> +    base_dir = '/builds/worker/workspace/moz-toolchain'

Hardcoding this path means it is difficult to run this locally. Consider deriving paths from `cwd` or passing in a base build path via an argument.

::: build/build-infer/build-infer.py:77
(Diff revision 1)
> +    infer_dir = base_dir + '/infer'
> +    source_dir = infer_dir + '/src'
> +    build_dir = infer_dir + '/build'

Always use `os.path.join()` or the `/` operator on `pathlib.Path` instances for portability. This also does sane things when paths end with path separator characters.

::: build/build-infer/build-infer.py:85
(Diff revision 1)
> +    parser.add_argument('--clean', required=False,
> +                        action='store_true',
> +                        help='Clean the build directory')
> +    parser.add_argument('--skip-tar', required=False,

Nit: `required=False` is the default and doesn't need to be specified.

::: build/build-infer/build-infer.py:98
(Diff revision 1)
> +    mkdir(base_dir)
> +    mkdir(infer_dir)
> +    mkdir(build_dir)

On Python 3, use `os.mkdirs(path, exist_ok=True)`.

This will create missing parent directories. And it won't raise an error if the directory already exists.

::: build/build-infer/build-infer.py:107
(Diff revision 1)
> +    for p in config.get('patches', []):
> +        run_in(source_dir, ['git', 'apply', dir_path+p])

You may want to run a `git reset` + `git clean` to ensure the working directory is in a clean state. In CI this shouldn't matter. But it will help anybody testing things locally.

::: build/build-infer/build-infer.py:109
(Diff revision 1)
> +        git_clone(infer_dir, infer_repo, source_dir, infer_revision)
> +    # apply a few patches
> +    dir_path = os.path.dirname(os.path.realpath(__file__)) + '/'
> +    for p in config.get('patches', []):
> +        run_in(source_dir, ['git', 'apply', dir_path+p])
> +        run_in(source_dir, ['chmod', 'u+x', 'build-infer.sh'])

This wouldn't be needed if `build.patch` didn't change the mode of the file.

::: taskcluster/ci/toolchain/linux.yml:138
(Diff revision 1)
> +    treeherder:
> +        kind: build
> +        platform: toolchains/opt
> +        symbol: TL(infer)
> +        tier: 1
> +    worker-type: aws-provisioner-v1/gecko-{level}-b-linux-large

This worker type is a ~32 core machine. Can you point me to the logs of a task so I can assess whether a more expensive worker is needed? Alternatively, you could preemptively drop the `-large` and have it use the same sized machines as other build tasks.

::: taskcluster/ci/toolchain/linux.yml:149
(Diff revision 1)
> +    run-on-projects:
> +        - try

This should not be needed.
Attachment #8990385 - Flags: review?(gps) → review-
Thank you so much for the comments, I will fix these as soon as I can!

Some of the issues you mentioned are present in `build/build-clang/build-clang.py` as well. Should I attempt to fix them there as well? If so, should I open a separate bug, or fix all issues in this one?
If the issues are present in `build-clang.py`, then they may be worth ignoring :)

It would be nice to port to Python 3 though. As any new Python code should be Python 3 to avoid technical debt. I'm pretty sure the only major Python 3 issue is the use of `print <<`. Everything else should *knocks on wood* just work.
(In reply to Gregory Szorc [:gps] from comment #4)
> If the issues are present in `build-clang.py`, then they may be worth
> ignoring :)

Ok, thank you for letting me know!

> It would be nice to port to Python 3 though. As any new Python code should
> be Python 3 to avoid technical debt. I'm pretty sure the only major Python 3
> issue is the use of `print <<`. Everything else should *knocks on wood* just
> work.

I agree. I was using Python 2 because I was following the implementation of `build-clang.py`.
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review262270

> Hardcoding this path means it is difficult to run this locally. Consider deriving paths from `cwd` or passing in a base build path via an argument.

I am not sure how to get this path in another way, but as far as I know only taskcluster executes this script, not the user.

> This wouldn't be needed if `build.patch` didn't change the mode of the file.

I will have to look some more into this.
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review266226

This looks mostly good. But I'm pretty sure it will fail in CI. I couldn't find a Try push of yours testing this since early July. Please verify it works on Try and then resubmit for an informal rubber stamp r+.

::: taskcluster/scripts/misc/build-infer-linux.sh:17
(Diff revision 2)
> +# gets a bit too verbose here
> +set +x
> +
> +cd build/build-infer
> +# |mach python| sets up a virtualenv for us!
> +../../mach python ./build-infer.py -c infer-linux64.json

Does this actually work? `build-infer.py` is a Python 3 script and is using Python 3 features. `mach` uses Python 2 `mach python` invokes the process with the same Python interpreter used to invoke `mach` IIRC. So I would expect this to fail.

You shouldn't need to use `mach` at all here. Just invoke `build-infer.py` and the shabang should find and use Python 3.
Attachment #8990385 - Flags: review?(gps) → review-
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review266720

This looks good to me. I still don't see a recent try push for this. I'd strongly encourage you to send this through Try before landing.
Attachment #8990385 - Flags: review?(gps) → review+
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review266748

::: build/build-infer/build-infer.py:92
(Diff revision 5)
> +    # apply a few patches
> +    dir_path = os.path.dirname(os.path.realpath(__file__))
> +    # clean the git directory by reseting all changes
> +    git_commands = [['clean', '-f'], ['reset', '--hard'],
> +                    ['submodule', 'foreach', 'git', 'reset', '--hard']]
> +    for command in git_commands:

This most likely will fails, since we are not in a git repo just yet.
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review266754

::: build/build-infer/build-infer.py:8
(Diff revision 5)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import os
> +import subprocess
> +import json

also import functools?

::: build/build-infer/build-infer.py:67
(Diff revision 5)
> +    # a reproducible build is to run it in a know absolute directory.
> +    # We use a directory that is registered as a volume in the Docker image.
> +    if args.base_dir:
> +        base_dir = args.base_dir
> +    else:
> +        base_dir = reduce(os.path.join,

I think in python3 reduce is part of functools?
Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

https://reviewboard.mozilla.org/r/255472/#review266940

::: build/build-infer/build-infer.py:8
(Diff revision 5)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import os
> +import subprocess
> +import json

You are right, functools is needed. I tested it in a python interpreter and it doesn't work without functools, but for some reason when I ran the script yesterday all went well...

::: build/build-infer/build-infer.py:92
(Diff revision 5)
> +    # apply a few patches
> +    dir_path = os.path.dirname(os.path.realpath(__file__))
> +    # clean the git directory by reseting all changes
> +    git_commands = [['clean', '-f'], ['reset', '--hard'],
> +                    ['submodule', 'foreach', 'git', 'reset', '--hard']]
> +    for command in git_commands:

Lines 85-86 clonde the repository, I could add a check like:

`if not os.path.exists(source_dir) or not is_git_repo(source_dir): clone...` 

The above makes sure that the following git commands will always execute correctly
Please also try a static-analysis-autotest job since that one depends on the infer toolchain.
Flags: needinfo?(rbartlensky)
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #19)
> Please also try a static-analysis-autotest job since that one depends on the
> infer toolchain.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6509be1d2a37cf27988cd8be6b2c602f7e2eceea&selectedJob=190526791

Passed
Flags: needinfo?(rbartlensky)
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e7b746dfdcf
Add infer to taskcluster and build. r=gps
https://hg.mozilla.org/mozilla-central/rev/2e7b746dfdcf
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.