-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migrated packages can't be successfully added to deps, even if in the allow list #42274
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
Comments
cc @franklinyow we can't publish any of these packages until we have an sdk where they are in the allow list, so this is a blocker for flutter depending on any NNBD migrated packages (and thus a blocker for the flutter tech preview) |
Note that this is also blocking all packages developed in the sdk from migrating to nnbd (meta, js, are important ones) |
@nshahan does dartdevc correctly handle package_config.json yet? |
@johnniwinther is there any work required from the back ends to make the allow list work? |
We didn't implement any handling of the package_config.json file in DDC. If we need to pass some information to the CFE we can do that but I'll wait to hear what Johnni says. |
@nshahan suggested the possibility that the error is from the pre-built sdk that is checked in rather than from dartdevc. @jakemac53 with your CL , if you try to run dartdevc.dart with the version of dart from the pre-built sdk, do you see this error? If so, this is the problem. |
Ok, that's the issue. If I patch in the CL from @jakemac53, gclient sync, build and then try running with the local build and the pre-built I get the following:
@athomas I think we just need a new prebuilt SDK checked in. |
Sounds good, are we going to have to do this for every package we want to migrate in? Should we try to do them all in one shot then? |
This should only affect packages that are indirectly used by dartdevc. My guess is that most of these are indirect dependencies from using bazel_worker. If the whitelist already includes all those dependencies already, I expect this would be a one-time thing. A prebuild SDK that has the full whitelist will be good to go, even if you roll the other packages later. An alternative solution here is to reduce the ddc dependencies. In this case, we can provide a separate dartdevc entrypoint that we use while building the SDK. Since we only have the dependency on bazel_worker to make the integration with webdev smoother, it won't really a necessary dependency of the compiler for the local build. If we add a second entrypoint that doesn't have this dependency and use it in the build steps, we'd also elide this issue. For instance, something among these lines: https://dart-review.googlesource.com/c/sdk/+/150860 |
I have https://dart-review.googlesource.com/c/sdk/+/150931 in the CQ to update the checked-in SDKs. |
#42274 Change-Id: I4ea2c533e4f47d42313a500c7c04c60c8a5c714b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150931 Auto-Submit: Alexander Thomas <[email protected]> Commit-Queue: Alexander Thomas <[email protected]> Commit-Queue: William Hesse <[email protected]> Reviewed-by: William Hesse <[email protected]>
The allow list does not contain anything today. It sounds like this will happen for ~all updates to that list, so we should probably update it all in one go to avoid this recurring issue. |
That could help, but I assume it still has some other deps like source_maps etc? |
There are some, but not as many. Here is the list from under dev_compiler/lib: Dart2js emits sourcemaps straight as a string and elides the last two dependencies. Probably not worth looking into that for dartdevc at this time. |
This reverts commit 045227e. Reason for revert: This seems to make a lot of windows builders fail with """ FileSystemException: Cannot copy file to 'C:/b/.../t_bytecode_simple_test_body.dart', path = 'C:\b\...\st_body.dart' (OS Error: The operation completed successfully. , errno = 0) #0 _File.throwIfError (dart:io/file_impl.dart:635:7) #1 _File.copySync (dart:io/file_impl.dart:340:5) #2 StandardTestSuite._makeCommands (package:test_runner/src/test_suite.dart:683:39) #3 StandardTestSuite._enqueueStandardTest (package:test_runner/src/test_suite.dart:611:22) #4 StandardTestSuite._testCasesFromTestFile (package:test_runner/src/test_suite.dart:590:7) #5 StandardTestSuite.findTestCases (package:test_runner/src/test_suite.dart:485:7) #6 TestCaseEnqueuer.enqueueTestSuites (package:test_runner/src/process_queue.dart:239:13) #7 new ProcessQueue (package:test_runner/src/process_queue.dart:177:22) #8 testConfigurations (package:test_runner/src/test_configurations.dart:255:3) #9 main (file:///.../test_runner.dart:39:5) """ Original change's description: > [infra] Update checked-in SDKs to 2.9.0-15.0.dev > > #42274 > > Change-Id: I4ea2c533e4f47d42313a500c7c04c60c8a5c714b > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150931 > Auto-Submit: Alexander Thomas <[email protected]> > Commit-Queue: Alexander Thomas <[email protected]> > Commit-Queue: William Hesse <[email protected]> > Reviewed-by: William Hesse <[email protected]> [email protected],[email protected] Change-Id: I71e885951c5d70e7dffa5d9ba7d1336d81986b72 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150938 Reviewed-by: Martin Kustermann <[email protected]> Commit-Queue: Martin Kustermann <[email protected]>
Per discussion offline, I think the plan for this is that we preemptively add all of the packages of interest to the allow list, roll that new pre-built SDK in, and then we believe that we should at that point be able to roll in ported versions of the packages without a new pre-built SDK being required. |
Ok, who is going to own adding all the packages to the list? |
@jakemac53 If we get you the list, would you mind wrangling the CL since you've already gone through it once? @kevmoo and/or @srawlins could probably put together the package list, maybe cross check with @jonahwilliams for the flutter side? It's not the end of the world if don't get it 100% on the first go, just need enough so that we don't have to go through this every day. |
@leafpetersen Sure, I can build the list with @kevmoo ? Is what we care about the list of packages required to run flutter and dart tests with strong null safety? |
That's a reasonable start. Probably everything that we pull in to the sdk in DEPS is worth adding as well (so long as they are packages that we control). |
https://dart-review.googlesource.com/c/sdk/+/151400 - I did not add the packages in DEPS for now. That expands the list quite a lot and I don't think we really want to do that (it increases the risk for every additional dep we add there) |
@sortie have you been able to look at the benchmark failure for fixnum? Is this also using the pre-built sdk or something along those lines or is there additional work that is needed to unblock landing these DEPS changes? |
It looks like I can't even add these packages to the allow list successfully, there is some sort of hack in for Additionally after fixing this the analyzer job still complains - it appears to think that it is in fact opted in regardless of the And apparently we also have jobs that are compiling it with strong mode, even though it has not been migrated. cc @nshahan can we remove this? |
As it is implemented in analyzer, |
All that |
I will fix the analyzer implementation. |
#42274 Change-Id: I3a566f14482de29e9aa499d8a8e7c4281c30fbd1 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151391 Auto-Submit: Alexander Thomas <[email protected]> Commit-Queue: Alexander Thomas <[email protected]> Commit-Queue: William Hesse <[email protected]> Reviewed-by: William Hesse <[email protected]>
https://dart-review.googlesource.com/c/sdk/+/151423 that should fix it landed. |
#42274 #42321 Change-Id: I5551ecd2803f9d26e720e5cf5671b3be8236bce9 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151423 Reviewed-by: Brian Wilkerson <[email protected]>
I filed an issue for the missing compile time error in package:js #42362. I think fixing that will actually break the SDK build so we can't just yet. @jakemac53 and I discussed offline and our plan is to migrate package:js along with the change for package:fixnum and rely on temporarily using the current SDK instead of the prebuilt to get those two packages on the allow list and opted in. |
I also had to migrate package:js to nnbd as a part of this Bug:#42274 Change-Id: Ia0223e013d2afb464c05eba71783827cf1fb4781 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151400 Auto-Submit: Jake Macdonald <[email protected]> Reviewed-by: Leaf Petersen <[email protected]>
@jakemac53 The benchmark problem is caused by golem not sending the package-config.json to the benchmark runners and the tryjob correctly detects that it would break golem. @sortie will have to fix that so that your CL can land. |
Ok, in the meantime I have landed changes to the allow list - so at this point we are blocked only on pulling in nnbd migrated DEPS. I do think we should do that, to help make sure we don't break any packages that are being pinned for the tech preview (or are at least aware they will break so we can forward-fix them, although that won't be an option for flutter due to pinning). |
I have deployed the change adding |
@jakemac53 – we're still blocked here by flutter-specific tests, right? |
Currently blocking by b/159723440 SDK build rules do not support NNBD deps |
b/159723440 resolved |
Bug:#42274 Change-Id: I18df7926fc8b767ab6ff6d3505f160147217a00f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/152605 Reviewed-by: Siva Annamalai <[email protected]> Commit-Queue: Jake Macdonald <[email protected]>
OK - so I have tried running with the flutter engine patch workflow (see https://dart-review.googlesource.com/c/sdk/+/152860). This has gotten me past some of the failures but I still see some others. For the flutter-hhh bots I still see this failure specifically https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8876134992926522320/+/steps/build_host_debug/0/stdout, which I think still means something is picking up the old package:collection, but is opting it in? |
For some more information here, this looks to be failing while trying to compile this analyzer script @a-siva any ideas? |
With flutter/engine#19469, package:collection and package:typed_data have successfully landed all the way through to flutter. This process does currently require breaking the HHH bots, so we will want to land prs in batches together to avoid to much breakage. However I think this is sufficient to close this issue. |
See my pending cl here to pull in the migrated fixnum, for nnbd strong mode benchmarks https://dart-review.googlesource.com/c/sdk/+/150170.
There are some build actions that appear to not be picking up the correct .dart_tool/package_config.json file, or something along those lines.
The text was updated successfully, but these errors were encountered: