check-webkit-style should warn about missing svn:mime-type for png files
I think we should also put this check into the pre-commit hook.
Created attachment 129058 [details] patch (work in progress) I made a png.py file which check the ~/.subversion/config file the auto propset and svn:mime-type=image/png are enabled (based on ttp://trac.webkit.org/wiki/UsingGitWithWebKit Misc. Tips and Tricks) because in git svn there is no propset command. Only auto propset can set the mimetype. I have some question. - Is it enough to check the config file? - I noticed check-webkit-style doesn't support non-text files. I tried to solve the task elegant, but I think I couldn't. What should I do? - If there is a style error related with pngs I want to add to the global error counter. How can I do this nicely? Any ideas that should be implement? (I know I have to write unit tests.)
Comment on attachment 129058 [details] patch (work in progress) View in context: https://bugs.webkit.org/attachment.cgi?id=129058&action=review I think checking to see if the user's subversion config file is correct is useful, however, that doesn't actually fix this specific bug where certain files might not have the right svn properties set. I.e., this should probably be a separate bug. > Tools/Scripts/webkitpy/style/checkers/png.py:25 > """Supports checking WebKit style in png files.""" This diff is weird. I don't see png.py in the tree currently. > Tools/Scripts/webkitpy/style/checkers/png.py:42 > + for line in open(os.path.expanduser("~") + "/.subversion/config", 'r').readlines(): Does this work with Windows python? Does this work with Windows svn? > Tools/Scripts/webkitpy/style/patchreader.py:65 > + _log.debug("Using class: " + checker.__class__.__name__) I would remove this log line.
(In reply to comment #3) Thank you the answer. > (From update of attachment 129058 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129058&action=review > > I think checking to see if the user's subversion config file is correct is useful, however, that doesn't actually fix this specific bug where certain files might not have the right svn properties set. I.e., this should probably be a separate bug. > > > Tools/Scripts/webkitpy/style/checkers/png.py:25 > > """Supports checking WebKit style in png files.""" > > This diff is weird. I don't see png.py in the tree currently. Because it's a new file, I added to my local git repo. > > > Tools/Scripts/webkitpy/style/checkers/png.py:42 > > + for line in open(os.path.expanduser("~") + "/.subversion/config", 'r').readlines(): > > Does this work with Windows python? Does this work with Windows svn? Good questions, I will check these. > > > Tools/Scripts/webkitpy/style/patchreader.py:65 > > + _log.debug("Using class: " + checker.__class__.__name__) > > I would remove this log line. Ok.
Created attachment 131040 [details] proposed fix This is the improved patch based on the comments and other observations. Some note: - I made this patch with this command: "git diff --binary --no-ext-diff --full-index --no-renames e69b1d148665f162528f0b816a409119f1aef20c --" if it's wrong, please tell a good one. (The check-webkit-style use this command.) - I can't test the Windows part of the png.py.
Comment on attachment 131040 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=131040&action=review I have some concerns/questions but this may be approvable if someone else feels confident in it (or with appropriate explanations). > Tools/Scripts/webkitpy/style/checker.py:498 > + return False Why was this needed for png but no other type? > Tools/Scripts/webkitpy/style/checkers/png.py:29 > +import sys sort > Tools/Scripts/webkitpy/style/checkers/png.py:50 > + print "There is no ~/.subversion/config" I don't think we print from these checkers. Also this won't get flagged as an error at all. Isn't that incorrect given the current code? > Tools/Scripts/webkitpy/style/checkers/png.py:51 > + #return 1 Please remove the commented out code. > Tools/Scripts/webkitpy/style/checkers/png.py:70 > + if errorstr != "": Just if errorstr: > Tools/Scripts/webkitpy/style/checkers/png.py:71 > + self.handle_style_error(0, 'image/png', 5, errorstr) I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file. Hopefully someone more familiar with this will look at this part of the code. > Tools/Scripts/webkitpy/style/checkers/png_unittest.py:41 > + self.assertEquals(checker.handle_style_error, mock_handle_style_error) This doesn't look like it verifies that the checker actually finds the error.
(In reply to comment #6) > > Tools/Scripts/webkitpy/style/checkers/png.py:71 > > + self.handle_style_error(0, 'image/png', 5, errorstr) > > I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file. You can if you works with svn, but you can't if you're a git-svn user.
(In reply to comment #7) > (In reply to comment #6) > > > Tools/Scripts/webkitpy/style/checkers/png.py:71 > > > + self.handle_style_error(0, 'image/png', 5, errorstr) > > > > I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file. > > You can if you works with svn, but you can't if you're a git-svn user. Interesting. Does this mean that this check will never catch anything for patches put up here by git users? (As long as the style checker machine is set-up correctly and runs git.)
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > > Tools/Scripts/webkitpy/style/checkers/png.py:71 > > > > + self.handle_style_error(0, 'image/png', 5, errorstr) > > > > > > I thought it would look at the diff to determine this. I thought you could set the prop without it being in the config file. > > > > You can if you works with svn, but you can't if you're a git-svn user. > > Interesting. Does this mean that this check will never catch anything for patches put up here by git users? (As long as the style checker machine is set-up correctly and runs git.) As far as I know svn:mime-type settings aren't in the patches. But you have to setup your svn config correctly on the machine where you commit from. If you run check-webkit-style before committing it will warn you if your svn config is incorrect.
(In reply to comment #6) > > Tools/Scripts/webkitpy/style/checker.py:498 > > + return False > > Why was this needed for png but no other type? Because that function get this parameter: Tools/Scripts/webkitpy/style/checker.py:312 _SKIPPED_FILES_WITHOUT_WARNING = [ "LayoutTests" + os.path.sep, ] and pngs are in LayoutTests folder. I considered your suggestions, thank you.
Comment on attachment 131040 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=131040&action=review > Tools/Scripts/webkitpy/style/checker.py:320 > + 'png', Sort. > Tools/Scripts/webkitpy/style/checker.py:484 > + PNG = 9 Sort. >> Tools/Scripts/webkitpy/style/checkers/png.py:29 >> +import sys > > sort sort.
(In reply to comment #9) > (In reply to comment #8) > > Interesting. Does this mean that this check will never catch anything for patches put up here by git users? (As long as the style checker machine is set-up correctly and runs git.) > > As far as I know svn:mime-type settings aren't in the patches. But you have > to setup your svn config correctly on the machine where you commit from. > > If you run check-webkit-style before committing it will warn you if your > svn config is incorrect. Right, this is what I was saying in comment #3. For git users, this is the best you can do. For svn users, it would be better to actually check the properties of png files (although checking the config file doesn't hurt either). I thought this bug was for the svn user case, but this seems like a reasonable first step.
Comment on attachment 131040 [details] proposed fix It might be a bit spammy to run this check on every png file since it'll emit the same error for every png file (we're not actually checking the png file).
Created attachment 132787 [details] proposed fix Improved working based on comments.
I couldn't test the svn and windows parts of code.
Comment on attachment 132787 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=132787&action=review > Tools/Scripts/webkitpy/style/checker.py:482 > JSON = 3 > + PNG = 9 > PYTHON = 4 > TEXT = 5 > WATCHLIST = 6 Please re-number these. > Tools/Scripts/webkitpy/style/checkers/png.py:29 > +import re > +import sys > +import os Sort. > Tools/Scripts/webkitpy/style/checkers/png.py:44 > + self.file_path = file_path > + self.handle_style_error = handle_style_error > + self.fs = filesystem or FileSystem() > + self.det = scm or SCMDetector(self.fs, Executive()).detect_scm_system(self.fs.getcwd()) Member variables only used by the class should be prefixed with _. I think that applies to all of these. Also, self._detector or self._scm_detector instead of self.det. > Tools/Scripts/webkitpy/style/checkers/png.py:53 > + try: > + detection = self.det.display_name() > + except: Do we need this try/except block? When would this fail? > Tools/Scripts/webkitpy/style/checkers/png.py:56 > + if(detection == "git"): Remove the ()s > Tools/Scripts/webkitpy/style/checkers/png.py:59 > + config_file_path = self.fs.expanduser("~") + "/.subversion/config" fs.join > Tools/Scripts/webkitpy/style/checkers/png.py:67 > + config_file_path = os.environ['APPDATA'] + "\Subversion\config" fs.join > Tools/Scripts/webkitpy/style/checkers/png.py:72 > + config_file = self.fs.open_text_file_for_reading(config_file_path) > + except IOError: > + errorstr = "There is no " + config_file_path > + self.handle_style_error(0, 'image/png', 5, errorstr) > + return This is the same as lines 60-64. We can pull it out of the if. > Tools/Scripts/webkitpy/style/checkers/png.py:75 > + for line in config_file.readlines(): > + linenumber += 1 for linenumber, line in enumerate(config_file.readlines()): > Tools/Scripts/webkitpy/style/checkers/png.py:77 > + if match and str(match.group(0))[0] != "e": str() is unnecessary because match.group(0) is already a string. > Tools/Scripts/webkitpy/style/checkers/png.py:87 > + if(detection == "svn"): Remove the ()s and make this an elif. > Tools/Scripts/webkitpy/style/checkers/png.py:89 > + if(prop_get != "image/png"): Remove the ()s. > Tools/Scripts/webkitpy/style/checkers/png_unittest.py:57 > + scm = MockSCMDetector('svn') Can we add some test cases for git? For example, a test case with each of the following: enable-auto-props = yes #enable-auto-props = yes enable-auto-props = yes #enable-auto-props = yes #enable-auto-props = yes enable-auto-props = yes enable-auto-props = no > Tools/Scripts/webkitpy/style/patchreader.py:60 > + call_only_once = 1 Use True instead of 1. > Tools/Scripts/webkitpy/style/patchreader.py:69 > + if(call_only_once): Remove the ()s. > Tools/Scripts/webkitpy/style/patchreader.py:73 > + if(det.display_name() == "git"): Remove the ()s. > Tools/Scripts/webkitpy/style/patchreader.py:74 > + call_only_once = 0 call_only_once = False
(In reply to comment #16) > > Tools/Scripts/webkitpy/style/checkers/png.py:53 > > + try: > > + detection = self.det.display_name() > > + except: > > Do we need this try/except block? When would this fail? I thought it's necessary because the detection can get None value and that case display_name() won't working and throw error. (/Tools/Scripts/webkitpy/common/checkout/scm/detection.py, def detect_scm_system(self, path, patch_directories=None):)
Created attachment 133272 [details] proposed fix fresh version The windows and the svn parts of the code are not tested! (I'm working on linux and git)
Comment on attachment 133272 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=133272&action=review > Tools/Scripts/webkitpy/style/checkers/png.py:69 > + match = re.search("^\s*.*enable-auto-props = (yes|no)", line) > + if (match and match.group(0)[0] != "e" and not there_is_enable_line) or (match and match.group(0).endswith("no")): > + errorstr_autoprop = "Have to enable auto props in the subversion config file (" + config_file_path + ", line:" + str(linenumber + 1) + "). " Can we just look for "^\s*enable-auto-props\s*=\s*yes"? If we don't find the line, we give this error message. I think matching commented out lines is making the logic unnecessarily complex. > Tools/Scripts/webkitpy/style/checkers/png.py:76 > + match = re.search("^\s*.*\*\.png = svn:mime-type=image/png", line) > + if match and match.group(0)[0] != "*" and not there_is_png_line: Can we just search for "^\s*\*\.png\s*=\s*svn:mime-type=image/png"? If we don't find the line, we give the png error message. > Tools/Scripts/webkitpy/style/checkers/png.py:96 > + if self._platform.startswith('linux') or self._platform.startswith('darwin'): > + config_file_path = self._fs.join(self._fs.expanduser("~") + "/", ".subversion/config") Nit: You don't need the + "/", os.path.join adds the path separator for you. > Tools/Scripts/webkitpy/style/checkers/png.py:98 > + config_file_path = self._fs.join(os.environ['APPDATA'] + "\\", "Subversion\config") Nit: You don't need the + "\\", os.path.join adds the path separator for you. > Tools/Scripts/webkitpy/style/checkers/png_unittest.py:50 > + def mock_handle_style_error(line_number, category, confidence, > + message): Nit: Indent is weird. I would just not wrap here.
Created attachment 133465 [details] proposed fix
Comment on attachment 133465 [details] proposed fix It would be great if someone could test this on win and mac before landing.
Comment on attachment 133465 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=133465&action=review > Tools/Scripts/webkitpy/style/checkers/png.py:86 > + > + if detection == "svn": nit: elif
(In reply to comment #21) > (From update of attachment 133465 [details]) > It would be great if someone could test this on win and mac before landing. It has been tested, it works good on win and mac also.
(In reply to comment #23) > (In reply to comment #21) > > (From update of attachment 133465 [details] [details]) > > It would be great if someone could test this on win and mac before landing. > > It has been tested, it works good on win and mac also. It has been Landed in r112770 http://trac.webkit.org/changeset/112270 And I apologies about this line: "Reviewed by NOBODY Tony Chang." :( Next time I will be far more careful. Sorry again.
Comment on attachment 133465 [details] proposed fix Sounds like this was already committed.
Do we need to fix something on the style bot? https://bugs.webkit.org/show_bug.cgi?id=81936#c13 We obviously don't want it failing like this. It uses a Git checkout.
(In reply to comment #26) > Do we need to fix something on the style bot? > https://bugs.webkit.org/show_bug.cgi?id=81936#c13 > > We obviously don't want it failing like this. It uses a Git checkout. Yes, it isn't good if it fails on the style bot. This warning is to warn git users to set their subversion config correctly before landing patches. (with git-svn). Because if they don't do it, mime-type of png files won't be image/png. This is the only way for git users to committ png files with correct mime-type. Could you set the style bot, please? - Have to enable auto props in the subversion config file (/home/ubuntu/.subversion/config "enable-auto-props = yes"). - Have to set the svn:mime-type in the subversion config file (/home/ubuntu/.subversion/config "*.png = svn:mime-type=image/png") These options are commented by default, I think you only have to uncomment them.
Oh, after https://bugs.webkit.org/show_bug.cgi?id=81936#c17 everything should work.
This causes .png removal failures: https://bugs.webkit.org/show_bug.cgi?id=82933
This check is really annoying because it doesn't solve the problem for me: LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png). [image/png] [5] LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png). [image/png] [5] LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png). [image/png] [5] LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png). [image/png] [5]
(In reply to comment #30) > This check is really annoying because it doesn't solve the problem for me: > > LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-win/fast/multicol/progression-reverse-expected.png). [image/png] [5] > LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac/fast/multicol/progression-reverse-expected.png). [image/png] [5] > LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png). [image/png] [5] > LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png:0: Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/chromium-linux/fast/multicol/progression-reverse-expected.png). [image/png] [5] check-webkit-style won't solve your problem, it only warns you if you do something wrong. We should set svn mimetype for all png files - https://lists.webkit.org/pipermail/webkit-dev/2012-March/019783.html , but you didn't. That's why check-webkit-style warned you and suggested what to do: svn propset svn:mime-type image/png LayoutTests/platform/chromium-mac-snowleopard/fast/multicol/progression-reverse-expected.png ) But https://bugs.webkit.org/show_bug.cgi?id=75825 will do it automatically once the fix is landed.
Thanks. I will wait patiently for Bug 75825. :)