Closed Bug 1413612 Opened 5 years ago Closed 5 years ago

./mach clang-format should fail when run outside of the topsrcdir

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement)

enhancement
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Sylvestre, Assigned: hrdktg, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [good first bug][lang=python])

Attachments

(2 files, 4 obsolete files)

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1411004#c4, we should fail ./mach clang-format is not run from topsrcdir

Should probably be done here:
https://dxr.mozilla.org/mozilla-central/source/tools/mach_commands.py#300
Mentor: sledru
(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1411004#c4, we
> should fail ./mach clang-format is not run from topsrcdir
> 
> Should probably be done here:
> https://dxr.mozilla.org/mozilla-central/source/tools/mach_commands.py#300

Need clarification->

Directory Structure:
mozilla-central
  folder1
  folder2
  ...

Does this mean that if ./mach clang-format is run from inside folder1 then we should just tell user to run the command from topsrcdir(i.e. 'mozilla-central').
If this is the intention then we can add another condition in
> https://dxr.mozilla.org/mozilla-central/source/tools/mach_commands.py#209
to check if path is not topsrcdir.
Flags: needinfo?(sledru)
Sorry, I could have been more explicit.
We would like to trigger an error in that case:
~/mozilla-central.hg$ ./mach clang-format -p ../mozilla-central.hg/config/
Flags: needinfo?(sledru)
Attached patch remove_relative.patch (obsolete) — Splinter Review
Actually i got an idea.

Instead of returning error, cant we remove the relative path upto $topsrcdir,
So the '../mozilla-central.hg/config' will be converted to '/config',
This will result in the correct regex matching and the exclude filter will work.

I'm uploading the patch for the same, Not sure if i should set the review flag.
Flags: needinfo?(sledru)
Good idea, I will have a look.
by the way, here is also the following case:
in xpcom:
../mach clang-format -p ../config/
is not doing anything.
Maybe we should also fix that.
what do you think?
Flags: needinfo?(sledru)
follow up of comment #4: maybe in a separate bug
Assignee: nobody → hrdktg
Attached patch handle_more.patch (obsolete) — Splinter Review
Okay i think i got it now..
My earlier code was ignorant.

I think that the problem lies with the fact that
> os.chdir(self.topsrcdir)
changes the current working directory and then goes on to create list of files.

So in my new patch==>
> 1) Before changing dir, I change the relative paths to absolute paths. (I hope this doesn't cause any side effect)
> 2) Then during the matching phase, the topsrcdir part is removed for the regex matching.

Please tell if the patch is worth reviewing.
Attachment #8927579 - Attachment is obsolete: true
Flags: needinfo?(sledru)
Comment on attachment 8927642 [details] [diff] [review]
handle_more.patch

>diff --git a/tools/mach_commands.py b/tools/mach_commands.py
>--- a/tools/mach_commands.py
>+++ b/tools/mach_commands.py
>@@ -191,6 +191,12 @@ class FormatProvider(MachCommandBase):
>                       ". Supported platforms are Windows/*, Linux/x86_64 and Darwin/x86_64")
>                 return 1
> 
>+        # Get absolute pathname
>+        tmp_path=[]
>+        for f in path:
>+            tmp_path.append(os.path.abspath(f))
>+        path=tmp_path
>+
You should probably move that into a method.

>         os.chdir(self.topsrcdir)
>         self.prompt = True
> 
>@@ -312,7 +318,14 @@ class FormatProvider(MachCommandBase):
> 
>         path_list = []
>         for f in paths:
>-            if re.match(ignored_dir_re, f):
>+            # Remove upto topsrcdir in pathname
Please move that stuff into a function as it is duplicate
>+            match_f = f.split(self.topsrcdir+'/',1)
Please run flake8 on it. It will fail the build otherwise.
For example, you need space around the "+"
>+            if len(match_f) == 2 :
>+                match_f = match_f[1]
>+            else :
>+                match_f = match_f[0]
Maybe use the ternary operator here:
match_f = match_f[1] if len(match_f) == 2 else match_f[0]

