Closed Bug 1467234 Opened 6 years ago Closed 6 years ago

Linting a directory behaves differently from a file (at least for cpp-virtual-final)

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

3 Branch
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1494069

People

(Reporter: Mardak, Assigned: ahal)

References

Details

Attachments

(2 obsolete files)

In bug 1440421, we ended up adding a cpp-virtual-final.yml exclude of `- '**/node_modules'` because `- browser/extensions/activity-stream/node_modules` didn't seem to work.

With the other bug (which mirrors activity-stream's git repository into mozilla-central), one could develop activity stream directly from mozilla-central by first running `npm install` from `browser/extensions/activity-stream`

Turns out excluding the full path was actually working, but ./mach lint <parent directory> wasn't working.

Below are the results of setting various exclude values in tools/lint/cpp-virtual-final.yml then running on the directory vs file:

$ ./mach lint browser/extensions/activity-stream/
$ ./mach lint browser/extensions/activity-stream/node_modules/nan/nan.h 

```
exclude: - browser/extensions/activity-stream/node_modules

✖ 1 problem (0 errors, 1 warning)
✖ 0 problems (0 errors, 0 warnings)
```

```
exclude: - browser/extensions/activity-stream

✖ 0 problems (0 errors, 0 warnings)
✖ 0 problems (0 errors, 0 warnings)
```

```
exclude: - node_modules

✖ 0 problems (0 errors, 0 warnings)
✖ 1 problem (0 errors, 1 warning)
```

```
exclude: - '**/node_modules'

✖ 0 problems (0 errors, 0 warnings)
✖ 0 problems (0 errors, 0 warnings)
```
Thanks for filing, that investigation is really useful! The path filtering logic is too complicated right now.
See Also: → 1448417
I actually ran into this as well, and came up with a fix.

However I have several major(ish) mozlint changes in the process of landing. I'd like to get those finished and shore up some tests before landing the fix for this. But hopefully I'll have this done soon.
Assignee: nobody → ahal
Status: NEW → ASSIGNED
Depends on: 1486866
This will make it easier to add new test cases in the future. It'll
also make it easier to print debug, as only the output for each
specific test case will be printed on failure.
Currently pathutils.filterpaths computes some extra paths that the underlying linter
should try to exclude if it can. It sets these on lintargs['exclude'], and relies on
the fact that it was passed by reference to work.

However, it seems like pytest does some magic under the hood that prevents this value
from being propagated back. It will also fail if 'filterpaths' was invoked in a
subprocess.

It's much simpler and cleaner to pass this value back directly, rather than rely on a
reference.

Depends on D5862
Attachment #9009108 - Attachment is obsolete: true
Attachment #9009109 - Attachment is obsolete: true
This is going to be fixed by bug 1494069
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: