Closed Bug 1289805 Opened 8 years ago Closed 8 years ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, defect)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(5 files)

Bug 1289805 - Properly normalize all paths so they are absolute,
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Bug 1289805 - Ensure the VCSFiles class returns paths that have been absolutized to the repository root,
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Bug 1289805 - Don't pass in --exclude to flake8 paths that contain custom configuration,
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Bug 1289805 - Refactor filterpaths to accept all lintargs,
58 bytes, text/x-review-board-request
smacleod
: review+
Details
Bug 1289805 - Resolve vcs arguments directly in LintRoller class,
58 bytes, text/x-review-board-request
smacleod
: review+
Details 
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.
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: 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.
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 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 :)
Sorry this took so long, I got caught up in other stuff. This latest patch also fixes the Windows test failure.
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.
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 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 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 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.
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 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 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 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 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 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+
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.
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
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
Product: Testing → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: