Skip to content
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

Make packages versioned with the Dart SDK available via the SDK #39167

Open
dnfield opened this issue Oct 29, 2019 · 20 comments
Open

Make packages versioned with the Dart SDK available via the SDK #39167

dnfield opened this issue Oct 29, 2019 · 20 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). customer-flutter type-enhancement A request for a change that isn't a bug

Comments

@dnfield
Copy link
Contributor

dnfield commented Oct 29, 2019

The packages in https://github.com/dart-lang/sdk/tree/master/pkg/ are generally tightly coupled with the SDK itself. In particular, the kernel package can only be used if it corresponds exactly with the kernel version from the SDK. The versioning of these packages is not always entirely clear (especially as to how they correspond with a given Dart SDK version), and this is only further complicated by when trying to use such packages in tandem with Flutter/Flutter tool, which can be running a version of Dart that does not correspond to any published package on pub.

It would be really great if Dart could ship these packages as part of the SDK in a package folder, and then pub could be taught to recognize things like this:

dependencies:
  kernel:
    sdk: dart

This would also allow the flutter_tools to depend on the relevant Dart packages this way, and allow for downstream consumers to know what version of kernel etc. for a given Flutter checkout.

I encountered this when exploring options to resolve flutter/flutter#43644, where we want to read kernel data to figure out what constants are present related to font glyphs so we can shake out fonts.

/cc @zanderso @jonahwilliams

@zanderso zanderso added the area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). label Oct 29, 2019
@zanderso zanderso added area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. customer-flutter type-enhancement A request for a change that isn't a bug labels Oct 29, 2019
@jakemac53
Copy link
Contributor

Doing this will make the barrier to breaking changes much higher in these packages. Any breaking change would need to go through the full breaking change proposal process since users won't have the option to do any version selection of their own.

It is my opinion that the real issue here is that these packages are not properly restricting their upper bound on the SDK. Any package which might be affected by new language features should always be setting an upper bound of < the next minor release number (for example, >=2.5.0 <2.7.0).

As an example this allows breaking changes in the kernel format in minor version bumps of the sdk, while also allowing the flexibility to make breaking changes in the kernel packages that won't break downstream users.

When there is no version available for the current sdk (or it can't be selected due to some other constraint), then you will get the typical error messages from pub on get/upgrade instead of static errors in your project.

The main downside of this proposal is it requires more effort on the package owners - sdk constraints need to be carefully considered and new versions need to be published with each stable sdk release.

@jakemac53
Copy link
Contributor

cc @stereotype441 @scheglov for thoughts regarding analyzer which has fairly frequent breaking changes

@dnfield
Copy link
Contributor Author

dnfield commented Oct 29, 2019

If this were done, I think the contract would have to be that packages distributed with the SDK do not follow the same semver rules as the SDK version. This is effectively the way it works today, only it's harder to tell what packages are actually compatible with a given version of the SDK. For example, I have no idea right now how to properly depend on package:kernel for a given commit of the Dart SDK - and if I manage to figure it out, I'll be broken again the next time the kernel version bumps (and potentially broken both for the kernel incompatibility as well as API breakage in a new version of package:kernel).

@natebosch
Copy link
Member

it's harder to tell what packages are actually compatible with a given version of the SDK

For clarification, these packages are compatible with the constraint described versions of the SDK, for some definition of "compatible". The constraint today means "you can run this in a Dart VM from these versions of the Dart SDK.", it doesn't mean "you can parse Dart code written against these versions of the Dart SDK" which is, I think, the meaning you are hoping for.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 29, 2019

Yes - for package kernel in particular, I want to be able to parse kernel files.

@jonasfj
Copy link
Member

jonasfj commented Oct 29, 2019

@dnfield, I'm trying to understand the motivation. Can new versions of package:kernel parse old kernel files?

Or is it only that the latest version of package:kernel is required for parsing the latest kernel file format?

If we did this, there would be no way to publish packages on pub.dev which depends on package:kernel and doesn't have a tight SDK constraint banning the next minor version of the Dart SDK.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 29, 2019

@jonasfj if you try to parse a kernel file that was compiled with a different kernel version (which is independent from the Dart version and not explicit in the version of package:kernel), you will get an exception. AFAIK, it is not forwards or backwards compatible.

I'm not sure why you couldn't still publish those packages to pub - you could either have:

dependencies:
  kernel:
    sdk: dart # pull version packaged with the Dart SDK on the local file system

or

dependencies:
  kernel: 1.3.13 # Pull version from pub.dev

right?

@jakemac53
Copy link
Contributor

They really need to only be an sdk or pub dependency, otherwise you get in a scenario where one package wants it from pub and another wants it from the sdk? Only one version of the package can exist and those would be incompatible.

Unless we barred publishing with any sdk dependencies or something but that doesn't seem very useful.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 29, 2019

Only one version of the package can exist and those would be incompatible.

Why wouldn't that be treated like any other tight version constraint conflict?

@dnfield
Copy link
Contributor Author

dnfield commented Oct 29, 2019

i.e. sdk: dart is a tight version constraint, much like saying kernel: 1.2.3 instead of kernel: ^1.2.3. If two packages had tight version constraints in any other way it would conflict and fail, and then the consumer has to decide which package(s) they really want and whether they can all play nicely together.

@jonasfj
Copy link
Member

jonasfj commented Oct 29, 2019

If I write:

dependencies:
  kernel:
    sdk: dart

And you won't give me a stable public API for package:kernel, then my package will break with the next Dart minor release, thus, I must also write:

dependencies:
  kernel:
    sdk: dart
environment:
  sdk: '>=2.5.0 <2.6.0'

And whenever a new Dart SDK is published, then I must update my package accordingly.

I'm not quite sure this is what we want, it just moves the version number into the SDK minor version. And ties kernel to the SDK running the program, which is not always desirable. The Dart SDK running my program, should not necessarily decide what kernel files my program can parse.

On the other hand, in many cases I would imagine that it is desirable that the package:kernel used matches the active SDK. As you would naturally be parsing it's output :)
But such programs could (and likely should) have an SDK constraint on a specific dart minor version. And a constraint on the package:kernel version that can parse the match SDK kernel version. Maybe this is something that the CHANGELOG for package:kernel could point out.

Not sure, but if package:kernel version 0.3.28 can't parse files that could be read with version 0.3.0, then maybe each version of package kernel is a break release?
(I would suspect there is some compatibility, probably this is the public API, but not the formats that this API can read)

@natebosch
Copy link
Member

i.e. sdk: dart is a tight version constraint

It isn't a tight constraint unless the SDK constraint is also a single version.

In practice we'll need a new feature for this from pub and the constraint will need to look like:

dependencies:
  kernel:
    sdk: dart
    version: ^1.2.0

@jonasfj
Copy link
Member

jonasfj commented Oct 29, 2019

When flutter did federated packages we talked about giving a package two version numbers. This we resolved by creating two packages.

Maybe there is two packages here:

  • package:kernel_api which is a series of abstract classes. The major version is only bumped when the interfaces for parsing kernel files are broken.
  • package:kernel_parser which has a function that return concrete instance of an abstract class from package:kernel_api, which can be used to parse kernel files. The major version of this package is bumped whenever the kernel format breaks. In practice it might follow the Dart SDK minor versions (for it's major version).

This say you version the file-format in a package different from the versioning of the interfaces used to read the file-format. If this makes any sense :)

@stereotype441
Copy link
Member

I'm a little late to the party here but I'm concerned because the analyzer depends on the front end, which depends on kernel, even though the analyzer doesn't actually directly make use of the kernel format. So from the analyzer's perspective, it's not actually true that "the kernel package can only be used if it corresponds exactly with the kernel version from the SDK". I'd hate to have to lock the analyzer version to the SDK version just because of an issue with kernel file formats, when the analyzer doesn't actually use the kernel binary format at all.

Our strategy in the past has been to consider the kernel and front_end APIs to be private to the SDK, and unstable in the sense that they may change at any time without any semantic versioning guarantees. This has been working out ok so far because the only thing published on pub that used kernel was front_end, and likewise the only thing published on pub that used front_end was the analyzer. (If the analyzer didn't need these two packages, we would never have published them on pub in the first place). We've always published these three packages in lock step, with tight versioning dependencies between them, so that we didn't have to worry about keeping the APIs between them stable.

But it sounds like now Flutter is using the kernel package to read kernel files? If that's the case, then IMHO that means we have to stop doing this "lock step publishing" trick and actually version our APIs like good citizens of the pub ecosystem. A good first step there would be to do something like what @jonasfj suggested in the last comment.

I happen to be in Aarhus this week--I'd be happy to discuss the situation in person with Jonas (and perhaps @johnniwinther).

@dnfield
Copy link
Contributor Author

dnfield commented Oct 30, 2019

To clarify, Flutter is not yet using kernel directly, and there's no sensible way to do so for parsing kernel files. I will likely pursue using a kernel transformer in our own front end wrapper for now instead. However, other customers using flutter may want to parse kernel files and not have to update Flutter's front end code to do so.

Maybe an intermediate solution for now is to break kernel into two parts, one for parsing kernel files and one for everything else.

@stereotype441
Copy link
Member

After some discussion about this in Aarhus between myself, @jonasfj, @johnniwinther, @stefantsov, and @jensjoha , we're contemplating the idea of moving the shared parts of analyzer and front_end into a new package (which, notably, would not depend on kernel). That would decouple the analyzer from kernel completely. It wouldn't directly address this issue, but it would shrink its scope by decoupling the decision of how to import kernel (and what version to import) from any analyzer considerations. We could then consider options like:

  • Flutter's transformer imports kernel directly from the SDK using a relative path
  • Kernel increments their major version number with every SDK release, so that Flutter can be assured they have a compatible version of kernel by putting the proper version in the pubspec
  • Kernel publishes a separate package for each SDK release (e.g. kernel_2_7 to go with Dart 2.7)
  • Kernel is split into two packages, along the lines of @jonasfj's suggestion above.

I'm currently awaiting feedback from the analyzer and front end teams about the idea.

@dnfield
Copy link
Contributor Author

dnfield commented Oct 30, 2019

That sounds like it would achieve everything I'm hoping for in this bug.

@natebosch
Copy link
Member

  • Kernel publishes a separate package for each SDK release (e.g. kernel_2_7 to go with Dart 2.7)

-1. This will cause a lot of churn.

  • Kernel increments their major version number with every SDK release, so that Flutter can be assured they have a compatible version of kernel by putting the proper version in the pubspec

Does the kernel file format change on every SDK release?

I also feel this is modeling the wrong thing. A coupling with the SDK should be expressed by the SDK constraint in the pubspec, not by the semver versioning.

I think what we want is:

  • Bump the major version for kernel now. This will ensure that anyone who wants this new scheme will have a constraint that prevents them from picking up old kernel versions that didn't follow this scheme.
  • Put a narrow SDK constraint in the kernel pubspec. Any time the kernel format change bump the lower bound. Any time an SDK is released with the same kernel format bump the upper bound.
  • Bump the major version of kernel only when there are breaking changes for consumers, regardless of file format changes.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Oct 30, 2019

I'm slightly concerned we might be solving the wrong problem. The difficulty we've faced with kernel due to version/distribution don't have to be solved with different release strategies for the kernel package itself. Maybe there needs to be a different interface for code generation/compiler plugins that can be used without depending on the SDK. After all, why does track-widget-creation live in the Dart SDK instead of flutter/flutter?

@zanderso
Copy link
Member

I'm slightly concerned we might be solving the wrong problem. The difficulty we've face with kernel due to version/distribution don't have to be solved with different release strategies for the kernel package itself. Maybe there needs to be a different interface for code generation/compiler plugins that can be used without depending on the SDK. After all, why does track-widget-creation live in the Dart SDK instead of flutter/flutter?

Right, the goal is to be able to write compiler plugins etc. out-of-tree. The API for doing that could have a goal of being stable across e.g. Dart SDK commits even if the underlying kernel format (by way of package:kernel or otherwise) isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). customer-flutter type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants