[cfe] dart compile kernel --depfile does not include package_config.json

[dcharkes]

Steps to reproduce

dart compile kernel --packages=path/to/package_config.json --output=xyz.dill --depfile=xyz.dill.d xyz.dart

Expected result:

xyz.dill.d contains path/to/package_config.json

Actual result:

xyz.dill.d contains only the list of .dart files.

@johnniwinther @jensjoha @chloestefantsova Is the omission of the package_config intentional or an oversight? The kernel compilation would most likely need to be rerun when the contents of the file passed in --packages changes.

[johnniwinther]

I think it's an oversight.

[jensjoha]

I think the thing was introduced in 9f4f0ac --- and has seemingly always been by taking component.uriToSource.keys which is only the dart files.

As this is not really a CFE feature but a VM one I can't speak to whether this is by design or not.

If you want something more you should give the file system a tracker:

  FileSystemDependencyTracker tracker = new FileSystemDependencyTracker();
  compilerOptions.fileSystem = StandardFileSystem.instanceWithTracking(tracker);

which records everything read through the file system object, e.g.

$ out/ReleaseX64/dart pkg/front_end/tool/deps.dart hello.dart
$ cat hello.dart.dill.d
hello.dart.dill: \
  .dart_tool/package_config.json \
  hello.dart \
  out/ReleaseX64/vm_platform_strong.dill

[dcharkes]

Thanks @jensjoha!

which records everything read through the file system object, e.g.
[...]
vm_platform_strong.dill

If we'd go that way, you'd also have to report the Dart executable itself. Or basically anything that influences when Dart changes instead of the program sources. (Which we do need to do in the context of the work in dart-lang/native#1770, but we don't want to go and content-hash the dart.exe all the time, instead we'd like to rely on the version file. @mkustermann)

So I think we can safely say that this feature is meant to track the sources of the program, not the version of Dart.

I don't see any obvious uses in the Flutter Tools or Dart SDK that would break by adding the package_config.json to the DEPS, so let me make a PR to do that.

---

If the --packages= param is not set, the selection of which package_config.json file is used is buried somewhat deep in the CFE. (I've found pkg/front_end/lib/src/base/processed_options.dart Future<PackageConfig> _getPackages() which forgets about the actual file and simply remembers the contents of the packages file.)

So the obvious implementation in pkg/vm/lib/kernel_front_end.dart only works for the case where the --package= param is passed in.

I don't know if there are any uses where --depfile is passed as an option, but --packages is not.

We could CFE report the package config used in the KernelCompilationResults. But that means that if a package_config.json is added in a child directory after the compilation done, the DEPS file doesn't contain info that on recompilation that new package_config.json would be used. Very unlikely, but from a correctness POV it illustrates that a solution like this would be problematic.

I don't know if there are any uses where --depfile is passed as an option, but --packages is not.

Maybe we should try making --depfile but not --packages an error.

https://dart-review.googlesource.com/c/sdk/+/398480

[dcharkes]

@jensjoha What are your thoughts about the KernelCompilationResults reporting which packages_config.json it discovered on the file system?