[mozlint] Properly normalize all specified paths and ensure they are absolute

RESOLVED FIXED in Firefox 51

Status

Testing
Lint
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

a year ago
This is needed so A) mozlint will find the right files even if odd combinations of relative paths and cwd are specified, and B) so lint results all show up under the right file (as opposed to some showing up under the absolute path, and others showing up under the relative path).

This also blocks bug 1288425 as it fixes some test failures there.
(Assignee)

Comment 1

a year ago
Created attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

It's important to use absolute paths so lint errors for the same files don't show up in two
different places. For example, eslint will absolutize a relative path, whereas flake8 will
not.

We can't just call os.path.abspath here because that will join the relative path to
os.getcwd(), which will be incorrect if the developer runs |mach lint| from anywhere other
than topsrcdir. This patch will walk os.getcwd()'s ancestors and attempt to join until the
path is valid.

Review commit: https://reviewboard.mozilla.org/r/67438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67438/
(Assignee)

Updated

a year ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/67438/#review64502

::: python/mozlint/mozlint/pathutils.py:26
(Diff revision 1)
> +        path = newpath
> +        count += 1
> +
> +
> +def normalize(path):
> +    if os.path.isabs(path) or os.path.exists(path):

hmm, I feel like this `exists` check could cause a false positive.

Imagine the following directory structure
```
top-level
  A
    B
      A
        B
```

Assume the cwd is `top-level/A/B/` and the specified path is `A/B/`, in this case:
  `os.path.exists('A/B/') == True`
  `os.path.abspath('A/B')` is `/path/to/top-level/A/B/A/B`

But the desired path is actually `/path/to/top-level/A/B`

What you actually need is a way to find the intended top level directory all the relative paths are based on. Maybe using hg or git commands based on the repo we're in? (e.g. `hg root`)?

::: python/mozlint/mozlint/pathutils.py:27
(Diff revision 1)
> +        count += 1
> +
> +
> +def normalize(path):
> +    if os.path.isabs(path) or os.path.exists(path):
> +        return os.path.normpath(os.path.abspath(path))

the call to `normpath` is redundant here, `abspath` should return a normalized path.
(Assignee)

Comment 3

a year ago
Firstly this patch looks like it still causes test failures on windows, so I have a bit more work to do anyway.

I'm not sure I agree that the desired path in that case would be /path/to/top-level/A/B, it's a bit ambiguous. For example say I run:

$ cd mozilla-central/testing
$ mach lint mochitest
$ mach lint testing/mochitest

I'd like both of those paths to be valid. It's important to support relative paths from CWD otherwise developers will get confused and angry, and it's also important to support relative paths from topsrcdir so that we can hardcode them in configs and so developers can re-use their shell history.
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review64502

> hmm, I feel like this `exists` check could cause a false positive.
> 
> Imagine the following directory structure
> ```
> top-level
>   A
>     B
>       A
>         B
> ```
> 
> Assume the cwd is `top-level/A/B/` and the specified path is `A/B/`, in this case:
>   `os.path.exists('A/B/') == True`
>   `os.path.abspath('A/B')` is `/path/to/top-level/A/B/A/B`
> 
> But the desired path is actually `/path/to/top-level/A/B`
> 
> What you actually need is a way to find the intended top level directory all the relative paths are based on. Maybe using hg or git commands based on the repo we're in? (e.g. `hg root`)?

I think I disagree that /path/to/top-level/A/B is the desired path in that scenario.

Another way of phrasing it would be, if a relative path is valid both from $CWD and topsrcdir, which one takes precedence? I think $CWD should take precedence. Otherwise a developer could run into a situation where they tab complete to a subdirectory, and then get super confused when that subdirectory isn't the thing that got linted.

I'll leave this issue open for now, please either rebute or drop :)
(Assignee)

Comment 6

a year ago
Sorry this took so long, I got caught up in other stuff. This latest patch also fixes the Windows test failure.

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review64502

> I think I disagree that /path/to/top-level/A/B is the desired path in that scenario.
> 
> Another way of phrasing it would be, if a relative path is valid both from $CWD and topsrcdir, which one takes precedence? I think $CWD should take precedence. Otherwise a developer could run into a situation where they tab complete to a subdirectory, and then get super confused when that subdirectory isn't the thing that got linted.
> 
> I'll leave this issue open for now, please either rebute or drop :)

