Closed
Bug 1413612
Opened 7 years ago
Closed 7 years ago
./mach clang-format should fail when run outside of the topsrcdir
Categories
(Developer Infrastructure :: Source Code Analysis, enhancement)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: Sylvestre, Assigned: hrdktg, Mentored)
References
Details
(Keywords: good-first-bug, Whiteboard: [good first bug][lang=python])
Attachments
(2 files, 4 obsolete files)
2.70 KB,
patch
|
Sylvestre
:
review+
|
Details | Diff | Splinter Review |
788 bytes,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•7 years ago
|
Mentor: sledru
Assignee | ||
Comment 1•7 years ago
|
||
(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)
Reporter | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Reporter | ||
Comment 4•7 years ago
|
||
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)
Reporter | ||
Comment 5•7 years ago
|
||
follow up of comment #4: maybe in a separate bug
Assignee: nobody → hrdktg
Assignee | ||
Comment 6•7 years ago
|
||
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)
Reporter | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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)
Reporter | ||
Comment 9•7 years ago
|
||
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+
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Reporter | ||
Comment 12•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 13•7 years ago
|
||
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
Assignee | ||
Comment 14•7 years ago
|
||
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)
Reporter | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/719f517b6f6f8938c83198b6f2eb8cfe5501bc16
Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre
Reporter | ||
Comment 16•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 18•7 years ago
|
||
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.
Assignee | ||
Comment 19•7 years ago
|
||
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)
Reporter | ||
Comment 20•7 years ago
|
||
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)
Updated•7 years ago
|
status-firefox58:
affected → ---
Updated•7 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•