>+            if re.match(ignored_dir_re, match_f):
>                 # Early exit if we have provided an ignored directory
>                 print("clang-format: Ignored third party code '{0}'".format(f))
>                 continue
>@@ -323,7 +336,13 @@ class FormatProvider(MachCommandBase):
>                     subs.sort()
>                     for filename in sorted(files):
>                         f_in_dir = os.path.join(folder, filename)
>-                        if f_in_dir.endswith(extensions) and not re.match(ignored_dir_re, f_in_dir):
>+                        # Remove upto topsrcdir in pathnames
>+                        match_f = f_in_dir.split(self.topsrcdir+'/',1)
>+                        if len(match_f) == 2 :
>+                            match_f = match_f[1]
>+                        else :
>+                            match_f = match_f[0]
>+                        if f_in_dir.endswith(extensions) and not re.match(ignored_dir_re, match_f):
>                             # Supported extension and accepted path
>                             path_list.append(f_in_dir)
>             else:
Flags: needinfo?(sledru)
Created 2 new methods=>
> 1) conv_to_abspath(self, paths)
> 2) is_ignored_path(self, ifnored_dir_re, f)

Ran flake8 on the written code and rectified the problems.
The line
> if f_in_dir.endswith(extensions) and not re.match(ignored_dir_re, f_in_dir):
gave error in flake8 for exceeding line length of 99 
So the line was broken down into two seperate lines
and the matching is now completely done in newly created method#2

Also regarding flake8,
The below errors are un-rectified as they were present before I added my code.
>mach_commands.py:229:153: E261 at least two spaces before inline comment
>mach_commands.py:231:161: E261 at least two spaces before inline comment
Attachment #8927642 - Attachment is obsolete: true
Attachment #8927892 - Flags: review?(sledru)
Comment on attachment 8927892 [details] [diff] [review]
Bug 1413612 - ./mach clang-format should fail when run outside of the topsrcdir. r=sylvestre

Perfect, thanks!
I tested the following:
from config/
$ ../mach clang-format -p ../config/
$ ../mach clang-format -p ../../mozilla-central.hg/config/
from ../
$ ./mozilla-central.hg/mach clang-format -p mozilla-central.hg/config/

They all process the files correctly. 

Nika, do you want to review that before it lands?
Flags: needinfo?(nika)
Attachment #8927892 - Flags: review?(sledru) → review+
Commit name isn't correct (we correctly handle being run outside of the topsrcdir), but other than that it looks fine to me. Thanks.

Please change the commit to say something along the lines of "allow running ./mach clang-format outside of topsrcdir".
Flags: needinfo?(nika)
Changed the Commit message to "Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre"
Attachment #8927892 - Attachment is obsolete: true
Attachment #8929697 - Flags: review?(sledru)
Comment on attachment 8929697 [details] [diff] [review]
Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre

Many thanks. Better than expected :)
Attachment #8929697 - Flags: review?(sledru) → review+
Keywords: checkin-needed
Unfortunately, this patch doesn't apply cleanly against mozilla-inbound. Can you please post a rebased patch on top of the latest changes?
Flags: needinfo?(hrdktg)
Keywords: checkin-needed
Made the patch using 'mozilla-inbound' repo, so this should apply cleanly now.
It doesnt apply to 'mozilla-central' now..

I hope I'm doing it correctly.

Also regarding these lines-->
>+            # The regexp is to make sure we are managing relative paths
>+            ignored_dir.append("^[\./]*" + line.rstrip())
I dont know whether i should change it back to
>+            ignored_dir.append(line.rstrip())

so i havent made any changes in these lines...As they dont have any effect on the working of new patch.
Attachment #8929697 - Attachment is obsolete: true
Flags: needinfo?(hrdktg)
Attachment #8930461 - Flags: review?(sledru)
https://hg.mozilla.org/integration/mozilla-inbound/rev/719f517b6f6f8938c83198b6f2eb8cfe5501bc16
Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre
Comment on attachment 8930461 [details] [diff] [review]
Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre

I landed that for you!
Attachment #8930461 - Flags: review?(sledru) → review+
https://hg.mozilla.org/mozilla-central/rev/719f517b6f6f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
TypeError: 'NoneType' object is not iterable

  File "/home/jwwang/codebase/mozilla-central/firefox/tools/mach_commands.py", line 194, in clang_format
    path = self.conv_to_abspath(path)
  File "/home/jwwang/codebase/mozilla-central/firefox/tools/mach_commands.py", line 218, in conv_to_abspath
    for f in paths:

`mach clang-format` gave me this error on revision b96f00947898 on Linux.
Created this patch over the pushed changes..applies cleanly on 'mozilla-inbound'

This patch correctly handles the case when the user executes clang-format without any path arguments.
> ./mach clang-format


Thank you for the feedbacks and reviews.
Attachment #8930822 - Flags: review?(sledru)
Comment on attachment 8930822 [details] [diff] [review]
Bug 1413612 - Fix ./mach clang-format when run without path argument. r=sylvestre

Approved in bug 1419986
Attachment #8930822 - Flags: review?(sledru)
Product: Core → 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.