Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569283002:
WebUI: Add GN rules for Vulcanize. (Closed)
DescriptionWebUI: Add GN rules for Vulcanize.
BUG=673825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2569283002
Cr-Commit-Position: refs/heads/master@{#445898}
Committed: https://chromium.googlesource.com/chromium/src/+/bb00fc0c1c03279556a11500de26ec31665af1ca
Patch Set 1 #Patch Set 2 : Use third_party/node #Patch Set 3 : Fix #Patch Set 4 : Update paths, fix presubmit warnings. #
Total comments: 2
Patch Set 5 : Merge branch 'gclient_sync_node' into add_gn_vulcanize #Patch Set 6 : integrate out-request-list #Patch Set 7 : Address TODO #Patch Set 8 : Rename #
Total comments: 27
Patch Set 9 : Move some files to separate CL. #Patch Set 10 : Addressing comments. #Patch Set 11 : Update after node/node_modules renaming. #
Total comments: 26
Patch Set 12 : Address most comments. #
Total comments: 6
Patch Set 13 : Addressing comments. #
Total comments: 4
Patch Set 14 : Address comments. #
Total comments: 18
Patch Set 15 : Add GN deps to node and NPM, to properly retrigger. #Patch Set 16 : Windows fixes #Patch Set 17 : use normpath for args.input #
Total comments: 6
Patch Set 18 : Address comments + Windows fixes #Patch Set 19 : _CWD #
Dependent Patchsets: Messages
Total messages: 37 (13 generated)
Description was changed from ========== WebUI: Add GN rules for Vulcanize. BUG= ========== to ========== WebUI: Add GN rules for Vulcanize. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2569283002/diff/60001/chrome/browser/resource... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/60001/chrome/browser/resource... chrome/browser/resources/unpack_pak.py:27: res = re.match('#define ([^ ]+) (\d+)', line) This is cool as-is, but I wanted to offer a tip in the hope it's helpful. Regex has special escapes for white-space and C-like keywords, e.g. [A-Za-z0-9_]. So, this could be (if you prefer): res = re.match('^\s*#define\s+(\w+)\s(\d+)', line) That may be more robust. Though it may not matter if the file being scanned is mechanically generated (which would be unlikely to have a variance in the layout).
https://codereview.chromium.org/2569283002/diff/60001/chrome/browser/resource... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/60001/chrome/browser/resource... chrome/browser/resources/unpack_pak.py:27: res = re.match('#define ([^ ]+) (\d+)', line) On 2017/01/12 00:58:10, dschuyler wrote: > This is cool as-is, but I wanted to offer a tip in the hope it's helpful. Regex > has special escapes for white-space and C-like keywords, e.g. [A-Za-z0-9_]. So, > this could be (if you prefer): > > res = re.match('^\s*#define\s+(\w+)\s(\d+)', line) > > That may be more robust. Though it may not matter if the file being scanned is > mechanically generated (which would be unlikely to have a variance in the > layout). Whoops, the ^ is redundant with re.match(). (The ^ would help with re.search(), but re.match() already implies 'at start of line').
Patchset #6 (id:90001) has been deleted
Description was changed from ========== WebUI: Add GN rules for Vulcanize. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Add GN rules for Vulcanize. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
I'll address Dave's comment about the regular expressions in a follow up patch (same CL). I did not have time to play with the suggestions yet, but wanted to get this out for review sooner rather than later. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
just some initial thoughts https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:19: #] remove https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:20: print(outputs) remove https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:31: "--htmlInFile", flagsLikeThis should actually be_like_this https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:59: _VULCANIZE_REDIRECT_ARGS = list(itertools.chain.from_iterable(map( what's goin on here? why can't this just be ['--redirect|%s' % m for m in _URL_MAPPINGS] ? https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:97: dependencies = map(partial( can you just use lambda instead? https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:169: # assert inputType == "FOLDER" remove
https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.py File third_party/node/node.py (right): https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.... third_party/node/node.py:6: import os nit: from os import path as os_path https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.... third_party/node/node.py:8: \n\n between file-level globals https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.... third_party/node/node.py:28: pass what's the point of this? could we remove it or make it more useful and run get_binary with sys.argv[1:]? https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... File third_party/node/node_modules.py (right): https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:6: import os same nit https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:11: i'm not super duper familiar with this, but I THINK it'll work def _get_binary(*args): return os.path.join(_THIRD_PARTY_NODE_MODULES_DIR, *args) https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:14: _THIRD_PARTY_NODE_MODULES_DIR, 'vulcanize', 'bin', 'vulcanize')); return _get_binary('vulcanize', 'bin', 'vulcanize') https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:30: pass delete
Addressed node.py and node_modules.py comments, and removed them from this CL. They are now at https://codereview.chromium.org/2638383006, per in-person suggestion. Will address comments in remaining files soon. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.py File third_party/node/node.py (right): https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.... third_party/node/node.py:6: import os On 2017/01/20 at 01:03:07, Dan Beam (slow - converging) wrote: > nit: from os import path as os_path Done. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.... third_party/node/node.py:8: On 2017/01/20 at 01:03:07, Dan Beam (slow - converging) wrote: > \n\n between file-level globals Done. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node.... third_party/node/node.py:28: pass On 2017/01/20 at 01:03:07, Dan Beam (slow - converging) wrote: > what's the point of this? could we remove it or make it more useful and run get_binary with sys.argv[1:]? Removed. I'll defer adding a main until it is needed. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... File third_party/node/node_modules.py (right): https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:6: import os On 2017/01/20 at 01:03:08, Dan Beam (slow - converging) wrote: > same nit Done. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:11: On 2017/01/20 at 01:03:08, Dan Beam (slow - converging) wrote: > i'm not super duper familiar with this, but I THINK it'll work > > def _get_binary(*args): > return os.path.join(_THIRD_PARTY_NODE_MODULES_DIR, *args) Done, it seems to work. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:14: _THIRD_PARTY_NODE_MODULES_DIR, 'vulcanize', 'bin', 'vulcanize')); On 2017/01/20 at 01:03:08, Dan Beam (slow - converging) wrote: > return _get_binary('vulcanize', 'bin', 'vulcanize') Done. https://codereview.chromium.org/2569283002/diff/150001/third_party/node/node_... third_party/node/node_modules.py:30: pass On 2017/01/20 at 01:03:08, Dan Beam (slow - converging) wrote: > delete Deleted.
https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:31: "--htmlInFile", On 2017/01/20 00:56:50, Dan Beam (slow - converging) wrote: > flagsLikeThis should actually be_like_this ping
https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:19: #] On 2017/01/20 at 00:56:50, Dan Beam (slow - converging) wrote: > remove Done. https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:20: print(outputs) On 2017/01/20 at 00:56:50, Dan Beam (slow - converging) wrote: > remove Done. https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:31: "--htmlInFile", On 2017/01/20 at 17:43:09, Dan Beam (slow - converging) wrote: > On 2017/01/20 00:56:50, Dan Beam (slow - converging) wrote: > > flagsLikeThis should actually be_like_this > > ping Done. https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:59: _VULCANIZE_REDIRECT_ARGS = list(itertools.chain.from_iterable(map( On 2017/01/20 at 00:56:50, Dan Beam (slow - converging) wrote: > what's goin on here? why can't this just be > > ['--redirect|%s' % m for m in _URL_MAPPINGS] > > ? Need to generate an array that looks as follows [ '--redirect', 'chrome://a/b|../c/d', '--redirect', 'chrome://c/d|../e/f', ] but your suggestion ['--redirect '%s|%s' % (u,p) for (u, p) in _URL_MAPPINGS] generates a different array that looks as follows [ '--redirect chrome://a/b|../c/d', '--redirect chrome://c/d|../e/f', ] The problem is that the '--redirect' part and the remaining part need to be separate elements of the returned array, otherwise the run_cmd call at line 115 is treating "--redirect foo" as a single argument. https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:97: dependencies = map(partial( On 2017/01/20 at 00:56:50, Dan Beam (slow - converging) wrote: > can you just use lambda instead? Done. https://codereview.chromium.org/2569283002/diff/150001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:169: # assert inputType == "FOLDER" On 2017/01/20 at 00:56:50, Dan Beam (slow - converging) wrote: > remove Done.
https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:9: \n\n or something https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:14: \n\n https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:27: res = re.match('#define ([^ ]+) (\d+)', line) waiiiiiiiit, whhhhaaaat? https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:42: #print '%s: %s' % (resource_id, text) debug https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:50: \n\n https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:54: 'out/gchrome_gn/gen/chrome/browser/resources/settings/' + nope https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:25: i think we could do one of two things: a) add one more line here b) remove all the lines between these slews of constants and argue they're /related enough/ to have no lines between them fwiw: look at src/PRESUBMIT.py for an example https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:88: for tup in mappings: I don't think |tup| is an explanatory variable name maybe this? for i, (redirect_url, file_path) in enumerate(mappings): if url.startswith(redirect_url): return url.replace(redirect_url, file_path + '/') https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:89: res = re.match(tup[0], url) don't use re, use url.startswith() https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:118: f.close() nit: use with, which will auto-close the file for us with open(os.path.join(os.getcwd(), depfile), 'w') as d: out_path = os.path.join(out_folder, html_out_file) d.write(out_path + ': ' + ' '.join(dependencies)) https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:132: ['--out-request-list', os.path.join(out_path, 'request_list.txt'), can we make a tempfile instead of using request_list.txt? https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:174: parser.add_argument('--depfile') nit: move to top? (alpha) https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:194: \n\n
Addressed most comments (all except one), but I also discovered an issue that needs fixing before this CL is ready for another look. Specifically the generated depfile for the case where we do GRIT -> unpack -> vulcanize -> GRIT, holds wrong paths, needs to be fixed. Will continue Monday, you can hold off until I ping again. Thanks. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:9: On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > \n\n or something Done. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:14: On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > \n\n Done. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:27: res = re.match('#define ([^ ]+) (\d+)', line) On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > waiiiiiiiit, whhhhaaaat? Right, sooo as discussed, we are parsing the generated .h file to effectively associate the GRIT output (which is a big concatenated file) back to the original files. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:42: #print '%s: %s' % (resource_id, text) On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > debug Removed. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:50: On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > \n\n Done. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:54: 'out/gchrome_gn/gen/chrome/browser/resources/settings/' + On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > nope Did not address this yet, but planning to do so before this CL is ready for another look. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:25: On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > i think we could do one of two things: > > a) add one more line here > > b) remove all the lines between these slews of constants and argue they're /related enough/ to have no lines between them > > fwiw: look at src/PRESUBMIT.py for an example Added one more line. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:88: for tup in mappings: On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > I don't think |tup| is an explanatory variable name > > maybe this? > > for i, (redirect_url, file_path) in enumerate(mappings): > if url.startswith(redirect_url): > return url.replace(redirect_url, file_path + '/') Done, sort of. No need to use enumerate(). https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:89: res = re.match(tup[0], url) On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > don't use re, use url.startswith() Done. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:118: f.close() On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > nit: use with, which will auto-close the file for us > > with open(os.path.join(os.getcwd(), depfile), 'w') as d: > out_path = os.path.join(out_folder, html_out_file) > d.write(out_path + ': ' + ' '.join(dependencies)) Done, used with. Did not create a temp out_path var though, because there is already a variable with that name in scope. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:132: ['--out-request-list', os.path.join(out_path, 'request_list.txt'), On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > can we make a tempfile instead of using request_list.txt? Per offline discussion, I am keeping this file under the output folder corresponding to the currently processed WebUI page (for example gen/chrome/browser/resources/md_downloads/), since this is easier for debugging. I also moved 'request_list.txt' into a constant, so that it is not repeated twice. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:174: parser.add_argument('--depfile') On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > nit: move to top? (alpha) Done. https://codereview.chromium.org/2569283002/diff/210001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:194: On 2017/01/20 at 23:49:30, Dan Beam (slow - converging) wrote: > \n\n Done.
looks pretty good except for the hardcoded paths https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:52: \n\n https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:56: 'out/gchrome_gn/gen/chrome/browser/resources/settings/' + you need to pass like root_gen_dir or something https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:120: out_folder, html_out_file + ': ') + ' '.join(dependencies)) move the + ': ' outside of the join, it looks like the file is named that
Addressed comments, and also addressed the issue I mentioned at comment#18. PTAL https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:52: On 2017/01/21 at 02:39:23, Dan Beam wrote: > \n\n Done. https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:56: 'out/gchrome_gn/gen/chrome/browser/resources/settings/' + On 2017/01/21 at 02:39:23, Dan Beam wrote: > you need to pass like root_gen_dir or something Removed main(). That was only useful while debugging, so that I could invoke unpack_pak.py directly, but is otherwise not useful for vulcanize_gn.py. https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/230001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:120: out_folder, html_out_file + ': ') + ' '.join(dependencies)) On 2017/01/21 at 02:39:23, Dan Beam wrote: > move the + ': ' outside of the join, it looks like the file is named that Done.
lgtm https://codereview.chromium.org/2569283002/diff/250001/chrome/browser/resourc... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/250001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:33: resource_ids[int(res.group(2))] = res.group(1) assert resources_ids https://codereview.chromium.org/2569283002/diff/250001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:42: resource_filenames[res.group(2)] = res.group(1) assert resource_filenames
Addressed comments. I am also spawning tryjobs in child CLs to see if everything works on other platforms after all the changes in this CL. https://codereview.chromium.org/2569283002/diff/250001/chrome/browser/resourc... File chrome/browser/resources/unpack_pak.py (right): https://codereview.chromium.org/2569283002/diff/250001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:33: resource_ids[int(res.group(2))] = res.group(1) On 2017/01/23 at 23:39:34, Dan Beam wrote: > assert resources_ids Done. https://codereview.chromium.org/2569283002/diff/250001/chrome/browser/resourc... chrome/browser/resources/unpack_pak.py:42: resource_filenames[res.group(2)] = res.group(1) On 2017/01/23 at 23:39:34, Dan Beam wrote: > assert resource_filenames Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:9: "//chrome/browser/resources/unpack_pak.py", FYI, added this line, such that vulcanize() actions re-trigger if unpack_pak.py is modified.
still lgtm https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... File chrome/browser/resources/vulcanize.gni (right): https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize.gni:9: "//chrome/browser/resources/unpack_pak.py", On 2017/01/24 00:36:48, dpapad wrote: > FYI, added this line, such that vulcanize() actions re-trigger if unpack_pak.py > is modified. good idea
https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:97: return url.replace(redirect_url, file_path + '/') maybe use os.sep instead? https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:149: os.path.join('/', html_in_file)]) might want to check / use as well here
https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:72: lambda m: ['--redirect', '%s|%s' % (m[0], m[1])], _URL_MAPPINGS))) everything with a | char needs to be quoted with double quotes so this would become: _VULCANIZE_REDIRECT_ARGS = list(itertools.chain.from_iterable(map( lambda m: ['--redirect', '"%s|%s"' % (m[0], m[1])], _URL_MAPPINGS))) ^ ^ https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:82: cmd = "'" + "' '".join(cmd_parts) + "'" this needs to be cmd = ' '.join(cmd_parts) as in: drop the single quotes https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:144: [_NODE_BINARY, node_modules.PathToVulcanize()] + just change run_cmd to run_node and move _NODE_BINARY to the first arg https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:147: '--redirect', '/|%s' % in_path, '--redirect', '"/|%s"' % in_path, ^ ^ https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:148: '--redirect', 'chrome://%s/|%s' % (host, in_path), '--redirect', '"chrome://%s/|%s"' % (host, in_path), ^ ^ https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:166: '--comments', '/Copyright|license|LICENSE|\<\/?if/', '--comments', '"/Copyright|license|LICENSE|\<\/?if/"', ^ ^
Patchset #16 (id:310001) has been deleted
https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:72: lambda m: ['--redirect', '%s|%s' % (m[0], m[1])], _URL_MAPPINGS))) On 2017/01/24 at 02:59:43, Dan Beam wrote: > everything with a | char needs to be quoted with double quotes > > so this would become: > > _VULCANIZE_REDIRECT_ARGS = list(itertools.chain.from_iterable(map( > lambda m: ['--redirect', '"%s|%s"' % (m[0], m[1])], _URL_MAPPINGS))) > ^ ^ Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:82: cmd = "'" + "' '".join(cmd_parts) + "'" On 2017/01/24 at 02:59:43, Dan Beam wrote: > this needs to be > > cmd = ' '.join(cmd_parts) > > as in: drop the single quotes Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:97: return url.replace(redirect_url, file_path + '/') On 2017/01/24 at 02:02:31, Dan Beam wrote: > maybe use os.sep instead? Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:144: [_NODE_BINARY, node_modules.PathToVulcanize()] + On 2017/01/24 at 02:59:43, Dan Beam wrote: > just change run_cmd to run_node and move _NODE_BINARY to the first arg Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:147: '--redirect', '/|%s' % in_path, On 2017/01/24 at 02:59:43, Dan Beam wrote: > '--redirect', '"/|%s"' % in_path, > ^ ^ Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:148: '--redirect', 'chrome://%s/|%s' % (host, in_path), On 2017/01/24 at 02:59:43, Dan Beam wrote: > '--redirect', '"chrome://%s/|%s"' % (host, in_path), > ^ ^ Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:149: os.path.join('/', html_in_file)]) On 2017/01/24 at 02:02:31, Dan Beam wrote: > might want to check / use as well here Done. https://codereview.chromium.org/2569283002/diff/270001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:166: '--comments', '/Copyright|license|LICENSE|\<\/?if/', On 2017/01/24 at 02:59:43, Dan Beam wrote: > '--comments', '"/Copyright|license|LICENSE|\<\/?if/"', > ^ ^ Done.
https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:14: _HERE_PATH = os.path.join(os.path.dirname(__file__)) don't make no sense https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:102: in_path = os.path.join(os.getcwd(), in_folder) can we cache os.getcwd()?
The child CL just had a green Windows tryjob! https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:14: _HERE_PATH = os.path.join(os.path.dirname(__file__)) On 2017/01/24 at 20:22:28, Dan Beam wrote: > don't make no sense Removed os.path.join() https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:102: in_path = os.path.join(os.getcwd(), in_folder) On 2017/01/24 at 20:22:28, Dan Beam wrote: > can we cache os.getcwd()? Done.
https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:102: in_path = os.path.join(os.getcwd(), in_folder) On 2017/01/24 23:19:27, dpapad wrote: > On 2017/01/24 at 20:22:28, Dan Beam wrote: > > can we cache os.getcwd()? > > Done. fwiw: I meant at a file level (i.e. _CMD, like I did in my patch)
https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... File chrome/browser/resources/vulcanize_gn.py (right): https://codereview.chromium.org/2569283002/diff/350001/chrome/browser/resourc... chrome/browser/resources/vulcanize_gn.py:102: in_path = os.path.join(os.getcwd(), in_folder) On 2017/01/24 at 23:22:41, Dan Beam wrote: > On 2017/01/24 23:19:27, dpapad wrote: > > On 2017/01/24 at 20:22:28, Dan Beam wrote: > > > can we cache os.getcwd()? > > > > Done. > > fwiw: I meant at a file level (i.e. _CMD, like I did in my patch) Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2569283002/#ps390001 (title: "_CWD")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 390001, "attempt_start_ts": 1485302350570540,
"parent_rev": "3a5fa84704f2917edc43a1f4c5a827b65e25d867", "commit_rev":
"bb00fc0c1c03279556a11500de26ec31665af1ca"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Add GN rules for Vulcanize. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Add GN rules for Vulcanize. BUG=673825 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2569283002 Cr-Commit-Position: refs/heads/master@{#445898} Committed: https://chromium.googlesource.com/chromium/src/+/bb00fc0c1c03279556a11500de26... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:390001) as https://chromium.googlesource.com/chromium/src/+/bb00fc0c1c03279556a11500de26... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
