Skip to content

Language Versioning: .packages file breaking changes? #365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jakemac53 opened this issue May 20, 2019 · 43 comments
Closed

Language Versioning: .packages file breaking changes? #365

jakemac53 opened this issue May 20, 2019 · 43 comments
Labels
nnbd NNBD related issues

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented May 20, 2019

As a part of the language versioning proposal (around here), there are some proposed breaking changes to the .packages file format.

The breaking changes

Specifically, I noticed two breaking changes:

  • Adding a #dart=<version> comment to the end of the paths (so the paths are no longer valid)
  • Adding a special identifier, *:<package> which denotes which package is the application package.

Why it matters

All kinds of tools generate and consume .packages files today, from build_* packages to internal bazel rules, they are very prevalent. Tests often create them too in order to do integration tests.

Many of these tools will be packages published on pub, with an upper bound sdk constraint of <3.0.0. So even if they are updated, we can't prevent users from getting a version that doesn't support the new format, or force a pub upgrade.

The current .packages format is very simple, and because of that most tools I have seen do not go through any shared library to parse it either. They do some form of this pattern instead:

  • read the file by lines
  • skip lines starting with a comment
  • split each line by the first :
  • you now have a list of (package, path) pairs

All of those implementations will be broken by the proposed changes, and will have to be individually updated/fixed.

Bazel specific notes

Currently today in bazel we generate .packages files in pure starlark code, which doesn't have access to the file system.

In order to generate these new files, we would have to add an attribute or something to our dart_library rule which specifies the min version. This should be doable but it will be interesting for the NNBD migration, I don't know what the proper default should be.

@srawlins
Copy link
Member

One solution would be to use Dart 3.0.0 as the first version that ships with the NNBD experiment on by default, yes?

@jakemac53
Copy link
Contributor Author

jakemac53 commented May 20, 2019

One solution would be to use Dart 3.0.0 as the first version that ships with the NNBD experiment on by default, yes?

If that also means that the .packages file format doesn't change until Dart 3.x, then yes. I don't know that those are related though.

It would be my preference that we don't break the .packages format until Dart 3.x, as I don't think anything else can be fully correct and it will be really difficult (but not impossible) to do something close enough to correct that ~nobody notices.

@jakemac53
Copy link
Contributor Author

I would also like to note that the current migration section doesn't call this out specifically, and it actually currently states that "The chosen design allows existing Pub packages to keep running without modification. ", which won't be true for any package that reads or produces .packages files.

@munificent
Copy link
Member

I don't think this is as dire as it seems. The ".packages" file is not really part of what constitutes a "Pub package", isn't uploaded to pub.dartlang.org, and generally isn't shared across machines.

What this means is that any given .packages file is usually only consumed by tools that are part of the same SDK or a previous version of it. So once some Dart tool has produced a ".packages" file with the new syntax, it will typically only end up consumed by tools that can also handle it.

Except for the case you note which is external packages like build which also parse (and produce) ".packages" files. So there is a real breakage, but it's not all tools that will break.

To mitigate that pain, there are a couple of things I think we could do:

  • Have a migration window. We put out an announcement saying "In a future version of Dart, we'll change the '.packages' file to have this new syntax. You should update your parsers now to gracefully handle it." Then we fix the build and other packages to not fall over. After that's all done and users have had some time to upgrade to it, then we change the tools to start producing the new syntax. It's still possible for a user to get broken, but this should minimize it.

  • Write a different file. Put the new syntax in a differently named file, like ".packages-even-better". Tools would produce both that and ".packages" but only the former will have the new syntax. Tools that don't handle the new syntax only read the ".packages" file. Eventually, we deprecate the old file and tools stop writing it. This is basically how we did the "packages" directory -> ".packages" file migration. It went very smoothly, though it was a chore to output and support both for a while.

I do think we should consider changing ".packages" to a more extensible format so that we don't run into this exact same problem again the next time we want to pass some package metadata to all the tools.

When ".packages" was first designed, I suggested the syntax be a Dart map literal. We already have high performance parsers for that, it's extensible, and supports comments (unlike JSON). I'm not picky, though. It would just be good to not run into this problem again.

@jakemac53
Copy link
Contributor Author

I do think we should consider changing ".packages" to a more extensible format so that we don't run into this exact same problem again the next time we want to pass some package metadata to all the tools.

If we are going to do a breaking change anyways, we might as well completely change the format at the same time. Now or never :D

@jakemac53
Copy link
Contributor Author

I also did some searching around internally and fwiw most of the cases I found were tests, which were creating .packages files. If the old format is still valid, then these won't be affected (although I don't know what the behavior will end up being, they may get broken if they end up opting into NNBD by accident for instance).

@lrhn
Copy link
Member

lrhn commented May 21, 2019

The old format is still valid. It just means that every package uses the languge version of the current SDK, and that code outside of a package will not have a default package, and will therefore also default to the language version of the current SDK.

The new format is backwards compatible as long as the parser does not make too many assumptions.
The value after a : is still a valid URI reference. If you resolve anything against that URI reference (after it was resolved against the file location), you get the same result as if there was no fragment.
If you don't validate the package names, then *:foo is just a package named *, which you won't be using. It is even valid according to the package-configuration file specification because the "package name" is only required to be a valid URI path segment with no colon or percent-sign, and that does not consist of only . characters. A * satisfies this. (The package name is a valid URI reference too).

If a tool assumes that all entries point to valid directories, then it will fail. If you only do package name lookup and package URI resolution, then the new file is backwards compatible.

@jakemac53
Copy link
Contributor Author

The old format is still valid. It just means that every package uses the languge version of the current SDK, and that code outside of a package will not have a default package, and will therefore also default to the language version of the current SDK.

That is quite broken though right? If tools that auto-generate .packages files by default still "work" but are actually stripping out the language version information for those packages, they will very likely be broken, because they are being silently upgraded to the current sdk version? Should the new SDK maybe even reject these .packages files entirely? Or consider an omitted version to be defaulted to some old sdk prior to the version identifier being added?

The new format is backwards compatible as long as the parser does not make too many assumptions.

At least some do make assumptions about it being a valid path, scissors for instance.

Also, note that Directory.fromUri doesn't support uris with fragments. So for instance the uris you get back from package_config package today are not directly usable with the core dart apis. I created a sample project using the canonical package_config package, and added the fragment to my .packages file, as follows:

import 'dart:io';
import 'package:package_config/packages_file.dart';

main() {
  var bytes = File('.packages').readAsBytesSync();
  var parsed = parse(bytes, Directory.current.uri);
  var pkgDir = Directory.fromUri(parsed['package_config']);
  print(pkgDir.existsSync());
}

That gives the following exception:

Unsupported operation: Cannot extract a file path from a URI with a fragment component

@jakemac53
Copy link
Contributor Author

@munificent do we have a concrete decision here? It looks like the pub team is already planning on implementing this which will start breaking people if we don't put together a plan dart-lang/pub#2154.

@jonasfj
Copy link
Member

jonasfj commented Jun 19, 2019

There is something to say for supporting a richer .packages format. Or at-least a versioned format, so tools can reject future versions.

If we ever dream of supporting things like: package aliasing, multiple versions of the same package, or explicit assets.
I'm not saying we have such plans, just that keeping the door open might be nice :)

@munificent
Copy link
Member

@lrhn is the person to ask, I think, since he's driving the feature.

@keertip
Copy link

keertip commented Aug 8, 2019

@aadilmaan, it would be good to get the tools that consume the .packages file (the front and back ends and others?) to start supporting this syntax so that the pub, bazel etc can update their .packages generation for the new syntax without breaking the world. Are there issues for tracking this work?

@keertip
Copy link

keertip commented Sep 4, 2019

