1405554 - mach clang-format should download from toolchain artifacts instead of tooltool

Status: RESOLVED FIXED

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

Description

[Mike Hommey [:glandium]]

See what mach static-analysis does.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Mike, I guess that also involves building it on the CI.
I should just add a build_clang_format option?
https://dxr.mozilla.org/mozilla-central/source/build/build-clang/build-clang.py?q=build%2Fbuild-clang%2Fbuild-clang.py&redirect_type=direct#579

Comment 2

[Mike Hommey [:glandium]]

Just download clang.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

This is version 3.9, too old for what we need... We need 5.0

Comment 4

[Mike Hommey [:glandium]]

Just add jobs for clang 5, and use that.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

We are already downloading clang-tidy here:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py?#1837

which now contains clang-format (thanks to bug 1414781)

The download stuff should be probably extracted into a dedicated object/method/functions.

Most of the code in this function:
https://dxr.mozilla.org/mozilla-central/source/tools/mach_commands.py#224
will be done.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Andi will take care of that! I dropped the ball on this.

Comment 7

[Andi [:andi]]

Attachment: Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

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

Comment 8

[Andi [:andi]]

This is wip, but i think it builds a fairly good idea where we're heading with this. A problem we must overcome is that the current clang-format version doesn't support some arguments present in our .clang-format like:

>>  SplitEmptyFunction: true

Introduced here: https://reviews.llvm.org/D34395

In order to overcome this i think we should update our clang-tidy target to a more modern clang trunk version.

Comment 9

[Andi [:andi]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

https://reviewboard.mozilla.org/r/212418/#review220150

::: commit-message-d5f42:1
(Diff revisions 1 - 2)
> -Bug 1405554 - Unifi clang-format with clang-tidy under clang-tools in mach.
> +Bug 1405554 - Unifi clang-format with clang-tidy under ClangTools in order to use the same clang package from toolchains.

Unify? Or Merge?

::: python/mozbuild/mozbuild/mach_commands.py:1295
(Diff revisions 1 - 2)
>              def __init__(self, task_id, artifact_name):
>                  cot = cache._download_manager.session.get(
>                      get_artifact_url(task_id, 'public/chainOfTrust.json.asc'))
> +                cot.raise_for_status()
>                  digest = algorithm = None
> -                if cot.status_code == 200:
> +                data = {}

Why did you remove that?
(code == 200)?

::: python/mozbuild/mozbuild/mach_commands.py:1572
(Diff revision 2)
>              return (warning, False)
>          return (warning, True)
>  
>  
>  @CommandProvider
> -class StaticAnalysis(MachCommandBase):
> +class ClangTools(MachCommandBase):

I think we should keep a more generic naming.
What if we integrate infer?
We would still be used StaticAnalysis but ClangTools would be wrong?

::: python/mozbuild/mozbuild/mach_commands.py:1679
(Diff revision 2)
>                       {'count': len(monitor.warnings_db)},
>                       '{count} warnings present.')
>              return rc
>  
>      @StaticAnalysisSubCommand('static-analysis', 'install',
> -                              'Install the static analysis helper tool')
> +                              'Install clang-tools helper tool for code static-analysis and format')

Ditto
Not sure we should making product specific?

Of course, I am only talking about user facing stuff.

::: python/mozbuild/mozbuild/mach_commands.py:1736
(Diff revision 2)
> +            return rc
> +
> +        if path is None:
> +            return self._run_clang_format_diff(self._clang_format_diff, show)
> +        else:
> +            return self._run_clang_format_path(self._clang_format_path, show, path)

Just this declaration should be enough?

Comment 11

[Andi [:andi]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

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

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

https://reviewboard.mozilla.org/r/212418/#review221024

Overall this looks pretty good! It needs some hopefully minor work before check-in though.

::: python/mozbuild/mozbuild/mach_commands.py:1953
(Diff revision 3)
> +        diff_process = Popen(self._get_clang_format_diff_command(), stdout=PIPE)
> +        args = [sys.executable, clang_format_diff, "-p1"]
> +        if not show:
> +            args.append("-i")
> +        cf_process = Popen(args, stdin=diff_process.stdout)
> +        return cf_process.communicate()[0]

This function isn't handling the exit code. So it could fail and lead to badness.

This could probably switch to `subprocess.check_output` instead of `Popen()` + `communicate()`.

::: python/mozbuild/mozbuild/mach_commands.py:1957
(Diff revision 3)
> +        match_f = f.split(self.topsrcdir + '/', 1)
> +        match_f = match_f[1] if len(match_f) == 2 else match_f[0]
> +        return re.match(ignored_dir_re, match_f)

Using `str.split()` to strip a path prefix is not the best way to do this. Could you use `startswith()` instead? That only works if the paths are normalized though. They should be.

::: python/mozbuild/mozbuild/mach_commands.py:1962
(Diff revision 3)
> +        match_f = f.split(self.topsrcdir + '/', 1)
> +        match_f = match_f[1] if len(match_f) == 2 else match_f[0]
> +        return re.match(ignored_dir_re, match_f)
> +
> +    def _generate_path_list(self, paths):
> +        pathToThirdparty = os.path.join(self.topsrcdir, self._format_ignore_file)

Nit: use snake_case for variable names

::: python/mozbuild/mozbuild/mach_commands.py:1964
(Diff revision 3)
> +        return re.match(ignored_dir_re, match_f)
> +
> +    def _generate_path_list(self, paths):
> +        pathToThirdparty = os.path.join(self.topsrcdir, self._format_ignore_file)
> +        ignored_dir = []
> +        for line in open(pathToThirdparty):

Nit: always specify file open mode for explicitness.

Also, always use context managers for `open()`. e.g.

```
with open(path_to_third_party, 'r') as fh:
    for line in fh:
        line = line.rstrip()
        ...
```

::: python/mozbuild/mozbuild/mach_commands.py:1969
(Diff revision 3)
> +        for line in open(pathToThirdparty):
> +            # Remove comments and empty lines
> +            if line.startswith('#') or len(line.strip()) == 0:
> +                continue
> +            # The regexp is to make sure we are managing relative paths
> +            ignored_dir.append("^[\./]*" + line.rstrip())

`\` is an escape character in Python strings. This should either be using r'' strings to disable that functionality. Or literal `\` should be written as `\\`.

FWIW, the behavior of backslash + a character is a bit wonky. Some (like `.`) get preserved as that ascii byte. Others (like `v`) are interpreted specially.

::: python/mozbuild/mozbuild/mach_commands.py:1969
(Diff revision 3)
> +        for line in open(pathToThirdparty):
> +            # Remove comments and empty lines
> +            if line.startswith('#') or len(line.strip()) == 0:
> +                continue
> +            # The regexp is to make sure we are managing relative paths
> +            ignored_dir.append("^[\./]*" + line.rstrip())

Since entries in the line get turned into regular expressions, if they are not regular expressions already, then `re.escape()` should be used to escape string literals into regular expressions, without allowing characters in those string literals to be interpreted as special characters in the regular expression.

::: python/mozbuild/mozbuild/mach_commands.py:2018
(Diff revision 3)
> +        for i in range(0, len(path_list), batchsize):
> +            l = path_list[i: (i + batchsize)]
> +            # Run clang-format on the list
> +            cf_process = Popen(args + l)
> +            # Wait until the process is over
> +            cf_process.communicate()[0]

We never check the exit code. Again, consider using `subprocess.check_output()` here.

::: python/mozbuild/mozbuild/mach_commands.py:2027
(Diff revision 3)
> +            if self.repository.name == 'hg':
> +                cf_process = Popen(["hg", "diff"] + paths)
> +            else:
> +                assert self.repository.name == 'git'
> +                cf_process = Popen(["git", "diff"] + paths)
> +            cf_process.communicate()[0]

Exit code not checked.

Comment 13

[Andi [:andi]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

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

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

https://reviewboard.mozilla.org/r/212418/#review222478

This is much, much better than the first version I reviewed. Just a few more places lacking exit code checks though. I'm optimistic it will get r+ on the next iteration!

::: python/mozbuild/mozbuild/mach_commands.py:1949
(Diff revision 4)
> +        diff_process = Popen(self._get_clang_format_diff_command(), stdout=PIPE)
> +        args = [sys.executable, clang_format_diff, "-p1"]
> +        if not show:
> +            args.append("-i")
> +        try:
> +            return check_output(args, stdin=diff_process.stdout)

This exits without waiting for or checking the result code of `diff_process`. The variable does go out of scope here, and changing its refcount to 0 will cause it to terminate the process. But this should be done explicitly to ensure it happens when you want it to. And so you can report any errors.

::: python/mozbuild/mozbuild/mach_commands.py:2024
(Diff revision 4)
> +            try:
> +                check_output(args + l)
> +            except CalledProcessError:
> +                # Something wrong happend
> +                print("clang-format: An error occured while running clang-format.")
> +                return

This returns None, which callers will interpret as a 0 exit code. That doesn't feel right. This should return the `returncode` attribute of the caught `CalledProcessError` instance. e.g.

```
except CalledProcessError as e:
    ....
    return e.returncode
```

::: python/mozbuild/mozbuild/mach_commands.py:2036
(Diff revision 4)
> +            except CalledProcessError:
> +                # Something wrong happend
> +                print("clang-format: Unable to run the diff command.")

Same issue here with regards to return code.

Comment 15

[Andi [:andi]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

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

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

https://reviewboard.mozilla.org/r/212418/#review224548

Looks good!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/773a25b97a66
Merge clang-format with clang-tidy under the same package from toolchains. r=gps

Comment 18

[Eliza Balazs [:ebalazs_]]

Backed out changeset 773a25b97a66 (bug 1405554) for f8 lint failure in /builds/worker/checkouts/gecko/tools/mach_commands.py

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=773a25b97a66150dfb90bffcb70a6a95376ca6c0&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=161121366

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=161121366&repo=autoland&lineNumber=234

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=024499d792bb45e8c8602ffbbebf1d50e9e6a22f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Comment 19

[Andi [:andi]]

Comment on attachment 8942150 [details]
Bug 1405554 - Merge clang-format with clang-tidy under the same package from toolchains.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/752a687900e2
Merge clang-format with clang-tidy under the same package from toolchains. r=gps

Comment 21

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/752a687900e2