We only add the files as a data dependency if use_vr_assets_profile is true (which is equivalent to is_chrome_branded) https://cs.chromium.org/chromium/src/tools/perf/contrib/vr_benchmarks/BUILD.gn?q=vr_asset+file:build.gn&sq=package:chromium&dr=C&l=41

Might be fixed by gating the generate_vr_assets_profile behind that as well so it's not run as part of gn_all unless the files are actually present?

(re "why that step isn't red?": android-rel in https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_tests/chromium.py?type=cs&q=linux-rel+-file:%5C.json+file:recipe&sq=package:chromium&g=0&l=141 isn't marked ninja_confirm_noop like e.g. linux-rel is. Once this bug here is fixed, we should add that.)


Looks like the outputs of the action is a directory: https://cs.chromium.org/chromium/src/tools/perf/contrib/vr_benchmarks/BUILD.gn?type=cs&q=generate_vr_assets_profile&sq=package:chromium&g=0&l=61

  outputs = [
    "$target_gen_dir/vr_assets_profile/",
  ]

That's probably the reason; outputs should generally be files, not directories.

Moving generate_vr_assets_profile behind the conditional will hide the problem on the android-rel bot since that doesn't do official builds. That's better than the current state, but it kind of sweeps the actual issue under the rug.

https://cs.chromium.org/chromium/src/tools/perf/contrib/vr_benchmarks/generate_vr_assets_profile.py?type=cs&sq=package:chromium&g=0&l=44

  # Check whether there are actually files to copy - if not, the lack of a
  # directory will cause Telemetry to fail if we actually try to use the profile
  # directory. This is a workaround for not being able to check whether we
  # should have the files in GN. This way, we won't just fail silently during
  # tests, i.e. use the fallback assets, but we don't get build errors on bots
  # due to trying to copy non-existent files.
  found_asset = False
  for asset in os.listdir(os.path.join(asset_dir, 'google_chrome')):
    if asset.endswith('.png') or asset.endswith('.jpeg'):
      found_asset = True
      break
  if not found_asset:
    return


This doesn't write the outputs if the inputs aren't there. That's no good. We shouldn't magically try to do things based on the presence of input files, please read this lengthy comment: https://cs.chromium.org/chromium/src/.gn?type=cs&q=%5C.gn&sq=package:chromium&g=0&l=342

If these files are exactly there in official builds, then the :generate_vr_assets_profile target should be conditional as you suggest -- and that block from the .py script should be deleted.

The files will be there in official builds if checkout_src_internal is set, as they are downloaded here https://cs.chromium.org/chromium/src/DEPS?l=2545.

However, most bots don't actually set that, instead doing some stuff in the bot config to checkout src_internal. We've had 823882 filed for a while to more closely tie asset downloading to the presence of src_internal, but it's been untouched for a while since it was only an issue for our perf tests (normally, we don't care about having the assets present since they're downloaded via the component updater, but we need to pre-install them for perf tests).

+cc tiborg as the assets guy for more input.

Tibor, do you know if this will be fixed if/when the assets are moved into the VR DFM?

That depends a lot on how we do perf tests with bundles. IIUC, perf tests use the Chrome APK. That will stick around for a bit as we will only move Chrome Modern and Monochrome to bundles initially. Therefore, moving the assets into the DFM will not solve the problem for the Chrome APK. There are other options we can discuss internally.

In the meantime, would it help to have another GN flag that decides whether :generate_vr_assets_profile is built and always set that to true on the bots? And then fail the :generate_vr_assets_profile step if it cannot find the assets file?

If I understand correctly, the asserts are only around in official builds, so the bot https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/android-rel/3523
 wouldn't have them, right? So always setting that new flag on all bots wouldn't work (?)

Do the perf bots set checkout_src_internal to true? If so, we could use that flag. If not, how do they get the assets?

The perf bots do, but this bot is not a perf bot.

Or, if you're saying we should only run the step if is_official is set and make it fail on missing files: That should work and sounds like a good approach to me.

Do they also set is_chrome_branded? If so, we can do what Brian suggested and gate the step on use_vr_assets_component (which is equivalent to is_chrome_branded, see https://cs.chromium.org/chromium/src/chrome/browser/vr/features.gni?rcl=38a0005a196fc249225ffe5b6eebd6b590f9f215&l=12). And fail the step if the files are not present.

They do. If the test is made to fail on missing inputs and we list files instead of a directory in outputs in addition to gating on use_vr_assets_component, that sounds good to me.

The problem with that is that we don't exactly know in GN what assets files exist (the exact files are configured in a JSON file). Wouldn't it also work if we set the directory as output and either always make it or fail in :generate_vr_assets_profile?

You could make the script write a dummy file in addition to stuff listed in the json file, and list that dummy file as output.

Why do the outputs have to be listed in a json file though? Couldn't we list them in the gn file instead of the json file?

The dummy file sounds good to me. We have them in a JSON file since the list of asset files needs to be parsed by a python script to upload the assets as a chrome component. That step is not part of the build.

Does it run after the build? If so, maybe the build could create that json file?

Or, if the file list doesn't change often, the gn file could list inputs and outputs and json file and verify that gn file and json file contain the same data?

That is a good idea actually. It may be even easier if we define the set of asset files as a GN list in a .gni file and parse that in the python script.

To be sure we're on the same page: The python script shouldn't literally parse the gni file, right? :-) If you have

foo_files = [
  "file1",
  "file2",
]

in gn, you can do

action(..) {
  args = ...

  args += [ "--filelist" ]
  args += foo_files
}


And in the python script do

parser.add_argument('--filelist', nargs='+')

and options.filelist will contain the list.

Right, that is when the python script is part of the build :) The JSON file we are talking about is vr_assets_component_files.json [1]. It is loaded by push_assets_component.py [2] (the script for uploading) and PRESUBMIT.py [3], which are not invoked as part of the build but directly from command line. So, GN is not there to put the list into a command line argument. That's the slightly tricky part. We could also do here what we do for pydeps (e.g. [4]). That's maybe even easier to parse in python and to load in GN via 

read_file(<file>, "list lines")

[1] https://cs.chromium.org/chromium/src/chrome/browser/resources/vr/assets/vr_assets_component_files.json
[2] https://cs.chromium.org/chromium/src/chrome/browser/resources/vr/assets/push_assets_component.py
[3] https://cs.chromium.org/chromium/src/chrome/browser/resources/vr/assets/PRESUBMIT.py
[4] https://cs.chromium.org/chromium/src/build/android/test_runner.pydeps?q=test_runner.pydeps&sq=package:chromium&dr=C&l=2

I believe this was fixed by https://chromium-review.googlesource.com/c/chromium/src/+/1373734