-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Edge SDK fails to compile front_end 0.1.4+1 #34675
Comments
cc @kmillikin @whesse This should block the next dev release of the sdk so bumping to p0 |
We just published version 0.33.0 of analyzer, 0.1.6 of front_end, and 0.3.6 of kernel. |
Great @bwilkerson, but it will take some time to roll the world forward still. I updated the repro instructions to use analyzer <0.33.0 since I confirmed things do work with the latest version. Also, if this is in fact a new error in the CFE (which it appears to be since things work fine on the current dev sdk) then we need to make sure we are intentionally releasing a breaking change in the SDK without a breaking version bump, for which the bar should be extremely high. |
A more immediate short term solution might be to cherry-pick 8a99ab0 to branch analyzer-0.32 (note: this branch isn't mirrored to github) and publish an intermediate point release of front_end (I believe the correct version number would be 0.1.4+2). |
Is this still a P0 given the new analyzer? |
Correction to my previous comment: we would need to publish intermediate point releases of analyzer, front_end, and kernel, since their pubspecs refer to each other via exact version numbers. |
This could be related to the cfe work in |
So, this looks like CFE added an error check that it itself was violating. That violation was fixed, but anything with an old version of the CFE (as a package) and new SDK will break. Two questions:
|
Talked to @stereotype441 and @kevmoo - currently proceeding with the following actions:
|
CL to create point releases of the analyzer/kernel/front_end is here: https://dart-review.googlesource.com/c/sdk/+/78149. Once that is reviewed and lands I'll do the point release. Note that more fixes were required than I anticipated; in addition to cherry-picking 8a99ab0, I had to cherry-pick c95ef87, and make some minor changes to analyzer test code. I talked to @keertip about whether any internal code is affected by this fix; she's investigating and doesn't have a definitive answer yet. I'm going to do some checking on my own as well. |
Great, checking internally is a really good step in addition to checking the external packages. Even if we can't end up checking external code the internal code should give a good signal. |
I've just published:
|
@stereotype441 – looks like the updated packages made build happy - https://travis-ci.org/dart-lang/build/jobs/436858283 |
an update: Paul is working on a lint rule to catch other issues like this at analysis time |
A lint? If we are blowing up at runtime, shouldn't this be an error?
…On Fri, Oct 5, 2018 at 11:06 AM Devon Carew ***@***.***> wrote:
an update: Paul is working on a lint rule to catch other issues like this
an analysis time
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#34675 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABCiqQN8YJ7h9P3cQXYHoujb4a99LJxks5uh5-agaJpZM4XIeOc>
.
|
I believe that's to give us a better sense of the scope of the problem. |
That's right. The reason I'm doing a lint is because it's easier to cherry-pick into the internal codebase as an isolated change, so we can hopefully find internal code that is in danger of tripping up without having to do a full internal roll. |
ack
…On Fri, Oct 5, 2018 at 12:34 PM Paul Berry ***@***.***> wrote:
That's right. The reason I'm doing a lint is because it's easier to
cherry-pick into the internal codebase as an isolated change, so we can
hopefully find internal code that is in danger of tripping up without
having to do a full internal roll.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#34675 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABCiirmiIaqYpbHpWrlQcy3FFQpaHizks5uh7QsgaJpZM4XIeOc>
.
|
Is this still a P0? |
We could possibly bump it down now that we at least have a fix for the front_end package itself (and a pub upgrade should fix issues for people). However, we should complete the other evaluations of the impact of the change prior to the 2.1 release to ensure we don't break to much of the world, which should still be high priority (maybe just a p1 though?). |
Update: I ran my lint on internal code this weekend and it discovered no additional issues. So to my knowledge, this bug has been completely addressed and can be closed--the point releases of analyzer/front_end/kernel addressed the immediate issue, and the lint addressed the concern that there could be other failures lurking. @jakemac53 does that sound correct to you? |
@stereotype441 yep, I agree We should keep an eye out for users reporting this issue and make sure we are communicating quickly that they need to upgrade their packages to get the fix. If users are stuck on older versions of the front_end package for one reason or another, they will have to stay on an older SDK until they can upgrade. I also glanced at the flutter repo and it looks like their packages already pin the 0.1.5 front_end so that should already be ok as well. |
This looks like either a new error in the CFE or a mismatch in versions between front_end and kernel, which was fixed in front_end 0.1.5. However, you can't actually get that version without depending on the alpha version of the analyzer, which the world hasn't rolled to yet.
SDK version: Dart VM version: 2.1.0-edge.0cc7993ee356e7858dcc05964253b4a4e9a48ef9 (Thu Oct 4 14:54:28 2018 +0000) on "linux_x64"
Fetched from: https://storage.googleapis.com/dart-archive/channels/be/raw/latest/sdk/dartsdk-linux-x64-release.zip
Example failure:
Reproduction Instructions
Create a new package which depends on the analyzer at <0.33.0 version (which was just released and the world has to migrate to still), and a dart script that imports analyzer:
pubspec.yaml:
main.dart:
Try to snapshot the script:
The text was updated successfully, but these errors were encountered: