1444167 - Create a mozlint job that will alert on file capitalization that is likely to break the MinGW build

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Tom Ritter [:tjr]]

See https://groups.google.com/d/msg/mozilla.dev.platform/r3mYWbb42pM/Wn51RneSAgAJ

We would like to make it less annoying for users developing for Windows to accommodate the MinGW build target.  In particular it should be 'easy' to lint for includes that are likely to break the MinGW build.

I don't know the best way to accomplish this task, but this seems to be one way to do it is with a linter, so I tried that first.

Comment 1

[Tom Ritter [:tjr]]

On my xeon tower machine, the attached lint job runs in ~4 minutes.

Comment 2

[Tom Ritter [:tjr]]

Attachment: Bug 1444167 Add a MinGW Header Capitalization lint job

MinGW headers are all lowercase, and because we build FF with MinGW
on Linux - capitalization matters.  But if you're plugging along,
working on Windows, you might forget about that, and wind up
inadvertently breaking the MinGW build because you capitalized
an include file (as is common in Windows development.)

This lint job takes the list of header files from MinGW
(generated by cd mingw-w64/mingw-w64-headers &&
  find . -name "*.h" | xargs -I bob -- basename bob | sort | uniq)
and will alert if they are in an #include statement but not
all-lowercase.

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

Comment 3

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 4

[Tom Ritter [:tjr]]

Attachment: Bug 1444167 Add MinGW mozlint build job

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

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 6

[Tom Ritter [:tjr]]

I tweaked it and it finishes much faster now (like 14 seconds)

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

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

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

https://reviewboard.mozilla.org/r/226256/#review232352

I'm unclear on what this lint buys us over the MinGW job just breaking.  Is it that the lint will run everywhere, so it has a higher likelihood of telling people when they're going to break things (e.g. on try, where they might not be running the MinGW build job)?

::: taskcluster/ci/source-test/mozlint.yml:83
(Diff revision 2)
> +mingw:
> +    description: lint for MinGW Capitalization issues
> +    platform: lint/opt
> +    treeherder:
> +        symbol: mingw
> +    run:
> +        mach: lint -l mingw -f treeherder

Nit: I think all of these `mingw` names are better as `mingw-capitalization` or `mingw-cap` if you wanted short names (e.g. for the treeherder symbol).  But I'm not going to insist.

::: taskcluster/ci/source-test/mozlint.yml:92
(Diff revision 2)
> +            - '**/*.cpp'
> +            - '**/*.h'

Is it really only C++ source files that cause issues?  Not C files?  Not `.cc` files that we might get from third parties?  Not Python files?  Not JavaScript files (especially .jsm files)?  Etc. etc...

Comment 10

[Tom Ritter [:tjr]]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> Comment on attachment 8957339 [details]
> Bug 1444167 Add MinGW mozlint build job
> 
> https://reviewboard.mozilla.org/r/226256/#review232352
> 
> I'm unclear on what this lint buys us over the MinGW job just breaking.  Is
> it that the lint will run everywhere, so it has a higher likelihood of
> telling people when they're going to break things (e.g. on try, where they
> might not be running the MinGW build job)?

Correct - feedback was that people weren't and didn't want to run the MinGW build all the time, and wanted a lighter-weight solution.

> Nit: I think all of these `mingw` names are better as `mingw-capitalization`
> or `mingw-cap` if you wanted short names (e.g. for the treeherder symbol). 
> But I'm not going to insist.

Sure.

> ::: taskcluster/ci/source-test/mozlint.yml:92
> (Diff revision 2)
> > +            - '**/*.cpp'
> > +            - '**/*.h'
> 
> Is it really only C++ source files that cause issues?  Not C files?  Not
> `.cc` files that we might get from third parties?  Not Python files?  Not
> JavaScript files (especially .jsm files)?  Etc. etc...

.cc could cause a problem; yes. .c ... maybe - I've never encountered it but I'll add it. This is specific to Windows include files, so py, js, jsm are not relevant to this job.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Tom Ritter [:tjr] from comment #10)
> (In reply to Nathan Froyd [:froydnj] from comment #9)
> > Comment on attachment 8957339 [details]
> > Bug 1444167 Add MinGW mozlint build job
> > 
> > https://reviewboard.mozilla.org/r/226256/#review232352
> > 
> > I'm unclear on what this lint buys us over the MinGW job just breaking.  Is
> > it that the lint will run everywhere, so it has a higher likelihood of
> > telling people when they're going to break things (e.g. on try, where they
> > might not be running the MinGW build job)?
> 
> Correct - feedback was that people weren't and didn't want to run the MinGW
> build all the time, and wanted a lighter-weight solution.

Heh, I don't think that hard about the build jobs I submit (they're just builds!) but OK, sure, I can see value in that.

> > ::: taskcluster/ci/source-test/mozlint.yml:92
> > (Diff revision 2)
> > > +            - '**/*.cpp'
> > > +            - '**/*.h'
> > 
> > Is it really only C++ source files that cause issues?  Not C files?  Not
> > `.cc` files that we might get from third parties?  Not Python files?  Not
> > JavaScript files (especially .jsm files)?  Etc. etc...
> 
> .cc could cause a problem; yes. .c ... maybe - I've never encountered it but
> I'll add it. This is specific to Windows include files, so py, js, jsm are
> not relevant to this job.

