Commit a590ee26 authored by Jason Simmons's avatar Jason Simmons

Remove BuildConfigurations (#3879)

Artifacts from local engine builds will be found based on the --local-engine flag
parent 7072c8f2
...@@ -10,7 +10,7 @@ import '../android/android_sdk.dart'; ...@@ -10,7 +10,7 @@ import '../android/android_sdk.dart';
import '../application_package.dart'; import '../application_package.dart';
import '../base/os.dart'; import '../base/os.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../device.dart'; import '../device.dart';
import '../flx.dart' as flx; import '../flx.dart' as flx;
import '../globals.dart'; import '../globals.dart';
......
...@@ -7,7 +7,7 @@ import 'dart:io'; ...@@ -7,7 +7,7 @@ import 'dart:io';
import 'package:path/path.dart' as path; import 'package:path/path.dart' as path;
import 'package:xml/xml.dart' as xml; import 'package:xml/xml.dart' as xml;
import 'build_configuration.dart'; import 'build_info.dart';
import 'ios/plist_utils.dart'; import 'ios/plist_utils.dart';
abstract class ApplicationPackage { abstract class ApplicationPackage {
......
...@@ -6,7 +6,7 @@ import 'dart:io'; ...@@ -6,7 +6,7 @@ import 'dart:io';
import 'package:path/path.dart' as path; import 'package:path/path.dart' as path;
import 'build_configuration.dart'; import 'build_info.dart';
import 'globals.dart'; import 'globals.dart';
enum ArtifactType { enum ArtifactType {
......
...@@ -4,8 +4,6 @@ ...@@ -4,8 +4,6 @@
import 'dart:io'; import 'dart:io';
import 'package:path/path.dart' as path;
import 'base/utils.dart'; import 'base/utils.dart';
import 'globals.dart'; import 'globals.dart';
...@@ -81,37 +79,3 @@ HostPlatform getCurrentHostPlatform() { ...@@ -81,37 +79,3 @@ HostPlatform getCurrentHostPlatform() {
return HostPlatform.linux_x64; return HostPlatform.linux_x64;
} }
TargetPlatform getCurrentHostPlatformAsTarget() {
if (Platform.isMacOS)
return TargetPlatform.darwin_x64;
if (Platform.isLinux)
return TargetPlatform.linux_x64;
printError('Unsupported host platform, defaulting to Linux');
return TargetPlatform.linux_x64;
}
class BuildConfiguration {
BuildConfiguration.prebuilt({
this.hostPlatform,
this.targetPlatform,
this.testable: false
}) : type = BuildType.prebuilt, buildDir = null;
BuildConfiguration.local({
this.type,
this.hostPlatform,
this.targetPlatform,
String enginePath,
String buildPath,
this.testable: false
}) : buildDir = path.normalize(path.join(enginePath, buildPath)) {
assert(type == BuildType.debug || type == BuildType.release);
}
final BuildType type;
final HostPlatform hostPlatform;
final TargetPlatform targetPlatform;
final String buildDir;
final bool testable;
}
...@@ -12,7 +12,6 @@ import 'package:yaml/yaml.dart' as yaml; ...@@ -12,7 +12,6 @@ import 'package:yaml/yaml.dart' as yaml;
import '../artifacts.dart'; import '../artifacts.dart';
import '../base/utils.dart'; import '../base/utils.dart';
import '../build_configuration.dart';
import '../dart/analysis.dart'; import '../dart/analysis.dart';
import '../dart/sdk.dart'; import '../dart/sdk.dart';
import '../globals.dart'; import '../globals.dart';
...@@ -181,10 +180,9 @@ class AnalyzeCommand extends FlutterCommand { ...@@ -181,10 +180,9 @@ class AnalyzeCommand extends FlutterCommand {
Map<String, String> packages = dependencies.asPackageMap(); Map<String, String> packages = dependencies.asPackageMap();
// override the sky_engine and sky_services packages if the user is using a local build // override the sky_engine and sky_services packages if the user is using a local build
String buildDir = buildConfigurations.firstWhere((BuildConfiguration config) => config.testable, orElse: () => null)?.buildDir; if (tools.engineBuildPath != null) {
if (buildDir != null) { packages['sky_engine'] = path.join(tools.engineBuildPath, 'gen/dart-pkg/sky_engine/lib');
packages['sky_engine'] = path.join(buildDir, 'gen/dart-pkg/sky_engine/lib'); packages['sky_services'] = path.join(tools.engineBuildPath, 'gen/dart-pkg/sky_services/lib');
packages['sky_services'] = path.join(buildDir, 'gen/dart-pkg/sky_services/lib');
} }
if (argResults['preamble']) { if (argResults['preamble']) {
......
...@@ -8,7 +8,7 @@ import 'dart:io'; ...@@ -8,7 +8,7 @@ import 'dart:io';
import 'package:path/path.dart' as path; import 'package:path/path.dart' as path;
import '../base/process.dart'; import '../base/process.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../globals.dart'; import '../globals.dart';
import '../runner/flutter_command.dart'; import '../runner/flutter_command.dart';
import 'run.dart'; import 'run.dart';
...@@ -132,7 +132,7 @@ String buildAotSnapshot( ...@@ -132,7 +132,7 @@ String buildAotSnapshot(
'--no-sim-use-hardfp', '--no-sim-use-hardfp',
]; ];
if (!(tools.engineRelease || buildMode == BuildMode.release)) { if (buildMode != BuildMode.release) {
genSnapshotCmd.addAll(<String>[ genSnapshotCmd.addAll(<String>[
'--no-checked', '--no-checked',
'--conditional_directives', '--conditional_directives',
......
...@@ -12,7 +12,7 @@ import '../base/file_system.dart' show ensureDirectoryExists; ...@@ -12,7 +12,7 @@ import '../base/file_system.dart' show ensureDirectoryExists;
import '../base/os.dart'; import '../base/os.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../base/utils.dart'; import '../base/utils.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../flx.dart' as flx; import '../flx.dart' as flx;
import '../globals.dart'; import '../globals.dart';
import '../runner/flutter_command.dart'; import '../runner/flutter_command.dart';
......
...@@ -5,7 +5,7 @@ ...@@ -5,7 +5,7 @@
import 'dart:async'; import 'dart:async';
import '../application_package.dart'; import '../application_package.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../globals.dart'; import '../globals.dart';
import '../ios/mac.dart'; import '../ios/mac.dart';
import '../runner/flutter_command.dart'; import '../runner/flutter_command.dart';
......
...@@ -10,7 +10,7 @@ import '../android/android_device.dart'; ...@@ -10,7 +10,7 @@ import '../android/android_device.dart';
import '../application_package.dart'; import '../application_package.dart';
import '../base/context.dart'; import '../base/context.dart';
import '../base/logger.dart'; import '../base/logger.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../device.dart'; import '../device.dart';
import '../globals.dart'; import '../globals.dart';
import '../ios/devices.dart'; import '../ios/devices.dart';
......
...@@ -13,7 +13,7 @@ import '../application_package.dart'; ...@@ -13,7 +13,7 @@ import '../application_package.dart';
import '../base/file_system.dart'; import '../base/file_system.dart';
import '../base/common.dart'; import '../base/common.dart';
import '../base/os.dart'; import '../base/os.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../device.dart'; import '../device.dart';
import '../globals.dart'; import '../globals.dart';
import '../ios/simulators.dart' show SimControl, IOSSimulatorUtils; import '../ios/simulators.dart' show SimControl, IOSSimulatorUtils;
......
...@@ -8,7 +8,7 @@ import 'dart:io'; ...@@ -8,7 +8,7 @@ import 'dart:io';
import '../base/os.dart'; import '../base/os.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../device.dart'; import '../device.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../globals.dart'; import '../globals.dart';
import 'run.dart'; import 'run.dart';
......
...@@ -11,7 +11,7 @@ import 'package:path/path.dart' as path; ...@@ -11,7 +11,7 @@ import 'package:path/path.dart' as path;
import '../application_package.dart'; import '../application_package.dart';
import '../base/common.dart'; import '../base/common.dart';
import '../base/logger.dart'; import '../base/logger.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../device.dart'; import '../device.dart';
import '../globals.dart'; import '../globals.dart';
import '../runner/flutter_command.dart'; import '../runner/flutter_command.dart';
......
...@@ -7,9 +7,8 @@ import 'dart:io'; ...@@ -7,9 +7,8 @@ import 'dart:io';
import 'package:path/path.dart' as path; import 'package:path/path.dart' as path;
import '../artifacts.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../flx.dart' as flx; import '../flx.dart' as flx;
import '../globals.dart'; import '../globals.dart';
import '../runner/flutter_command.dart'; import '../runner/flutter_command.dart';
...@@ -73,19 +72,6 @@ class RunMojoCommand extends FlutterCommand { ...@@ -73,19 +72,6 @@ class RunMojoCommand extends FlutterCommand {
return _makePathAbsolute(path.join(argResults['mojo-path'], 'out', mojoBuildType, 'mojo_shell')); return _makePathAbsolute(path.join(argResults['mojo-path'], 'out', mojoBuildType, 'mojo_shell'));
} }
BuildConfiguration _getCurrentHostConfig() {
BuildConfiguration result;
TargetPlatform target = argResults['android'] ?
TargetPlatform.android_arm : getCurrentHostPlatformAsTarget();
for (BuildConfiguration config in buildConfigurations) {
if (config.targetPlatform == target) {
result = config;
break;
}
}
return result;
}
Future<List<String>> _getShellConfig(String targetApp) async { Future<List<String>> _getShellConfig(String targetApp) async {
List<String> args = <String>[]; List<String> args = <String>[];
...@@ -93,17 +79,8 @@ class RunMojoCommand extends FlutterCommand { ...@@ -93,17 +79,8 @@ class RunMojoCommand extends FlutterCommand {
final String command = useDevtools ? _getDevtoolsPath() : _getMojoShellPath(); final String command = useDevtools ? _getDevtoolsPath() : _getMojoShellPath();
args.add(command); args.add(command);
BuildConfiguration config = _getCurrentHostConfig(); TargetPlatform targetPlatform = argResults['android'] ? TargetPlatform.android_arm : TargetPlatform.linux_x64;
String flutterPath = path.join(tools.getEngineArtifactsDirectory(targetPlatform, BuildMode.debug).path, 'flutter.mojo');
String flutterPath;
if (config == null || config.type == BuildType.prebuilt) {
TargetPlatform targetPlatform = argResults['android'] ? TargetPlatform.android_arm : TargetPlatform.linux_x64;
Artifact artifact = ArtifactStore.getArtifact(type: ArtifactType.mojo, targetPlatform: targetPlatform);
flutterPath = _makePathAbsolute(ArtifactStore.getPath(artifact));
} else {
String localPath = path.join(config.buildDir, 'flutter.mojo');
flutterPath = _makePathAbsolute(localPath);
}
if (argResults['android']) if (argResults['android'])
args.add('--android'); args.add('--android');
......
...@@ -8,11 +8,10 @@ import 'dart:io'; ...@@ -8,11 +8,10 @@ import 'dart:io';
import 'package:path/path.dart' as path; import 'package:path/path.dart' as path;
import 'package:test/src/executable.dart' as executable; // ignore: implementation_imports import 'package:test/src/executable.dart' as executable; // ignore: implementation_imports
import '../artifacts.dart';
import '../build_configuration.dart';
import '../globals.dart'; import '../globals.dart';
import '../runner/flutter_command.dart'; import '../runner/flutter_command.dart';
import '../test/flutter_platform.dart' as loader; import '../test/flutter_platform.dart' as loader;
import '../toolchain.dart';
class TestCommand extends FlutterCommand { class TestCommand extends FlutterCommand {
TestCommand() { TestCommand() {
...@@ -41,23 +40,6 @@ class TestCommand extends FlutterCommand { ...@@ -41,23 +40,6 @@ class TestCommand extends FlutterCommand {
return true; return true;
}; };
Future<String> _getShellPath(BuildConfiguration config) async {
if (config.type == BuildType.prebuilt) {
Artifact artifact = ArtifactStore.getArtifact(
type: ArtifactType.shell, targetPlatform: config.targetPlatform);
return ArtifactStore.getPath(artifact);
} else {
switch (config.targetPlatform) {
case TargetPlatform.linux_x64:
return path.join(config.buildDir, 'sky_shell');
case TargetPlatform.darwin_x64:
return path.join(config.buildDir, 'SkyShell.app', 'Contents', 'MacOS', 'SkyShell');
default:
throw new Exception('Unsupported platform.');
}
}
}
Iterable<String> _findTests(Directory directory) { Iterable<String> _findTests(Directory directory) {
return directory.listSync(recursive: true, followLinks: false) return directory.listSync(recursive: true, followLinks: false)
.where((FileSystemEntity entity) => entity.path.endsWith('_test.dart') && .where((FileSystemEntity entity) => entity.path.endsWith('_test.dart') &&
...@@ -102,27 +84,13 @@ class TestCommand extends FlutterCommand { ...@@ -102,27 +84,13 @@ class TestCommand extends FlutterCommand {
testArgs.insert(0, '--'); testArgs.insert(0, '--');
if (Platform.environment['TERM'] == 'dumb') if (Platform.environment['TERM'] == 'dumb')
testArgs.insert(0, '--no-color'); testArgs.insert(0, '--no-color');
List<BuildConfiguration> configs = buildConfigurations;
bool foundOne = false;
loader.installHook(); loader.installHook();
for (BuildConfiguration config in configs) { loader.shellPath = tools.getHostToolPath(HostTool.SkyShell);
if (!config.testable) if (!FileSystemEntity.isFileSync(loader.shellPath)) {
continue; printError('Cannot find Flutter shell at ${loader.shellPath}');
foundOne = true;
loader.shellPath = path.absolute(await _getShellPath(config));
if (!FileSystemEntity.isFileSync(loader.shellPath)) {
printError('Cannot find Flutter shell at ${loader.shellPath}');
return 1;
}
await _runTests(testArgs, testDir);
if (exitCode != 0)
return exitCode;
}
if (!foundOne) {
printError('At least one of --engine-debug or --engine-release must be set, to specify the local build products to test.');
return 1; return 1;
} }
return await _runTests(testArgs, testDir);
return 0;
} }
} }
...@@ -14,7 +14,7 @@ import 'application_package.dart'; ...@@ -14,7 +14,7 @@ import 'application_package.dart';
import 'base/common.dart'; import 'base/common.dart';
import 'base/os.dart'; import 'base/os.dart';
import 'base/utils.dart'; import 'base/utils.dart';
import 'build_configuration.dart'; import 'build_info.dart';
import 'globals.dart'; import 'globals.dart';
import 'ios/devices.dart'; import 'ios/devices.dart';
import 'ios/simulators.dart'; import 'ios/simulators.dart';
......
...@@ -11,7 +11,7 @@ import 'package:path/path.dart' as path; ...@@ -11,7 +11,7 @@ import 'package:path/path.dart' as path;
import '../application_package.dart'; import '../application_package.dart';
import '../base/os.dart'; import '../base/os.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../device.dart'; import '../device.dart';
import '../globals.dart'; import '../globals.dart';
import 'mac.dart'; import 'mac.dart';
......
...@@ -9,7 +9,7 @@ import 'package:path/path.dart' as path; ...@@ -9,7 +9,7 @@ import 'package:path/path.dart' as path;
import '../artifacts.dart'; import '../artifacts.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../globals.dart'; import '../globals.dart';
import '../runner/flutter_command_runner.dart'; import '../runner/flutter_command_runner.dart';
......
...@@ -11,7 +11,7 @@ import 'package:path/path.dart' as path; ...@@ -11,7 +11,7 @@ import 'package:path/path.dart' as path;
import '../application_package.dart'; import '../application_package.dart';
import '../base/context.dart'; import '../base/context.dart';
import '../base/process.dart'; import '../base/process.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../device.dart'; import '../device.dart';
import '../flx.dart' as flx; import '../flx.dart' as flx;
import '../globals.dart'; import '../globals.dart';
......
...@@ -8,7 +8,7 @@ import 'dart:io'; ...@@ -8,7 +8,7 @@ import 'dart:io';
import 'package:args/command_runner.dart'; import 'package:args/command_runner.dart';
import '../application_package.dart'; import '../application_package.dart';
import '../build_configuration.dart'; import '../build_info.dart';
import '../dart/pub.dart'; import '../dart/pub.dart';
import '../device.dart'; import '../device.dart';
import '../flx.dart' as flx; import '../flx.dart' as flx;
...@@ -43,8 +43,6 @@ abstract class FlutterCommand extends Command { ...@@ -43,8 +43,6 @@ abstract class FlutterCommand extends Command {
bool get shouldRunPub => _usesPubOption && argResults['pub']; bool get shouldRunPub => _usesPubOption && argResults['pub'];
List<BuildConfiguration> get buildConfigurations => runner.buildConfigurations;
void usesTargetOption() { void usesTargetOption() {
argParser.addOption('target', argParser.addOption('target',
abbr: 't', abbr: 't',
......
...@@ -7,14 +7,20 @@ import 'dart:io'; ...@@ -7,14 +7,20 @@ import 'dart:io';
import 'package:path/path.dart' as path; import 'package:path/path.dart' as path;
import 'base/context.dart'; import 'base/context.dart';
import 'build_configuration.dart'; import 'build_info.dart';
import 'cache.dart'; import 'cache.dart';
import 'globals.dart'; import 'globals.dart';
enum HostTool { enum HostTool {
SkySnapshot, SkySnapshot,
SkyShell,
} }
const Map<HostTool, String> _kHostToolFileName = const <HostTool, String>{
HostTool.SkySnapshot: 'sky_snapshot',
HostTool.SkyShell: 'sky_shell',
};
/// A ToolConfiguration can return the tools directory for the current host platform /// A ToolConfiguration can return the tools directory for the current host platform
/// and the engine artifact directory for a given target platform. It is configurable /// and the engine artifact directory for a given target platform. It is configurable
/// via command-line arguments in order to support local engine builds. /// via command-line arguments in order to support local engine builds.
...@@ -38,9 +44,6 @@ class ToolConfiguration { ...@@ -38,9 +44,6 @@ class ToolConfiguration {
/// Path to a local engine build acting as a source for artifacts (--local-engine). /// Path to a local engine build acting as a source for artifacts (--local-engine).
String engineBuildPath; String engineBuildPath;
/// The engine mode to use (only relevent when [engineSrcPath] is set).
bool engineRelease;
bool get isLocalEngine => engineSrcPath != null; bool get isLocalEngine => engineSrcPath != null;
/// Return the directory that contains engine artifacts for the given targets. /// Return the directory that contains engine artifacts for the given targets.
...@@ -53,46 +56,8 @@ class ToolConfiguration { ...@@ -53,46 +56,8 @@ class ToolConfiguration {
} }
Directory _getEngineArtifactsDirectory(TargetPlatform platform, BuildMode mode) { Directory _getEngineArtifactsDirectory(TargetPlatform platform, BuildMode mode) {
if (engineSrcPath != null) { if (engineBuildPath != null) {
List<String> buildDir = <String>[]; return new Directory(engineBuildPath);
switch (platform) {
case TargetPlatform.android_arm:
case TargetPlatform.android_x64:
case TargetPlatform.android_x86:
buildDir.add('android');
break;
// TODO(devoncarew): We will need an ios vs ios_x86 target (for ios vs. ios_sim).
case TargetPlatform.ios:
buildDir.add('ios');
break;
// These targets don't have engine artifacts.
case TargetPlatform.darwin_x64:
case TargetPlatform.linux_x64:
buildDir.add('host');
break;
}
buildDir.add(getModeName(mode));
if (!engineRelease)
buildDir.add('unopt');
// Add a suffix for the target architecture.
switch (platform) {
case TargetPlatform.android_x64:
buildDir.add('x64');
break;
case TargetPlatform.android_x86:
buildDir.add('x86');
break;
default:
break;
}
return new Directory(path.join(engineSrcPath, 'out', buildDir.join('_')));
} else { } else {
String suffix = mode != BuildMode.debug ? '-${getModeName(mode)}' : ''; String suffix = mode != BuildMode.debug ? '-${getModeName(mode)}' : '';
...@@ -104,15 +69,25 @@ class ToolConfiguration { ...@@ -104,15 +69,25 @@ class ToolConfiguration {
} }
String getHostToolPath(HostTool tool) { String getHostToolPath(HostTool tool) {
if (tool != HostTool.SkySnapshot) if (engineBuildPath == null) {
throw new Exception('Unexpected host tool: $tool');
if (isLocalEngine) {
return path.join(engineBuildPath, 'clang_x64', 'sky_snapshot');
} else {
return path.join(_cache.getArtifactDirectory('engine').path, return path.join(_cache.getArtifactDirectory('engine').path,
getNameForHostPlatform(getCurrentHostPlatform()), getNameForHostPlatform(getCurrentHostPlatform()),
'sky_snapshot'); _kHostToolFileName[tool]);
}
if (tool == HostTool.SkySnapshot) {
String clangPath = path.join(engineBuildPath, 'clang_x64', 'sky_snapshot');
if (FileSystemEntity.isFileSync(clangPath))
return clangPath;
return path.join(engineBuildPath, 'sky_snapshot');
} else if (tool == HostTool.SkyShell) {
if (getCurrentHostPlatform() == HostPlatform.linux_x64) {
return path.join(engineBuildPath, 'sky_shell');
} else if (getCurrentHostPlatform() == HostPlatform.darwin_x64) {
return path.join(engineBuildPath, 'SkyShell.app', 'Contents', 'MacOS', 'SkyShell');
}
} }
throw 'Unexpected host tool: $tool';
} }
} }
...@@ -6,7 +6,7 @@ import 'dart:async'; ...@@ -6,7 +6,7 @@ import 'dart:async';
import 'package:flutter_tools/src/android/android_device.dart'; import 'package:flutter_tools/src/android/android_device.dart';
import 'package:flutter_tools/src/application_package.dart'; import 'package:flutter_tools/src/application_package.dart';
import 'package:flutter_tools/src/build_configuration.dart'; import 'package:flutter_tools/src/build_info.dart';
import 'package:flutter_tools/src/device.dart'; import 'package:flutter_tools/src/device.dart';
import 'package:flutter_tools/src/ios/devices.dart'; import 'package:flutter_tools/src/ios/devices.dart';
import 'package:flutter_tools/src/ios/simulators.dart'; import 'package:flutter_tools/src/ios/simulators.dart';
......
...@@ -4,7 +4,7 @@ ...@@ -4,7 +4,7 @@
import 'dart:io'; import 'dart:io';
import 'package:flutter_tools/src/build_configuration.dart'; import 'package:flutter_tools/src/build_info.dart';
import 'package:flutter_tools/src/cache.dart'; import 'package:flutter_tools/src/cache.dart';
import 'package:flutter_tools/src/toolchain.dart'; import 'package:flutter_tools/src/toolchain.dart';
import 'package:test/test.dart'; import 'package:test/test.dart';
...@@ -41,7 +41,7 @@ void main() { ...@@ -41,7 +41,7 @@ void main() {
testUsingContext('using enginePath', () { testUsingContext('using enginePath', () {
ToolConfiguration toolConfig = new ToolConfiguration(); ToolConfiguration toolConfig = new ToolConfiguration();
toolConfig.engineSrcPath = 'engine'; toolConfig.engineSrcPath = 'engine';
toolConfig.engineRelease = true; toolConfig.engineBuildPath = 'engine/out/android_debug';
expect( expect(
toolConfig.getEngineArtifactsDirectory(TargetPlatform.android_arm, BuildMode.debug).path, toolConfig.getEngineArtifactsDirectory(TargetPlatform.android_arm, BuildMode.debug).path,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment