Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096723002:
Make pub generate .packages file. (Closed)
|
Created:
5 years, 8 months ago by Lasse Reichstein Nielsen Modified:
5 years, 6 months ago CC:
reviews_dartlang.org, bakster, sethladd, Bob Nystrom Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMake pub generate .packages file.
R=nweiz@google.com
Committed: https://github.com/dart-lang/pub/commit/6a1d7582e87c5a8324990a59fcd2c057aff25963
Patch Set 1 #Patch Set 2 : Use PackageGraph for data instead of collecting it myself. Move to separate file. #Patch Set 3 : Change extension to "cfg". Remove unused code. #
Total comments: 33
Patch Set 4 : Missing instance of packages.map->packages.cfg #
Total comments: 2
Patch Set 5 : Address comments. #
Total comments: 4
Patch Set 6 : Move change to pub package. #Patch Set 7 : Add test. #
Total comments: 57
Patch Set 8 : Address comments. #
Total comments: 18
Patch Set 9 : Address comments. #Patch Set 10 : Added more tests #
Total comments: 5
Patch Set 11 : Address comments. #
Messages
Total messages: 30 (5 generated)
rnystrom@google.com changed reviewers: + rnystrom@google.com
Looks pretty straightforward. It will be nice to be able to tear out the old symlink code too. We'll have to figure out what the migration plan for that is.
lrn@google.com changed reviewers: + nweiz@google.com
I've moved the packages.map code to a separate library, and begun adding code for creating the symlinks from that library too. I'll try to migrate all the symlink-code to the new library, so we'll end up with a single entry-point for creating either package.map, packages/ or both.
PTAL. Will try to add tests now.
bak@google.com changed reviewers: + bak@google.com
Random comments to the code. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:553: if (fileName == 'packages' || fileName == "packages.map") return []; const <String>[] https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:35: var text = new StringBuffer(); var text = new StringBuffer()..write(HEADER); https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:40: # AUTO GENERATED - DO NOT EDIT Consider hoisting the multi-line string outside the function to make the core more readable. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:47: text.writeln(uri); text..write(packageName) ..write("=") ..write(new Uri.directory(location));
https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:157: writePackagesMap(_packageGraph); Nit: I think this can fit on a single line. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:553: if (fileName == 'packages' || fileName == "packages.map") return []; On 2015/05/12 12:02:55, bakster wrote: > const <String>[] Please *don't* do this. It's contrary to pub's style elsewhere. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:1: /// Keeps track of used package locations, and can create a package dictionary. New files need a copyright comment. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:5: library package_locations; It seems strange to have this as a separate library when it's not very much code and the parallel symlink code is in Entrypoint. Also, all libraries should be namespaced under "pub." https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:14: /// Creates a `packages.cfg` file with the location of the packges in `graph`. This comment (and the code) indicates that this just creates a package spec, but the library comment indicates that it can also create package symlinks. Make that consistent. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:23: String content = _createPackagesMap(graph.packages); "var content" Don't type-annotate local variables. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:25: writeTextFile(file, content, dontLogContents: true); Why aren't the contents logged? We aren't expecting anything sensitive in these files, and we're already logging the lockfile's contents. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:27: log.exception(error, stackTrace); Why isn't this exception fatal? https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:31: String _createPackagesMap(Map<String, Package> packages) { Add a doc comment. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:37: # This file has been generate by the Dart tool pub on $now. "by the Dart tool pub" -> "by pub" https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:40: # AUTO GENERATED - DO NOT EDIT For the lockfile, we use a much terser comment: # Generated by pub # See http://pub.dartlang.org/doc/glossary.html#lockfile It might be nice for users if we do the same thing here. We can then have a more verbose (and updatable) explanation on the site. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:42: for (var packageName in packages.keys.toList()..sort()) { "packages.keys.toList()..sort()" -> "ordered(packages.keys)" https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:47: text.writeln(uri); On 2015/05/12 12:02:56, bakster wrote: > text..write(packageName) > ..write("=") > ..write(new Uri.directory(location)); (Note: the last one should be "writeln", not "write")
sethladd@google.com changed reviewers: + sethladd@google.com
https://chromiumcodereview.appspot.com/1096723002/diff/60001/sdk/lib/_interna... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://chromiumcodereview.appspot.com/1096723002/diff/60001/sdk/lib/_interna... sdk/lib/_internal/pub/lib/src/package_locations.dart:39: # Dart tools, including Dart VM and and Dart analyzer, rely on the content. double and here.
https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:157: writePackagesMap(_packageGraph); On 2015/05/12 18:21:29, nweiz wrote: > Nit: I think this can fit on a single line. Done. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:553: if (fileName == 'packages' || fileName == "packages.map") return []; Also the compare to packages.cfg is unnecessary since this is only used to filter directories. I'll undo this change again. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:1: /// Keeps track of used package locations, and can create a package dictionary. On 2015/05/12 18:21:29, nweiz wrote: > New files need a copyright comment. Done. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:5: library package_locations; Plan is to move symlink code here too, so it's all in a single place. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:14: /// Creates a `packages.cfg` file with the location of the packges in `graph`. Will remove the "symlink" comment until it becomes true. I'm working on that, but plan to do it in a different CL. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:23: String content = _createPackagesMap(graph.packages); On 2015/05/12 18:21:29, nweiz wrote: > "var content" > > Don't type-annotate local variables. Acknowledged. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:25: writeTextFile(file, content, dontLogContents: true); On 2015/05/12 18:21:30, nweiz wrote: > Why aren't the contents logged? We aren't expecting anything sensitive in these > files, and we're already logging the lockfile's contents. Acknowledged. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:27: log.exception(error, stackTrace); Because the place I copied the code from wasn't using a fatal error? :) https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:40: # AUTO GENERATED - DO NOT EDIT The string literal is not constant (it contains a "$now" interpolation. I'll make it a constant and do a string-replace instead of an interpolation to put in the time. Shorter header sounds like a great idea, but we can change to that when we have a target for the link. I prefer to give generated files a time-stamp. I can see a reason not to, if they are checked into version control (like the lockfile). https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:42: for (var packageName in packages.keys.toList()..sort()) { On 2015/05/12 18:21:30, nweiz wrote: > "packages.keys.toList()..sort()" -> "ordered(packages.keys)" Done. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:47: text.writeln(uri); On 2015/05/12 18:21:29, nweiz wrote: > On 2015/05/12 12:02:56, bakster wrote: > > text..write(packageName) > > ..write("=") > > ..write(new Uri.directory(location)); > > (Note: the last one should be "writeln", not "write") Done. https://codereview.chromium.org/1096723002/diff/60001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/60001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:39: # Dart tools, including Dart VM and and Dart analyzer, rely on the content. On 2015/05/12 20:17:40, sethladd wrote: > double and here. Done. https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:25: void writePackagesMap(PackageGraph graph, {String file}) { I feel like it would be prettier if I passed in the EntryPoint, not the PackageGraph, here. The only reason not to is that getting the package graph from an entry-point is an async operation. WDYT?
karlklose@google.com changed reviewers: + karlklose@google.com
https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:553: if (fileName == 'packages' || fileName == "packages.map") return []; On 2015/05/12 18:21:29, nweiz wrote: > On 2015/05/12 12:02:55, bakster wrote: > > const <String>[] > > Please *don't* do this. It's contrary to pub's style elsewhere. Returning a 'List<dynamic>' can lead to subtle errors because the instance will pass all kind of counter-intuitive type checks even in checked mode; I would try avoid it in all other places in pub, too.
https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:553: if (fileName == 'packages' || fileName == "packages.map") return []; I actually do use untyped lists internally in objects. They are typically accessed through typed functions, so I get static type checks and dynamic type checks on values entering and leaving the object. But then, I never test if an object is a List<String> - I either know it is a list of strings, or I test every element.
https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:25: void writePackagesMap(PackageGraph graph, {String file}) { On 2015/05/13 11:00:53, Lasse Reichstein Nielsen wrote: > I feel like it would be prettier if I passed in the EntryPoint, not the > PackageGraph, here. The only reason not to is that getting the package graph > from an entry-point is an async operation. > WDYT? We do tons of async operations in pub already, so I don't think there's anything wrong with doing one more.
https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/entrypoint.dart:553: if (fileName == 'packages' || fileName == "packages.map") return []; On 2015/05/13 11:44:01, karlklose wrote: > On 2015/05/12 18:21:29, nweiz wrote: > > On 2015/05/12 12:02:55, bakster wrote: > > > const <String>[] > > > > Please *don't* do this. It's contrary to pub's style elsewhere. > > Returning a 'List<dynamic>' can lead to subtle errors because the instance will > pass all kind of counter-intuitive type checks even in checked mode; I would try > avoid it in all other places in pub, too. In general the pub style is based on readability over checkability, so since untyped lists haven't caused problems in practice we use them because they're more readable. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:27: log.exception(error, stackTrace); On 2015/05/13 11:00:52, Lasse Reichstein Nielsen wrote: > Because the place I copied the code from wasn't using a fatal error? :) Where's that? An error in the package symlinking is fatal. https://codereview.chromium.org/1096723002/diff/40001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:40: # AUTO GENERATED - DO NOT EDIT On 2015/05/13 11:00:53, Lasse Reichstein Nielsen wrote: > The string literal is not constant (it contains a "$now" interpolation. > I'll make it a constant and do a string-replace instead of an interpolation to > put in the time. > > Shorter header sounds like a great idea, but we can change to that when we have > a target for the link. Can you add a TODO for this with a reference to an issue filed against dartlang.org? > I prefer to give generated files a time-stamp. I can see a reason not to, if > they are checked into version control (like the lockfile). SGTM https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:25: void writePackagesMap(PackageGraph graph, {String file}) { On 2015/05/13 11:00:53, Lasse Reichstein Nielsen wrote: > I feel like it would be prettier if I passed in the EntryPoint, not the > PackageGraph, here. The only reason not to is that getting the package graph > from an entry-point is an async operation. > WDYT? If this is only going to be called from Entrypoint and takes an instance of Entrypoint, it feels weird to me that it's not just in Entrypoint.
Need to write a test now. Any pointers on how to set it up to run pubGet() and check that the file is created with the expected content? https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... File sdk/lib/_internal/pub/lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/80001/sdk/lib/_internal/pub/l... sdk/lib/_internal/pub/lib/src/package_locations.dart:25: void writePackagesMap(PackageGraph graph, {String file}) { Passing an EntryPoint is probably overthinking it. It just needs the information in the package-graph to find the names and locations of pacakges. It could even work with just the packages (graph.packages.values) except for determining the default location.
> Need to write a test now. Any pointers on how to set it up > to run pubGet() and check that the file is created with the > expected content? Check out the tests under test/get and test/upgrade: we use the scheduled_test descriptor API to both set up and validate the filesystem. The best way to validate the ".packages" file is probably to add a custom descriptor to test/descriptor.dart that takes a map from names to locations and creates a descriptor for the corresponding packages file.
Reworked to use package:package_config and added a test. PTAL. What kind of tests would you normally do for a feature like this?
Additional tests I'd add: * Make sure this works with path dependencies. In particular, make sure relative path dependencies produce relative paths in the packages file. * The packages file doesn't get created if "pub get" fails. * The new packages file overwrites an existing file. https://codereview.chromium.org/1096723002/diff/120001/lib/src/entrypoint.dart File lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/120001/lib/src/entrypoint.dar... lib/src/entrypoint.dart:156: print("WRITE?"); Remove this. https://codereview.chromium.org/1096723002/diff/120001/lib/src/entrypoint.dar... lib/src/entrypoint.dart:157: if (_packageSymlinks) writePackagesMap(_packageGraph); Don't make this conditional on [_packageSymlinks]. That flag is there to control whether users get access to the soon-to-be-deprecated behavior, rather than whether they get any sort of package reference at all. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... File lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:5: /// Keeps track of used package locations, and can create a package dictionary. "used package locations" -> "the locations of packages installed in the entrypoint" https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:7: /// A package dictionary is currently a `.packages` file. What do you mean by "currently"? https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:9: // TODO(lrn): Also move packages/ directory management to this library. Nit: Move this above the library tag. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:11: import 'package:package_config/packages_file.dart' as pkgfile; We tend to use full words for identifiers except in a few special cases. I'd call this prefix "packages_file". https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:18: /// Creates a `.packages` file with the location of the packges in `graph`. "location" -> "locations", "packges" -> "packages", "`graph`" -> "[graph]" https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:20: /// The file is written next to the entry-point of [graph], "next to" -> "in the root directory of", "entry-point" -> "entrypoint" https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:24: void writePackagesMap(PackageGraph graph, {String file}) { [file] isn't used in this patch and I don't see any reason for its use, so it should be removed. Nit: we call it a "packages file" elsewhere, so these method names should reflect that. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:26: print("WRITE!: $file"); Remove this as well as debugging prints below. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:33: } catch (error, stackTrace) { This should be a fatal error. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:45: Dart tools, including Dart VM and Dart analyzer, rely on the content. "Dart VM" -> "the Dart VM". https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:49: /// Creates `.packages` file content from the packages in a package graph. "Creates" -> "Returns the contents of the" https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:50: String _createPackagesMap(PackageGraph packageGraph, File packagesFile) { "packagesFile" is a string. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:53: Uri fileUri = new Uri.file(packagesFile); Nit: don't type-annotate local variables, and move this down closer to where it's actually used. Use [p.toUri] here for consistency with the rest of the code. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:58: var location = packages[packageName].path("lib/"); Don't include a trailing "/" here. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:59: uriMap[packageName] = new Uri.directory(location); I don't think [Uri.directory] exists. Regardless, use [p.toUri] instead. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:63: pkgfile.write(text, uriMap, baseUri: fileUri, comment: header); We shouldn't pass [baseUri] here. Most types of dependencies will require paths to the pub cache, and making those paths relative will cause breakage if the package spec is moved around. https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml#newcode32 pubspec.yaml:32: package_config: "^0.0.0" The latest version is 0.0.3+1 and there's no changelog, so this should probably be "^0.0.3". https://codereview.chromium.org/1096723002/diff/120001/test/descriptor.dart File test/descriptor.dart (right): https://codereview.chromium.org/1096723002/diff/120001/test/descriptor.dart#n... test/descriptor.dart:190: Descriptor dotPackagesFile(String filePath, I'd just call this "packagesFile". Having the user explicitly pass in the full path of the descriptor breaks its ability to be used in arbitrary contexts. This should return a full descriptor rather than one that can just be used for validation. It's fine if the descriptor just does textual verification, but it's important that it can also be used to *create* .packages files. This will become more important as we move away from packages directories and move more of our tests over to using packages files instead. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... File test/get/dot_packages_test.dart (right): https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS d.file 2015 I'd call this file "packages_file_test". Also, since it's not specific to "pub get", I'd move it to "test/" and run it in "forBothPubGetAndUpgrade". https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:5: library pub_tests; Remove this. Some old tests have it, but new tests shouldn't. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:12: integration('re-gets a package if its source has changed', () { Update the test name. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:15: deps: {'baz': '2.2.2'}, We don't do argument alignment like this. It'll also definitely create noise if we ever run the formatter over this code. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:28: d.dir(appPath, [d.dotPackagesFile({"foo": "1.2.3", This should contain a link back to the application itself.
Addressed comments (except making error fatal - how?), PTAL again. https://codereview.chromium.org/1096723002/diff/120001/lib/src/entrypoint.dart File lib/src/entrypoint.dart (right): https://codereview.chromium.org/1096723002/diff/120001/lib/src/entrypoint.dar... lib/src/entrypoint.dart:157: if (_packageSymlinks) writePackagesMap(_packageGraph); ACK. Made uncondiational. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... File lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:5: /// Keeps track of used package locations, and can create a package dictionary. Reworded. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:7: /// A package dictionary is currently a `.packages` file. Too specualtive, yes. Removed. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:9: // TODO(lrn): Also move packages/ directory management to this library. On 2015/06/10 22:33:58, nweiz wrote: > Nit: Move this above the library tag. Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:11: import 'package:package_config/packages_file.dart' as pkgfile; Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:18: /// Creates a `.packages` file with the location of the packges in `graph`. On 2015/06/10 22:33:58, nweiz wrote: > "location" -> "locations", "packges" -> "packages", "`graph`" -> "[graph]" Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:20: /// The file is written next to the entry-point of [graph], On 2015/06/10 22:33:58, nweiz wrote: > "next to" -> "in the root directory of", "entry-point" -> "entrypoint" Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:24: void writePackagesMap(PackageGraph graph, {String file}) { On 2015/06/10 22:33:58, nweiz wrote: > [file] isn't used in this patch and I don't see any reason for its use, so it > should be removed. > > Nit: we call it a "packages file" elsewhere, so these method names should > reflect that. Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:26: print("WRITE!: $file"); On 2015/06/10 22:33:58, nweiz wrote: > Remove this as well as debugging prints below. Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:33: } catch (error, stackTrace) { How do you make it fatal? Not catching it? https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:45: Dart tools, including Dart VM and Dart analyzer, rely on the content. On 2015/06/10 22:33:58, nweiz wrote: > "Dart VM" -> "the Dart VM". Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:49: /// Creates `.packages` file content from the packages in a package graph. On 2015/06/10 22:33:58, nweiz wrote: > "Creates" -> "Returns the contents of the" Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:50: String _createPackagesMap(PackageGraph packageGraph, File packagesFile) { On 2015/06/10 22:33:58, nweiz wrote: > "packagesFile" is a string. Acknowledged. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:53: Uri fileUri = new Uri.file(packagesFile); On 2015/06/10 22:33:58, nweiz wrote: > Nit: don't type-annotate local variables, and move this down closer to where > it's actually used. > > Use [p.toUri] here for consistency with the rest of the code. Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:58: var location = packages[packageName].path("lib/"); Done. The result will have the trailing / anyway, yey! https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:59: uriMap[packageName] = new Uri.directory(location); It exists/will exist in v1.11, but using path.toUri anyway. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:63: pkgfile.write(text, uriMap, baseUri: fileUri, comment: header); Sounds fair. Omitting baseUri should avoid any relativization. https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml#newcode32 pubspec.yaml:32: package_config: "^0.0.0" Done. Went to 0.0.4, just to be sure. https://codereview.chromium.org/1096723002/diff/120001/test/descriptor.dart File test/descriptor.dart (right): https://codereview.chromium.org/1096723002/diff/120001/test/descriptor.dart#n... test/descriptor.dart:190: Descriptor dotPackagesFile(String filePath, On 2015/06/10 22:33:59, nweiz wrote: > I'd just call this "packagesFile". > > Having the user explicitly pass in the full path of the descriptor breaks its > ability to be used in arbitrary contexts. Since we changed the file to have only absolute URIs, the location is no longer needed, so YEY! > This should return a full descriptor rather than one that can just be used for > validation. It's fine if the descriptor just does textual verification, but it's > important that it can also be used to *create* .packages files. This will become > more important as we move away from packages directories and move more of our > tests over to using packages files instead. ACK. https://codereview.chromium.org/1096723002/diff/120001/test/descriptor.dart#n... test/descriptor.dart:197: Uri fileLocation = new Uri.directory(appPath).resolve(filePath); and p.toUri I guess. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... File test/get/dot_packages_test.dart (right): https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:1: // Copyright (c) 2012, the Dart project authors. Please see the AUTHORS d.file On 2015/06/10 22:33:59, nweiz wrote: > 2015 > > I'd call this file "packages_file_test". Also, since it's not specific to "pub > get", I'd move it to "test/" and run it in "forBothPubGetAndUpgrade". Done. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:5: library pub_tests; On 2015/06/10 22:33:59, nweiz wrote: > Remove this. Some old tests have it, but new tests shouldn't. Done. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:12: integration('re-gets a package if its source has changed', () { On 2015/06/10 22:33:59, nweiz wrote: > Update the test name. Done. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:15: deps: {'baz': '2.2.2'}, Arguments fixed to match formatter. The d.dir below gets completely fouled up by formatter, though. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:28: d.dir(appPath, [d.dotPackagesFile({"foo": "1.2.3", True. Updating test to check fail if there are more declarations than expected.
This is getting there! Don't forget those additional tests, though. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... File lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:33: } catch (error, stackTrace) { On 2015/06/11 11:21:02, Lasse Reichstein Nielsen wrote: > How do you make it fatal? Not catching it? Yeah, that's all that I mean. https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml#newcode32 pubspec.yaml:32: package_config: "^0.0.0" On 2015/06/11 11:21:02, Lasse Reichstein Nielsen wrote: > Done. Went to 0.0.4, just to be sure. It looks like 0.0.4 isn't released yet. https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... File test/get/dot_packages_test.dart (right): https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:15: deps: {'baz': '2.2.2'}, On 2015/06/11 11:21:02, Lasse Reichstein Nielsen wrote: > Arguments fixed to match formatter. > > The d.dir below gets completely fouled up by formatter, though. Yeah, I think Bob is working on better support for argument lists mixed with collections right now. https://codereview.chromium.org/1096723002/diff/140001/lib/src/package_locati... File lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/140001/lib/src/package_locati... lib/src/package_locations.dart:5: /// Keeps track of locatikons of packages, and can create a `.packages` file. "locatikons" -> "locations" https://codereview.chromium.org/1096723002/diff/140001/lib/src/package_locati... lib/src/package_locations.dart:24: deleteEntry(packagesFilePath); Is this explicit deletion necessary? I'm pretty sure writing to an existing file will overwrite its contents. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart File test/descriptor.dart (right): https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:193: /// The [dependencies] maps package names to version strings. Remove "The" https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:196: /// entries (one per key in []) with a path that contains the version string. "[]" -> "[dependencies]" https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:200: class _PackagesFileDescriptor extends Descriptor { This is big enough now it probably warrants its own file under descriptor/. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:211: _dependencies.forEach((k, v) { Nit: use full words for variables. Also, something semantic like "package" and "version" would be preferable to "key" and "value". https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:218: return Chain.track(new File(p.join(parent, name)) Chain.track isn't necessary here anymore. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:232: return Chain.track(new File(fullPath).readAsBytes()) Ditto. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:238: // matcher library, though. Remove this TODO.
https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... File test/get/dot_packages_test.dart (right): https://codereview.chromium.org/1096723002/diff/120001/test/get/dot_packages_... test/get/dot_packages_test.dart:15: deps: {'baz': '2.2.2'}, On 2015/06/12 23:48:08, nweiz wrote: > On 2015/06/11 11:21:02, Lasse Reichstein Nielsen wrote: > > Arguments fixed to match formatter. > > > > The d.dir below gets completely fouled up by formatter, though. > > Yeah, I think Bob is working on better support for argument lists mixed with > collections right now. Yup. It's landed on master in the dart_style repo now. I'll try to get a release out next week.
PTAL again :) https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... File lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:33: } catch (error, stackTrace) { On 2015/06/12 23:48:08, nweiz wrote: > On 2015/06/11 11:21:02, Lasse Reichstein Nielsen wrote: > > How do you make it fatal? Not catching it? > > Yeah, that's all that I mean. Done. https://codereview.chromium.org/1096723002/diff/120001/lib/src/package_locati... lib/src/package_locations.dart:33: } catch (error, stackTrace) { On 2015/06/12 23:48:08, nweiz wrote: > On 2015/06/11 11:21:02, Lasse Reichstein Nielsen wrote: > > How do you make it fatal? Not catching it? > > Yeah, that's all that I mean. Done. https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/1096723002/diff/120001/pubspec.yaml#newcode32 pubspec.yaml:32: package_config: "^0.0.0" But now it is - just being a little premature :) https://codereview.chromium.org/1096723002/diff/140001/lib/src/package_locati... File lib/src/package_locations.dart (right): https://codereview.chromium.org/1096723002/diff/140001/lib/src/package_locati... lib/src/package_locations.dart:5: /// Keeps track of locatikons of packages, and can create a `.packages` file. On 2015/06/12 23:48:08, nweiz wrote: > "locatikons" -> "locations" Done. https://codereview.chromium.org/1096723002/diff/140001/lib/src/package_locati... lib/src/package_locations.dart:24: deleteEntry(packagesFilePath); No idea. I was copying the packages/ directory behavior, and it makes more sense to delete there. I'll just remove the deletion. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart File test/descriptor.dart (right): https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:193: /// The [dependencies] maps package names to version strings. On 2015/06/12 23:48:08, nweiz wrote: > Remove "The" Done. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:196: /// entries (one per key in []) with a path that contains the version string. On 2015/06/12 23:48:08, nweiz wrote: > "[]" -> "[dependencies]" Done. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:200: class _PackagesFileDescriptor extends Descriptor { On 2015/06/12 23:48:08, nweiz wrote: > This is big enough now it probably warrants its own file under descriptor/. Done. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:211: _dependencies.forEach((k, v) { On 2015/06/12 23:48:08, nweiz wrote: > Nit: use full words for variables. Also, something semantic like "package" and > "version" would be preferable to "key" and "value". Done. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:218: return Chain.track(new File(p.join(parent, name)) On 2015/06/12 23:48:08, nweiz wrote: > Chain.track isn't necessary here anymore. Done. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:232: return Chain.track(new File(fullPath).readAsBytes()) On 2015/06/12 23:48:08, nweiz wrote: > Ditto. Done. https://codereview.chromium.org/1096723002/diff/140001/test/descriptor.dart#n... test/descriptor.dart:238: // matcher library, though. On 2015/06/12 23:48:08, nweiz wrote: > Remove this TODO. Done.
lgtm! https://codereview.chromium.org/1096723002/diff/180001/test/descriptor/packag... File test/descriptor/packages.dart (right): https://codereview.chromium.org/1096723002/diff/180001/test/descriptor/packag... test/descriptor/packages.dart:18: Nit: extra newline. https://codereview.chromium.org/1096723002/diff/180001/test/packages_file_tes... File test/packages_file_test.dart (right): https://codereview.chromium.org/1096723002/diff/180001/test/packages_file_tes... test/packages_file_test.dart:67: Nit: extra newline https://codereview.chromium.org/1096723002/diff/180001/test/packages_file_tes... test/packages_file_test.dart:90: }).validate(); We probably shouldn't be doing anything with the packages directory in the .packages file tests.
One other thing: before you commit, remove "proof-of-concept" from the CL description.
https://codereview.chromium.org/1096723002/diff/180001/test/packages_file_tes... File test/packages_file_test.dart (right): https://codereview.chromium.org/1096723002/diff/180001/test/packages_file_tes... test/packages_file_test.dart:67: On 2015/06/23 19:56:43, nweiz wrote: > Nit: extra newline Done. https://codereview.chromium.org/1096723002/diff/180001/test/packages_file_tes... test/packages_file_test.dart:90: }).validate(); On 2015/06/23 19:56:43, nweiz wrote: > We probably shouldn't be doing anything with the packages directory in the > .packages file tests. Done.
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as 6a1d7582e87c5a8324990a59fcd2c057aff25963 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