@lrhn , any decision on supporting a richer .packages format? It would be good to get this resolved soon, assuming that NNBD support will be making use of this new format, and all tools will need to support it.

@lrhn
Copy link
Member

lrhn commented Sep 5, 2019

No current plan to change the format. The package_config package has been updated with support for the new features and the front-end and pub tools are working on using it.

I'm open to using a more capable format, but I don't want to rush it, so it would be nice to know which actual features it must support.

(I'd personally prefer JSON over YAML, if only JSON had comments.)

@DanTup
Copy link

DanTup commented Sep 16, 2019

As a heads up, the VS Code extension parses this file to convert package URIs to file paths (all interaction with VS Code uses file paths, it doesn't know what package URIs are). I think the other editors are probably doing it too (and that may include other community integrations for other editors).

The changes above probably break my parsing (it's naive and was made up by just looking at the contents, as I couldn't find a spec). I'd prefer to not parse this file in the editor and had started to move away from using it, but I think we may need to go back.

If the changes above will/have gone ahead, is there an official spec of the format so I can update it and ensure it covers all required cases correctly? (the link in Jake's very first comment here is a 404, so I couldn't find any more info there).

Edit: Here's how Dart-Code is currently parsing it (much like Jake's notes above - skip lines that start with # and then split on colons assuming a name/path pair, with detection of lib/, lib\, lib to get the local package name).

https://github.com/Dart-Code/Dart-Code/blob/b8c0f6f138b31748822c0f4ff471ddfa277c7cfa/src/shared/pub/package_map.ts#L23

@DanTup
Copy link

DanTup commented Sep 16, 2019

(I'd personally prefer JSON over YAML, if only JSON had comments.)

FWIW, VS Code uses JSON for its config, and they just allow comments. They have support for a language "JSON with comments" in the editor too. I don't think it's much different to having a completely custom format - it just happens to be the same as JSON with comments (I'm sure if more people started doing this, the common parsers would get settings to handle it - and if not, stripping comments and then using an existing JSON parser probably isn't too complicated versus parsing a completely custom format).

@jakemac53
Copy link
Contributor Author

fwiw I updated my link just now - looks like the file was renamed

@lrhn
Copy link
Member

lrhn commented Sep 17, 2019

The official specification for the current .packages file format is https://github.com/dart-archive/dart_enhancement_proposals/blob/master/Accepted/0005%20-%20Package%20Specification/DEP-pkgspec.md.

The language versioning changes to the format are sketched in https://github.com/dart-lang/language/blob/master/accepted/future-releases/language-versioning/feature-specification.md.
The changes are:

  • Allow a single entry with an empty key and a package name as value (same restrictions on the values as the existing key restrictions).
  • Allow fragments on the remaining value URIs (they always were allowed since the values can be any URI reference, but were never used because fragments never had any effect when you resolve a path against the value URI). The fragments will be parsed as x-www-form-urlencoded key/value pairs.

This is an extension of the existing syntax (mainly the one empty key being allowed, but not required), so old .packages files will be valid under the new syntax as well.

The recommended way to work with .packages files is package:package_config. It is currently being updated to also support the new syntax (which is an extension of the old syntax, so old files will still parse).

I'll write up a specification for the new format, and maybe even the new package resolution.

@DanTup
Copy link

DanTup commented Sep 18, 2019

@lrhn thanks! I'm not sure why I failed to find that document when first parsing this!

One question I have is about the "default package" shown in this doc where it has :floo (which looks like it has changed *:floo). I can't find a section of the doc detailing this - am I right in thinking that there this is an additional line in the file, which just names the package (but that it will still appear with its path)? For example:

Old:

# Generated by pub on 2019-09-17 15:53:45.682066.
analyzer:file:///analyzer/lib/
ansicolor:file:///ansicolor/lib/
my_test:lib/

New:

:my_test
analyzer:file:///analyzer/lib/#dart=2.0
ansicolor:file:///ansicolor/lib/#dart=2.0
my_test:lib/#dart=2.0

Previously I was looking for packages that have lib or lib/as their path to know which is the owning package, but now if I find a line started within : I should use that name instead?

Edit: PR for Dart-Code at Dart-Code/Dart-Code#2001. I'll merge it once the above is confirmed.

@devoncarew
Copy link
Member

IntelliJ's .packages parser will be broken here, as I suspect would be almost all other tools that parse this format - they all parse it as @jakemac53 described in the first comment on this thread.

The recommended way to work with .packages files is package:package_config.

This is not possible for any tool not written in Dart.

What this means is that any given .packages file is usually only consumed by tools that are part of the same SDK or a previous version of it.

There are many, critical, Dart and Flutter tools that are not developed in the SDK.

Where are we on this change? I want to better evaluate the impact. We would have been in a much, much better place here if we hadn't invented our own file format.

@jonasfj
Copy link
Member

jonasfj commented Sep 18, 2019

Where are we on this change? I want to better evaluate the impact.

I've started implementing this in pub, with a few hacks to the analyzer, all but 3 tests pass (something with Dart snapshot invocations).

I may be wrong, but I assumed we extended .packages with a URL fragment to maintain backwards compatibility. However, from this discussion it seems 3rd party tools will be broken too, furthermore, I know that all tools using package:package_config will break, as they will need to:
A) Upgrade to the latest version of package:package_config, and,
B) Pass allowDefaultPackage: true to all invocations of package_config.package_file.parse.

Older versions of package:package_config will not ignore the default package name entry (:<packageName>). They will instead throw an error. In fact, so will the latest version too, if allowDefaultPackage: true is not passed.


Creating a new file might be less disruptive. Maybe we could generate both .packages and .packages.json for the rest of the Dart 2.x series (or use a binary format like BER, if JSON is too slow).

It's not unlikely that users will jump between Dart/Flutter versions/channels using flutter upgrade, homebrew, fvm, dvm, etc.

@jakemac53
Copy link
Contributor Author

jakemac53 commented Sep 18, 2019

I may be wrong, but I assumed we extended .packages with a URL fragment to maintain backwards compatibility.

Right, the reason I opened this issue originally is that assumption is unfortunately not accurate. External tools ended up for the most part treating the value as a path and not a uri because that seemed like an entirely sensible thing to do. It is very unexpected for these to have fragments or query params as that gives no meaningful information about how to locate the package (which is all this file is supposed to do).

While this is not breaking in terms of the original DEP in practice it is breaking for these tools.

@devoncarew
Copy link
Member

While this is not breaking in terms of the original DEP in practice it is breaking for these tools.

I agree here.

I've started implementing this in pub, with a few hacks to the analyzer, all but 3 tests pass (something with Dart snapshot invocations).

Thanks for the update. I think we need to pause the rollout, and do one of either:

  • project manage this; determine the impacted tools, send a breaking change message as per our process, update tools to handle both formats, ship those, and only start writing the new file format once the critical tools have updated (there is often a lead time of months for some of them)
  • re-visit the format changes; have a discussion about how best to encode the information in either the existing file or a new file. There are likely several options with various tradeoffs.

cc @mit and @lrhn, @munificent @jakemac53 @jonasfj

@lrhn
Copy link
Member

lrhn commented Sep 19, 2019

@DanTup

One question I have is about the "default package" shown in this doc where it has :floo (which looks like it has changed *:floo). I can't find a section of the doc detailing this - am I right in thinking that there this is an additional line in the file, which just names the package (but that it will still appear with its path)?

The example looks right.

Previously I was looking for packages that have lib or lib/ as their path to know which is the owning package, but now if I find a line started within : I should use that name instead?

You are trying to infer the name of a Pub package from the .packages file which only really applies to Dart packages. Those are two different concepts - Pub packages contain Dart packages (the content of their lib directory), but also other things like tests or tools.

The technically perfect way to find the pub package of a file is to find the surrounding pubspec.yaml file. To find the package's name, read the pubspec.yaml file and get its name entry. That works even before you run pub get to generate the .packages file.

The .packages file has a simple format, but no guaranteed choice of representation for the path. It would not be considered a breaking change if a version of Pub started emitting absolute paths instead of relative paths, so

my_test:lib/

becomes

my_test:file:///home/myself/mypkgs/my_test/lib/

That's an equally correct way to point to the Dart package files, but would not be found if you only look for lib or lib/. So, you are relying on an accident of representation which is not guaranteed.
(What you could do instead is to resolve the package location URIs against the .packages file's URI, then check if the file path of that points to the lib dir of the current directory).

When Pub starts adding "default package" entries, it will most likely do so for all Pub packages and have the current Pub package be the default package. At that point, I think it is safe to assume that it is the name of the current Pub package. (You can manually create a .packages file which considers your test directory as belonging to another package, but why would you?)

@DanTup
Copy link

DanTup commented Sep 19, 2019

The technically perfect way to find the pub package of a file is to find the surrounding pubspec.yaml file. To find the package's name, read the pubspec.yaml file and get its name entry.

Yeah, that's what I thought but I was trying to avoid having to parse YAML when I wrote this (I'm trying to cut down on nodejs dependencies we're shipping to end users after things like event-stream.. most npm packages don't think twice about bringing a tree of 4,000 transient dependencies). It'd be easier if pubspec was JSON 😉

We actually only use the local package name for a few minor things (eg. we show package names in hover tooltips, but wanted to skip that for the project itself, and we want to exclude the project itself from the dependencies tree). I think the hover one is solved by the analyzer now anyway, and probably I could switch the dependency one to use the path - so maybe I could just drop this entirely and avoid the complications - I'll file an issue to look at it.

@leafpetersen leafpetersen added the nnbd NNBD related issues label Sep 20, 2019
@leafpetersen
Copy link
Member

I think we need to pause the rollout

Note that this is a blocker for the NNBD release, so if we pause, we need to unpause fairly quickly, particularly given the tool lead times mentioned above.

@munificent
Copy link
Member

We would have been in a much, much better place here if we hadn't invented our own file format.

Yeah. :( I tried at the time.

I think we may want to consider moving to a second file now that has an extensible format and eventually deprecating .packages. It's the least disruptive to existing tools, and we have an existence proof that it works gracefully because that's how we moved from packages directories to .packages.

@lrhn
Copy link
Member

lrhn commented Sep 23, 2019

I agree that if the planned changes are actually breaking anyway, then we might as well do a new format now.
Maybe: b8a6940?short_path=13c4ad6

@bkonyi
Copy link

bkonyi commented Sep 23, 2019

Just an FYI, changing this format will break Observatory initially as its .package file is hard coded to point to local copies of its dependencies.

@nshahan
Copy link

nshahan commented Sep 24, 2019

@sortie If any of the testing/benchmarking infra depends on the .packages format this will be breaking there too.

@jonahwilliams
Copy link

This change will likely break the flutter tool - but this is written in Dart so we can switch to using package_config.

Unfortunately the fuchsia source tree also contains multiple python programs that parse and consume .packages files. Updating these will be much more difficult. Ideally, this would be done as a soft transition - where we can continue using the old file format while we work towards supporting the new one.

Is there a time frame when this new .packages format will be available? It would be great to have some time in Q4 to update our tooling in advance

@devoncarew
Copy link
Member

devoncarew commented Sep 24, 2019

Unfortunately the fuchsia source tree also contains multiple python programs that parse and consume .packages files. Updating these will be much more difficult. Ideally, this would be done as a soft transition

We had a discussion about this - we are now planning to have a soft transition. Overall, we'll:

  • create a new file to store the resolved packages location
  • use json as the format
  • store it in the .dart_tool/ directory
  • continue to write both the old file and the new file for some period of time
  • send out a breaking change email for the change

@sortie
Copy link

sortie commented Sep 25, 2019

@nshahan Thanks for thinking about us. Our benchmarking infrastructure does not rely on the format of .packages, that is, it never attempts to parse it. We do rewrite some absolute paths in there but that will continue to work. We also supply our own and that will continue to work. I asked and we also don't have any such dependencies in our regular CI.

@leafpetersen
Copy link
Member

For discussion: should we actually stop generating the .packages file, or just keep generating it indefinitely? What's the cost of generating it, vs the cost of making the breaking change of removing it?

@jakemac53
Copy link
Contributor Author

I think we probably should keep it around until Dart 3.0, for two specific reasons:

  • The --packages=<packages-file> argument that various tools support should continue to work until that time, and should continue to accept the same format of file.
  • Similarly, Isolate.spawnUri should continue to accept a packageConfig argument (it can be deprecated, but should probably continue to work with the same file format).

Otherwise this is likely a pretty significant breaking change.

@leafpetersen
Copy link
Member

We have a resolution to the immediate issue. When we remove support for .packages there will be additional migration work.

@DanTup
Copy link

DanTup commented Jun 22, 2020

Is the new JSON format fairly static now? Should I be thinking about switching Dart-Code over to it?

@jonasfj
Copy link
Member

jonasfj commented Jun 30, 2020

@DanTup, yes, afaik it's being used by dart when present, so if we change it we have to do so in backwards compatible ways.
Or we'll have to bump the version number.

Read about it here -> https://github.com/dart-lang/language/blob/master/accepted/future-releases/language-versioning/package-config-file-v2.md

@lrhn
Copy link
Member

lrhn commented Jun 30, 2020

If you do change to use the new format (which you should), please use package:package_config to read the file. One reason we could not just modify the existing .packages file was that there were too many cases of custom parsers making unwarranted assumptions. There are things that can be represented in several different ways, and we do not promise that we won't change the representation, only that it will still mean the same if you parse the file according to the specification. The package_config package does that for you.

@DanTup
Copy link

DanTup commented Jun 30, 2020

If you do change to use the new format (which you should), please use package:package_config to read the file

Dart-Code is written in TypeScript/JS (and runs in a Node VM) so this isn't straight forward. Long-term, it'd be nice to have a debug adapter written in Dart (using the DAP), but that probably won't happen in the near-term.

(I suspect the same goes for IntelliJ - I think it's likely parsing this in Java?)

@DanTup
Copy link

DanTup commented Aug 5, 2020

@lrhn here's the implementation I added in TS for Dart-Code:

https://github.com/Dart-Code/Dart-Code/blob/6bd2d8c5175715abf68dd920f3dca497d0ce8832/src/shared/pub/package_map.ts#L107-L133

It seems fairly straight-forward (the only thing we care about is the path for resolving a package: URI to) and seems to match what's at https://github.com/dart-lang/package_config/blob/9c586d04bd26fef01215fd10e7ab96a3050cfa64/lib/src/package_config_json.dart#L148-L155 if I've understood it correctly. If you see any obvious issues with it, let me know!

@lrhn
Copy link
Member

lrhn commented Aug 21, 2020

@DanTup A quick reading looks OK to me. Does the url.fileURLToPath(uri) return a platform specific path? Because then it might end in \ on Windows, and then the later .endsWith('/') won't match.

@DanTup
Copy link

DanTup commented Aug 24, 2020

@lrhn Oops, good catch! If the trailing slash was included in the path, we would've ended up with /\ on the end for Windows. It didn't seem to cause any issues (we send it through path.resolve which seems smart enough to handle), but I'll fix it up. parsedUri could've also ended up with different slashes depending on whether it used fileURLToPath (platform-specific) or just unescape (which would always return what was in the packages file, which is always / for the relative URIs even on Windows).

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nnbd NNBD related issues
Projects
None yet
Development

No branches or pull requests