dart-lang / pub

The pub command line tool
https://dart.dev/tools/pub/cmd
BSD 3-Clause "New" or "Revised" License
1.05k stars 230 forks source link

quiet pub option #3998

Open Hixie opened 1 year ago

Hixie commented 1 year ago

It would be great if we could have an option to switch pub into a quiet mode where it outputs nothing unless something goes wrong. Pub is unfortunately more verbose than desirable right now:

Building flutter tool...
Resolving dependencies... (1.7s)
Got dependencies.

We don't need that output, the Building flutter tool... is enough.

Resolving dependencies...
  file 6.1.4 (7.0.0 available)
  material_color_utilities 0.5.0 (0.8.0 available)
  process 4.2.4 (5.0.0 available)
> vm_service 11.10.0 (was 11.9.0)
Changed 1 dependency!

This is especially egregious when there's a lot of dependencies. (I could see outputting a single line like Upgraded "vm_service" from 11.9.0 to 11.10.0, and "file" from 6.1.3 to 6.1.4., but the rest is not useful.)

andrewkolos commented 1 year ago

This is especially egregious when there's a lot of dependencies. (I could see outputting a single line like Upgraded "vm_service" from 11.9.0 to 11.10.0, and "file" from 6.1.3 to 6.1.4., but the rest is not useful.)

I think PUB_SUMMARY_ONLY^1 can be set in the environment to suppress output about each individual dependency, though this still falls short of the level of quietness this issue is requesting. I also think a CLI option would also be better for the sake of discoverability compared to an undocumented env var.

sigurdm commented 1 year ago

after discussing with @jonasfj:

when you first build the flutter or dart tools,

When building the flutter tools, the call to the pub process could be seen as an implementation detail, and we suggest running those calls with stdio turned off entirely.

Ideally these calls should never fail, and if they fail, the stdio can be captured and shown in an error trace to be filled into a bug report.

when you do something like flutter test we implicitly run pub get

Here we would suggest running the implicit pub get with PUB_SUMMARY_ONLY=TRUE. We believe that it is useful that the user sees the pub progress line (counting seconds if the resolution takes more than one second), and it is useful to know that a resolution has been performed.

We want to do the same thing for dart run and friends in https://github.com/dart-lang/sdk/issues/50422. (still work in progress...)

We could be convinced to reduce the output further such that the Resolving dependencies... line is deleted after the operation is done (if in a terminal) when PUB_SUMMARY_ONLY=TRUE. That way only a single line is left in the terminal from a successful call to pub.

a CLI option would also be better for the sake of discoverability compared to an undocumented env var.

We are a bit reluctant to expose this as a public CLI flag, trying to keep the exposed command-line interface simple.

Do we think the general user would want a --quiet mode?

Hixie commented 1 year ago

When building the flutter tools, the call to the pub process could be seen as an implementation detail, and we suggest running those calls with stdio turned off entirely. Ideally these calls should never fail, and if they fail, the stdio can be captured and shown in an error trace to be filled into a bug report.

Unfortunately these are run from bash scripts / batch files, so collecting the output and printing it on failure is non-trivial. Errors in these cases are rare but when they happen it's critical that we show the message.

We believe that it is useful that the user sees the pub progress line (counting seconds if the resolution takes more than one second), and it is useful to know that a resolution has been performed.

The problem is that pub's progress meter is a different style than flutter's progress meter so it basically just ends up looking ugly because we have multiple progress meter styles.

We are a bit reluctant to expose this as a public CLI flag, trying to keep the exposed command-line interface simple.

While I empathize, I think an undocumented (at least in the --help) environment variable is hurting simplicity much more than a command line argument would. :-)

If we wanted though we could make it a command line argument that only appears when you do --help --verbose, we do that a lot for flutter command line arguments.

jonasfj commented 1 year ago

Splitting the the two use-cases, here is two proposals for what we could do.

It's entirely possible that missing some details, or missing how some things in Flutter packages/ is supposed to work :rofl:

Use case (A): Fetching dependencies for flutter_tools

Unfortunately these are run from bash scripts / batch files, so collecting the output and printing it on failure is non-trivial. Errors in these cases are rare but when they happen it's critical that we show the message.

Are users expected to try and debug what happens here?

Maybe, what the bash script should do when building flutter_tools is to run:

dart pub get --enforce-lockfile > /dev/null 2> /dev/null || (
  echo 'Unable to fetch dependencies!'
  echo 'Ensure "packages/flutter_tools/pubspec.lock" satisfies "packages/flutter_tools/pubspec.yaml".' &&
  echo 'Try to run "dart pub get" inside "$FLUTTER_ROOT/packages/flutter_tools/".' &&
  exit 1
)

Then check packages/flutter_tools/pubspec.lock into the flutter/flutter repository.

Note. --enforce-lockfile will:

For the purpose of getting dependencies required by flutter_tools this is probably reasonable. And might free you from having tools that pin dependencies/transitive-dependencies inside packages/flutter_tools/pubspec.yaml.

If fetching dependencies fail, then it should always be because:

In case of (i), (ii) or (iii), this is most likely the user is hacking on the Flutter SDK, the solution is to cd $FLUTTER_ROOT/packages/flutter_tools/ && dart pub get (without --enforce-lockfile such that the pubspec.lock is updated).

In any case, what I'm saying is: For flutter_tools you might not want a quite mode. Changing the workflow a bit might be preferable. Currently, packages are all pinned in packages/flutter_tools/pubspec.yaml, so if a user gets a conflict, it's not really useful to look at the pub output (or the conflict). Instead they should do flutter update-packages --force-upgrade.

This only works because flutter_tools is not something people use a dependency, and therefore you really only need pubspec.lock to pin dependencies. Ofcourse, you might want to prevent them from using it as a library, it's currently an SDK package people can depend on (https://github.com/dart-lang/pub/issues/3980).

@sigurdm, am I missing any pitfalls here?


Use case (B): Fetching dependencies on-the-fly in flutter test

In an ideal world: flutter test should have access to ensurePubspecResolved and just call that. Maybe, one day we'll figure out how to embed pub in flutter_tools the way it is in dartdev.

Until then, flutter_tools probably has to call dart pub get as a subprocess. And here PUB_SUMMARY_ONLY=TRUE is probably the correct mode. That will print the summary of changes, if there are any.

For performance it might be reasonable to have flutter_tools do run a few checks on file modifications timestamps, such that if pubspec.yaml < pubspec.lock < .dart_tool/package_config.json, and then it skips the call to dart pub get. That would be close to what ensurePubspecResolved without the same performance, and with a loss of correctness/consistency when user has path-dependencies that have been modified. Or maybe this is too risky, it can definitely cause pain in some mono-repo use-cases.

@sigurdm isn't this close to what ensurePubspecResolved will do in dart test?

Hixie commented 1 year ago

Unfortunately these are run from bash scripts / batch files, so collecting the output and printing it on failure is non-trivial. Errors in these cases are rare but when they happen it's critical that we show the message.

Are users expected to try and debug what happens here?

No, they're expected to just copy and paste everything that happened into an issue and let us debug it.

Unfortunately errors here mean we can't really rely on anything on their machine working, e.g. we can't assume they have enough disk space to store a temporary file.

Maybe, what the bash script should do when building flutter_tools is to run:

dart pub get --enforce-lockfile > /dev/null 2> /dev/null || (
  echo 'Unable to fetch dependencies!'
  echo 'Ensure "packages/flutter_tools/pubspec.lock" satisfies "packages/flutter_tools/pubspec.yaml".' &&
  echo 'Try to run "dart pub get" inside "$FLUTTER_ROOT/packages/flutter_tools/".' &&
  exit 1
)

The problem is that they can't run dart pub get because dart is what we're trying to build. They'd have to run bin/cache/dart-sdk/bin/dart pub get. That's already too complicated to be telling new users to deal with. We really just want to give them the message "Please file a bug and just dump everything in the console above into the bug".

For the purpose of getting dependencies required by flutter_tools this is probably reasonable. And might free you from having tools that pin dependencies/transitive-dependencies inside packages/flutter_tools/pubspec.yaml.

We have to pin the packages in packages/flutter/pubspec.yaml so that people depending on flutter (the SDK) get the same versions of the packages that we tested with, and once we've done that it's easier to use the same pinning mechanism for the entire repo than try to special-case specific pubspecs.

If fetching dependencies fail, then it should always be because:

  • (i) The Dart SDK version changed,
  • (ii) packages/flutter_tools/pubspec.yaml changed,
  • (iii) packages/<package>/pubspec.yaml changed (where <package> is a dependency of flutter_tools).

These are almost always going to be true when we're running this for the tool, FWIW.

  • A package version was deleted from pub.dev.
  • pub.dev is down.
  • User modified files or pubspec.yaml files inside of their PUB_CACHE (do this, all bets are off).
  • dart pub get has a significant bug.

Oh there are so many more reasons. :-) Network is compromised, network has a fault configuration, network has a faulty proxy, disk is damaged, disk is full, the system has a virus checker that is damaging the files downloaded, the machine itself has malware, the machine's memory is faulty, ionizing radiation flipped a bit in memory, the filesystem or a relevant part of the filesystem is read-only, etc etc etc.

so if a user gets a conflict, it's not really useful to look at the pub output (or the conflict). Instead they should do flutter update-packages --force-upgrade.

Users (as opposed to contributors) should absolutely never run flutter update-packages. We're working on making that something that can't happen (https://github.com/flutter/flutter/issues/132800).

I'm not particularly concerned about contributors for the purposes of this conversation (modulo the point near the end of this comment).

Use case (B): Fetching dependencies on-the-fly in flutter test

In an ideal world: flutter test should have access to ensurePubspecResolved and just call that. Maybe, one day we'll figure out how to embed pub in flutter_tools the way it is in dartdev.

Until then, flutter_tools probably has to call dart pub get as a subprocess. And here PUB_SUMMARY_ONLY=TRUE is probably the correct mode. That will print the summary of changes, if there are any.

Ideally we'd like to print nothing at all, though, unless there's an error. It's not clear to me that there's any benefit in printing any summary (I personally have literally never cared about the output of pub get when it's run automatically by another tool, for example; the only times I care about the output is when I'm debugging some problem).

For performance it might be reasonable to have flutter_tools do run a few checks on file modifications timestamps, such that if pubspec.yaml < pubspec.lock < .dart_tool/package_config.json, and then it skips the call to dart pub get. That would be close to what ensurePubspecResolved without the same performance, and with a loss of correctness/consistency when user has path-dependencies that have been modified. Or maybe this is too risky, it can definitely cause pain in some mono-repo use-cases.

I believe we've done that for a while: https://github.com/flutter/flutter/blob/4022864c659ff2f0691aff4efce7be43a3ce757c/packages/flutter_tools/lib/src/dart/pub.dart#L291-L305

Thinking about this more, what we really need is two "quiet" modes:

The second one here is something we'd like for CI, where currently we use pub in verbose mode and it generates phenomenal amounts of log data that gets in the way of debugging other problems; we've considered switching it out of verbose mode but then when things go wrong we don't have enough data. (I could live with this outputting a brief summary when there's no error, but if there's no error it should never be more than, say, 5 lines of output, IMHO. Certainly not one line per package or per updated package.)

FWIW, there is an additional mode you may wish to consider implementing, which is a "no animations" mode. I am told users interacting with the console using a screen reader do not like animations in tools, such as the fast changing timer. We're working on providing a "no CLI animations" mode in the "flutter" tool; once we have that, we'll probably want to pass that command line flag to "pub" too. (This would be moot for the "flutter" tool's purposes if this issue is addressed by having a flag that just silences output entirely, since then there's no animation to disable.)

jonasfj commented 1 year ago

Oh there are so many more reasons. :-)

hehe, yes, :rofl:

Thinking about this more, what we really need is two "quiet" modes:

I guess I'm trying to understand the use-cases.

I am told users interacting with the console using a screen reader do not like animations in tools, such as the fast changing timer. We're working on providing a "no CLI animations" mode in the "flutter" tool; once we have that, we'll probably want to pass that command line flag to "pub" too.

Interesting! Let's make this an environment variable. There is already a convention for NO_COLOR whether that should imply no-animation, or if we should suggest a NO_ANIMATION environment variable, I don't know.

But this kind of aspect is painful to pass around as flags. Users should just set env vars in their shell and be done with it, across all commands (ideally).

jonasfj commented 1 year ago

Use case (A): Fetching dependencies for flutter_tools

Okay, maybe this needs a quiet mode, that only prints stuff when there is an error.

Note. I'm a bit reluctant add output modes left and right, we're still cleaning up after the lengthy list of verbosity modes we used to have. And we'd like output modes to align across all Dart tooling.

But maybe we can do a PUB_FAILURES_ONLY=true kind of mode?

(I like env vars for communication of Flutter specific info to pub because: (i) it's less visible to others, (ii) if we drop support things won't start crashing because of an unrecognized flag -- but it's plausible that I'm just creating more confusion).

Use case (B): Fetching dependencies on-the-fly in flutter test

What about an output-mode PUB_SUMMARY_ONLY=true where we print:

For cases such as (B), where we want to ensure dependencies are resolved before running another command test/run/analyze/build/etc. then it seems reasonable that:

For use-cases such as (B), we are touching files/context that user owns. So I think this is reasonable.

Note. (B) is distinct from cases like flutter pub get where we want a full summary + hints, etc.


I agree that we should align progress animations across Flutter / Dart tooling, but that seems like an orthogonal issue.

Hixie commented 1 year ago

I am told users interacting with the console using a screen reader do not like animations in tools, such as the fast changing timer. We're working on providing a "no CLI animations" mode in the "flutter" tool; once we have that, we'll probably want to pass that command line flag to "pub" too.

Interesting! Let's make this an environment variable. There is already a convention for NO_COLOR whether that should imply no-animation, or if we should suggest a NO_ANIMATION environment variable, I don't know.

For flutter what we're doing is having persistent configuration state (flutter config --no-cli-animations), and we're also honoring the TERM variable which has historically had the unfortunately-named value dumb for terminals that don't support even the most basic of features, so if TERM=dumb we disable animations for that invocation.

I've filed an issue with the NO_COLOR folks to see if they know of a standard for disabling animations.

But maybe we can do a PUB_FAILURES_ONLY=true kind of mode?

Works for me.

(I like env vars for communication of Flutter specific info to pub because: (i) it's less visible to others, (ii) if we drop support things won't start crashing because of an unrecognized flag -- but it's plausible that I'm just creating more confusion).

I have a slight preference for arguments for the reasons described earlier but I'm much more interested in having the feature than about the specific solution they use.

What about an output-mode PUB_SUMMARY_ONLY=true where we print:

  • Resolving dependencies... (1.2s) while fetching dependencies, and delete it when done.
  • If pubspec.lock was changed, then we show the changes made here.

    • Lines like > vm_service 11.10.0 (was 11.9.0)
    • This usually only happens if you modified pubspec.yaml without running pub get.
  • We show a conclusion line, which is one of:

    • Changed 1 dependency! (serves to indicate that pubspec.lock was changed)
    • Got dependencies. (serves to indicate that .dart_tool/package_config.json was changed).

This is much more information than I think anyone using flutter test wants to see. When they are running tests they really care about only one thing: did my tests pass? Giving them information about packages is really not something they care about. I think for this use case I would actually prefer using the PUB_FAILURES_ONLY=true mode than the PUB_SUMMARY_ONLY=true you describe here if those was the choices.

  • Output should be short (disabled, if not connected to a console).

Progress output should be disabled if it's not a console but actually I think (as discussed above) having diagnostic output is more useful (in moderation) in logs than on the console.

Note. (B) is distinct from cases like flutter pub get where we want a full summary + hints, etc.

Agreed.

I agree that we should align progress animations across Flutter / Dart tooling, but that seems like an orthogonal issue.

I think in the case where one app runs another, it's just easier of the controlling app is the only one showing progress information. Trying to keep all our various styles consistent is rather difficult even within a single CLI app let alone across multiple teams. :-)

sigurdm commented 1 year ago

When they are running tests they really care about only one thing: did my tests pass?

I think there's a lot of truth here - the test results are the user's main focus - and the other lines of output are mainly distracting.

However - if they did an update to pubspec.yaml, and didn't run pub get (that is, if their resolution is outdated). They should (IMO) care about the resolution happening - things getting upgraded/downgraded, new dependencies added/removed should not happen without some kind of notification to the user.

When there are no changes we shouldn't show anything (do we do that currently?)

I think in the case where one app runs another, it's just easier of the controlling app is the only one showing progress information.

We have been discussing showing more detailed progress information eg. what packages are being downloaded, perhaps a progress bar. That would not be possible in that case.

We are not married to the specific style of the progress bar/spinner etc.

But to make this fully aligned, pub would probably have to be embedded deeper into the flutter_tools binary (like what we do for dart-dev today) Last we discussed this, we ended up concluding that there are non-trivial packaging issues - but perhaps we can look into that again.

jonasfj commented 1 year ago

When there are no changes we shouldn't show anything (do we do that currently?)

Yes, Resolving dependencies\nGot dependencies since this is a pub get invocation. It's only if they detect timestamp are aligned and therefore there is no pub get invocation that nothing gets printed.

Also currently, if there are changes, we print a summary, which not only contains changes, but also packages that have newer versions available. We could perhaps only show changes.

sigurdm commented 1 year ago

It's only if they detect timestamp are aligned and therefore there is no pub get invocation that nothing gets printed.

Yeah, that's what I meant. If flutter detects that the timestamps are in the right order, nothing gets printed.

That will be usual case.

However - if they did an update to pubspec.yaml, and didn't run pub get (that is, if their resolution is outdated). They should (IMO) care about the resolution happening - things getting upgraded/downgraded, new dependencies added/removed should not happen without some kind of notification to the user.

I'll backpedal on my own statement here. Perhaps this information is of little enough concern that we could leave it out. (They'll see the changes in git diff if the pubspec.lock is checked into version control). Not really sure anymore.

If that is indeed the case, flutter test could run pub get in non-interactive mode, and only if it fails output a line with a command to reproduce:

# when things go well nothing is output:
$ flutter test 
00:03 +65: All tests passed! 

# When there is an error during resolution
$ flutter test 
Failed resolving dependencies. Run `flutter pub get` for more information.

How does that sound?

Hixie commented 1 year ago

I could live with that. I worry that it'll be confusing for intermittent failures (they'll never get to see what the failure was), but it's a huge improvement over the current situation IMHO.