I have to think about this invocation a little more but, the one you do higher up is being fed relative paths which have come from the vcs (relative to the repository root). That call is definitely opening you up to choosing a path relative to the working directory when the intention was to have it relative from the repo root.
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review64502

> I have to think about this invocation a little more but, the one you do higher up is being fed relative paths which have come from the vcs (relative to the repository root). That call is definitely opening you up to choosing a path relative to the working directory when the intention was to have it relative from the repo root.

That's a good point. I guess the problem is that it's being fed a mix of paths that can either come from vcs or manually by a developer (or even a mix of both). And in the former case it should be relative to topsrcdir, and in the latter case it should be relative to $CWD. Hm, maybe I can ensure the paths from the VCFiles class are already asbolutized, and therefore won't hit this problem..

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review64502

> That's a good point. I guess the problem is that it's being fed a mix of paths that can either come from vcs or manually by a developer (or even a mix of both). And in the former case it should be relative to topsrcdir, and in the latter case it should be relative to $CWD. Hm, maybe I can ensure the paths from the VCFiles class are already asbolutized, and therefore won't hit this problem..

Ya the vcs should be able to give you the absolute path of the topsrcdir, so from that you can just combine them.

I will note that the commit message for this change is in conflict with your rebuttal to my initial comment:
> We can't just call os.path.abspath here because that will join the relative path to
> os.getcwd(), which will be incorrect if the developer runs |mach lint| from anywhere other
> than topsrcdir. This patch will walk os.getcwd()'s ancestors and attempt to join until the
> path is valid.

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review64502

> Ya the vcs should be able to give you the absolute path of the topsrcdir, so from that you can just combine them.
> 
> I will note that the commit message for this change is in conflict with your rebuttal to my initial comment:
> > We can't just call os.path.abspath here because that will join the relative path to
> > os.getcwd(), which will be incorrect if the developer runs |mach lint| from anywhere other
> > than topsrcdir. This patch will walk os.getcwd()'s ancestors and attempt to join until the
> > path is valid.

So I think if you switch the VCS paths to using the vcs root, you *can* just call `os.path.abspath`, because it will be exactly what we want for paths passed in by the user. You'll also need to treat the exclude/include paths as coming from the vcs root. I think.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review64502

> So I think if you switch the VCS paths to using the vcs root, you *can* just call `os.path.abspath`, because it will be exactly what we want for paths passed in by the user. You'll also need to treat the exclude/include paths as coming from the vcs root. I think.

The new commit should fix the --rev/--workdir paths, but I didn't handle include/exclude. I guess you're right, if I fix the include/exclude directives to join to root as well, then I'll be able to just call abspath. The one downside to this is that mozlint will then require a VCS, but I guess that's not a big deal. Or maybe 'root' can default to $CWD if no vcs is found.

I'll upload another series soon.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
The last push was to re-add the call to normpath that I had removed. Forgot that windows needs it. Here's the try run for the latest series:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3f2880854d8ece6751309889db44524009eb6b3

Looks good.

Comment 21

a year ago
mozreview-review
Comment on attachment 8780555 [details]
Bug 1289805 - Don't pass in --exclude to flake8 paths that contain custom configuration,

https://reviewboard.mozilla.org/r/71252/#review69444
Attachment #8780555 - Flags: review?(smacleod) → review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8780556 [details]
Bug 1289805 - Refactor filterpaths to accept all lintargs,

https://reviewboard.mozilla.org/r/71254/#review69452
Attachment #8780556 - Flags: review?(smacleod) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8779464 [details]
Bug 1289805 - Ensure the VCSFiles class returns paths that have been absolutized to the repository root,

https://reviewboard.mozilla.org/r/70448/#review69460

::: python/mozlint/mozlint/cli.py:75
(Diff revision 2)
> +        def test_root(cmd):
> +            proc = subprocess.Popen(cmd, stdout=subprocess.PIPE)
> +            output = proc.communicate()[0].strip()
> +
> +            if proc.returncode == 0:
> +                self._vcs = cmd[0]
> +                self._root = output
> +            return proc.returncode
> +
> +        # First check if we're in an hg repo, if not try git
> +        if test_root(['hg', 'root']) != 0:
> +            test_root(['git', 'rev-parse', '--show-toplevel'])
> +        return self._root

