[M] Change in dart/sdk[main]: Remove left-over `.packages`, clean up `pubspec.yaml` files.
Lasse Nielsen (Gerrit)
Lasse Nielsen added 1 comment![]( "Open in Gerrit")
Removing references to `.packages` and making all `pubspec.yaml` files be including in `generate_package_config.dart` (I hope).
Related details
- Ben Konyi
- Kevin Moore
- Paul Berry
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Moore (Gerrit)
Kevin Moore voted Code-Review+1![]( "Open in Gerrit")
- Ben Konyi
- Lasse Nielsen
- Paul Berry
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Berry (Gerrit)
Paul Berry added 4 comments![]( "Open in Gerrit")
baseUri = Uri.parse('dart:$path/');This looks like more than just a clean-up. AFAICT, it changes the behavior so that e.g. `dart:core` becomes `dart:core/`. Previously, it became `dart:core/core.dart` (see comment on line 16).
Is this a purposeful behavioral change? If so, let's move it to its own CL so it can get proper review
name: dartfuzzWhat's the rationale for removing this file?
This also feels like more than just clean-up, so consider moving to another CL.
name: botsWhat's the rationale for adding this file?
This also feels like more than just clean-up, so consider moving to another CL.
platform('runtime/tools/profiling'),These changes also seem to go beyond what I would have expected as clean-up. I think they should be reviewed separately, in a CL that explains the purpose for the changes.
Related details
- Ben Konyi
- Kevin Moore
- Lasse Nielsen
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen added 4 comments![]( "Open in Gerrit")
baseUri = Uri.parse('dart:$path/');This looks like more than just a clean-up. AFAICT, it changes the behavior so that e.g. `dart:core` becomes `dart:core/`. Previously, it became `dart:core/core.dart` (see comment on line 16).
Is this a purposeful behavioral change? If so, let's move it to its own CL so it can get proper review
There is no functional change, the `$path.dart` part had no effect on the resolution.
It doesn't change the URI of `dart:core` itself, that is still the canonical URI for the platform library. It only changes the base that other URIs are resolved against when the base is `dart:core`.
It makes no difference what's after the last `/` of the base URI when you resolve against it, that part will always be replaced.
(Unless you resolve with a completely empty URI, an empty string, which we can assume that the platform libraries won't do, and if they do, that would have created `dart:core/core.dart` which is a different URI than `dart:core`, so it wouldn't have worked anyway.)
name: dartfuzzWhat's the rationale for removing this file?
This also feels like more than just clean-up, so consider moving to another CL.
The rationale is that this is dead-config pruning.
The pubspec is not used, ever, so removing it should change nothing.
It's wasn't included by `generate_package_config.dart`, so the dependencies are not actually used for anything, code running won't see it as a package (you can't refer to it as `package:dartfuzz` anywhere in the SDK), and looking at the dependencies could be misleading, if they weren't so trivial.
Every dependency in there is also a dependency for the `tools/` package (and numeerous others): analyzer, args, matcher, and lints as dev-dependency, and the every dependency version is "any".
I chose between updating the pubspec to be useful and adding it to the generator script, or removing it, and couldn't see any benefit from adding it.
The only thing that *could* change is how `dart analyze` works on the directory,
since that's the one tool that can use `pubspec.yaml` as input instead of `package_config.json`. But with an SDK and language version of 2.10, that's clearly not happening.
name: botsWhat's the rationale for adding this file?
This also feels like more than just clean-up, so consider moving to another CL.
This one was probably not necessary.
One issue with adding the `tools/` pubspec to the generator script was that it gave the `tools/bots/compare_results.dart` script a package URI (`package:sdk_tools/bots/compare_results.dart`). When it's run (or analyzed) it has the package URI as URI, which makes its import of `../../pkg/test_runner/...` not work.
To avoid that, I added first this pubspec, figuring that since the directory has a `lib/` directory, maybe it should be a package by itself, and when that didn't work, also `lib/` in `tools/`. The latter should be enough, so I can remove this file again.
platform('runtime/tools/profiling'),These changes also seem to go beyond what I would have expected as clean-up. I think they should be reviewed separately, in a CL that explains the purpose for the changes.
The intended "clean up" here is to avoid having dangling `pubspec.yaml` files that are not included by `generate_package_config.dart`.
I can call it something else, but it is cleaning up a mess 😊
But I can see that I am combinging the removal of `.packages` with the clean-up of `pubspec.yaml` files and simplicification of the resolution code. I can try to split those three up.
Related details
- Ben Konyi
- Kevin Moore
- Paul Berry
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ben Konyi
| Code-Review | +1 |
1 comment
vm_service and DDS related changes LGTM.
Related details
- Kevin Moore
- Lasse Nielsen
- Paul Berry
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen added 1 comment![]( "Open in Gerrit")
This CL was split into three parts, fx: https://dart-review.googlesource.com/c/sdk/+/414020
Sorry I forgot to abandon the original.
Related details
- Ben Konyi
- Kevin Moore
- Paul Berry
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |