Skip to content

pub run --v2 for use in dartdev #2435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions lib/src/command/run.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,15 @@ class RunCommand extends PubCommand {
argParser.addFlag('enable-asserts', help: 'Enable assert statements.');
argParser.addFlag('checked', abbr: 'c', hide: true);
argParser.addOption('mode', help: 'Deprecated option', hide: true);
// mode exposed for `dartdev run` to use as subprocess.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we find a more descriptive name? Maybe --as-subprocess or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could say --dartdev-mode

argParser.addFlag('v2', hide: true);
}

@override
Future run() async {
if (argResults['v2']) {
return await _runV2();
}
if (argResults.rest.isEmpty) {
usageException('Must specify an executable to run.');
}
Expand Down Expand Up @@ -86,4 +91,99 @@ class RunCommand extends PubCommand {
});
await flushThenExit(exitCode);
}

/// Implement a v2 mode for use in `dartdev run`.
///
/// Usage: `dartdev run [package[:command]]`
///
/// If `package` is not given, defaults to current root package.
/// If `command` is not given, defaults to name of `package`.
/// If neither `package` or `command` is given and `command` with name of
/// the current package doesn't exist we fallback to `'main'`.
Comment on lines +101 to +102
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the magic fallback to 'main'? Do we have templates that add a bin/main.dart by default?

I worry that adding something new - bin/<package>.dart changes the behavior of pub run. So if I've had a bin/main.dart and have been happily working, adding a new file gives new different magic behavior...

What does the error message look like if you pub run and there is no bin/<package.dart or bin/main.dart? Does it complain about missing main.dart only? That could encourage more people to add that, and if people publish those would the workflow end up looking like pub run some_package:main for most use cases? Or does pub run some_package also run bin/main.dart in that package?

Copy link
Member Author

@jonasfj jonasfj Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only does the fallback if bin/main.dart exists. @mit-mit wanted it because he wanted dartdev run without any arguments to work with out existing templates. I see that as valuable, but on the other hand I agree that using bin/main.dart will give a bad workflow in other cases.

I think the reasoning was that if you make a console app that is not distributed on pub.dev, using bin/main.dart is what our templates have done for years, and the drawbacks to supporting that are fairly slim.
IMO, people aren't going to notice that bin/main.dart works, because it's only a fallback that is used when:

  • calling without arguments, and,
  • bin/<packageName>.dart does not exist, and,
  • bin/main.dart does exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as long as the error message if none off the fallbacks exist mentions bin/<packageName>.dart, and does not mention bin/main.dart I think it's probably OK.

I hope to avoid new things that would subtly encourage that pattern, but I'm fine with attempting to keep compatibility with the other places that were already encouraging that pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What advantages does bin/<packageName>.dart have over bin/main.dart?

Flutter apps seem to use lib/main.dart, hmm...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonasfj you meant "bin/.dart does NOT exist, and" right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my only concern is that I don't understand why <packageName>.dart was originally selected? Using main as the main entry point is pretty common across a range of languages. Does it have some functional benefit that I'm not seeing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never liked/understood that convention either.
Neither for the main library (lib/<packageName>.dart) nor binary (bin/packageName>.dart).
There is too much repetition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python has __init__.py, node has index.js, rust has mod.rs, I don't know that <packageName>.dart is such a bad choice.

Copy link
Contributor

