Add "pub run --list" command to show all available executables #2 by akhomchenko · Pull Request #1680 · dart-lang/pub

akhomchenko · 2017-08-02T04:41:46Z

Good day.

This is a new iteration of #1323 based of #1427 as I was not able to push to that PR.

For now I've performed a merge with origin/master and updated the tests.
I'm going to address #1427 (comment)

googlebot · 2017-08-02T04:41:48Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

akhomchenko · 2017-08-02T04:42:33Z

test/run/list_test.dart

+
+    await pubGet();
+    var pub = await pubRun(args: ['--list']);
+    expect(await pub.stdoutStream().isEmpty, isTrue);


Not sure if this is the best way

jakemac53

Some additional small nits, it looks like the following still needs to be addressed from previous comments:

move to the pub deps command
add support for --dev, or add tests if it works

jakemac53 · 2017-08-02T17:43:30Z

lib/src/command/run.dart

+      ..addAll(entrypoint.root.immediateDependencies
+          .map((dep) => entrypoint.packageGraph.packages[dep.name]));
+
+    packages.forEach((Package package) {


nit: prefer for (var package in packages) { ... } (see here).

jakemac53 · 2017-08-02T17:48:03Z

lib/src/command/run.dart

+    return package
+        .listFiles(beneath: 'bin', recursive: false)
+        .where((executable) => p.extension(executable) == '.dart')
+        .map(p.basenameWithoutExtension);


Technically we should also check if these dart files have a top level main. You should be able to use parseDartFile from the analyzer to get the CompilationUnit, and then isEntrypoint from pub to do the check.

jakemac53 · 2017-08-02T17:59:08Z

lib/src/command/run.dart

+      // If executable matches the package name omit the name of executable in
+      // the output.
+      return executables.first != packageName
+          ? '${log.bold(packageName)}:${executables.first}'


It looks like @nex3 preferred bold for executable names and not bold for package names (see here)

jakemac53 · 2017-08-02T17:59:44Z

lib/src/command/run.dart

+          return e1.compareTo(e2);
+      });
+
+      return '${log.bold(packageName)}: ${executables.join(', ')}';


ditto, probably don't bold the commas though so you will need to wrap each executable in bold and join

jakemac53 · 2017-08-02T18:01:12Z

lib/src/command/run.dart

  RunCommand() {
    argParser.addFlag("checked",
        abbr: "c", help: "Enable runtime type checks and assertions.");
+    argParser.addFlag('list',


To me once this moves to the deps command then --list is no longer descriptive enough (I would expect it to just list deps not executables). Maybe --list-executables?

Actually it looks like @nex3 suggested just --executables, that sounds good to me as well and is a bit less verbose.

akhomchenko · 2017-08-03T07:11:48Z

I've addressed small issues. Moving command as flag to deps is the next step.

jakemac53 · 2017-08-03T13:58:52Z

looking good so far :)

akhomchenko · 2017-08-07T23:41:41Z

While I have some code to cleanup and tests to fix before review, I have some open questions. Thanks in advance.

akhomchenko · 2017-08-07T23:43:11Z

lib/src/command/deps.dart

+}
+
+/// The immediate non-dev dependencies this package specifies in its pubspec.
+List<PackageRange> _nonDevDependencies(Package package) {


Should I move this to a public API of Package? E.g. here: https://github.com/dart-lang/pub/blob/master/lib/src/package.dart#L78 right after immediateDependencies?

I think that the dependencies getter is actually identical to this, so I would just use that.

It does not include dependencyOverrides. Should I not care about them in such a case? (As they have the same binaries names)

Yes the dependencyOverrides are not relevant here. I don't think you can use that to add additional dependencies, or at least its not a supported use of that field afaik.

akhomchenko · 2017-08-07T23:45:03Z

lib/src/command/deps.dart

+  ///
+  /// Returns file names without extensions.
+  List<String> _getExecutablesFor(Package package) => package
+      .listFiles(beneath: 'bin', recursive: false)


There is already such a method in Package - executableIds. While I'm going to adapt _getExecutablesFor to use it, should _isDartExecutable be a part of executableIds?

Hmm, good question. To me it seems like executableIds should already be doing an equivalent _isDartExecutable check, but technically changing it to do that would be a breaking change.

The actual isEntrypoint check for instance doesn't actually catch all cases, so technically we could break existing packages so you could no longer run their executable. In reality my guess is it would not break anything, but its possible it would.

cc @kevmoo @munificent what do you think?

akhomchenko · 2017-08-07T23:46:06Z

test/deps/executables_test.dart

+
+const _validMain = 'main() {}';
+
+main() {


I haven't found a good way to test the same things.

I was choosing between:

for loop

current approach

even more test-fn-generators

You could consider wrapping multiple calls to test in _testExecutablesOutput, something like this:

_testExecutablesOutput(String name, output, {devOutput}) { devOutput ??= output; for (var dev in [true, false]) { test(name + dev ? ' --dev' : ' --no-dev', () async { await pubGet(); await runPub( args: ['deps', '--executables'] ..addAll(dev ? ['--dev'] : ['--no-dev']), output: dev ? devOutput : output); }); } }

Up to you though, that pattern can make it hard to support the named args to test methods etc.

jakemac53 · 2017-08-08T16:51:12Z

test/deps/executables_test.dart

+
+const _validMain = 'main() {}';
+
+main() {


You could consider wrapping multiple calls to test in _testExecutablesOutput, something like this:

_testExecutablesOutput(String name, output, {devOutput}) { devOutput ??= output; for (var dev in [true, false]) { test(name + dev ? ' --dev' : ' --no-dev', () async { await pubGet(); await runPub( args: ['deps', '--executables'] ..addAll(dev ? ['--dev'] : ['--no-dev']), output: dev ? devOutput : output); }); } }

Up to you though, that pattern can make it hard to support the named args to test methods etc.

jakemac53 · 2017-08-08T16:54:36Z

test/deps/executables_test.dart

+    setUp(() async {
+      await d.dir(appPath, [
+        d.appPubspec(),
+        d.dir('binn', [d.file('foo.dart', 'main() {')])


extra n :)

jakemac53 · 2017-08-09T14:52:01Z

test/deps/executables_test.dart

    test("are skipped if --no-dev flag is set", _testNonDevDepsOutput('\n'));
  });
-
-  group("lists dependencies overrides", () {


One thing that would probably be worth adding is a test where you have two versions of foo in different directories with different files under bin. Make the original dep be a path dep to one of them, and override it to the other one. You should get a list of the binaries in the overridden one not the original (pretty sure this would pass today).

I'll try it by I think it will not pass because of this

Actually, it will pass in case of --dev flag, but it will fail with --no-dev

Thats was my question here

I think it should still pass, the name of the dependencies doesn't change which is all we actually use, then we look up the package in the PackageGraph which should be overridden version.

If it doesn't pass, then I was wrong and we will need to do some more work to support this.

akhomchenko · 2017-08-09T22:25:29Z

So I think that PR is ready for a review, while I still do not like how tests are done.
If their style is ok for you, feel free to ignore my comment.

By the way, thank you for your review time and suggestions.

P.S. should I squash commit or you'll do it in case of merge?
(Probably it's better for me to squash my commits not to lose original author commit)

jakemac53 · 2017-08-11T14:03:15Z

@antonmoiseev can you please confirm here that you are ok with your original commit being contributed to this project?

jakemac53 · 2017-08-11T14:05:38Z

@gagoman I can do the squash/merge if you don't mind, I will retain the original author information though (otherwise if you squash and force push I have to re-review everything since its really a new commit and github doesn't handle the diffs well in that case).

jakemac53 · 2017-08-11T14:11:41Z

Actually I changed my mind, since this PR isn't very large and I can't push to your branch it makes sense for you to just do the squash of your commits, feel free to do that at any time.

* removed "run --list" command * buffer is used to output executables * tests were adapted to verify --dev/--no-dev flag

akhomchenko · 2017-08-11T18:24:26Z

@jakemac53 done

jakemac53 · 2017-08-14T14:51:07Z

Everything LGTM, but unfortunately we have to wait for @antonmoiseev to respond here with his approval to merge the initial commit before this can be submitted :(.

antonmoiseev · 2017-08-14T14:59:58Z

Sorry for the delay. Sure, please feel free to merge it if it's good enough.

On Mon, 14 Aug 2017 at 18:51, Jacob MacDonald ***@***.***> wrote: Everything LGTM, but unfortunately we have to wait for @antonmoiseev <https://github.com/antonmoiseev> to respond here with his approval to merge the initial commit before this can be submitted :(. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1680 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALKRTmRX5NzMzhxu6ZLy0DrqHrjhCgVks5sYF7dgaJpZM4Oql66> .

-- Sent from iPhone

jakemac53 · 2017-08-14T15:00:47Z

Thanks @antonmoiseev!

antonmoiseev and others added 2 commits July 22, 2016 13:49

Add "pub run --list" command to show all available executables.

c9f521d

See dart-lang#1323

Merge remote-tracking branch 'origin/master' into feature/1323

3d32057

googlebot added the cla: no label Aug 2, 2017

akhomchenko commented Aug 2, 2017

View reviewed changes

kevmoo requested a review from jakemac53 August 2, 2017 17:19

jakemac53 reviewed Aug 2, 2017

View reviewed changes

akhomchenko force-pushed the feature/1323 branch 2 times, most recently from 3cd5470 to 0976e90 Compare August 7, 2017 23:28

akhomchenko commented Aug 7, 2017

View reviewed changes

jakemac53 reviewed Aug 8, 2017

View reviewed changes

jakemac53 reviewed Aug 9, 2017

View reviewed changes

akhomchenko force-pushed the feature/1323 branch from 3172790 to 6d2f9c7 Compare August 9, 2017 22:52

added "pub deps --executables" command to show all available executables

9c3d6e0

* removed "run --list" command * buffer is used to output executables * tests were adapted to verify --dev/--no-dev flag

akhomchenko force-pushed the feature/1323 branch from 6d2f9c7 to 9c3d6e0 Compare August 11, 2017 17:48

jakemac53 merged commit c4faeaf into dart-lang:master Aug 14, 2017

akhomchenko deleted the feature/1323 branch August 14, 2017 18:19

akhomchenko mentioned this pull request Oct 6, 2017

Way to list all bins available to me in my app? #1323

Closed

Conversation

akhomchenko commented Aug 2, 2017

Uh oh!

googlebot commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakemac53 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhomchenko commented Aug 3, 2017

Uh oh!

jakemac53 commented Aug 3, 2017

Uh oh!

akhomchenko commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhomchenko Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhomchenko Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhomchenko commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakemac53 commented Aug 11, 2017

Uh oh!

jakemac53 commented Aug 11, 2017

Uh oh!

jakemac53 commented Aug 11, 2017

Uh oh!

akhomchenko commented Aug 11, 2017

Uh oh!

jakemac53 commented Aug 14, 2017

Uh oh!

antonmoiseev commented Aug 14, 2017 via email

Uh oh!

jakemac53 commented Aug 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

akhomchenko commented Aug 7, 2017 •

edited

Loading

akhomchenko Aug 8, 2017 •

edited

Loading

akhomchenko Aug 7, 2017 •

edited

Loading

akhomchenko commented Aug 9, 2017 •

edited

Loading