Add a --[no]-packages-dir flag.

This replaces the hidden --no-package-symlinks flag.

Closes #1340

[email protected]

Review URL: https://codereview.chromium.org//2250643003 .

Author: nex3

lib/src/command.dart

@@ -37,8 +37,7 @@ abstract class PubCommand extends Command {
   Entrypoint get entrypoint {
     // Lazy load it.
     if (_entrypoint == null) {
-      _entrypoint = new Entrypoint('.', cache,
-          packageSymlinks: globalResults['package-symlinks']);
+      _entrypoint = new Entrypoint('.', cache);
     }
     return _entrypoint;
   }

lib/src/command/downgrade.dart

@@ -24,12 +24,19 @@ class DowngradeCommand extends PubCommand {
 
     argParser.addFlag('dry-run', abbr: 'n', negatable: false,
         help: "Report what dependencies would change but don't change any.");
+
+    argParser.addFlag('packages-dir',
+        negatable: true, defaultsTo: true,
+        help: "Generate a packages/ directory when installing packages.");
   }
 
   Future run() async {
     var dryRun = argResults['dry-run'];
     await entrypoint.acquireDependencies(SolveType.DOWNGRADE,
-        useLatest: argResults.rest, dryRun: dryRun);
+        useLatest: argResults.rest,
+        dryRun: dryRun,
+        packagesDir: argResults['packages-dir']);
+
     if (isOffline) {
       log.warning("Warning: Downgrading when offline may not update you to "
           "the oldest versions of your dependencies.");

lib/src/command/get.dart

@@ -25,11 +25,16 @@ class GetCommand extends PubCommand {
 
     argParser.addFlag('precompile', defaultsTo: true,
         help: "Precompile executables and transformed dependencies.");
+
+    argParser.addFlag('packages-dir',
+        negatable: true, defaultsTo: true,
+        help: "Generate a packages/ directory when installing packages.");
   }
 
   Future run() {
     return entrypoint.acquireDependencies(SolveType.GET,
         dryRun: argResults['dry-run'],
-        precompile: argResults['precompile']);
+        precompile: argResults['precompile'],
+        packagesDir: argResults['packages-dir']);
   }
 }

lib/src/command/upgrade.dart

@@ -28,13 +28,18 @@ class UpgradeCommand extends PubCommand {
 
     argParser.addFlag('precompile', defaultsTo: true,
         help: "Precompile executables and transformed dependencies.");
+
+    argParser.addFlag('packages-dir',
+        negatable: true, defaultsTo: true,
+        help: "Generate a packages/ directory when installing packages.");
   }
 
   Future run() async {
     await entrypoint.acquireDependencies(SolveType.UPGRADE,
         useLatest: argResults.rest,
         dryRun: argResults['dry-run'],
-        precompile: argResults['precompile']);
+        precompile: argResults['precompile'],
+        packagesDir: argResults['packages-dir']);
 
     if (isOffline) {
       log.warning("Warning: Upgrading when offline may not update you to the "

lib/src/command_runner.dart

@@ -60,9 +60,6 @@ class PubCommandRunner extends CommandRunner {
         negatable: false, help: 'Execute commands with prejudice.');
     argParser.addFlag('sparkle', hide: !isAprilFools,
         negatable: false, help: 'A more sparkly experience.');
-    argParser.addFlag('package-symlinks',
-        negatable: true, defaultsTo: true, hide: true,
-        help: "Generate packages/ directories when installing packages.");
 
     addCommand(new BuildCommand());
     addCommand(new CacheCommand());

lib/src/entrypoint.dart

@@ -71,10 +71,6 @@ class Entrypoint {
   /// the network.
   final SystemCache cache;
 
-  /// Whether to create and symlink a "packages" directory containing links to
-  /// the installed packages.
-  final bool _packageSymlinks;
-
   /// Whether this entrypoint is in memory only, as opposed to representing a
   /// real directory on disk.
   final bool _inMemory;
@@ -119,7 +115,7 @@ class Entrypoint {
   PackageGraph _packageGraph;
 
   /// The path to the entrypoint's "packages" directory.
-  String get packagesDir => root.path('packages');
+  String get packagesPath => root.path('packages');
 
   /// The path to the entrypoint's ".packages" file.
   String get packagesFile => root.path('.packages');
@@ -141,29 +137,21 @@ class Entrypoint {
   String get _snapshotPath => root.path('.pub', 'bin');
 
   /// Loads the entrypoint from a package at [rootDir].
-  ///
-  /// If [packageSymlinks] is `true`, this will create a "packages" directory
-  /// with symlinks to the installed packages. This directory will be symlinked
-  /// into any directory that might contain an entrypoint.
-  Entrypoint(String rootDir, SystemCache cache, {bool packageSymlinks: true,
-          this.isGlobal: false})
+  Entrypoint(String rootDir, SystemCache cache, {this.isGlobal: false})
       : root = new Package.load(null, rootDir, cache.sources),
         cache = cache,
-        _packageSymlinks = packageSymlinks,
         _inMemory = false;
 
   /// Creates an entrypoint given package and lockfile objects.
   Entrypoint.inMemory(this.root, this._lockFile, this.cache,
           {this.isGlobal: false})
-      : _packageSymlinks = false,
-        _inMemory = true;
+      : _inMemory = true;
 
   /// Creates an entrypoint given a package and a [solveResult], from which the
   /// package graph and lockfile will be computed.
   Entrypoint.fromSolveResult(this.root, this.cache, SolveResult solveResult,
           {this.isGlobal: false})
-      : _packageSymlinks = false,
-        _inMemory = true {
+      : _inMemory = true {
     _packageGraph = new PackageGraph.fromSolveResult(this, solveResult);
     _lockFile = _packageGraph.lockFile;
   }
@@ -186,9 +174,14 @@ class Entrypoint {
   /// If [precompile] is `true` (the default), this snapshots dependencies'
   /// executables and runs transformers on transformed dependencies.
   ///
+  /// If [packagesDir] is `true`, this will create "packages" directory with
+  /// symlinks to the installed packages. This directory will be symlinked into
+  /// any directory that might contain an entrypoint.
+  ///
   /// Updates [lockFile] and [packageRoot] accordingly.
   Future acquireDependencies(SolveType type, {List<String> useLatest,
-      bool dryRun: false, bool precompile: true}) async {
+      bool dryRun: false, bool precompile: true, bool packagesDir: false})
+      async {
     var result = await resolveVersions(type, cache, root,
         lockFile: lockFile, useLatest: useLatest);
     if (!result.succeeded) throw result.error;
@@ -201,17 +194,18 @@ class Entrypoint {
     }
 
     // Install the packages and maybe link them into the entrypoint.
-    if (_packageSymlinks) {
-      cleanDir(packagesDir);
+    if (packagesDir) {
+      cleanDir(packagesPath);
     } else {
-      deleteEntry(packagesDir);
+      deleteEntry(packagesPath);
     }
 
-    await Future.wait(result.packages.map(_get));
+    await Future.wait(result.packages
+        .map((id) => _get(id, packagesDir: packagesDir)));
     _saveLockFile(result);
 
-    if (_packageSymlinks) _linkSelf();
-    _linkOrDeleteSecondaryPackageDirs();
+    if (packagesDir) _linkSelf();
+    _linkOrDeleteSecondaryPackageDirs(packagesDir: packagesDir);
 
     result.summarizeChanges(type, dryRun: dryRun);
 
@@ -449,18 +443,18 @@ class Entrypoint {
   /// This automatically downloads the package to the system-wide cache as well
   /// if it requires network access to retrieve (specifically, if the package's
   /// source is a [CachedSource]).
-  Future _get(PackageId id) async {
+  Future _get(PackageId id, {bool packagesDir: false}) async {
     if (id.isRoot) return;
 
     var source = cache.source(id.source);
-    if (!_packageSymlinks) {
+    if (!packagesDir) {
       if (source is CachedSource) await source.downloadToSystemCache(id);
       return;
     }
 
-    var packageDir = p.join(packagesDir, id.name);
-    if (entryExists(packageDir)) deleteEntry(packageDir);
-    await source.get(id, packageDir);
+    var packagePath = p.join(packagesPath, id.name);
+    if (entryExists(packagePath)) deleteEntry(packagePath);
+    await source.get(id, packagePath);
   }
 
   /// Throws a [DataError] if the `.packages` file doesn't exist or if it's
@@ -654,42 +648,47 @@ class Entrypoint {
   /// Creates a self-referential symlink in the `packages` directory that allows
   /// a package to import its own files using `package:`.
   void _linkSelf() {
-    var linkPath = p.join(packagesDir, root.name);
+    var linkPath = p.join(packagesPath, root.name);
     // Create the symlink if it doesn't exist.
     if (entryExists(linkPath)) return;
-    ensureDir(packagesDir);
+    ensureDir(packagesPath);
     createPackageSymlink(root.name, root.dir, linkPath,
         isSelfLink: true, relative: true);
   }
 
-  /// If [packageSymlinks] is true, add "packages" directories to the whitelist
-  /// of directories that may contain Dart entrypoints.
+  /// If [packagesDir] is true, add "packages" directories to the whitelist of
+  /// directories that may contain Dart entrypoints.
   ///
   /// Otherwise, delete any "packages" directories in the whitelist of
   /// directories that may contain Dart entrypoints.
-  void _linkOrDeleteSecondaryPackageDirs() {
+  void _linkOrDeleteSecondaryPackageDirs({bool packagesDir: false}) {
     // Only the main "bin" directory gets a "packages" directory, not its
     // subdirectories.
     var binDir = root.path('bin');
-    if (dirExists(binDir)) _linkOrDeleteSecondaryPackageDir(binDir);
+    if (dirExists(binDir)) {
+      _linkOrDeleteSecondaryPackageDir(binDir, packagesDir: packagesDir);
+    }
 
     // The others get "packages" directories in subdirectories too.
     for (var dir in ['benchmark', 'example', 'test', 'tool', 'web']) {
-      _linkOrDeleteSecondaryPackageDirsRecursively(root.path(dir));
+      _linkOrDeleteSecondaryPackageDirsRecursively(root.path(dir),
+          packagesDir: packagesDir);
     }
  }
 
-  /// If [packageSymlinks] is true, creates a symlink to the "packages"
-  /// directory in [dir] and all its subdirectories.
+  /// If [packagesDir] is true, creates a symlink to the "packages" directory in
+  /// [dir] and all its subdirectories.
   ///
   /// Otherwise, deletes any "packages" directories in [dir] and all its
   /// subdirectories.
-  void _linkOrDeleteSecondaryPackageDirsRecursively(String dir) {
+  void _linkOrDeleteSecondaryPackageDirsRecursively(String dir,
+      {bool packagesDir: false}) {
     if (!dirExists(dir)) return;
-    _linkOrDeleteSecondaryPackageDir(dir);
-    _listDirWithoutPackages(dir)
-        .where(dirExists)
-        .forEach(_linkOrDeleteSecondaryPackageDir);
+    _linkOrDeleteSecondaryPackageDir(dir, packagesDir: packagesDir);
+    for (var subdir in _listDirWithoutPackages(dir)) {
+      if (!dirExists(subdir)) continue;
+      _linkOrDeleteSecondaryPackageDir(subdir, packagesDir: packagesDir);
+    }
   }
 
   // TODO(nweiz): roll this into [listDir] in io.dart once issue 4775 is fixed.
@@ -705,13 +704,13 @@ class Entrypoint {
     });
   }
 
-  /// If [packageSymlinks] is true, creates a symlink to the "packages"
-  /// directory in [dir].
+  /// If [packagesDir] is true, creates a symlink to the "packages" directory in
+  /// [dir].
   ///
   /// Otherwise, deletes a "packages" directories in [dir] if one exists.
-  void _linkOrDeleteSecondaryPackageDir(String dir) {
+  void _linkOrDeleteSecondaryPackageDir(String dir, {bool packagesDir: false}) {
     var symlink = p.join(dir, 'packages');
     if (entryExists(symlink)) deleteEntry(symlink);
-    if (_packageSymlinks) createSymlink(packagesDir, symlink, relative: true);
+    if (packagesDir) createSymlink(packagesPath, symlink, relative: true);
   }
 }

test/no_package_symlinks_test.dart renamed to test/no_packages_dir_test.dart

@@ -9,7 +9,7 @@ import 'test_pub.dart';
 
 main() {
   forBothPubGetAndUpgrade((command) {
-    group("with --no-package-symlinks", () {
+    group("with --no-packages-dir", () {
       integration("installs hosted dependencies to the cache", () {
         servePackages((builder) {
           builder.serve("foo", "1.0.0");
@@ -18,7 +18,7 @@ main() {
 
         d.appDir({"foo": "any", "bar": "any"}).create();
 
-        pubCommand(command, args: ["--no-package-symlinks"]);
+        pubCommand(command, args: ["--no-packages-dir"]);
 
         d.nothing("$appPath/packages").validate();
 
@@ -42,7 +42,7 @@ main() {
 
         d.appDir({"foo": {"git": "../foo.git"}}).create();
 
-        pubCommand(command, args: ["--no-package-symlinks"]);
+        pubCommand(command, args: ["--no-packages-dir"]);
 
         d.nothing("$appPath/packages").validate();
 
@@ -66,7 +66,7 @@ main() {
           })
         ]).create();
 
-        pubCommand(command, args: ["--no-package-symlinks"]);
+        pubCommand(command, args: ["--no-packages-dir"]);
 
         d.nothing("$appPath/packages").validate();
         d.matcherFile("$appPath/pubspec.lock", contains("foo"));
@@ -81,7 +81,7 @@ main() {
           d.dir("web/subdir/packages")
         ]).create();
 
-        pubCommand(command, args: ["--no-package-symlinks"]);
+        pubCommand(command, args: ["--no-packages-dir"]);
 
         d.dir(appPath, [
           d.nothing("packages"),
@@ -100,7 +100,7 @@ main() {
           d.dir("lib/packages")
         ]).create();
 
-        pubCommand(command, args: ["--no-package-symlinks"]);
+        pubCommand(command, args: ["--no-packages-dir"]);
 
         d.dir(appPath, [
           d.nothing("packages"),