Skip to content
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

Give channel descriptions in flutter channel, use branch instead of upstream for channel name #126936

Merged
merged 1 commit into from May 23, 2023

Conversation

Hixie
Copy link
Contributor

@Hixie Hixie commented May 16, 2023

How we determine the channel name

Historically, we used the current branch's upstream to figure out the current channel name. I have no idea why. I traced it back to https://github.com/flutter/flutter/pull/446/files where @abarth implement this and I reviewed that PR and left no comment on it at the time.

I think this is confusing. You can be on a branch and it tells you that your channel is different. That seems weird.

This PR changes the logic to uses the current branch as the channel name.

How we display channels

The main reason this PR exists is to add channel descriptions to the flutter channel list:

ianh@burmese:~/dev/flutter/packages/flutter_tools$ flutter channel
Flutter channels:
  master (tip of tree, for contributors)
  main (tip of tree, follows master channel)
  beta (updated monthly, recommended for experienced users)
  stable (updated quarterly, for new users and for production app releases)
* foo_bar

Currently not on an official channel.
ianh@burmese:~/dev/flutter/packages/flutter_tools$

Other changes

I made a few other changes while I was at it:

  • If you're not on an official channel, we used to imply --show-all, but now we don't, we just show the official channels plus yours. This avoids flooding the screen in the case the user is on a weird channel and just wants to know what channel they're on.
  • I made the tool more consistent about how it handles unofficial branches. Now it's always [user branch].
  • I slightly adjusted how unknown versions are rendered so it's clearer the version is unknown rather than just having the word "Unknown" floating in the output without context.
  • Simplified some of the code.
  • Made some of the tests more strict (checking all output rather than just some aspects of it).
  • Changed the MockFlutterVersion to implement the FlutterVersion API more strictly.
  • I made sure we escape the output to .metadata to avoid potential injection bugs (previously we just inlined the version and channel name verbatim with no escaping, which is super sketchy).
  • Tweaked the help text for the downgrade command to be clearer.
  • Removed some misleading text in some error messages.
  • Made the .metadata generator consistent with the template file.
  • Removed some obsolete code to do with the dev branch.

Reviewer notes

I'm worried that there are implications to some of these changes that I am not aware of, so please don't assume I know what I'm doing when reviewing this code. :-)

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 16, 2023
@Hixie Hixie force-pushed the channel branch 3 times, most recently from 9259260 to 38550ca Compare May 17, 2023 01:08
@Hixie Hixie force-pushed the channel branch 3 times, most recently from d781ec9 to 8d01a6a Compare May 18, 2023 00:11
@Hixie Hixie force-pushed the channel branch 2 times, most recently from f6186f4 to a95a836 Compare May 18, 2023 22:50
);
}
try {
await processUtils.run(
// The `--` bit (because it's followed by nothing) means that we don't actually change
// anything in the working tree, which avoids the need to first go into detached HEAD mode.
Copy link
Member

Choose a reason for hiding this comment

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

ahh, thanks for the comment

'main',
'beta',
'stable',
};

const Map<String, String> kChannelDescriptions = <String, String>{
'master': 'tip of tree, for contributors',
Copy link
Member

Choose a reason for hiding this comment

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

I've found several contributors confused by the term "tip of tree", or "tree" in general with regards to a repository. What about: "latest development branch, for contributors"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -709,8 +717,8 @@ class GitTagVersion {
String gitRef = 'HEAD'
}) {
if (fetchTags) {
final String channel = _runGit('git rev-parse --abbrev-ref HEAD', processUtils, workingDirectory);
if (channel == 'dev' || channel == 'beta' || channel == 'stable') {
final String channel = _runGit('git symbolic-ref --short HEAD', processUtils, workingDirectory);
Copy link
Member

Choose a reason for hiding this comment

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

for posterity, was git rev-parse --abbrev-ref HEAD giving us the wrong string?

Copy link
Member

Choose a reason for hiding this comment

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

Also, looks like this is a new git sub-command after an internal refactor. I wonder if all of our users will have it in their git? https://git-scm.com/docs/git-symbolic-ref#_notes

Copy link
Member

Choose a reason for hiding this comment

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

Well, it was at least around in 2011, so I'm guessing it should be fine https://stackoverflow.com/questions/4986000/whats-the-recommended-usage-of-a-git-symbolic-reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell what we did before was just wrong and this command has existed for ages.
But it's git.
So who knows.
I certainly cannot say with certainty that this is correct.

Copy link
Member

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 22, 2023

auto label is removed for flutter/flutter, pr: 126936, due to - This commit is not mergeable and has conflicts. Please rebase your PR and fix all the conflicts.

@Hixie Hixie added the autosubmit Merge PR when tree becomes green via auto submit App label May 23, 2023
@auto-submit auto-submit bot merged commit 9c7a9e7 into flutter:master May 23, 2023
124 checks passed
@Hixie Hixie deleted the channel branch May 23, 2023 19:59
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 24, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 24, 2023
flutter/flutter@f86c529...216605b

2023-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from aba9dc4640c2 to eebcf36cd38f (3 revisions) (flutter/flutter#127458)
2023-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 76afb431a740 to aba9dc4640c2 (1 revision) (flutter/flutter#127453)
2023-05-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc6df9512aa2 to 76afb431a740 (3 revisions) (flutter/flutter#127445)
2023-05-23 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 6b107e7a7ee5e054e0bcce60de5181d21e2f00fb to 2713f7303c96cb1e69627957ec16eea0fd7f94a4 (flutter/flutter#127438)
2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3535e0d301dd to fc6df9512aa2 (1 revision) (flutter/flutter#127440)
2023-05-23 47866232+chunhtai@users.noreply.github.com Migrates android semanitcs integration to integration test (flutter/flutter#127128)
2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 23b04314d5d2 to 3535e0d301dd (3 revisions) (flutter/flutter#127428)
2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ccf50f344d5d to 23b04314d5d2 (2 revisions) (flutter/flutter#127421)
2023-05-23 ian@hixie.ch Give channel descriptions in `flutter channel`, use branch instead of upstream for channel name (flutter/flutter#126936)
2023-05-23 72562119+tgucio@users.noreply.github.com Avoid catching errors in TextPainter tests (flutter/flutter#127307)
2023-05-23 christopherfujino@gmail.com [flutter_tools] delete entitlements files after copying to macos build dir (flutter/flutter#127417)
2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from a6e37bde65fd to ccf50f344d5d (7 revisions) (flutter/flutter#127416)
2023-05-23 113669907+NikolajHarderNota@users.noreply.github.com modal bottom sheet barrier label (flutter/flutter#126431)
2023-05-23 jmccandless@google.com Fix the breaking of multi-code-unit characters in obscure mode (flutter/flutter#123366)
2023-05-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 168b0bf3f70d to a6e37bde65fd (1 revision) (flutter/flutter#127397)
2023-05-23 vashworth@google.com Revert "Log all output of ios-deploy" (flutter/flutter#127405)
2023-05-23 engine-flutter-autoroll@skia.org Roll Packages from 83959fb to d449a17 (6 revisions) (flutter/flutter#127394)
2023-05-23 vashworth@google.com Log all output of ios-deploy (flutter/flutter#127222)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
… upstream for channel name (flutter#126936)

## How we determine the channel name

Historically, we used the current branch's upstream to figure out the current channel name. I have no idea why. I traced it back to https://github.com/flutter/flutter/pull/446/files where @abarth implement this and I reviewed that PR and left no comment on it at the time.

I think this is confusing. You can be on a branch and it tells you that your channel is different. That seems weird.

This PR changes the logic to uses the current branch as the channel name.

## How we display channels

The main reason this PR exists is to add channel descriptions to the `flutter channel` list:

```
ianh@burmese:~/dev/flutter/packages/flutter_tools$ flutter channel
Flutter channels:
  master (tip of tree, for contributors)
  main (tip of tree, follows master channel)
  beta (updated monthly, recommended for experienced users)
  stable (updated quarterly, for new users and for production app releases)
* foo_bar

Currently not on an official channel.
ianh@burmese:~/dev/flutter/packages/flutter_tools$
```

## Other changes

I made a few other changes while I was at it:

* If you're not on an official channel, we used to imply `--show-all`, but now we don't, we just show the official channels plus yours. This avoids flooding the screen in the case the user is on a weird channel and just wants to know what channel they're on.
* I made the tool more consistent about how it handles unofficial branches. Now it's always `[user branch]`.
* I slightly adjusted how unknown versions are rendered so it's clearer the version is unknown rather than just having the word "Unknown" floating in the output without context.
* Simplified some of the code.
* Made some of the tests more strict (checking all output rather than just some aspects of it).
* Changed the MockFlutterVersion to implement the FlutterVersion API more strictly.
* I made sure we escape the output to `.metadata` to avoid potential injection bugs (previously we just inlined the version and channel name verbatim with no escaping, which is super sketchy).
* Tweaked the help text for the `downgrade` command to be clearer.
* Removed some misleading text in some error messages.
* Made the `.metadata` generator consistent with the template file.
* Removed some obsolete code to do with the `dev` branch.

## Reviewer notes

I'm worried that there are implications to some of these changes that I am not aware of, so please don't assume I know what I'm doing when reviewing this code. :-)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants