Remove CFE/analyzer/compiler/VM support for .packages file

[sigmundch]

We some surprising behavior when we give the Dart VM a --packages=path/to/packages/file flag. We tripped on this as we try to migrate code away from using .packages, so this may be relevant for eternal users after we deprecate .packages in the near future.

Repro steps require:

• launching the VM with a package URI as the input script
• providing a --packages flag pointing to a non-existing .packages file (but in the location that is expected to be, so the .dart_tool/package_config.json file can be resolved).
• having .dart_tool/package_config.json in the expected location relative to the old conventions with .packages files.

Expected behavior: the input script URI shouldn't be resolved, and an error should be emitted.
Actual behavior: the script URI is resolved and executes, however later within the app the Isolate.resolvePackageUri cannot resolve the input URI script (because it uses the non-existing .packages file, instead of the implicit package_config.json file).

Here is a full repro example:

lib/foo.dart

import 'dart:io';
import 'dart:isolate';

main() async {
  print(Platform.script);
  print(await Isolate.resolvePackageUri(Platform.script));
}

.dart_tool/package_config.json:

{
  "configVersion": 2,
  "packages": [
    {
      "name": "foo",
      "rootUri": "../",
      "packageUri": "lib/",
      "languageVersion": "2.16"
    }
  ]
}

If you invoke the Dart VM as dart --packages=.dart_tool/package_config.json package:foo/foo.dart, you see the following output:

package:foo/foo.dart
file:///usr/local/google/home/sigmund/playground/no_packages/two/lib/foo.dart

If however you invoke it as dart --packages=.packages package:foo/foo.dart, you see the following output:

package:foo/foo.dart
null

If you provide a different name for the packages file: dart --packages=.nothing package:foo/foo.dart, you see the following output:

Error: Error when reading '.nothing': No such file or directory
Error: Couldn't resolve the package 'foo' in 'package:foo/foo.dart'.
org-dartlang-untranslatable-uri:package%3Afoo%2Ffoo.dart: Error: Not found: 'package:foo/foo.dart'

This last output is what I'd expect to get in the first place.

[mit-mit]

@a-siva can we get this one assigned? I believe it's the last know issue in related to these changes.

[a-siva]

This seems to be a front end issue, the code here in pkg/front_end/lib/src/base/processed_options.dart doesn't check for the existence of the .package file instead it first looks to see if a .dart_tool/package_config.json file exists in that same location it and if it does then it uses it.

  Future<PackageConfig> createPackagesFromFile(Uri file) async {
    // If the input is a ".packages" file we assume the standard layout, and
    // if a ".dart_tool/package_config.json" exists, we'll use that (and require
    // it to be a json file).
    PackageConfig? result;
    if (file.path.endsWith("/.packages")) {
      // .packages -> try the package_config first.
      Uri tryFirst = file.resolve(".dart_tool/package_config.json");
      result = await _createPackagesFromFile(tryFirst, false, true);
      if (result != null) return result;
    }
    result = await _createPackagesFromFile(file, true, false);
    return result ?? PackageConfig.empty;
  }

If I modify the above code as follows
if (file.path.endsWith("/.packages") && File(file.path).existsSync()) {
Then it behaves the same way for both
dart --packages=.packages package:foo/foo.dart and dart --packages=.nothing package:foo/foo.dart

[jensjoha]

As I recall this was discussed with the design of the automagic redirect to package_config, so I think this is working as intended. I'll see if I can dig out an issue.

[mit-mit]

@jensjoha the plan is to deprecate the support for .packages in the next release, and then entirely remove it (as in, don't use the file, even if it's around or explicitly passed) in a following release. So even if we had such a design earlier, we should reconsider that.

[jensjoha]

As specified here:

#40864 (comment)

From the above:

1. Specified json file => we use that.
2. Specified .packages filename => we look for and find the json file
3. We specify a file not called .packages => we (try to) use it.

The

however later within the app the Isolate.resolvePackageUri cannot resolve the input URI script (because it uses the non-existing .packages file, instead of the implicit package_config.json file).

seems wrong --- but I have no idea what resolvePackageUri does though.

@jensjoha the plan is to deprecate the support for .packages in the next release, and then entirely remove it (as in, don't use the file, even if it's around or explicitly passed) in a following release. So even if we had such a design earlier, we should reconsider that.

Someone should specify something then --- /cc @lrhn

[lrhn]

Allowing redirection from a specified .packages file to the actual .dart_tool/package_config.json file was a deliberate way to avoid breaking existing scripts when we introduced the JSON configuration.
That still works, but we may want to start emitting warnings when we do so, so people can migrate their scripts.

The resolvePackageUri from dart:isolate should use the package URI configuration that the current isolate used for source, to convert a package: URI to a non-package: URI for the same file, so you can load it using dart:io.

This only makes sense for the stand-alone VM, not AoT, since there are no source directories available if you're not running from source. I don't know what the method does in AoT, but I'd make it throw UnsupportedError. (That said, I bet some people compile to a native executable, then run in the original source environment, just because it's more efficient. Perhaps for testing. So, in that case it does make sense to check if the original configuration file still exist, and only throw a StateError if it doesn't. We don't want to embed the contents, since it's going to be mostly links to the pub-cache, which won't make sense anyway if the original configuration file isn't there any more.)

We also provide Platform.packageConfig which is not particularly well-defined. It suggests that it provides the command line argument, but that's useless when that's not the package configuration we actually use, so it should point to the actual package configuration file. It probably doesn't, and I'm guessing the VM tries to read that file to implement resolvePackageUri.
Again, only makes sense when running from source.

My suggestion is that the front end provides a way to access the actual package resolution information that it used for a compilation (package resolution file path, maybe PackageConfig object on demand), so the VM can ask for that and won't have to re-parse the configuration file (or even the wrong file).

So, I'd say the bug is in the VM methods. We can choose to support an efficient implementation of that from the front-end, or we can leave it up to the VM to do the exact same package-configuration file search as the front-end, so that it gets the same results.

If we choose to remove the redirect from out tools (and to be precise: Any tool which currently does not redirect from .packages to .dart_tool/package_config.json is violating the design document), then that's fine.
It will mean that any existing script which specifies -psomething/,packages will stop working. That's fair, we've removed support for .pacakges entirely, so we no longer know what it means, so we can't figure out what you meant instead.

[mit-mit]

I think we should go ahead and remove that redirection. We're about to stop the generation of the file, so I think we can remove the redirection too.

[johnniwinther]

@mit-mit @lrhn We need a specification of what we are supposed to do now. For instance, if we just remove the redirection, we would start using the .packages file instead of the .dart_tool/package_config.json that we are currently redirecting to. Is that what we want?

[lrhn]

Let's remove support for .packages entirely.

That is:

• No redirection. If you ask for -p .packages, we read .packages.
• No support for reading INI-file syntax at all. If you specify -p .packages, we read .packages and try to parse it as JSON. If that fails, you get an error.

That will break scripts depending on the redirection. That's OK, they are no longer supported since they use the unsupported .packages feature. That's what removing support means, and we are fine with that.

As always, which error message to show in that case is completely open, and nicely telling people that .packages is no longer supported, and that they should point to .dart_tool/package_config.json instead, would be a good user-experience.

How to do this:

Where you use package:package_config to load a config file, change calls to loadPackageConfig/loadPackageConfigUri by removing any preferNewest: true arguments. Then, if a configuration is loaded, check whether its config.version < 2 (meaning it's an INI file, they have configVersion 1) and if so, report an error instead of continuing.

If you use findPackageConfig or findPackageConfigUri instead, you can't currently prevent finding .packages files.

I'll add a minVersion to the methods, so you can ask to only find version 2+ configurations and ignore earlier versions.

[mit-mit]

I think we should specifically test for the file name .packages and rather than failing with a general ".packages is not a valid JSON-formatted package config" error, offer a more targeted message like "Dart no longer supports .packages. Please pass a package configuration file. See dart.dev/go/dot-package-migration" for details.".

[jensjoha]

To sum up the conversation so far:

May 16:

the plan is to deprecate the support for .packages in the next release, and then entirely remove it (as in, don't use the file, even if it's around or explicitly passed) in a following release. So even if we had such a design earlier, we should reconsider that.

May 16:

That still works, but we may want to start emitting warnings when we do so, so people can migrate their scripts.

May 18:

I think we should go ahead and remove that redirection.

May 20:

Let's remove support for .packages entirely.

That is:

• No redirection. If you ask for -p .packages, we read .packages.
• No support for reading INI-file syntax at all. If you specify -p .packages, we read .packages and try to parse it as JSON. If that fails, you get an error.

That will break scripts depending on the redirection. That's OK, they are no longer supported since they use the unsupported .packages feature. That's what removing support means, and we are fine with that.

May 20:

I think we should specifically test for the file name .packages [...]

This still leaves me confused as what the CFE team should do.

Let me try to ask some specific questions:

1. For right now (which I'm guessing is what becomes the next release --- Dart 2.18 I'm guessing), should we either:
   1.1) Make no changes.
   1.2) Continue to redirect, but issue a warning when doing so.
   1.3) Stop redirection, read the file specified, if it is an old version issue a warning
   1.4) Stop redirecting, read the file specified, if it is an old version give an error, (and as a consequence basically stop the compilation (though by issuing lots of errors)).

2. For the future (the next next version --- Dart 2.19 I'm guessing), should we either:
   2.1) Make no changes.
   2.2) Continue to redirect, but issue a warning when doing so.
   2.3) Stop redirection, read the file specified, if it is an old version issue a warning
   2.4) Stop redirecting, read the file specified, if it is an old version give an error, (and as a consequence basically stop the compilation (though by issuing lots of errors)).

[lrhn]

I don't know where we are with the next release. If it's not too close, meaning we'll have at least one more dev-release before stable release, I'd do 1.4, otherwise 2.4. So, definitely X.4, only depending on when we release the change. @mit?

I'd make the error message depend on whether the specified name is .packages or not. The entirety of the ".packages" support in the front end will be recognizing that someone is using the old name and either not finding a file, or finding a file which is not a valid JSON configuration file, and give an error message directing them to specify a .dart_tool/package_config.json file.

That also means that when searching for a configuration file, .packages files should be ignored. Searching for a configuration file only looks for .dart_tool/package_config.json files.

If you specify -p .packages and .packages happen to be a valid JSON configuration file, there is no error.

[mit-mit]

We shipped our last stable a week ago, so we are several months, dozens of dev-releases, and several beta-releases from the next stable. We also announced that .packages was going away several times. So we do not need to stage this. We are already at the "break the last few that may not have migrated"-stage.

So, my answers to your specific questions, are to immediately:

• Stop redirecting from one file to another
• Error if passed .packages file

[jensjoha]

Maybe I should start by stating that I don't have a strong opinion about what is decided here (I guess technically I do, but I don't have energy to fight for it) --- but I don't want to have to implement the change several times because we're not clear on what we actually want (we've already done that a bunch of times for the language versioning feature that the new package format was a part of).

I will point out, though, that the Breaking change 48272 states that

We've planning on changing the dart pub get & flutter pub get commands to no longer produce the .packages file in the next stable release, and to fully remove support for that file in the following stable release.

At a time where the next stable would be 2.17.

It then states

Update: The change to no longer generate the .packages file is now scheduled to happen in the Dart 2.18 release.

The way I would read that, would be that the whole process was shifted by one release, meaning that the fully remove support for that file would go into 2.19.

[mit-mit]

The only thing we pushed to 2.19 is support for generating it: "to no longer generate the .packages file"

That part is retained as there may be tools owned by others than the Dart or Flutter team that for some reason havn't updated, and that we're not aware of, and thus still have a dependency on the .packages file. We're giving those another release to get migrated.

I'll see if the wording in the breaking change issue needs to be clarified a bit.