From e0e438bf8b5dbc65c307eba35b947ed4ff18aaea Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Tue, 11 Feb 2020 20:14:06 +0000 Subject: [PATCH] Implement analysis options modification. Change-Id: I899067652bf00b3273559b59969d6b7c30883900 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135343 Commit-Queue: Janice Collins Reviewed-by: Mike Fairhurst --- .../bin/steamroll_ecosystem.dart | 11 +- .../lib/src/fantasyland/fantasy_repo.dart | 4 +- .../src/fantasyland/fantasy_repo_impl.dart | 6 +- .../src/fantasyland/fantasy_sub_package.dart | 121 ++++++++++++++++++ .../src/fantasyland/fantasy_workspace.dart | 10 +- .../fantasyland/fantasy_workspace_impl.dart | 43 ++++--- pkg/nnbd_migration/pubspec.yaml | 2 + .../test/fantasyland/fantasy_repo_test.dart | 10 +- .../fantasyland/fantasy_sub_package_test.dart | 17 +++ .../fantasyland/fantasy_workspace_test.dart | 3 +- pkg/nnbd_migration/tool/src/package.dart | 4 +- 11 files changed, 197 insertions(+), 34 deletions(-) diff --git a/pkg/nnbd_migration/bin/steamroll_ecosystem.dart b/pkg/nnbd_migration/bin/steamroll_ecosystem.dart index cc2124f3fd8..52222e3b210 100644 --- a/pkg/nnbd_migration/bin/steamroll_ecosystem.dart +++ b/pkg/nnbd_migration/bin/steamroll_ecosystem.dart @@ -14,6 +14,7 @@ import 'package:args/args.dart'; final parser = ArgParser() ..addMultiOption('extra-packages', abbr: 'e', splitCommas: true) ..addOption('package-name', abbr: 'p') + ..addFlag('allow-update', defaultsTo: false, negatable: true) ..addFlag('analysis-options-hack', defaultsTo: true, negatable: true) ..addFlag('strip-sdk-constraint-hack', defaultsTo: true, negatable: true) ..addFlag('force-migrate-deps', defaultsTo: true, negatable: true) @@ -45,11 +46,17 @@ Future main(List args) async { assert(extraPackages != null); FantasyWorkspace workspace = await buildFantasyLand( - packageName, extraPackages, path.canonicalize(results.rest.first)); + packageName, + extraPackages, + path.canonicalize(results.rest.first), + results['allow-update'] as bool); workspace.makeAllSymlinks(); if (results['analysis-options-hack'] as bool) { - stderr.writeln('warning: analysis options hack not implemented'); + await Future.wait([ + for (FantasySubPackage p in workspace.subPackages.values) + p.enableExperimentHack() + ]); } if (results['strip-sdk-constraint-hack'] as bool) { diff --git a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo.dart b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo.dart index 4b6f5b01270..bd32f260b9a 100644 --- a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo.dart +++ b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo.dart @@ -110,11 +110,11 @@ abstract class FantasyRepo { Folder get repoRoot; static Future buildGitRepoFrom( - FantasyRepoSettings repoSettings, String repoRootPath, + FantasyRepoSettings repoSettings, String repoRootPath, bool allowUpdate, {FantasyRepoDependencies fantasyRepoDependencies}) async { FantasyRepoGitImpl newRepo = FantasyRepoGitImpl(repoSettings, repoRootPath, fantasyRepoDependencies: fantasyRepoDependencies); - await newRepo.init(); + await newRepo.init(allowUpdate); return newRepo; } } diff --git a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo_impl.dart b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo_impl.dart index ab3472484bf..7c30ba6cb3f 100644 --- a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo_impl.dart +++ b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_repo_impl.dart @@ -55,12 +55,12 @@ class FantasyRepoGitImpl extends FantasyRepo { /// /// May throw [FantasyRepoException] in the event of problems and does /// not clean up filesystem state. - Future init() async { + Future init([bool allowUpdate = true]) async { assert(_isInitialized == false); - if (repoRoot.exists) { + if (repoRoot.exists && allowUpdate) { await _update(_external.launcher); // TODO(jcollins-g): handle "update" of pinned revision edge case - } else { + } else if (!repoRoot.exists) { await _clone(_external.launcher); } _isInitialized = true; diff --git a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_sub_package.dart b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_sub_package.dart index e9afbfa1a7c..46f2e3d504b 100644 --- a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_sub_package.dart +++ b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_sub_package.dart @@ -2,12 +2,17 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'package:analyzer_plugin/protocol/protocol_common.dart'; import 'package:analyzer/file_system/file_system.dart'; import 'package:analyzer/file_system/physical_file_system.dart'; +import 'package:analyzer/src/dart/analysis/experiments.dart'; import 'package:analyzer/src/lint/pub.dart'; +import 'package:analyzer/src/task/options.dart'; import 'package:nnbd_migration/src/fantasyland/fantasy_repo.dart'; import 'package:nnbd_migration/src/fantasyland/fantasy_workspace_impl.dart'; import 'package:path/path.dart' as path; +import 'package:source_span/source_span.dart'; +import 'package:yaml/yaml.dart'; final Map _subPackageTable = { '_fe_analyzer_shared': FantasySubPackageSettings( @@ -278,4 +283,120 @@ class FantasySubPackage { await _acceptPubspecVisitor(visitor); return visitor.results; } + + /// Delete any `pub get` output that interferes with a workspace. + Future cleanUp() async { + File pubspecLock = packageRoot.getChildAssumingFile('pubspec.lock'); + File dotPackages = packageRoot.getChildAssumingFile('.packages'); + Folder dartTool = packageRoot.getChildAssumingFolder('.dart_tool'); + File packageConfigJson = + dartTool.getChildAssumingFile('package_config.json'); + for (File f in [pubspecLock, dotPackages, packageConfigJson]) { + if (f.exists) f.delete(); + } + } + + void processYamlException(String operation, path, exception) { + // TODO(jcollins-g): implement + } + + /// Modify all analysis_options.yaml file to include the nullability + /// experiment. + Future enableExperimentHack() async { + // This is completely bonkers, cut and paste from non_nullable_fix.dart. + // But it is temporary, right? + // TODO(jcollins-g): Remove this hack once no longer needed. + File optionsFile = + packageRoot.getChildAssumingFile('analysis_options.yaml'); + SourceChange sourceChange = SourceChange('fantasy_sub_package-$name'); + String optionsContent; + YamlNode optionsMap; + if (optionsFile.exists) { + try { + optionsContent = optionsFile.readAsStringSync(); + } on FileSystemException catch (e) { + processYamlException('read', optionsFile.path, e); + return; + } + try { + optionsMap = loadYaml(optionsContent) as YamlNode; + } on YamlException catch (e) { + processYamlException('parse', optionsFile.path, e); + return; + } + } + + SourceSpan parentSpan; + String content; + YamlNode analyzerOptions; + if (optionsMap is YamlMap) { + analyzerOptions = optionsMap.nodes[AnalyzerOptions.analyzer]; + } + if (analyzerOptions == null) { + var start = SourceLocation(0, line: 0, column: 0); + parentSpan = SourceSpan(start, start, ''); + content = ''' +analyzer: + enable-experiment: + - non-nullable + +'''; + } else if (analyzerOptions is YamlMap) { + YamlNode experiments = + analyzerOptions.nodes[AnalyzerOptions.enableExperiment]; + if (experiments == null) { + parentSpan = analyzerOptions.span; + content = ''' + + enable-experiment: + - non-nullable'''; + } else if (experiments is YamlList) { + experiments.nodes.firstWhere( + (node) => node.span.text == EnableString.non_nullable, + orElse: () { + parentSpan = experiments.span; + content = ''' + + - non-nullable'''; + return null; + }, + ); + } + } + + if (parentSpan != null) { + final space = ' '.codeUnitAt(0); + final cr = '\r'.codeUnitAt(0); + final lf = '\n'.codeUnitAt(0); + + int offset = parentSpan.end.offset; + while (offset > 0) { + int ch = optionsContent.codeUnitAt(offset - 1); + if (ch == space || ch == cr) { + --offset; + } else if (ch == lf) { + --offset; + } else { + break; + } + } + SourceFileEdit fileEdit = SourceFileEdit(optionsFile.path, 0, + edits: [SourceEdit(offset, 0, content)]); + for (SourceEdit sourceEdit in fileEdit.edits) { + sourceChange.addEdit(fileEdit.file, fileEdit.fileStamp, sourceEdit); + } + } + _applyEdits(sourceChange); + } + + void _applyEdits(SourceChange sourceChange) { + for (var fileEdit in sourceChange.edits) { + File toEdit = packageRoot.getChildAssumingFile(fileEdit.file); + String contents = toEdit.exists ? toEdit.readAsStringSync() : ''; + for (SourceEdit edit in fileEdit.edits) { + contents = edit.apply(contents); + } + toEdit.writeAsStringSync(contents); + } + } } diff --git a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace.dart b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace.dart index 4c83b156f30..6c551411134 100644 --- a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace.dart +++ b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace.dart @@ -17,14 +17,16 @@ abstract class FantasyWorkspace { /// Add a package to the workspace, given [packageSettings]. /// /// Completes when the repository and subPackage is added. + /// If allowUpdate is true, the repository may be updated to the latest + /// version. Future addPackageToWorkspace( - FantasySubPackageSettings packageSettings); + FantasySubPackageSettings packageSettings, bool allowUpdate); } /// Build a "fantasyland"-style repository structure suitable for applying /// a migration to. -Future buildFantasyLand( - String topLevelPackage, List extraPackages, String fantasyLandDir) { +Future buildFantasyLand(String topLevelPackage, + List extraPackages, String fantasyLandDir, bool allowUpdate) { return FantasyWorkspaceTopLevelDevDepsImpl.buildFor( - topLevelPackage, extraPackages, fantasyLandDir); + topLevelPackage, extraPackages, fantasyLandDir, allowUpdate); } diff --git a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace_impl.dart b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace_impl.dart index d3a7cffaf91..0659bfdad77 100644 --- a/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace_impl.dart +++ b/pkg/nnbd_migration/lib/src/fantasyland/fantasy_workspace_impl.dart @@ -22,7 +22,7 @@ class FantasyWorkspaceError extends Error { // TODO(jcollins-g): consider refactor that makes resourceProvider required. class FantasyWorkspaceDependencies { - final Future Function(FantasyRepoSettings, String, + final Future Function(FantasyRepoSettings, String, bool, {FantasyRepoDependencies fantasyRepoDependencies}) buildGitRepoFrom; final ResourceProvider resourceProvider; final SubprocessLauncher launcher; @@ -30,7 +30,7 @@ class FantasyWorkspaceDependencies { FantasyWorkspaceDependencies( {ResourceProvider resourceProvider, SubprocessLauncher launcher, - Future Function(FantasyRepoSettings, String, + Future Function(FantasyRepoSettings, String, bool, {FantasyRepoDependencies fantasyRepoDependencies}) buildGitRepoFrom}) : resourceProvider = @@ -85,16 +85,17 @@ abstract class FantasyWorkspaceBase extends FantasyWorkspace { /// dependencies. /// /// Which dependencies are automatically added is implementation dependent. - Future addPackageNameToWorkspace(String packageName); + Future addPackageNameToWorkspace(String packageName, bool allowUpdate); Future addPackageToWorkspace( - FantasySubPackageSettings packageSettings) async { + FantasySubPackageSettings packageSettings, bool allowUpdate) async { FantasyRepo containingRepo = - await addRepoToWorkspace(packageSettings.repoSettings); + await addRepoToWorkspace(packageSettings.repoSettings, allowUpdate); FantasySubPackage fantasySubPackage = FantasySubPackage(packageSettings, containingRepo); // TODO(jcollins-g): throw if double add subPackages[packageSettings] = fantasySubPackage; + fantasySubPackage.cleanUp(); return fantasySubPackage; } @@ -102,15 +103,19 @@ abstract class FantasyWorkspaceBase extends FantasyWorkspace { /// Add one repository to the workspace. /// + /// If allowUpdate is true, the repository will be pulled before being + /// synced. + /// /// The returned [Future] completes when the repository is synced and cloned. - Future addRepoToWorkspace(FantasyRepoSettings repoSettings) { + Future addRepoToWorkspace( + FantasyRepoSettings repoSettings, bool allowUpdate) { if (_repos.containsKey(repoSettings.name)) return _repos[repoSettings.name]; Folder repoRoot = _external.resourceProvider.getFolder(_external .resourceProvider.pathContext .canonicalize(_external.resourceProvider.pathContext .join(workspaceRootPath, _repoSubDir, repoSettings.name))); _repos[repoSettings.name] = _external.buildGitRepoFrom( - repoSettings, repoRoot.path, + repoSettings, repoRoot.path, allowUpdate, fantasyRepoDependencies: FantasyRepoDependencies.fromWorkspaceDependencies(_external)); return _repos[repoSettings.name]; @@ -152,41 +157,46 @@ class FantasyWorkspaceTopLevelDevDepsImpl extends FantasyWorkspaceBase { : super._(workspaceRootPath, workspaceDependencies: workspaceDependencies); - static Future buildFor(String topLevelPackage, - List extraPackageNames, String workspaceRootPath, + static Future buildFor( + String topLevelPackage, + List extraPackageNames, + String workspaceRootPath, + bool allowUpdate, {FantasyWorkspaceDependencies workspaceDependencies}) async { var workspace = FantasyWorkspaceTopLevelDevDepsImpl._( topLevelPackage, workspaceRootPath, workspaceDependencies: workspaceDependencies); await Future.wait([ for (var n in [topLevelPackage, ...extraPackageNames]) - workspace.addPackageNameToWorkspace(n) + workspace.addPackageNameToWorkspace(n, allowUpdate) ]); return workspace; } Future addPackageNameToWorkspace( - String packageName) async { + String packageName, bool allowUpdate) async { FantasySubPackageSettings packageSettings = FantasySubPackageSettings.fromName(packageName); - return await addPackageToWorkspace(packageSettings); + return await addPackageToWorkspace(packageSettings, allowUpdate); } @override Future addPackageToWorkspace( - FantasySubPackageSettings packageSettings, + FantasySubPackageSettings packageSettings, bool allowUpdate, {Set seenPackages}) async { seenPackages ??= {}; if (seenPackages.contains(packageSettings)) return null; seenPackages.add(packageSettings); - await _addPackageToWorkspace(packageSettings, seenPackages); + return await _addPackageToWorkspace( + packageSettings, allowUpdate, seenPackages); } Future _addPackageToWorkspace( FantasySubPackageSettings packageSettings, + bool allowUpdate, Set seenPackages) async { FantasySubPackage fantasySubPackage = - await super.addPackageToWorkspace(packageSettings); + await super.addPackageToWorkspace(packageSettings, allowUpdate); String packageName = packageSettings.name; await rewritePackageConfigWith(fantasySubPackage); @@ -200,7 +210,8 @@ class FantasyWorkspaceTopLevelDevDepsImpl extends FantasyWorkspaceBase { await Future.wait([ for (var subPackageSettings in dependencies) - addPackageToWorkspace(subPackageSettings, seenPackages: seenPackages) + addPackageToWorkspace(subPackageSettings, allowUpdate, + seenPackages: seenPackages) ]); return fantasySubPackage; } diff --git a/pkg/nnbd_migration/pubspec.yaml b/pkg/nnbd_migration/pubspec.yaml index 47fa190edf6..5a212365740 100644 --- a/pkg/nnbd_migration/pubspec.yaml +++ b/pkg/nnbd_migration/pubspec.yaml @@ -5,7 +5,9 @@ environment: dependencies: _fe_analyzer_shared: 1.0.0 analyzer: ^0.37.0 + analyzer_plugin: ^0.2.2 path: ^1.6.2 + yaml: any dev_dependencies: args: ^1.5.2 mockito: any diff --git a/pkg/nnbd_migration/test/fantasyland/fantasy_repo_test.dart b/pkg/nnbd_migration/test/fantasyland/fantasy_repo_test.dart index e1d1b67c513..7a32dfbfb94 100644 --- a/pkg/nnbd_migration/test/fantasyland/fantasy_repo_test.dart +++ b/pkg/nnbd_migration/test/fantasyland/fantasy_repo_test.dart @@ -114,7 +114,9 @@ class FantasyRepoE2ETest { // checked out. io.Directory repoRoot = io.Directory(path.join(tempDir.path, 'repoRoot')); await FantasyRepo.buildGitRepoFrom( - FantasyRepoSettings('repoE2Etest', origRepoDir.path), repoRoot.path, + FantasyRepoSettings('repoE2Etest', origRepoDir.path), + repoRoot.path, + true, fantasyRepoDependencies: fantasyRepoDependencies); dotPackages = io.File(path.join(repoRoot.path, '.packages')); @@ -141,7 +143,9 @@ class FantasyRepoE2ETest { // Finally, use the repoBuilder to update a repository from head and verify // we did it right. await FantasyRepo.buildGitRepoFrom( - FantasyRepoSettings('repoE2Etest', origRepoDir.path), repoRoot.path, + FantasyRepoSettings('repoE2Etest', origRepoDir.path), + repoRoot.path, + true, fantasyRepoDependencies: fantasyRepoDependencies); aNewFile = io.File(path.join(repoRoot.path, 'hello_new_file_here')); @@ -172,7 +176,7 @@ class FantasyRepoTest extends FilesystemTestBase { FantasyRepoSettings settings = FantasyRepoSettings.fromName(repoName); FantasyRepoDependencies fantasyRepoDependencies = FantasyRepoDependencies( launcher: mockLauncher, resourceProvider: resourceProvider); - await FantasyRepo.buildGitRepoFrom(settings, repoPath, + await FantasyRepo.buildGitRepoFrom(settings, repoPath, true, fantasyRepoDependencies: fantasyRepoDependencies); } diff --git a/pkg/nnbd_migration/test/fantasyland/fantasy_sub_package_test.dart b/pkg/nnbd_migration/test/fantasyland/fantasy_sub_package_test.dart index 7459c328ea0..1d348f0a505 100644 --- a/pkg/nnbd_migration/test/fantasyland/fantasy_sub_package_test.dart +++ b/pkg/nnbd_migration/test/fantasyland/fantasy_sub_package_test.dart @@ -69,6 +69,7 @@ class FantasySubPackageTest extends FilesystemTestBase { FantasySubPackage fantasySubPackage; Folder repoRoot; File pubspecYaml; + File analysisOptionsYaml; setUp() { super.setUp(); @@ -85,6 +86,8 @@ class FantasySubPackageTest extends FilesystemTestBase { // the pubspecYaml. pubspecYaml = resourceProvider .getFile(join(repoRootPath, 'unreal_package_dir', 'pubspec.yaml')); + analysisOptionsYaml = resourceProvider.getFile( + join(repoRootPath, 'unreal_package_dir', 'analysis_options.yaml')); } test_recognizeAllDependencies() async { @@ -149,4 +152,18 @@ class FantasySubPackageTest extends FilesystemTestBase { '''); await fantasySubPackage.getPackageAllDependencies(); } + + test_analysisOptionsCreate() async { + pubspecYaml.writeAsStringSync(''' + name: unreal_package + '''); + await fantasySubPackage.enableExperimentHack(); + expect(analysisOptionsYaml.exists, isTrue); + expect( + analysisOptionsYaml.readAsStringSync(), + 'analyzer:\n' + ' enable-experiment:\n' + ' - non-nullable\n' + '\n'); + } } diff --git a/pkg/nnbd_migration/test/fantasyland/fantasy_workspace_test.dart b/pkg/nnbd_migration/test/fantasyland/fantasy_workspace_test.dart index 9765b03d47c..5fb3a9b7506 100644 --- a/pkg/nnbd_migration/test/fantasyland/fantasy_workspace_test.dart +++ b/pkg/nnbd_migration/test/fantasyland/fantasy_workspace_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -import 'dart:io'; - import 'package:mockito/mockito.dart'; import 'package:nnbd_migration/src/fantasyland/fantasy_workspace.dart'; import 'package:nnbd_migration/src/fantasyland/fantasy_workspace_impl.dart'; @@ -40,6 +38,7 @@ class FantasyWorkspaceIntegrationTest extends FilesystemTestBase { 'test_package', ['extra_package_1', 'extra_package_2'], convertPath('/fantasyland'), + true, workspaceDependencies: workspaceDependencies); expect(getFolder('/fantasyland').exists, isTrue); for (var n in ['test_package', 'extra_package_1', 'extra_package_2']) { diff --git a/pkg/nnbd_migration/tool/src/package.dart b/pkg/nnbd_migration/tool/src/package.dart index 2dc229badba..b0c42e6ba1e 100644 --- a/pkg/nnbd_migration/tool/src/package.dart +++ b/pkg/nnbd_migration/tool/src/package.dart @@ -91,8 +91,8 @@ class FantasyLandPackage extends Package { String name, Playground playground) async { return FantasyLandPackage._( name, - await buildFantasyLand( - name, [], path.join(playground.playgroundPath, '${name}__flat'))); + await buildFantasyLand(name, [], + path.join(playground.playgroundPath, '${name}__flat'), true)); } @override