Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
998373004:
Sync: Generalize entity injection in Android tests (Closed)
|
Created:
5 years, 9 months ago by pval...(no longer on Chromium) Modified:
5 years, 8 months ago CC:
chromium-reviews, maniscalco+watch_chromium.org, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, tim+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSync: Generalize entity injection in Android tests
This CL introduces build infrastructure to generate Java objects from
sync protobufs. This allows Java tests to create the EntitySpecifics
of entities that they would like to inject.
As of this CL, only entities with unique client tags (e.g., preferences,
typed URLs) are supported for injection.
BUG=365774
Committed: https://crrev.com/cc3d8cbe481e8f21341d8006a8f62a8915343ae6
Cr-Commit-Position: refs/heads/master@{#324558}
Patch Set 1 : #
Total comments: 20
Patch Set 2 : #
Total comments: 9
Patch Set 3 : #
Total comments: 19
Patch Set 4 : zea review #Patch Set 5 : rebase #Patch Set 6 : fix CreateForInjection calls #Messages
Total messages: 30 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
pvalenzuela@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@, This is not yet ready for commit as there are still some open issues. I've commented these with a '?' where I'd like some of your help. :-) https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:32: os.system("sed -i 's/option retain_unknown_fields = true;/" should I match whitespace here? for example, what if the proto files are like this: option retain_unknown_fields = true; https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:483: # instead of duplicating this list? ? https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:533: # How can we autogenerate this list? ? I think this could go away if sync_prepare_protos_for_java_tests and test_support_sync_proto_java were part of the same target. See question below about that. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:596: # of SHARED_INTERMEDIATE_DIR because it could be in a single target. ? I couldn't figure out how to force ordering of actions. protoc_java.gypi defines its own actions (compile the protos), but I need my actions to run first. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:601: # How can we autogenerate this list? ? Similar to above, I think this could be removed if all work is done in the same target.
On 2015/03/13 00:32:45, pvalenzuela wrote: > nyquist@, > > This is not yet ready for commit as there are still some open issues. I've > commented these with a '?' where I'd like some of your help. :-) > > https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... > File sync/protocol/prepare_protos_for_java_tests.py (right): > > https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... > sync/protocol/prepare_protos_for_java_tests.py:32: os.system("sed -i 's/option > retain_unknown_fields = true;/" > should I match whitespace here? for example, what if the proto files are like > this: > > option retain_unknown_fields = true; > > https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi > File sync/sync_tests.gypi (right): > > https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... > sync/sync_tests.gypi:483: # instead of duplicating this list? > ? > > https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... > sync/sync_tests.gypi:533: # How can we autogenerate this list? > ? > > I think this could go away if sync_prepare_protos_for_java_tests and > test_support_sync_proto_java were part of the same target. See question below > about that. > > https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... > sync/sync_tests.gypi:596: # of SHARED_INTERMEDIATE_DIR because it could be in a > single target. > ? > > I couldn't figure out how to force ordering of actions. protoc_java.gypi defines > its own actions (compile the protos), but I need my actions to run first. > > https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... > sync/sync_tests.gypi:601: # How can we autogenerate this list? > ? > > Similar to above, I think this could be removed if all work is done in the same > target. ping
https://codereview.chromium.org/998373004/diff/60001/chrome/android/sync_shel... File chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java (right): https://codereview.chromium.org/998373004/diff/60001/chrome/android/sync_shel... chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java:275: mFakeServerHelper.injectUniqueClientEntity(url /* name */, specifics); This is so cool! https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:1: #!/usr/bin/python I think the new hotness is to use #!/usr/bin/env python https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:11: def main(): What does this script do? Could you add a python-specific docstring at the top of the file? https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:32: os.system("sed -i 's/option retain_unknown_fields = true;/" On 2015/03/13 at 00:32:45, pvalenzuela wrote: > should I match whitespace here? for example, what if the proto files are like this: > > option retain_unknown_fields = true; I'd use built in tools in python for regexp here instead of shelling out. It would require reading the file, but I find that more correct I believe. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:483: # instead of duplicating this list? On 2015/03/13 at 00:32:45, pvalenzuela wrote: > ? Yeah, I think if you split that out into its own list, you can re-use the list of files. We do that for example for sharing file lists between GYP and GN. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:596: # of SHARED_INTERMEDIATE_DIR because it could be in a single target. On 2015/03/13 at 00:32:45, pvalenzuela wrote: > ? > > I couldn't figure out how to force ordering of actions. protoc_java.gypi defines its own actions (compile the protos), but I need my actions to run first. Yeah, I'm unsure if you can easily order actions in GYP. I've done similar "action-as-target" tricks other places at least. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:601: # How can we autogenerate this list? On 2015/03/13 at 00:32:45, pvalenzuela wrote: > ? > > Similar to above, I think this could be removed if all work is done in the same target. In GN you could use 'process_file_template' to generate this, or use foreach (if process_file_template doesn't work). For GYP, you could move the whole list into a .gypi-file with a prefix. So in your gpi <(PREFIX)/sync.proto <(PREFIX)/app_notification_specifics.proto ... Then, inside the variable block in each action, you'd set the prefix correctly for that action, and then you'd include the .gypi where you want the list.
https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:596: # of SHARED_INTERMEDIATE_DIR because it could be in a single target. On 2015/03/24 at 00:25:36, nyquist wrote: > On 2015/03/13 at 00:32:45, pvalenzuela wrote: > > ? > > > > I couldn't figure out how to force ordering of actions. protoc_java.gypi defines its own actions (compile the protos), but I need my actions to run first. > > Yeah, I'm unsure if you can easily order actions in GYP. I've done similar "action-as-target" tricks other places at least. Oh, by the way. Ordering between actions should work correctly if you have your inputs and outputs correctly set. That should enforce ordering.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Ok, I made some progress here. PTAL. https://codereview.chromium.org/998373004/diff/60001/chrome/android/sync_shel... File chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java (right): https://codereview.chromium.org/998373004/diff/60001/chrome/android/sync_shel... chrome/android/sync_shell/javatests/src/chromium/chrome/browser/sync/SyncTest.java:275: mFakeServerHelper.injectUniqueClientEntity(url /* name */, specifics); On 2015/03/24 00:25:35, nyquist wrote: > This is so cool! :) it'll be cooler when we have tests for all/most types https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:1: #!/usr/bin/python On 2015/03/24 00:25:35, nyquist wrote: > I think the new hotness is to use > #!/usr/bin/env python Done. https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:11: def main(): On 2015/03/24 00:25:36, nyquist wrote: > What does this script do? Could you add a python-specific docstring at the top > of the file? Done. https://codereview.chromium.org/998373004/diff/60001/sync/protocol/prepare_pr... sync/protocol/prepare_protos_for_java_tests.py:32: os.system("sed -i 's/option retain_unknown_fields = true;/" On 2015/03/24 00:25:36, nyquist wrote: > On 2015/03/13 at 00:32:45, pvalenzuela wrote: > > should I match whitespace here? for example, what if the proto files are like > this: > > > > option retain_unknown_fields = true; > > I'd use built in tools in python for regexp here instead of shelling out. It > would require reading the file, but I find that more correct I believe. Done. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:483: # instead of duplicating this list? On 2015/03/24 00:25:36, nyquist wrote: > On 2015/03/13 at 00:32:45, pvalenzuela wrote: > > ? > > Yeah, I think if you split that out into its own list, you can re-use the list > of files. We do that for example for sharing file lists between GYP and GN. fixed in sync/sync.gyp. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:596: # of SHARED_INTERMEDIATE_DIR because it could be in a single target. On 2015/03/24 01:16:54, nyquist wrote: > On 2015/03/24 at 00:25:36, nyquist wrote: > > On 2015/03/13 at 00:32:45, pvalenzuela wrote: > > > ? > > > > > > I couldn't figure out how to force ordering of actions. protoc_java.gypi > defines its own actions (compile the protos), but I need my actions to run > first. > > > > Yeah, I'm unsure if you can easily order actions in GYP. I've done similar > "action-as-target" tricks other places at least. > > Oh, by the way. Ordering between actions should work correctly if you have your > inputs and outputs correctly set. That should enforce ordering. The targers have been merged by properly defining inputs/outputs. https://codereview.chromium.org/998373004/diff/60001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:601: # How can we autogenerate this list? On 2015/03/24 00:25:36, nyquist wrote: > On 2015/03/13 at 00:32:45, pvalenzuela wrote: > > ? > > > > Similar to above, I think this could be removed if all work is done in the > same target. > > In GN you could use 'process_file_template' to generate this, or use foreach (if > process_file_template doesn't work). > > For GYP, you could move the whole list into a .gypi-file with a prefix. > So in your gpi > <(PREFIX)/sync.proto > <(PREFIX)/app_notification_specifics.proto > ... > > Then, inside the variable block in each action, you'd set the prefix correctly > for that action, and then you'd include the .gypi where you want the list. Added a sync/protocol/protocol.gypi to accomplish this. https://codereview.chromium.org/998373004/diff/120001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/120001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:473: # create GN version Do you consider this a requirement for this CL? I don't even think the ChromeSyncShell compiles in GN right now...
ping
This is so exciting! lgtm https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:43: for file_name in original_proto_contents: Nit: How about for file_name, original_contents in original_proto_contents.iteritems(): new_contents = ConvertProtoFileContents(original_contents) ? https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:57: r'option retain_unknown_fields = true;') Nit: How about r'^\s*?option\s+?retain_unknown_fields\s*?=\s*?true\s*?;' ? https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:65: syntax_regex = re.compile(r'syntax\s*=.*;') Nit: Add '^\s*' before syntax? https://codereview.chromium.org/998373004/diff/120001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/120001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:473: # create GN version On 2015/03/26 21:17:57, pvalenzuela wrote: > Do you consider this a requirement for this CL? I don't even think the > ChromeSyncShell compiles in GN right now... I'd trust you to follow up on a TODO.
pvalenzuela@chromium.org changed reviewers: + zea@chromium.org
+zea for review of changes in sync/protocol https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:43: for file_name in original_proto_contents: On 2015/04/01 00:01:00, nyquist (OOO APR 3-7) wrote: > Nit: How about > for file_name, original_contents in original_proto_contents.iteritems(): > new_contents = ConvertProtoFileContents(original_contents) > ? Done. https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:57: r'option retain_unknown_fields = true;') On 2015/04/01 00:01:00, nyquist (OOO APR 3-7) wrote: > Nit: How about r'^\s*?option\s+?retain_unknown_fields\s*?=\s*?true\s*?;' ? I don't think the non-greedy addition (?) is necessary. Other than that, I've updated the regex. https://codereview.chromium.org/998373004/diff/120001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:65: syntax_regex = re.compile(r'syntax\s*=.*;') On 2015/04/01 00:01:00, nyquist (OOO APR 3-7) wrote: > Nit: Add '^\s*' before syntax? Done. https://codereview.chromium.org/998373004/diff/120001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/120001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:473: # create GN version On 2015/04/01 00:01:00, nyquist (OOO APR 3-7) wrote: > On 2015/03/26 21:17:57, pvalenzuela wrote: > > Do you consider this a requirement for this CL? I don't even think the > > ChromeSyncShell compiles in GN right now... > > I'd trust you to follow up on a TODO. todo+bug added
This is awesome! Couple comments/questions https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2968: '../sync/sync.gyp:test_support_sync_proto_java', what's the typical naming scheme here? it seems like base_java_test_support is using a different convention than test_support_sync_proto_java. Should we match them? https://codereview.chromium.org/998373004/diff/140001/sync/protocol/prepare_p... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/140001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:49: def ConvertProtoFileContents(contents): I wonder if we should have a test for this? Although I guess if a bug were to be introduced here the compiles would fail, right? https://codereview.chromium.org/998373004/diff/140001/sync/protocol/protocol.... File sync/protocol/protocol.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/protocol/protocol.... sync/protocol/protocol.gypi:7: # The directory that contains the sync protocol buffer definitions. nit: comment that this can be overriden for certain builds to, for example to point to an intermediate directory? https://codereview.chromium.org/998373004/diff/140001/sync/sync.gyp File sync/sync.gyp (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync.gyp#newcode19 sync/sync.gyp:19: 'protocol/protocol.gypi', nit: abc order https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:477: 'proto_in_dir': '<(INTERMEDIATE_DIR)/sync_protos', is this used for anything? https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:499: '<@(sync_proto_sources)' This might be my lack of gyp experience, but sync_proto_sources, as defined here, won't include INTERMEDIATE_DIR in its path? Even though we do expect sync_proto_source_paths (a few lines below) to have INTERMEDIATE_DIR?
https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2968: '../sync/sync.gyp:test_support_sync_proto_java', On 2015/04/09 20:38:16, Nicolas Zea wrote: > what's the typical naming scheme here? it seems like base_java_test_support is > using a different convention than test_support_sync_proto_java. Should we match > them? Most test infra targets I've encountered are test_support_FOO and all Java proto targets are FOO_proto_java, so my creation here is a Frankenstein-like result. I think base_java_test_support may be the outlier here, although there are some other FOO_java_test_support targets. All things considered, I'm fine with the current name as it describes the important parts: it's for tests, it's for sync, and it contains protos. Another idea: sync_test_proto_java? https://codereview.chromium.org/998373004/diff/140001/sync/protocol/prepare_p... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/140001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:49: def ConvertProtoFileContents(contents): On 2015/04/09 20:38:16, Nicolas Zea wrote: > I wonder if we should have a test for this? Although I guess if a bug were to be > introduced here the compiles would fail, right? Yes, compilation would fail. However, that's no guarantee this script works as 100% intended. There is precedence for adding a build process Python test (see this change: https://codereview.chromium.org/896663002). Thoughts? https://codereview.chromium.org/998373004/diff/140001/sync/protocol/protocol.... File sync/protocol/protocol.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/protocol/protocol.... sync/protocol/protocol.gypi:7: # The directory that contains the sync protocol buffer definitions. On 2015/04/09 20:38:16, Nicolas Zea wrote: > nit: comment that this can be overriden for certain builds to, for example to > point to an intermediate directory? Done. https://codereview.chromium.org/998373004/diff/140001/sync/sync.gyp File sync/sync.gyp (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync.gyp#newcode19 sync/sync.gyp:19: 'protocol/protocol.gypi', On 2015/04/09 20:38:16, Nicolas Zea wrote: > nit: abc order Done. https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:477: 'proto_in_dir': '<(INTERMEDIATE_DIR)/sync_protos', On 2015/04/09 20:38:16, Nicolas Zea wrote: > is this used for anything? yeah. see protoc_java.gypi (included in line 506 here) https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:499: '<@(sync_proto_sources)' On 2015/04/09 20:38:16, Nicolas Zea wrote: > This might be my lack of gyp experience, but sync_proto_sources, as defined > here, won't include INTERMEDIATE_DIR in its path? Even though we do expect > sync_proto_source_paths (a few lines below) to have INTERMEDIATE_DIR? I believe the dir is in the path/can be accessed by default. This is supported by the fact that it works as-is, and this description (from GYP docs): "INTERMEDIATE_DIR: A directory that can be used to place intermediate build results in. INTERMEDIATE_DIR is only guaranteed to be accessible within a single target (See targets). This variable is most useful within the context of rules and actions (See rules, See actions). Compare with SHARED_INTERMEDIATE_DIR."
LGTM! https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2968: '../sync/sync.gyp:test_support_sync_proto_java', On 2015/04/09 21:28:05, pvalenzuela wrote: > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > what's the typical naming scheme here? it seems like base_java_test_support is > > using a different convention than test_support_sync_proto_java. Should we > match > > them? > > Most test infra targets I've encountered are test_support_FOO and all Java proto > targets are FOO_proto_java, so my creation here is a Frankenstein-like result. > > I think base_java_test_support may be the outlier here, although there are some > other FOO_java_test_support targets. > > All things considered, I'm fine with the current name as it describes the > important parts: it's for tests, it's for sync, and it contains protos. Another > idea: sync_test_proto_java? I'm fine with this, as long as it's mostly consistent. https://codereview.chromium.org/998373004/diff/140001/sync/protocol/prepare_p... File sync/protocol/prepare_protos_for_java_tests.py (right): https://codereview.chromium.org/998373004/diff/140001/sync/protocol/prepare_p... sync/protocol/prepare_protos_for_java_tests.py:49: def ConvertProtoFileContents(contents): On 2015/04/09 21:28:05, pvalenzuela wrote: > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > I wonder if we should have a test for this? Although I guess if a bug were to > be > > introduced here the compiles would fail, right? > > Yes, compilation would fail. However, that's no guarantee this script works as > 100% intended. There is precedence for adding a build process Python test (see > this change: https://codereview.chromium.org/896663002). > > Thoughts? As long as compile would fail I think we're fine (and that the targets that would fail are part of the normal trybots, which they are). https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:477: 'proto_in_dir': '<(INTERMEDIATE_DIR)/sync_protos', On 2015/04/09 21:28:05, pvalenzuela wrote: > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > is this used for anything? > > yeah. see protoc_java.gypi (included in line 506 here) Acknowledged.
https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2968: '../sync/sync.gyp:test_support_sync_proto_java', On 2015/04/09 21:40:42, Nicolas Zea wrote: > On 2015/04/09 21:28:05, pvalenzuela wrote: > > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > > what's the typical naming scheme here? it seems like base_java_test_support > is > > > using a different convention than test_support_sync_proto_java. Should we > > match > > > them? > > > > Most test infra targets I've encountered are test_support_FOO and all Java > proto > > targets are FOO_proto_java, so my creation here is a Frankenstein-like result. > > > > I think base_java_test_support may be the outlier here, although there are > some > > other FOO_java_test_support targets. > > > > All things considered, I'm fine with the current name as it describes the > > important parts: it's for tests, it's for sync, and it contains protos. > Another > > idea: sync_test_proto_java? > > I'm fine with this, as long as it's mostly consistent. To be honest, I think I like the sync_proto_java_test_support better. But not important though, since like Patrick said, it's a Frankestein thing with two naming concepts mixed. https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:499: '<@(sync_proto_sources)' On 2015/04/09 21:28:05, pvalenzuela wrote: > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > This might be my lack of gyp experience, but sync_proto_sources, as defined > > here, won't include INTERMEDIATE_DIR in its path? Even though we do expect > > sync_proto_source_paths (a few lines below) to have INTERMEDIATE_DIR? > > I believe the dir is in the path/can be accessed by default. This is supported > by the fact that it works as-is, and this description (from GYP docs): > > "INTERMEDIATE_DIR: A directory that can be used to place intermediate build > results in. INTERMEDIATE_DIR is only guaranteed to be accessible within a single > target (See targets). This variable is most useful within the context of rules > and actions (See rules, See actions). Compare with SHARED_INTERMEDIATE_DIR." zea: sync_proto_sources is set to the expansion of sync_proto_source_paths, and sync_proto_source_paths have sync_proto_sources_dir as a prefix in them. Were you concerned about early/late expansion of the variables since it's an indirect usage of it?
https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:499: '<@(sync_proto_sources)' On 2015/04/09 21:53:52, nyquist wrote: > On 2015/04/09 21:28:05, pvalenzuela wrote: > > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > > This might be my lack of gyp experience, but sync_proto_sources, as defined > > > here, won't include INTERMEDIATE_DIR in its path? Even though we do expect > > > sync_proto_source_paths (a few lines below) to have INTERMEDIATE_DIR? > > > > I believe the dir is in the path/can be accessed by default. This is supported > > by the fact that it works as-is, and this description (from GYP docs): > > > > "INTERMEDIATE_DIR: A directory that can be used to place intermediate build > > results in. INTERMEDIATE_DIR is only guaranteed to be accessible within a > single > > target (See targets). This variable is most useful within the context of rules > > and actions (See rules, See actions). Compare with SHARED_INTERMEDIATE_DIR." > > zea: sync_proto_sources is set to the expansion of sync_proto_source_paths, and > sync_proto_source_paths have sync_proto_sources_dir as a prefix in them. Were > you concerned about early/late expansion of the variables since it's an indirect > usage of it? Right, it wasn't clear to me when the actual expansion would take place. But if it works then I guess this is right.
https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/998373004/diff/140001/sync/sync_tests.gypi#ne... sync/sync_tests.gypi:499: '<@(sync_proto_sources)' On 2015/04/09 21:59:56, Nicolas Zea wrote: > On 2015/04/09 21:53:52, nyquist wrote: > > On 2015/04/09 21:28:05, pvalenzuela wrote: > > > On 2015/04/09 20:38:16, Nicolas Zea wrote: > > > > This might be my lack of gyp experience, but sync_proto_sources, as > defined > > > > here, won't include INTERMEDIATE_DIR in its path? Even though we do expect > > > > sync_proto_source_paths (a few lines below) to have INTERMEDIATE_DIR? > > > > > > I believe the dir is in the path/can be accessed by default. This is > supported > > > by the fact that it works as-is, and this description (from GYP docs): > > > > > > "INTERMEDIATE_DIR: A directory that can be used to place intermediate build > > > results in. INTERMEDIATE_DIR is only guaranteed to be accessible within a > > single > > > target (See targets). This variable is most useful within the context of > rules > > > and actions (See rules, See actions). Compare with SHARED_INTERMEDIATE_DIR." > > > > zea: sync_proto_sources is set to the expansion of sync_proto_source_paths, > and > > sync_proto_source_paths have sync_proto_sources_dir as a prefix in them. Were > > you concerned about early/late expansion of the variables since it's an > indirect > > usage of it? > > Right, it wasn't clear to me when the actual expansion would take place. But if > it works then I guess this is right. ah, I misread the question. I believe it happens in the right order (given its current functionality).
The CQ bit was checked by pvalenzuela@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/998373004/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/998373004/180001
The CQ bit was unchecked by pvalenzuela@chromium.org
The CQ bit was checked by pvalenzuela@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/998373004/#ps200001 (title: "fix CreateForInjection calls")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/998373004/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cc3d8cbe481e8f21341d8006a8f62a8915343ae6 Cr-Commit-Position: refs/heads/master@{#324558} |
