blame says this was written by rockot, but https://chromium-review.googlesource.com/c/chromium/src/+/1231920 says this was written by khorimoto. Please sort out who should look at this :-)

For reference, there are now 3 obj files (out of tens of thousands) in a chrome build that depend on the name of the build directory: This one, and two caused by a goma bug ( issue 899442 ). It'd be good to have this fixed soon, so that we can make the Windows deterministic bot use two different build directories and make sure we don't regress.

Probably just needs something like this: https://cs.chromium.org/search/?q=%5C-E.*%5Cn.*root_gen_dir+pcre:yes&sq=package:chromium&type=cs

Yeah, sorry, I was OOO. I will fix this.

The -E flag is there. The gzipped_resource_map_source is pretty new and not used often; maybe it just doesn't work with variable references? (https://chromium.googlesource.com/chromium/src/+/78cd41e37b74854855b45102083572a8eb18f051)

If you're on it, I'll stop looking, thanks :-)

Yeah sorry I thought this was something else. Looks like a bug in grit I guess?

It seems to me that those generated strings should indeed contain the literal "${root_gen_dir}" but should NOT contain the actual output dir expansion. Right?

Passing on to you because hopefully you either know how to solve this or where to find a better owner.

The gzipped_ part is irrelevant, removing that from the grd file doens't help. I think resource_map just can't be used with ${root_gen_dir} at the moment. That combination only appears in 4 files (https://cs.chromium.org/search/?q=file:%5C.grd+root_gen_dir+resource_map&sq=package:chromium&type=cs) and 3 of them aren't used in debug builds (which is what I'm currently looking at).

Since the path is broken, resource_map probably isn't what you need anyhow, right? Or at least you don't need the path in there?

The path is only used as a unique identifying string AFAICT. So a string is
necessary, but those specific contents are not.

https://chromium-review.googlesource.com/c/chromium/src/+/1307937 makes us use a different string uid that's path-independent.

+dpapad who wrote the other three .grd files that use resource_file_map_source together with a variable reference (https://cs.chromium.org/search/?q=file_map_source+%5C$%5C%7B&sq=package:chromium&type=cs). dpapad, this doesn't work right in grit. I'm guessing your three files are broken as well (but I'm not currently doing a release build, so I don't know). Can we use resource_map_source there instead too? Or do you want to fix grit instead?

@thakis: Can you clarify which part does not currently work? The following files

settings_resources_vulcanized.grd
extensions_resources_vulcanized.grd
print_preview_resources_vulcanized.grd

are holding the optimized resources for chrome://settings, chrome://extensions and chrome://print, and seem to work fine (otherwise those pages would not load correctly).

For example when I visit chrome://settings/crisper.js, I see the correct contents of the bundled JS file for Settings. 

> Can we use resource_map_source there instead too?

Those files are used in C++ similarly to [1]. I don't know whether using resource_map_source affects that, but as long as we can somehow register them to the WebUI data source, should be ok?

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/md_settings_ui.cc?l=395-404

This makes the var ref go away at least:

 def _GetItemPath(item):
-  return item.GetInputPath().replace("\\", "/")
+  return item.ToRealPath(item.GetInputPath()).replace("\\", "/")

But with that, the map still looks like 

  {"../../out/gn/gen/mojo/public/js/mojo_bindings.js", IDR_MOJO_MOJO_BINDINGS_JS, true},


which still contains a ref to the build dir name. ${root_gen_dir} expands to just "gen", I wonder where in grit the "../../out/gn" comes from...

dpapad: If you have access to it, can you paste the contents of out/Release/gen/chrome/grit/settings_resources_map.cc in a build where it's generated from that vulcanized grd file?

cf https://chromium.googlesource.com/external/grit-i18n/+/0fae8b05cb6f7d1995040d75313205d86945871e

FWIW, the gzipped_resource_file_map_source format was added at [1], and it seems still relevant to me.

The generated files can be found on codesearch as well, see

 - https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/grit/settings_resources.h
 - https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/grit/settings_resources_map.cc


[1] https://chromium-review.googlesource.com/c/chromium/src/+/823271/

Actually I realize that the lings point to the Debug, not Release. Attached the release build's output.

Deleted: settings_resources_map.cc 1.2 KB

settings_resources_map.cc 1.2 KB View Download

Right, your attached file contains:

  {"../../../../out/gchrome/${root_gen_dir}/chrome/browser/resources/settings/lazy_load.vulcanized.html", IDR_MD_SETTINGS_LAZY_LOAD_VULCANIZED_HTML},


...which is a bogus path. Hence my belief that nothing can depend on the value of that string, and maybe gzipped_resource_map_source (without file_) might work for you too.



This grit patch makes it so that _file_map_sources work with variables, but so far I'm not sure if anything needs it.

C:\src\chrome\src>git diff tools\grit
diff --git a/tools/grit/grit/format/resource_map.py b/tools/grit/grit/format/resource_map.py
index 5fee06b85403..b8a0f078791f 100755
--- a/tools/grit/grit/format/resource_map.py
+++ b/tools/grit/grit/format/resource_map.py
@@ -143,4 +143,4 @@ def _GetItemName(item):


 def _GetItemPath(item):
-  return item.GetInputPath().replace("\\", "/")
+  return item.ToRealPath(item.GetInputPath()).replace("\\", "/")
diff --git a/tools/grit/grit/node/base.py b/tools/grit/grit/node/base.py
index 76fbb31e0f6b..15cf7cdc95d2 100755
--- a/tools/grit/grit/node/base.py
+++ b/tools/grit/grit/node/base.py
@@ -383,8 +383,14 @@ class Node(object):
     Return:
       'resource'
     '''
-    return util.normpath(os.path.join(self.GetRoot().GetBaseDir(),
-                                      os.path.expandvars(path_from_basedir)))
+    res = util.normpath(os.path.join(self.GetRoot().GetBaseDir(),
+                                     os.path.expandvars(path_from_basedir)))
+    if res.startswith('../') or res.startswith('..\\'):
+      # Suppose res is '../../out/gn/foo' and the cwd is out/gn. Try converting
+      # this to just 'foo'. (If res is '../../chrome/foo', it will remain
+      # unchanged here.)
+      res = os.path.relpath(os.path.abspath(res))
+    return res

   def GetInputPath(self):
     '''Returns a path, relative to the base directory set for the grd file,

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84a6a5f0b7244e679579c6e15957b016e6407dee

commit 84a6a5f0b7244e679579c6e15957b016e6407dee
Author: Nico Weber <thakis@chromium.org>
Date: Tue Oct 30 18:15:43 2018

mojo: Use resource_map_source instead of resource_file_map_source.

resource_map_source generates a map looking like

  { "IDR_FOO", IDR_FOO }

while resource_file_map_source generations a map looking like

  { "path/to/resource", IDR_FOO }

The latter currently doesn't work for files that contain variable references
in their name (e.g. ${root_gen_dir}).

The mojo code just needs any unique string identifier, so just use
resource_map_source instead.

No intended behavior change.

Bug:  899437 
Change-Id: Ifc72086bd3b26997fb29d7d43306ba790a2c4e73
Reviewed-on: https://chromium-review.googlesource.com/c/1307937
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603965}
[modify] https://crrev.com/84a6a5f0b7244e679579c6e15957b016e6407dee/mojo/public/js/mojo_bindings_resources.grd
[modify] https://crrev.com/84a6a5f0b7244e679579c6e15957b016e6407dee/tools/grit/grit/format/resource_map.py
[modify] https://crrev.com/84a6a5f0b7244e679579c6e15957b016e6407dee/tools/grit/grit/tool/build.py

> .which is a bogus path. Hence my belief that nothing can depend on the value of that string, and maybe gzipped_resource_map_source 

I am noticing that now as well. Not sure how this bogus path does not cause any issues currently, but in theory it should be replaced by the correct path because root_gen_dir is defined and passed to GRIT at [1] below?

https://cs.chromium.org/chromium/src/chrome/browser/resources/BUILD.gn?[1] type=cs&q=settings_resources_vulcanized.grd&g=0&l=141-143,131

Correcting  link 

https://cs.chromium.org/chromium/src/chrome/browser/resources/BUILD.gn?type=cs&q=settings_resources_vulcanized.grd&g=0&l=141-143,131

It's passed, but that define is currently ignored for the resource_map output type. The patch in comment 17 fixes that (note the call to ToRealPath() that it adds), but since these paths are currently busted that suggests nothing depends on them being correct. So maybe we could just move these 3 vulcanize.grd files over from gzipped_resource_file_map_source to gzipped_resource_map_source ?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d6cfa24c431768730688e068eb1129ac5c8a631

commit 0d6cfa24c431768730688e068eb1129ac5c8a631
Author: Nico Weber <thakis@chromium.org>
Date: Wed Oct 31 01:02:11 2018

Use resource_map_source instead of resource_file_map_source for vulcanized.grd files.

See https://chromium-review.googlesource.com/c/1307937 for why.

Bug:  899437 
Change-Id: I996feae20627688fefd09419dedc78f015347cf4
Reviewed-on: https://chromium-review.googlesource.com/c/1308494
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604090}
[modify] https://crrev.com/0d6cfa24c431768730688e068eb1129ac5c8a631/chrome/browser/resources/md_extensions/extensions_resources_vulcanized.grd
[modify] https://crrev.com/0d6cfa24c431768730688e068eb1129ac5c8a631/chrome/browser/resources/print_preview/print_preview_resources_vulcanized.grd
[modify] https://crrev.com/0d6cfa24c431768730688e068eb1129ac5c8a631/chrome/browser/resources/settings/settings_resources_vulcanized.grd

I tried adding an assert finding future problems like this (https://chromium-review.googlesource.com/1309114) and found that it fired on this file:

https://cs.chromium.org/chromium/src/out/Debug/gen/content/browser/devtools/grit/devtools_resources_map.cc?type=cs&q=compressed_protocol_file&sq=package:chromium&g=0&l=12

It contains this entry:

  {"../../${compressed_protocol_file}", COMPRESSED_PROTOCOL_JSON},


alexclarke, you added that in https://chromium.googlesource.com/chromium/src/+/79a55e4e9f3d76197bfa057d5a6792f59c36df34

That filename is obviously not used -- could devtools_resources_map.cc use resource_map_source instead of resource_file_map_source too?

Yes it probably could.  I'm no longer working on headless, perhaps Pavel can triage.

thakis@, we have other resources that are files and have proper filenames in devtools_resources_map.cc. I can't seem to find any grit docs that would document the difference between resource_map_source and resource_file_map_source, will other resources work if we change the type? Or should we split devtools resources into two files?

resource_file_map_source was added at https://chromium-review.googlesource.com/c/chromium/src/+/823271/. You can find more context there on why this was an improvement, but basically this boils down to having an easier way of knowing which files are gzipped or not from within the C++ code.

https://chromium-review.googlesource.com/c/chromium/src/+/823271 only added gzipped_resource_file_map_source; resource_file_map_source precedes it I think.

Here's a resource_file_map_source: https://cs.chromium.org/chromium/src/out/Debug/gen/chrome/grit/component_extension_resources_map.cc?q=component_extension_resources_map&sq=package:chromium&dr=C&l=11

It has entries like

  {"network_speech_synthesis/tts_extension.js", IDR_NETWORK_SPEECH_SYNTHESIS_JS},


Here's a resource_map_source: https://cs.chromium.org/chromium/src/out/Debug/gen/ui/views/resources/grit/views_resources_map.cc?q=views_resources_map.cc&sq=package:chromium&dr Here the string name isn't a path but just a string representation of the ID:

  {"IDR_APP_TOP_LEFT", IDR_APP_TOP_LEFT},


If you have some resources that need paths and some that need an ID, maybe splitting that in two is a good approach. (But see also comment 17.)

I've had a look on how we use kDevToolsResources from devtools_resources_map.cc and I don't think we really need an entry for  COMPRESSED_PROTOCOL_JSON there.
On the other hand, splitting devtools resources into two packs just because of one resource would be somewhat unfortunate. So can we fix this on the grit level perhaps? I think the path names of generated files wouldn't make much sense at run-time in general, so not sure if it's practically useful to support them properly as #17 suggests -- perhaps, have an attribute that excludes a resource from the map like this: https://chromium-review.googlesource.com/c/chromium/src/+/1342778 -- wdyt?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e41a90785a4888a0cdbf0439113a7c37b8e47b37

commit e41a90785a4888a0cdbf0439113a7c37b8e47b37
Author: Andrey Kosyakov <caseq@chromium.org>
Date: Tue Nov 20 00:25:34 2018

grit: support skipping entries in generated resource map

Bug:  899437 
Change-Id: I4d6167d30a7b7014c17393960fce3ecb1470b80e
Reviewed-on: https://chromium-review.googlesource.com/c/1342778
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609526}
[modify] https://crrev.com/e41a90785a4888a0cdbf0439113a7c37b8e47b37/third_party/blink/renderer/devtools/scripts/build/generate_devtools_grd.py
[modify] https://crrev.com/e41a90785a4888a0cdbf0439113a7c37b8e47b37/tools/grit/grit/format/resource_map_unittest.py
[modify] https://crrev.com/e41a90785a4888a0cdbf0439113a7c37b8e47b37/tools/grit/grit/node/include.py
[modify] https://crrev.com/e41a90785a4888a0cdbf0439113a7c37b8e47b37/tools/grit/grit/node/include_unittest.py

The issue with devtools_resources_map.cc is fixed by the commit above, but I'm not sure whether the issue should be marked as fixed at the point -- so over to thakis@ to decide on that (perhaps add an assert on file being present, as mentioned in #23?)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9021c14b774c518c33666c5c91c4e402e380d61

commit d9021c14b774c518c33666c5c91c4e402e380d61
Author: Nico Weber <thakis@chromium.org>
Date: Tue Nov 20 14:46:17 2018

grit: Leave a breadcrumb to  crbug.com/899437  for the next time someone runs into it.

Bug:  899437 
Change-Id: I25ab289eaf053f09974413431e70ec648def95f5
Reviewed-on: https://chromium-review.googlesource.com/c/1309114
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609695}
[modify] https://crrev.com/d9021c14b774c518c33666c5c91c4e402e380d61/chromeos/resources/chromeos_resources.grd
[modify] https://crrev.com/d9021c14b774c518c33666c5c91c4e402e380d61/tools/grit/grit/format/resource_map.py

@thakis: I am hitting the assert that was added at [1], while trying to register a generated file in webui_resources.grd at [2].

I tried changing gzipped_resource_file_map_source to gzipped_resource_map_source similar to what you did for mojo_bindings_resources.grd at [3], but even though that fixes the build error (assertion no longer triggers), it does not work (chrome://resources URLs no longer load).

Any ideas? Is the patch at comment#17 the solution?

[1] https://cs.chromium.org/chromium/src/tools/grit/grit/format/resource_map.py?l=151
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1385448
[3] https://chromium-review.googlesource.com/c/chromium/src/+/1307937

Can you do https://chromium-review.googlesource.com/c/chromium/src/+/1342778/4/third_party/blink/renderer/devtools/scripts/build/generate_devtools_grd.py for https://chromium-review.googlesource.com/c/chromium/src/+/1385448/5/ui/webui/resources/webui_resources.grd ?

Updated the CL with your suggestion, but does not seem to work. The build error goes away, but the URL corresponding to the generated file does not work. You can repro this as follows:

1) Patch https://chromium-review.googlesource.com/c/chromium/src/+/1385448 locally (patchset 6).
2) Build chrome
3) Open chrome and visit chrome://resourcecs/js/assert.m.js. It should return the contents of the generated file, but it does not.

I believe this happens because shared_resources.cc relies on the map contents to associate all listed resources with a chrome:// URL, see [1]. 

[1] https://cs.chromium.org/chromium/src/content/browser/webui/shared_resources_data_source.cc?l=132. 

Correction: Expected URL is chrome://resources/js/assert.m.js (had a typo in the previous comment).

Does the patch in comment 17 help?

Does not seem to work (chrome:// URLs fail to load, including DevTools, so I can't really tell exactly in which way it is failing).

I also had to manually resolve conflicts when applying that patch, so maybe it's just me not resolving conflicts correctly.

Sounds like you need to debug things a bit :-)

I don't know what exactly you want to happen, but having a generated file in a source map has inherently somewhat wobbly semantics.

> I don't know what exactly you want to happen, but having a generated file in a source map has inherently somewhat wobbly semantics.

I believe what I am trying to achieve is a valid use case. Explaining below:

1) Create a GN rule that generates a file based on an existing file. For example
   From ui/webui/resources/js/assert.js generate out/<out_folder>/gen/ui/webui/js/assert.m.js
2) Include the generated file in the build by referring to it by a .grd file.
3) Have the generated file served from a chrome:// URL, for example chrome://resources/js/assert.m.js (similar to chrome://resources/js/assert.js).

> Sounds like you need to debug things a bit :-)

I will keep debugging, and either update this thread, or file a new bug. FWIW, even if I remove the assertion that you added, the file is still not served correctly, so I'll focus on fixing that first, before returning back to the assertion error.

@thakis: Updated my CL so what I am trying to achieve works (without the assertion), see https://chromium-review.googlesource.com/c/chromium/src/+/1385448.

1) Patch locally and build.
2) Open chrome and visit chrome://resources/js/assert.m.js. Should be seeing the contents of the generated files.

Now, I think the problem is that the generated C++ file (from the grit() GN Rule), does not expand the root_gen_dir variable, even though it knows about it at [1]. IIUC, a better fix than adding the assertion would be to ensure that the variable is expanded so that the map does not have a key that looks like the following

../../../out/gchrome/${root_gen_dir}/ui/webui/resources/js/

Instead it should convert this key to

../../../out/gchrome/gen/ui/webui/resources/js/

WDYT?

[1] https://cs.chromium.org/chromium/src/ui/resources/BUILD.gn?type=cs&q=webui_resources.grd&g=0&l=53

@thakis: One additional though. IIUC, the problem is not just that the root_gen_dir variable is not expanded, but the fact that "gchrome" is included in the code.

If you patch my CL, but your output folder has a different name, obviously the code will not work anymore. So in order to fix this, do you think the following would be reasonable?

When autogenerating C++ files from GRIT, replace the output folder's name with a fixed string, so that it does not depend on the build environment? Essentially

../../../out/gchrome/${root_gen_dir}/ui/webui/resources/js/
should become something like

../../../out/@out@/gen/ui/webui/resources/js/

where I randomly chose "@out@" as the fixed string.

Re-opening this bug for now.

@thakis: See https://chromium-review.googlesource.com/c/chromium/src/+/1385448/8/tools/grit/grit/format/resource_map.py. I updated the code to work with variables, as well as with referring to out/<folder_name> folders in a safe way, so that it does not depend on each developer's setup.

This allows adding map entries to auto-generated files. Will extract those changes to their own CL and send them out.

Re comment 43: That's what the patch in comment 17 does.

Re other comments: It sounds like you're trying to do something that doesn't work well in grit, independent of the fixed path being in the output. So I'd recommend opening a new bug for what you want to do and keep this one closed.

> Re comment 43: That's what the patch in comment 17 does.

The patch at comment #17 is doing something slightly different (and also did not seem to work when I applied it locally).

Filed  https://crbug.com/919916 . Re-closing this bug.