1473951 - Add infer as an artifact

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Robert Bartlensky [:rbartlensky]]

We would like to have infer as an artifact, just like in the case of clang-tidy.

Comment 1

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1473951: Add infer to taskcluster and build.

Review commit: https://reviewboard.mozilla.org/r/255472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255472/

Comment 2

[Gregory Szorc [:gps]]

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.

Comment 3

[Robert Bartlensky [:rbartlensky]]

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?

Comment 4

[Gregory Szorc [:gps]]

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.

Comment 5

[Robert Bartlensky [:rbartlensky]]

(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 6

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/1-2/

Comment 7

[Robert Bartlensky [:rbartlensky]]

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 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/2-3/

Comment 10

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/3-4/

Comment 11

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/4-5/

Comment 12

[Gregory Szorc [:gps]]

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.

Comment 13

[Andi [:andi]]

Let's see how this is going: https://tools.taskcluster.net/groups/NuqmbXgFS-uljiV3-Q6B5g/tasks/LSNCnKagSSafgyzV2fGBzA/details

Comment 14

[Andi [:andi]]

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 15

[Andi [:andi]]

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 16

[Robert Bartlensky [:rbartlensky]]

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

Comment 17

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/5-6/

Comment 18

[Robert Bartlensky [:rbartlensky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60d88bc65582f6ef83c7b92f564179c0f6cd3c27

Now infer builds successfully!

Comment 19

[Andi [:andi]]

Please also try a static-analysis-autotest job since that one depends on the infer toolchain.

Comment 20

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/6-7/

Comment 21

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/7-8/

Comment 22

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8990385 [details]
Bug 1473951: Add infer to taskcluster and build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255472/diff/8-9/

Comment 23

[Robert Bartlensky [: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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7b746dfdcf
Add infer to taskcluster and build. r=gps

Comment 25

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2e7b746dfdcf