@sigurdm sigurdm Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is not the right place to take this discussion.
...
But <packageName>.dart is bad (ok, let's say it has drawbacks) for at least the following reasons:

  • if we rename the package we have to rename the main file, making this a more difficult operation (also making it even harder than it already is to vendor a package inside another - a use case I know you care about)

  • it made this feature (that just seems so obvious): Import shorthand syntax language#649 be non-trivial to specify.

I guess it comes down to composability. - why would the name of the main library have to depend on the surrounding package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is also why bin/main.dart will not be used if you do dartdev run <packageName> -- it only works for the local root package, ie. dartdev run (without arguments).

This is what I worry about, but maybe I'm imagining problems that don't exist.

What I'm worried is that pub run is conceptually a shorthand for "Run this package" becomes associated with bin/main.dart because that works. We know that pub run <package> is shorthand for "Run that named package". But now there is a difference in behavior between "Run this package" and "Run that package".

Regarding lib/<package>.dart - it's true that it's somewhat unnecessary repetition, but FWIW I think import 'package:something/lib.dart'; would end up with complaints about weird conventions too.

I don't think at this point its worth the churn to try to change it.

///
/// Runs `bin/<command>.dart` from package `<package>`. If `<package>` is not
/// mutable (local root package or path-dependency) a source snapshot will be
/// cached in `.dart_tool/pub/bin/<package>/<command>.dart.snapshot.dart2`.
Future _runV2() async {
var package = entrypoint.root.name;
var command = package;
var args = <String>[];

if (argResults.rest.isNotEmpty) {
if (argResults.rest[0].contains(RegExp(r'[/\\]'))) {
usageException('[<package>[:command]] cannot contain "/" or "\\"');
}

package = argResults.rest[0];
if (package.contains(':')) {
final parts = package.split(':');
if (parts.length > 2) {
usageException('[<package>[:command]] cannot contain multiple ":"');
}
package = parts[0];
command = parts[1];
} else {
command = package;
}
args = argResults.rest.skip(1).toList();
}

String snapshotPath(String command) => p.join(
entrypoint.cachePath,
'bin',
package,
'$command.dart.snapshot.dart2',
);

// If snapshot exists, we strive to avoid using [entrypoint.packageGraph]
// because this will load additional files. Instead we just run with the
// snapshot. Note. that `pub get|upgrade` will purge snapshots.
var snapshotExists = fileExists(snapshotPath(command));

// Don't ever compile snapshots for mutable packages, since their code may
// change later on. Don't check if this the case if a snapshot already
// exists.
var useSnapshot = snapshotExists ||
(package != entrypoint.root.name &&
!entrypoint.packageGraph.isPackageMutable(package));

// If argResults.rest.isEmpty, package == command, and 'bin/$command.dart'
// doesn't exist we use command = 'main' (if it exists).
// We don't need to check this if a snapshot already exists.
// This is a hack around the fact that we want 'dartdev run' to run either
// `bin/<packageName>.dart` or `bin/main.dart`, because `bin/main.dart` is
// a historical convention we've done in templates for a long time.
if (!snapshotExists && argResults.rest.isEmpty && package == command) {
final pkg = entrypoint.packageGraph.packages[package];
if (pkg == null) {
usageException('No such package "$package"');
}
if (!fileExists(pkg.path('bin', '$command.dart')) &&
fileExists(pkg.path('bin', 'main.dart'))) {
command = 'main';
snapshotExists = fileExists(snapshotPath(command));
useSnapshot = snapshotExists ||
(package != entrypoint.root.name &&
!entrypoint.packageGraph.isPackageMutable(package));
}
}

return await flushThenExit(await runExecutable(
entrypoint,
package,
'$command.dart',
args,
enableAsserts: argResults['enable-asserts'] || argResults['checked'],
snapshotPath: useSnapshot ? snapshotPath(command) : null,
recompile: () {
final pkg = entrypoint.packageGraph.packages[package];
// The recompile function will only be called when [package] exists.
assert(pkg != null);
return entrypoint.precompileExecutable(
package,
pkg.path('bin', '$command.dart'),
);
},
));
}
}
70 changes: 70 additions & 0 deletions test/run/v2/app_can_read_from_stdin_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
// 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:test/test.dart';

import '../../descriptor.dart' as d;
import '../../test_pub.dart';

void main() {
test('the spawned application can read line-by-line from stdin', () async {
await d.dir(appPath, [
d.appPubspec(),
d.dir('bin', [
d.file('script.dart', """
import 'dart:io';

main() {
print("started");
var line1 = stdin.readLineSync();
print("between");
var line2 = stdin.readLineSync();
print(line1);
print(line2);
}
""")
])
]).create();

await pubGet();
var pub = await pubRunV2(args: ['myapp:script']);

await expectLater(pub.stdout, emits('started'));
pub.stdin.writeln('first');
await expectLater(pub.stdout, emits('between'));
pub.stdin.writeln('second');
expect(pub.stdout, emits('first'));
expect(pub.stdout, emits('second'));
await pub.shouldExit(0);
});

test('the spawned application can read streamed from stdin', () async {
await d.dir(appPath, [
d.appPubspec(),
d.dir('bin', [
d.file('script.dart', """
import 'dart:io';

main() {
print("started");
stdin.listen(stdout.add);
}
""")
])
]).create();

await pubGet();
var pub = await pubRunV2(args: ['myapp:script']);

await expectLater(pub.stdout, emits('started'));
pub.stdin.writeln('first');
await expectLater(pub.stdout, emits('first'));
pub.stdin.writeln('second');
await expectLater(pub.stdout, emits('second'));
pub.stdin.writeln('third');
await expectLater(pub.stdout, emits('third'));
await pub.stdin.close();
await pub.shouldExit(0);
});
}
39 changes: 39 additions & 0 deletions test/run/v2/errors_if_only_transitive_dependency_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
// 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:test/test.dart';

import 'package:pub/src/exit_codes.dart' as exit_codes;

import '../../descriptor.dart' as d;
import '../../test_pub.dart';

