Closed Bug 1444167 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(2 files)

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.
On my xeon tower machine, the attached lint job runs in ~4 minutes.
I tweaked it and it finishes much faster now (like 14 seconds)
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...
(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.
(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 on attachment 8957339 [details]
Bug 1444167 Add MinGW mozlint build job

OK, let's do this.
Attachment #8957339 - Flags: review?(core-build-config-reviews) → review+
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()
Attachment #8957273 - Flags: review?(ahalberstadt) → review-
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.
Attachment #8957273 - Flags: review?(ahalberstadt) → review+
Attachment #8957339 - Flags: review?(core-build-config-reviews)
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
(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.
(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.
Pushed by rgurzau@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ee257334c7d
https://hg.mozilla.org/mozilla-central/rev/0034af9f6246
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.