Skip to content

Commit

Permalink
chore: add explanation to resource_file_conflict.patch (#15612)
Browse files Browse the repository at this point in the history
* chore: add explanation to resource_file_conflict.patch

* Update resource_file_conflict.patch
  • Loading branch information
nornagon authored and John Kleinschmidt committed Nov 7, 2018
1 parent bf3edf8 commit 20a540e
Showing 1 changed file with 44 additions and 0 deletions.
44 changes: 44 additions & 0 deletions patches/common/chromium/resource_file_conflict.patch
Expand Up @@ -8,6 +8,50 @@ that chrome code hardcodes require that we generate resources at these
paths, but GN throws errors if there are multiple targets that generate the
same files.

This is due to the hardcoded names here:
https://chromium.googlesource.com/chromium/src/+/69.0.3497.106/ui/base/resource/resource_bundle.cc#780
and here:
https://chromium.googlesource.com/chromium/src/+/69.0.3497.106/ui/base/resource/resource_bundle_mac.mm#50

This isn't needed on Mac because resource files are copied into the app bundle,
and are built in `$root_out_dir/electron_repack` (while Chromium's resources
target `$root_out_dir/repack`), but on Windows and Linux, the resource files go
directly in `$root_out_dir`, and so they conflict.

We don't actually ever generate Chromium's resource paks, but without this
patch, GN refuses to generate the ninja files:

ERROR at //tools/grit/repack.gni:35:3: Duplicate output file.
action(_repack_target_name) {
^----------------------------
Two or more targets generate the same output:
chrome_100_percent.pak

This is can often be fixed by changing one of the target names, or by
setting an output_name on one of them.

Collisions:
//chrome:packed_resources_100_percent
//electron:packed_resources_100_percent

See //tools/grit/repack.gni:35:3: Collision.
action(_repack_target_name) {
^----------------------------


Some alternatives to this patch:

1. Refactor upstream in such a way that the "chrome" pak names were
configurable, for instance by adding a method to ResourceBundle::Delegate that
LoadChromeResources would check.
2. Pass a Delegate that overrides `GetPathForResourcePack`, check for the
`chrome_{100,200}_percent.pak` filenames, and rewrite them to
`electron_{100,200}_percent.pak`.
3. Initialize the resource bundle with DO_NOT_LOAD_COMMON_RESOURCES and load
the paks ourselves.

None of these options seems like a substantial maintainability win over this patch to me (@nornagon).

diff --git a/chrome/BUILD.gn b/chrome/BUILD.gn
index a47af42a5a5afc1560f11ee0ccfa5fc177745caa..db8311121e755eb955ff05dee61a9706ae747977 100644
--- a/chrome/BUILD.gn
Expand Down

0 comments on commit 20a540e

Please sign in to comment.