Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1338393003:
GN: Fix ChromePublic crash on launch from misconfigured resource overrides (Closed)
|
Created:
5 years, 3 months ago by agrieve Modified:
5 years, 2 months ago Reviewers:
newt (away) CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: Fix ChromePublic crash on launch from misconfigured resource overrides
Turns out process_resources.py never actually correctly supported
resource overrides when using --extra-r-text-files, --extra-res-packages
BUG=531529
Committed: https://crrev.com/1db26ac88b5195d575f23fe6120ec1f643950300
Cr-Commit-Position: refs/heads/master@{#349116}
Patch Set 1 #
Total comments: 10
Patch Set 2 : review comments 1` #
Total comments: 2
Patch Set 3 : Don't allow duplicate package names in process_resources.py #Patch Set 4 : line length #
Depends on Patchset: Messages
Total messages: 22 (7 generated)
agrieve@chromium.org changed reviewers: + newt@chromium.org
https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:154: # Map of package_id->resource_type->entry s/package_id/package_name/ https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:157: seen_entries_by_package = collections.defaultdict(set) This is never read from. Remove it? https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:161: if os.path.exists(r_text_file): To reduce nesting: if not os.path.exists(r_text_file): continue Also, why is it OK for R.txt not to exist? Why isn't that an error? https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:170: seen_entries.add(entry_key) What's the reason for enforcing that an entry is included in only one R.java file? https://codereview.chromium.org/1338393003/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1338393003/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:342: ":chrome_java_resources", Does this ensure that these resources override chrome_java_resources? I'd document that fact clearly since it's subtle and can easily break if this ever gets refactored.
https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:154: # Map of package_id->resource_type->entry On 2015/09/15 00:59:37, newt wrote: > s/package_id/package_name/ Done. https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:157: seen_entries_by_package = collections.defaultdict(set) On 2015/09/15 00:59:37, newt wrote: > This is never read from. Remove it? It's used on line 162. https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:161: if os.path.exists(r_text_file): On 2015/09/15 00:59:37, newt wrote: > To reduce nesting: > > if not os.path.exists(r_text_file): > continue > > Also, why is it OK for R.txt not to exist? Why isn't that an error? Done (nesting) Possible it's not needed, but it was there already and I'm too much of a coward to remove it. https://codereview.chromium.org/1338393003/diff/1/build/android/gyp/process_r... build/android/gyp/process_resources.py:170: seen_entries.add(entry_key) On 2015/09/15 00:59:37, newt wrote: > What's the reason for enforcing that an entry is included in only one R.java > file? It's not that we're making sure it appears in only one R.java, it's that we're making sure that it doesn't appear multiple times within the same R.java (or else compile fails). https://codereview.chromium.org/1338393003/diff/1/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1338393003/diff/1/chrome/android/BUILD.gn#new... chrome/android/BUILD.gn:342: ":chrome_java_resources", On 2015/09/15 00:59:37, newt wrote: > Does this ensure that these resources override chrome_java_resources? I'd > document that fact clearly since it's subtle and can easily break if this ever > gets refactored. Added a comment. The logic for this actually already existed within write_build_config.py, but the logic in process_resources.py was broken.
lgtm after question https://codereview.chromium.org/1338393003/diff/20001/build/android/gyp/proce... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/1338393003/diff/20001/build/android/gyp/proce... build/android/gyp/process_resources.py:157: seen_entries_by_package = collections.defaultdict(set) Are there legitimate cases where extra_packages contains the same package name multiple times? If so, I understand why this exists... but I don't see why we'd need to support duplicate package names in extra_packages.
https://codereview.chromium.org/1338393003/diff/20001/build/android/gyp/proce... File build/android/gyp/process_resources.py (right): https://codereview.chromium.org/1338393003/diff/20001/build/android/gyp/proce... build/android/gyp/process_resources.py:157: seen_entries_by_package = collections.defaultdict(set) On 2015/09/15 17:07:53, newt wrote: > Are there legitimate cases where extra_packages contains the same package name > multiple times? If so, I understand why this exists... but I don't see why we'd > need to support duplicate package names in extra_packages. Ah, now I see. You're reusing org.chromium.chrome when overriding the resources downstream. The alternative is to simply use a different package name when overriding the resources. The package name is irrelevant after all when overriding resources -- it's the resource names that matter.
On 2015/09/15 17:14:01, newt wrote: > https://codereview.chromium.org/1338393003/diff/20001/build/android/gyp/proce... > File build/android/gyp/process_resources.py (right): > > https://codereview.chromium.org/1338393003/diff/20001/build/android/gyp/proce... > build/android/gyp/process_resources.py:157: seen_entries_by_package = > collections.defaultdict(set) > On 2015/09/15 17:07:53, newt wrote: > > Are there legitimate cases where extra_packages contains the same package name > > multiple times? If so, I understand why this exists... but I don't see why > we'd > > need to support duplicate package names in extra_packages. > > Ah, now I see. You're reusing org.chromium.chrome when overriding the resources > downstream. The alternative is to simply use a different package name when > overriding the resources. The package name is irrelevant after all when > overriding resources -- it's the resource names that matter. Ah, and now I see! There's no namespacing of resources, so overrides can use any package. I've removed the package merging and made duplicate package names a hard error. I've also update the GN rules to just not have any package_name for the overrides (and thus skip generating an R.java for them as well). PTAL
lgtm++
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338393003/20002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338393003/20002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org Link to the patchset: https://codereview.chromium.org/1338393003/#ps50001 (title: "line length")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338393003/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338393003/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1338393003/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1338393003/50001
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1db26ac88b5195d575f23fe6120ec1f643950300 Cr-Commit-Position: refs/heads/master@{#349116}
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1db26ac88b5195d575f23fe6120ec1f643950300 Cr-Commit-Position: refs/heads/master@{#349116} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
