Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 3 months ago by msimonides Modified:
7 years, 2 months ago Reviewers:
cduvall, not at google - send to devlin CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake SchemaLoader independent of current working directory.
The SchemaLoader looks for schema files in the current directory. This
isn't very robust and may cause problems for other products than
Chromium if they run the script from a GYP file in another directory.
The SchemaLoader is supplied two arguments now: the path to API files
relative to the root path (for use in #includes) and the regular path
to API files (for file access).
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225977
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use two separate paths #
Total comments: 6
Patch Set 3 : Code review fixes #
Created: 7 years, 2 months ago
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... tools/json_schema_compiler/schema_loader.py:47: schema_path = os.path.join(self._root, self._api_path, schema); can the caller just make sure that the root is properly prepended to api_path?
https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... tools/json_schema_compiler/schema_loader.py:47: schema_path = os.path.join(self._root, self._api_path, schema); On 2013/09/19 15:43:34, kalman wrote: > can the caller just make sure that the root is properly prepended to api_path? This would be a problem with line 35-36: Model().AddNamespace(). If I modify the _api_path as you suggest, the generated header files will have incorrect #include paths.
https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... tools/json_schema_compiler/schema_loader.py:47: schema_path = os.path.join(self._root, self._api_path, schema); On 2013/09/20 11:32:01, msimonides wrote: > On 2013/09/19 15:43:34, kalman wrote: > > can the caller just make sure that the root is properly prepended to api_path? > > This would be a problem with line 35-36: Model().AddNamespace(). If I modify the > _api_path as you suggest, the generated header files will have incorrect > #include paths. Ah. So there's a long-standing cleanup where we don't actually need the api path to be passed into those Namespaces, they should be passed into the code generators. But unless you're keen to fix that now (which would be great, but I would understand if not), I'd still rather pass in 2 separate paths rather than 1 path and a root.
On 2013/09/20 20:37:57, kalman wrote: > https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... > File tools/json_schema_compiler/schema_loader.py (right): > > https://codereview.chromium.org/23534063/diff/1/tools/json_schema_compiler/sc... > tools/json_schema_compiler/schema_loader.py:47: schema_path = > os.path.join(self._root, self._api_path, schema); > On 2013/09/20 11:32:01, msimonides wrote: > > On 2013/09/19 15:43:34, kalman wrote: > > > can the caller just make sure that the root is properly prepended to > api_path? > > > > This would be a problem with line 35-36: Model().AddNamespace(). If I modify > the > > _api_path as you suggest, the generated header files will have incorrect > > #include paths. > > Ah. So there's a long-standing cleanup where we don't actually need the api path > to be passed into those Namespaces, they should be passed into the code > generators. > > But unless you're keen to fix that now (which would be great, but I would > understand if not), I'd still rather pass in 2 separate paths rather than 1 path > and a root. I had a look at the Namespaces and generators code and I'm afraid I know too little about the schema compiler and how it's supposed to work to mess with it on a bigger scale than local fixes, at least at the moment. So I'm taking the easy path and supplying two paths (it does look better).
ping :)
oops, sorry! you can ping me earlier. lgtm with those comments. https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:50: api_def = schema_loader.LoadSchema(short_filename + schema_extension) if you use os.path.split(schema)[1] rather than (short_filename + schema_extension) then you can actually delete the 2 lines above this one since it doesn't look like those variables are actually used. https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... tools/json_schema_compiler/schema_loader.py:15: def __init__(self, api_path_relative_to_root, api_path): How about: display_path and real_path. Also, add a bit of a comment in the docstring. https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... tools/json_schema_compiler/schema_loader.py:45: the api_path argument passed to the constructor.''' Docstring style is '''Foo Bar. ''' not '''Foo Bar '''
https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... File tools/json_schema_compiler/compiler.py (right): https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... tools/json_schema_compiler/compiler.py:50: api_def = schema_loader.LoadSchema(short_filename + schema_extension) On 2013/09/27 16:10:20, kalman wrote: > if you use os.path.split(schema)[1] rather than (short_filename + > schema_extension) then you can actually delete the 2 lines above this one since > it doesn't look like those variables are actually used. Done. https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... File tools/json_schema_compiler/schema_loader.py (right): https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... tools/json_schema_compiler/schema_loader.py:15: def __init__(self, api_path_relative_to_root, api_path): On 2013/09/27 16:10:20, kalman wrote: > How about: display_path and real_path. Also, add a bit of a comment in the > docstring. Done. https://codereview.chromium.org/23534063/diff/8001/tools/json_schema_compiler... tools/json_schema_compiler/schema_loader.py:45: the api_path argument passed to the constructor.''' On 2013/09/27 16:10:20, kalman wrote: > Docstring style is > > '''Foo > Bar. > ''' > > not > > '''Foo > Bar > ''' Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msimonides@opera.com/23534063/16001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msimonides@opera.com/23534063/16001
Message was sent while issue was closed.
Change committed as 225977 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
