Summary: | webkit-patch land should automatically add svn:mime-type for .png files | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Csaba Osztrogonác <ossy> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, bank, cjerdonek, dpranke, galpeter, levin, ojan, ossy, robert, schenney, webkit.review.bot, webkit-sed | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | 86373 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2012-01-08 21:38:16 PST
Can anyone help me, where can I start to solve this problem? I believe this can only be done for SVN. You'd have to edit svn.py to add this ability, and the presumably make a new Step() class which was called by various Command objects and which added this mime type for necessaary png files. I'm no longer convinced this is the right way to solve this problem. Do we already have a scrxipt which can fix this globally for the svn repository? Should we just run that scxript every night on a bot somewhere? Created attachment 135140 [details]
script to auto-update svn mime-types
I wrote this script and ran it once through the repo. I don't have time to properly write tests for it and get the patch reviewed/committed. If someone else wants to take it over, they're more than welcome to. Put this script in Tools/Scripts and it should do the right thing.
(In reply to comment #4) > Created an attachment (id=135140) [details] > script to auto-update svn mime-types Thank you! > > I wrote this script and ran it once through the repo. I don't have time to properly write tests for it and get the patch reviewed/committed. If someone else wants to take it over, they're more than welcome to. Put this script in Tools/Scripts and it should do the right thing. I take care of it. I realized I could use the svn config file reader and the config file path getter parts from the webkitpy/style/checkers/png.py. I want to avoid the code duplication. My idea is I outsource those lines to a detached file and use it. The question is where can I place this file? (I would put it to webkitpy/common but maybe I think it wrong) Created attachment 137527 [details]
work in progress
I finished the task, but I have a question. I saw there is no unittest all of the steps (webkitpy/tool/steps), should I write?
Created attachment 138335 [details]
proposed fix
Could somebody check my patch, please? Comment on attachment 138335 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=138335&action=review I took a quick look. One note below. Ideally Eric or Dirk would review your patch. > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:43 > + detector = SCMDetector(self._fs, Executive()).detect_scm_system(self._fs.getcwd()) We should get the Executive from the tool so that it gets mocked properly during testing. Comment on attachment 138335 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=138335&action=review > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:30 > +import sys It looks like sys is unused in this file ... > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:33 > +def check(self, platform, fs): Is 'platform' a PlatformInfo object? It looks like it is ... maybe this should take a host or a systemhost instead? Also, some form of docstring would be good for this function. What is this function supposed to be checking for, and how should the return value be used? > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:72 > + if platform.startswith('linux') or platform.startswith('darwin'): This should probably be if platform.is_win(): config_file_path = fs.join(os.environ ... else: config_file_path = fs.join(fs.expand ... > Tools/Scripts/webkitpy/style/checkers/png.py:45 > self._platform = platform or sys.platform Does the style checker have access to a host or a tool object? It should get the platform info from that ... > Tools/Scripts/webkitpy/style/checkers/png.py:52 > + errorcode = checksvnconfigfile.check(self, self._platform, self._fs) Instead of returning an errorcode that is a bit mask, you should consider either returning two values in a tuple, or splitting this into two functions. > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:36 > + self._fs = FileSystem() You can also get the filesystem from the tool. Created attachment 140028 [details]
proposed fix
The improved patch based on comments.
Comment on attachment 140028 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=140028&action=review This looks pretty good (with the one quibble), but normally we require unit tests for new code. Can you add some unit tests as well? > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:46 > + return (1, 1, 1) You should use True/False instead of 1/0. (In reply to comment #13) > (From update of attachment 140028 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140028&action=review > > This looks pretty good (with the one quibble), but normally we require unit tests for new code. Can you add some unit tests as well? Yes, I can. I asked it earlier but I didn't get answer. > > > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:46 > > + return (1, 1, 1) > > You should use True/False instead of 1/0. OK. Created attachment 140967 [details]
proposed fix
I wrote unittest and use boolean type instead of 1/0.
Comment on attachment 140967 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=140967&action=review marking cq- just so you can take a look at my suggestion and see if it works. looks good otherwise. > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:51 > + for line in config_file.split('\n'): I wonder if this would be easier and clearer if you didn't split the file and just did two re.search()'s against the whole string, i.e. errorcode_autoprops = not re.search("\s*enable-auto-props\s*=\s*yes", config_file) errorcode_png = not re.search("\s*\*\.png\s*=\s*svn:mime-type=image/png", config_file) or something like that (I'm not sure how you'd handle commented out lines, so you might have to experiment a bit)? Created attachment 141155 [details]
proposed fix
I did what you suggested, the code is clearer indeed. Thank you!
Created attachment 141181 [details]
proposed fix
I rewrote the regex because png_unittest thrown error. (I forgot run unittest before uploaded the previous patch, sorry)
Comment on attachment 141181 [details] proposed fix Landed in https://trac.webkit.org/changeset/116935 Now I can't land anything. :-( webkit-patch land Traceback (most recent call last): File "/home/schenney/development/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkit-patch", line 69, in <module> main() File "/home/schenney/development/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkit-patch", line 64, in main WebKitPatch(os.path.abspath(__file__)).main() File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 311, in main result = command.check_arguments_and_execute(options, args, self) File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 120, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 51, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors self._run(tool, options, state) File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/usr/local/google/users/schenney/chromium-patches/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py", line 68, in run if filename.endswith('.png') and detector.exists(filename) and detector.propget('svn:mime-type', filename) == "": NameError: global name 'detector' is not defined Ouch. :-/ Let me roll out immediately. (In reply to comment #21) > Ouch. :-/ Let me roll out immediately. Rollout landed in https://trac.webkit.org/changeset/116947 Comment on attachment 141181 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=141181&action=review > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68 > + if filename.endswith('.png') and detector.exists(filename) and detector.propget('svn:mime-type', filename) == "": detector -> self._detector > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:70 > + detector.propset('svn:mime-type', 'image/png', filename) detector -> self._detector (In reply to comment #23) > (From update of attachment 141181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141181&action=review > > > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68 > > + if filename.endswith('.png') and detector.exists(filename) and detector.propget('svn:mime-type', filename) == "": > > detector -> self._detector > > > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:70 > > + detector.propset('svn:mime-type', 'image/png', filename) > > detector -> self._detector It seems this codepath isn't (unit)tested well. Created attachment 141960 [details]
proposed fix
I fixed the problem and tested on svn, it seems good.
Created attachment 147528 [details]
proposed fix
I updated the patch. Could somebody check it, please?
Comment on attachment 147528 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=147528&action=review assuming my comments are correct, fix the comments and variable names and this should be fine. > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:36 > + was the file read successful, is there enable the auto-props, is there enable the svn:mime-type for png I'm not sure if this is a bug or if this is just written wrong, but it looks like you want to return True if something failed, right? So I would rewrite this as "is this file missing, is auto-props missing, is the svn:mime-type for png missing" > Tools/Scripts/webkitpy/style/checkers/png.py:58 > + (file_read, autoprop, png) = checksvnconfigfile.check(self, self._host, self._fs) I would rename these variables the same way I renamed the docstring, so file_missing, autoprop_missing, png_missing Created attachment 147802 [details]
proposed fix
I renamed the variables and rewrote the comment.
View in context: https://bugs.webkit.org/attachment.cgi?id=147802&action=review > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:32 > +def check(self, host, fs): I think the 'self' here is a little misleading, and why do we need it at all? > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:52 > +def _config_file_path(self, host, fs): Why do you need the 'self'? It seems it is unused. > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:53 > + config_file = "" I don't see this variable used in this method. > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:57 > + config_file_path = fs.join(os.environ['APPDATA'], "Subversion\config") > + else: > + config_file_path = fs.join(fs.expanduser("~"), ".subversion/config") fs.join(..) can take more than 2 arguments, the config should be the 3. > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:62 > + return "Have to enable auto props in the subversion config file (" + config_file_path + " \"enable-auto-props = yes\"). " Instead of escaping the " use ' at the start and end. Oh.. and use the %s formatter, that is more readable. > Tools/Scripts/webkitpy/common/checksvnconfigfile.py:66 > + return "Have to set the svn:mime-type in the subversion config file (" + config_file_path + " \"*.png = svn:mime-type=image/png\")." ditto > Tools/Scripts/webkitpy/style/checkers/png.py:59 > + config_file_path = checksvnconfigfile._config_file_path(self, self._host, self._fs) Calling a method prefixed with underscore isn't really nice. Ditch the leading underscore. > Tools/Scripts/webkitpy/style/checkers/png.py:62 > + self._handle_style_error(0, 'image/png', 5, "There is no SVN config file. (" + config_file_path + ")") use %s formatter > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:42 > + if len(png_files) > 0: No need for length check, just: "if png_files:" > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:46 > + (file_read, autoprop, png) = checksvnconfigfile.check(self, self._host, self._fs) use the same naming like in the PNGChecker. > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68 > + if filename.endswith('.png') and self._detector.exists(filename) and self._detector.propget('svn:mime-type', filename) != 'image/png': The file extension is already checked in the _check_pngs method. Why do you check it again? > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:46 > + options = MockOptions() > + options.git_commit = 'MOCK git commit' Pass the git_commit as a kwarg to the MockOptions. > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:54 > + "changed_files": ["test.png"], Add a non-png file to the list. > Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:59 > + except SystemExit, e: > + self.assertEquals(type(e), type(SystemExit())) Why do we check if the type is a SystemExit exception? The except will only catch SystemExit ones. Created attachment 149495 [details] Patch Fixed patch based on Comment #29. (Balázs isn't online now, but he asked me to upload his updated patch. Could somebody take a look for the patch, please? (In reply to comment #30) > Created an attachment (id=149495) [details] > Patch > > Fixed patch based on Comment #29. (Balázs isn't online now, but he asked me to upload his updated patch. looks a lot better now (infomral r=me :)) Comment on attachment 149495 [details]
Patch
looks fine. Sorry for the delay; this fell through the cracks somehow.
Comment on attachment 149495 [details] Patch Clearing flags on attachment: 149495 Committed r122311: <http://trac.webkit.org/changeset/122311> All reviewed patches have been landed. Closing bug. |