I think it'd be cleaner to just go with
```Python
for cmd in (['hg', 'root'],
            ['git', 'rev-parse', '--show-toplevel']):
    proc = subprocess.Popen(cmd, stdout=subprocess.PIPE)
    output = proc.communicate()[0].strip()

    if proc.returncode == 0
        self._vcs = cmd[0]
        self._root = output
        return self._root
```

especially since we don't really care what the return code is in the case of an error.

::: python/mozlint/mozlint/cli.py:91
(Diff revision 2)
> -            if not subprocess.call(['hg', 'root'], stdout=DEVNULL):
> -                self._vcs = 'hg'
> +        if not self._vcs:
> +            self.root
> -            elif not subprocess.call(['git', 'rev-parse'], stdout=DEVNULL):
> -                self._vcs = 'git'
>          return self._vcs

`return self._vcs or (self.root and self._vcs)`
Attachment #8779464 - Flags: review?(smacleod) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8780557 [details]
Bug 1289805 - Resolve vcs arguments directly in LintRoller class,

https://reviewboard.mozilla.org/r/71256/#review69462
Attachment #8780557 - Flags: review?(smacleod) → review+

Comment 25

a year ago
mozreview-review
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review69466

::: python/mozlint/mozlint/pathutils.py:80
(Diff revision 5)
> -    exclude = map(FilterPath, exclude or [])
> +    include = [FilterPath(os.path.join(root, p)) for p in include]
> +    exclude = [FilterPath(os.path.join(root, p)) for p in exclude]

Should this maybe be done only if the path isn't already absolute?
Attachment #8775222 - Flags: review?(smacleod) → review+
(Assignee)

Comment 26

a year ago
mozreview-review-reply
Comment on attachment 8775222 [details]
Bug 1289805 - Properly normalize all paths so they are absolute,

https://reviewboard.mozilla.org/r/67438/#review69466

> Should this maybe be done only if the path isn't already absolute?

Yeah good point. I think these can only be defined in .lint configuration files, so I would hope that they would never be absolute.. but doesn't hurt to be safe.
(Assignee)

Comment 27

a year ago
mozreview-review-reply
Comment on attachment 8779464 [details]
Bug 1289805 - Ensure the VCSFiles class returns paths that have been absolutized to the repository root,

https://reviewboard.mozilla.org/r/70448/#review69460

> I think it'd be cleaner to just go with
> ```Python
> for cmd in (['hg', 'root'],
>             ['git', 'rev-parse', '--show-toplevel']):
>     proc = subprocess.Popen(cmd, stdout=subprocess.PIPE)
>     output = proc.communicate()[0].strip()
> 
>     if proc.returncode == 0
>         self._vcs = cmd[0]
>         self._root = output
>         return self._root
> ```
> 
> especially since we don't really care what the return code is in the case of an error.

Agreed, this looks better.

> `return self._vcs or (self.root and self._vcs)`

Huh, I never knew about using 'and' expressions that way. TIL
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

a year ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98876f70367e
Don't pass in --exclude to flake8 paths that contain custom configuration, r=smacleod
https://hg.mozilla.org/integration/autoland/rev/fff9177fcfe4
Refactor filterpaths to accept all lintargs, r=smacleod
https://hg.mozilla.org/integration/autoland/rev/5e14767aa060
Ensure the VCSFiles class returns paths that have been absolutized to the repository root, r=smacleod
https://hg.mozilla.org/integration/autoland/rev/f01fdb750198
Resolve vcs arguments directly in LintRoller class, r=smacleod
https://hg.mozilla.org/integration/autoland/rev/ee76e6c52088
Properly normalize all paths so they are absolute, r=smacleod

Comment 34

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98876f70367e
https://hg.mozilla.org/mozilla-central/rev/fff9177fcfe4
https://hg.mozilla.org/mozilla-central/rev/5e14767aa060
https://hg.mozilla.org/mozilla-central/rev/f01fdb750198
https://hg.mozilla.org/mozilla-central/rev/ee76e6c52088
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.