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

(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.

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/

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.

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?

follow up of comment #4: maybe in a separate bug

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.

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:

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

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?

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".

Changed the Commit message to "Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre"

Comment on attachment 8929697 [details] [diff] [review] Bug 1413612 - Allow running ./mach clang-format outside of topsrcdir. r=sylvestre Many thanks. Better than expected :)

Unfortunately, this patch doesn't apply cleanly against mozilla-inbound. Can you please post a rebased patch on top of the latest changes?

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.

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!

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.

Comment on attachment 8930822 [details] [diff] [review] Bug 1413612 - Fix ./mach clang-format when run without path argument. r=sylvestre Approved in bug 1419986