OK, that makes sense.  I think I was getting confused about a different issue (file renames that don't actually rename the file on case-insensitive filesystems?) but I think we'd need something different to handle that case.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

OK, let's do this.

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

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

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

https://reviewboard.mozilla.org/r/226198/#review232928

Thanks, this is looking good. No major comments, though enough to warrant a re-review.

::: tools/lint/mingw-capitalization.yml:28
(Diff revision 5)
> +        - toolkit/crashreporter/google-breakpad/src/common/windows
> +        - third_party/python/psutil/psutil
> +    type: external
> +    level: warning
> +    payload: python.mingw-capitalization:lint
> +    setup: python.mingw-capitalization:setup

You can omit this and remove the empty `setup` function.

::: tools/lint/python/mingw-capitalization.py:1
(Diff revision 5)
> +# This Source Code Form is subject to the terms of the Mozilla Public

The `python` directory was meant for linters that *operate* on python files. Maybe you could create a new `cpp` directory and put this in there instead (don't forget to create an empty `__init__.py` file so the directory is recognized as a python module).

::: tools/lint/python/mingw-capitalization.py:29
(Diff revision 5)
> +def lint(paths, config, **lintargs):
> +    results = []
> +
> +    m = MinGWCapitalization()
> +    for path in paths:
> +        results.extend(m._lint(path, config, **lintargs))

Neat, I never considered re-using the other types within an ExternalType.

::: tools/lint/python/mingw-capitalization.py:33
(Diff revision 5)
> +    for path in paths:
> +        results.extend(m._lint(path, config, **lintargs))
> +    return results
> +
> +
> +headers_str = """

Please store this in a file that lives alongside this one. Here's how I'd find it (after imports):

    here = os.path.abspath(os.path.dirname(__file__))
    HEADERS_FILE = os.path.join(here, 'headers.txt')

Then in MinGWCapitalization:

    def __init__(self, *args, **kwargs):
        super(MinGWCapitalization, self).__init__(*args, **kwargs)
        with open(HEADERS_FILE, 'r') as fh:
            self.headers = fh.read().strip().splitlines()

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 17

[Tom Ritter [:tjr]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

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

Comment 18

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

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

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

https://reviewboard.mozilla.org/r/226198/#review233168

::: tools/lint/cpp/mingw-capitalization.py:25
(Diff revision 7)
> +            self.headers = fh.read().strip().splitlines()
> +
> +    def condition(self, payload, line):
> +        if not line.startswith("#include"):
> +            return False
> +        regex = "^#include\s*<(" + "|".join(self.headers) + ")>"

Might as well set `self.regex` in the `__init__` so we aren't building this string each time.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

https://reviewboard.mozilla.org/r/226256/#review233190

Comment 22

[Tom Ritter [:tjr]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

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

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8957273 [details]
Bug 1444167 Add a MinGW Header Capitalization lint job

https://reviewboard.mozilla.org/r/226198/#review233208

::: tools/lint/mingw-capitalization.yml:9
(Diff revision 8)
> +      "A Windows include file is not lowercase, and may break the MinGW build"
> +    extensions: ['h', 'cpp', 'cc', 'c']
> +    include: ['.']
> +    exclude:
> +        # We do not compile WebRTC with MinGW yet
> +        - media/webrtc

Feels like we will be duplicating more and more of in the future:
https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt

Comment 25

[Tom Ritter [:tjr]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Comment on attachment 8957273 [details]
> Bug 1444167 Add a MinGW Header Capitalization lint job
> 
> https://reviewboard.mozilla.org/r/226198/#review233208
> 
> ::: tools/lint/mingw-capitalization.yml:9
> (Diff revision 8)
> > +      "A Windows include file is not lowercase, and may break the MinGW build"
> > +    extensions: ['h', 'cpp', 'cc', 'c']
> > +    include: ['.']
> > +    exclude:
> > +        # We do not compile WebRTC with MinGW yet
> > +        - media/webrtc
> 
> Feels like we will be duplicating more and more of in the future:
> https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/
> ThirdPartyPaths.txt

I imagine parts of it will always wind up being duplicated. Some third party libraries we don't compile with MinGW (yet - WebRTC is in the backlog), most we do. Some libraries only have some compiler warning flags turned on, others turned off...

I'm duplicating all of that over here https://github.com/mozilla-services/third-party-library-alert because I need more metadata than just that file.

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to Tom Ritter [:tjr] from comment #25)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> > Comment on attachment 8957273 [details]
> > Bug 1444167 Add a MinGW Header Capitalization lint job
> > 
> > https://reviewboard.mozilla.org/r/226198/#review233208
> > 
> > ::: tools/lint/mingw-capitalization.yml:9
> > (Diff revision 8)
> > > +      "A Windows include file is not lowercase, and may break the MinGW build"
> > > +    extensions: ['h', 'cpp', 'cc', 'c']
> > > +    include: ['.']
> > > +    exclude:
> > > +        # We do not compile WebRTC with MinGW yet
> > > +        - media/webrtc
> > 
> > Feels like we will be duplicating more and more of in the future:
> > https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/
> > ThirdPartyPaths.txt
> 
> I imagine parts of it will always wind up being duplicated. Some third party
> libraries we don't compile with MinGW (yet - WebRTC is in the backlog), most
> we do. Some libraries only have some compiler warning flags turned on,
> others turned off...
> 
> I'm duplicating all of that over here
> https://github.com/mozilla-services/third-party-library-alert because I need
> more metadata than just that file.

Can I gently nudge towards growing more metadata in mozilla-central, and consuming it from things out of tree, rather than having the consumers own all of the metadata?

We've had success with that approach before.  For example, bits of the build tooling (bootstrap) will fetch a named revision of a file in the tree to get started, etc.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ee257334c7d
Add a MinGW Header Capitalization lint job r=ahal
https://hg.mozilla.org/integration/autoland/rev/0034af9f6246
Add MinGW mozlint build job r=froydnj

Comment 28

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6ee257334c7d
https://hg.mozilla.org/mozilla-central/rev/0034af9f6246