pub run --v2 for use in dartdev #2435

jonasfj · 2020-04-06T14:05:29Z

@jwren, if the first argument given to dartdev run matches [<package>[:<command>]] where <package> is [a-z_][a-z0-9_]* and <command> is [^/]+ then we forward to pub run --v2 (as introduced here).

Long term, we have to find a way to embed this properly and move the entire run related logic into a single place. Maybe pub just exports some functions for asserting that pubspec.yaml is installed, and what packages are installed, and whether they are immutable.. I don't think we have to clean this up for an MVP.

jwren

LGTM. I suspect that after it lands and you think the new capability is what you'll remove the v2 option and it will just be the new default?

natebosch

Can you clarify why we need this? Is there a design doc somewhere? Is this strictly because we don't think we will be able to have null safe code for pub before we want to support running with null safety and pub run?

natebosch · 2020-04-07T00:28:50Z

lib/src/command/run.dart

@@ -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.


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

Or we could say --dartdev-mode

natebosch · 2020-04-07T00:30:16Z

lib/src/command/run.dart

+  /// If neither `package` or `command` is given and `command` with name of
+  /// the current package doesn't exist we fallback to `'main'`.


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?

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.

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.

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

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

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

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?

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.

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

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?

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.

lib/src/command/run.dart

natebosch · 2020-04-07T00:35:20Z

lib/src/command/run.dart

+    var args = <String>[];
+
+    if (argResults.rest.isNotEmpty) {
+      if (argResults.rest[0].contains('/')) {


there are likely many characters it can't contain - why is / special? Do we need to check for \ on windows?

We could probably check more. We could disallow all characters not allowed in filenames, but that's probably not necessary as checking if a file with an illegal filename exists hopefully returns false.

But checking for slash and backslash is nice, that way we avoid going into sub-folders by accident.

test/run/v2/app_can_read_from_stdin_test.dart

jonasfj · 2020-04-22T15:17:39Z

LGTM. I suspect that after it lands and you think the new capability is what you'll remove the v2 option and it will just be the new default?

The plan is to use pub run --v2 when implementing dartdev run, eventually the pub command will go away and we will only have the pub run --v2 behavior, as it's exposed through dartdev run.

jonasfj · 2020-04-22T16:36:57Z

With this we can design dartdev run <arg> [...] such that if <arg> match [package[:command]] we forward the call to pub run --v2 <arg> [...].

And if <arg> doesn't match [package[:command]] we assume <arg> [...] is meant to be a options and file-path to be forwarded to the dartvm. Thus, we would spawn dart <arg> [...].

This means that dartdev run --observe=8080 path/to/my-file.dart works, because --observe=8080 doesn't match the [package[:command]] pattern.

However, it also means that dartdev run path/to/file.dart will not go through pub run --v2, which means we will run the dart path/to/file.dart without checking if dependencies from pubspec.yaml are present.

IMO, this is fine for an MVP. But I can see how long-term we would want to:

check that dependencies are installed when doing dartdev run path/to/file.dart, and
allow pass-through of VM arguments (such as --observe) when doing dartdev run [package[:command]].

natebosch · 2020-04-24T18:32:53Z

test/test_pub.dart

+Future<PubProcess> pubRunV2({Iterable<String> args}) async {
+  final pub = await startPub(args: ['run', '--v2', ...args]);
+
+  // Loading sources and transformers isn't normally printed, but the pub test


transformers have been removed. Was this copied from somewhere? Is it still necessary?

Blatantly stolen from pubRun above, but yes, it would be interesting to see if this is necessary.

pub run --v2 for use in dartdev

b4ac73d

jonasfj requested a review from sigurdm April 6, 2020 14:05

googlebot added the cla: yes label Apr 6, 2020

jwren approved these changes Apr 6, 2020

View reviewed changes

natebosch requested changes Apr 7, 2020

View reviewed changes

jonasfj added 2 commits April 22, 2020 18:20

Update year in license header

b729b42

Clarify snapshots and forbid slash/baskslash

41076fb

jonasfj requested a review from natebosch April 24, 2020 10:50

natebosch approved these changes Apr 24, 2020

View reviewed changes

natebosch reviewed Apr 24, 2020

View reviewed changes

sigurdm approved these changes May 1, 2020

View reviewed changes

jonasfj merged commit 363ed6f into dart-lang:master May 1, 2020

jonasfj deleted the pub-run-v2 branch May 1, 2020 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pub run --v2 for use in dartdev #2435

pub run --v2 for use in dartdev #2435

jonasfj commented Apr 6, 2020

jwren left a comment

natebosch left a comment

natebosch Apr 7, 2020

jonasfj Apr 22, 2020

natebosch Apr 7, 2020

jonasfj Apr 22, 2020 •

edited

Loading

natebosch Apr 22, 2020

mit-mit Apr 23, 2020

mit-mit Apr 23, 2020

mit-mit Apr 24, 2020

sigurdm Apr 24, 2020

jonasfj Apr 24, 2020

sigurdm Apr 24, 2020 •

edited by natebosch

Loading

natebosch Apr 24, 2020

natebosch Apr 7, 2020

jonasfj Apr 22, 2020

jonasfj commented Apr 22, 2020

jonasfj commented Apr 22, 2020

natebosch Apr 24, 2020

jonasfj Apr 27, 2020

		/// If neither `package` or `command` is given and `command` with name of
		/// the current package doesn't exist we fallback to `'main'`.

pub run --v2 for use in dartdev #2435

pub run --v2 for use in dartdev #2435

Conversation

jonasfj commented Apr 6, 2020

jwren left a comment

Choose a reason for hiding this comment

natebosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigurdm Apr 24, 2020 • edited by natebosch Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj commented Apr 22, 2020

jonasfj commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasfj Apr 22, 2020 •

edited

Loading

sigurdm Apr 24, 2020 •

edited by natebosch

Loading