void main() {
test('Errors if the script is in a non-immediate dependency.', () async {
await d.dir('foo', [
d.libPubspec('foo', '1.0.0'),
d.dir('bin', [d.file('bar.dart', "main() => print('foobar');")])
]).create();

await d.dir('bar', [
d.libPubspec('bar', '1.0.0', deps: {
'foo': {'path': '../foo'}
})
]).create();

await d.dir(appPath, [
d.appPubspec({
'bar': {'path': '../bar'}
})
]).create();

await pubGet();

var pub = await pubRunV2(args: ['foo:script']);
expect(pub.stderr, emits('Package "foo" is not an immediate dependency.'));
expect(pub.stderr,
emits('Cannot run executables in transitive dependencies.'));
await pub.shouldExit(exit_codes.DATA);
});
}
35 changes: 35 additions & 0 deletions test/run/v2/errors_if_path_in_dependency_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
// 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:test/test.dart';

import 'package:pub/src/exit_codes.dart' as exit_codes;

import '../../descriptor.dart' as d;
import '../../test_pub.dart';

void main() {
test(
'Errors if the executable is in a subdirectory in a '
'dependency.', () async {
await d.dir('foo', [d.libPubspec('foo', '1.0.0')]).create();

await d.dir(appPath, [
d.appPubspec({
'foo': {'path': '../foo'}
})
]).create();

await runPub(args: ['run', 'foo:sub/dir'], error: '''
Cannot run an executable in a subdirectory of a dependency.

Usage: pub run <executable> [args...]
-h, --help Print this usage information.
--[no-]enable-asserts Enable assert statements.

Run "pub help" to see global options.
See https://dart.dev/tools/pub/cmd/pub-run for detailed documentation.
''', exitCode: exit_codes.USAGE);
});
}
54 changes: 54 additions & 0 deletions test/run/v2/forwards_signal_posix_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
// 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.

// Windows doesn't support sending signals.
@TestOn('!windows')
import 'dart:io';

import 'package:test/test.dart';

import '../../descriptor.dart' as d;
import '../../test_pub.dart';

const _catchableSignals = [
ProcessSignal.sighup,
ProcessSignal.sigterm,
ProcessSignal.sigusr1,
ProcessSignal.sigusr2,
ProcessSignal.sigwinch,
];

const SCRIPT = """
import 'dart:io';

main() {
ProcessSignal.SIGHUP.watch().first.then(print);
ProcessSignal.SIGTERM.watch().first.then(print);
ProcessSignal.SIGUSR1.watch().first.then(print);
ProcessSignal.SIGUSR2.watch().first.then(print);
ProcessSignal.SIGWINCH.watch().first.then(print);

print("ready");
}
""";

void main() {
test('forwards signals to the inner script', () async {
await d.dir(appPath, [
d.appPubspec(),
d.dir('bin', [d.file('script.dart', SCRIPT)])
]).create();

await pubGet();
var pub = await pubRunV2(args: ['myapp:script']);

await expectLater(pub.stdout, emits('ready'));
for (var signal in _catchableSignals) {
pub.signal(signal);
await expectLater(pub.stdout, emits(signal.toString()));
}

await pub.kill();
});
}
35 changes: 35 additions & 0 deletions test/run/v2/loads_package_imports_in_a_dependency_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
// 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:test/test.dart';

import '../../descriptor.dart' as d;
import '../../test_pub.dart';

void main() {
test('loads package imports in a dependency', () async {
await d.dir('foo', [
d.libPubspec('foo', '1.0.0'),
d.dir('lib', [d.file('foo.dart', "final value = 'foobar';")]),
d.dir('bin', [
d.file('bar.dart', '''
import "package:foo/foo.dart";

main() => print(value);
''')
])
]).create();

await d.dir(appPath, [
d.appPubspec({
'foo': {'path': '../foo'}
})
]).create();

await pubGet();
var pub = await pubRunV2(args: ['foo:bar']);
expect(pub.stdout, emits('foobar'));
await pub.shouldExit();
});
}
24 changes: 24 additions & 0 deletions test/run/v2/nonexistent_dependency_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
// 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:test/test.dart';

import 'package:pub/src/exit_codes.dart' as exit_codes;

import '../../descriptor.dart' as d;
import '../../test_pub.dart';

void main() {
test('Errors if the script is in an unknown package.', () async {
await d.dir(appPath, [d.appPubspec()]).create();

await pubGet();
var pub = await pubRunV2(args: ['foo:script']);
expect(
pub.stderr,
emits('Could not find package "foo". Did you forget to add a '
'dependency?'));
await pub.shouldExit(exit_codes.DATA);
});
}
Loading