-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
New release process #14061
New release process #14061
Conversation
cc @tvolkert for review I've already landed a separate change that renames all the tags from the old format ("0.0.20") to the new one ("v0.0.20"). Having the "v" will make it easier to add other kinds of tags later. Also I removed an extremely old and confusing "v0.0.20-alpha" tag. |
I've also updated https://github.com/flutter/flutter/wiki/Release-process accordingly already. |
I've also created the "beta" and "dev" branches. Right now they're at the old v0.0.20 build. |
final StringBuffer buf = new StringBuffer(); | ||
buf.writeln('name: Flutter'); | ||
buf.writeln('homepage: https://flutter.io'); | ||
buf.writeln('version: $version'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Travis doc script is complaining about "version: unknown"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
class ChannelCommand extends FlutterCommand { | ||
ChannelCommand({ bool verboseHelp: false }) { | ||
argParser.addFlag( | ||
'all', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abbreviation of a
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
final String currentBranch = runSync( | ||
<String>['git', 'rev-parse', '--abbrev-ref', 'HEAD'], | ||
workingDirectory: Cache.flutterRoot); | ||
static Future<Null> listChannels({ bool showAll }) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why public? And if it were still private, it could be an instance method that just refers to argResults
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it private again. I had thought I was going to use this from other files, but only upgradeChannel ended up being used.
I left it using an argument rather than argResults.
}, | ||
); | ||
if (result != 0) | ||
throwToolExit('List channels failed: $result', exitCode: result); | ||
} | ||
|
||
Future<Null> _switchChannel(String branchName) async { | ||
printStatus('Switching to flutter channel named $branchName'); | ||
static Future<Null> switchChannel(String branchName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
'release', | ||
]); | ||
|
||
static Map<String, String> obsoleteBranches = <String, String>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docs explaining what this maps to and from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -42,18 +35,47 @@ class FlutterVersion { | |||
|
|||
_frameworkRevision = _runGit('git log -n 1 --pretty=format:%H'); | |||
_frameworkAge = _runGit('git log -n 1 --pretty=format:%ar'); | |||
|
|||
final String version = _runGit('git describe --match v*.*.* --exclude *-* --first-parent --long --tags'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's some serious voodoo right there
@@ -42,18 +35,47 @@ class FlutterVersion { | |||
|
|||
_frameworkRevision = _runGit('git log -n 1 --pretty=format:%H'); | |||
_frameworkAge = _runGit('git log -n 1 --pretty=format:%ar'); | |||
|
|||
final String version = _runGit('git describe --match v*.*.* --exclude *-* --first-parent --long --tags'); | |||
final RegExp versionPattern = new RegExp('^v([0-9]+)\.([0-9]+)\.([0-9]+\)-([0-9]+)-g([a-f0-9]+)\$'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to read this and know which fields represent what pieces. Can you encapsulate this in a class that takes the output of the git command in its constructor and exposes meaningfully-named fields for each of the pieces it extracts from the regexp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using https://pub.dartlang.org/packages/pub_semver instead? (used in pub
AFAIK)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zoechi Instead of what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing the version, pumping major/minor/patch/dev, comparing versions
Just wanted to bring it to your attention in case you were not aware of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version that is being parsed here isn't a semver version, it's a git tag description long format.
As far as adding 1 is concerned, I don't think we need to use a library for that. :-) We're trying to minimize our dependencies, so a library would have to do something a lot more complicated than that to justify using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tvolkert done
@@ -73,14 +98,18 @@ class FlutterVersion { | |||
|
|||
String _runGit(String command) => runSync(command.split(' '), workingDirectory: Cache.flutterRoot); | |||
|
|||
void ensureVersionFile() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be written out during construction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it does make sense to keep this as a separate method called by the command runner, then lat's make it async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather not have a constructor that mutates the world (notwithstanding that it already runs subprocesses and stuff, i just don't want to make it worse...). But I've made it async.
@@ -158,12 +187,12 @@ class FlutterVersion { | |||
/// Return the branch name. | |||
/// | |||
/// If [whitelistBranchName] is true and the branch is unknown, | |||
/// the branch name will be returned as 'dev'. | |||
/// the branch name will be returned as '[user-branch]'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: surround '[user-branch]' in backticks so Dartdoc knows not to try to resolve it as a reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@override | ||
String toString() { | ||
final String flutterText = 'Flutter • channel $channel • ${repositoryUrl == null ? 'unknown source' : repositoryUrl}'; | ||
final String flutterText = 'Flutter $_frameworkVersion • channel $channel • ${repositoryUrl == null ? 'unknown source' : repositoryUrl}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a separate • version $_frameworkVersion
bullet. Otherwise they might see "Flutter unknown", which would be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've special-cased the unknown version case to not display the string, so it just says "Flutter" in that case.
final String frameworkText = 'Framework • revision $frameworkRevisionShort ($frameworkAge) • $frameworkCommitDate'; | ||
final String engineText = 'Engine • revision $engineRevisionShort'; | ||
final String toolsText = 'Tools • Dart $dartSdkVersion'; | ||
|
||
// Flutter • channel master • https://github.com/flutter/flutter.git | ||
// Flutter v1.3.922-pre.2 • channel master • https://github.com/flutter/flutter.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then update this to match
<String>['git', 'rev-parse', '--abbrev-ref', 'HEAD'], | ||
workingDirectory: Cache.flutterRoot); | ||
static Future<Null> listChannels({ bool showAll }) async { | ||
// Beware: currentBranch could contain PII. See getBranchName(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But getBranchName()
redacts it if it's not a known branch name, so we should be safe here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, scratch that - we don't set whitelistBranchName
to true here
@@ -158,12 +187,12 @@ class FlutterVersion { | |||
/// Return the branch name. | |||
/// | |||
/// If [whitelistBranchName] is true and the branch is unknown, | |||
/// the branch name will be returned as 'dev'. | |||
/// the branch name will be returned as '[user-branch]'. | |||
String getBranchName({ bool whitelistBranchName: false }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitelistBranchName
seems like a weird name for this argument... it's more like redactUnknownBranches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Fixed.
dev/tools/roll_dev.dart
Outdated
@@ -0,0 +1,150 @@ | |||
// Copyright 2017 The Chromium Authors. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about creating dev/tools/lib
and putting this there? It'd allow things like Google's roll script to import this and run it inline rather than having to spawn a new Dart process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dev/tools/roll_dev.dart
Outdated
print('stdout from git:\n${result.stdout}\n'); | ||
if (result.stderr.isNotEmpty) | ||
print('stderr from git:\n${result.stderr}\n'); | ||
exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result.exitCode instead of 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I'm not a fan of propagating exit codes that way, because it's essentially merging two namespaces (theirs and ours and any other processes we might do this to). In practice what matters is 0 vs 1.
dev/tools/roll_dev.dart
Outdated
print('stderr from git:\n${result.stderr}\n'); | ||
exit(1); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline at end of file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
dev/tools/roll_dev.dart
Outdated
return null; // for the analyzer's sake | ||
} | ||
|
||
void runGit(String command, String explanation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this could be unified with getGitOutput()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
factored out common stuff
dev/tools/roll_dev.dart
Outdated
case kZ: | ||
parts[2] += 1; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default case exits with a failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -73,6 +73,7 @@ function upgrade_flutter () { | |||
|
|||
local revision=`(cd "$FLUTTER_ROOT"; git rev-parse HEAD)` | |||
if [ ! -f "$SNAPSHOT_PATH" ] || [ ! -s "$STAMP_PATH" ] || [ `cat "$STAMP_PATH"` != "$revision" ] || [ "$FLUTTER_TOOLS_DIR/pubspec.yaml" -nt "$FLUTTER_TOOLS_DIR/pubspec.lock" ]; then | |||
rm -f "$FLUTTER_ROOT/version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be mirrored in the Windows version of the script (flutter.bat
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added:
IF EXIST "%FLUTTER_ROOT%\version" DEL %FLUTTER_ROOT%\version
...just after :do_snapshot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want the argument to DEL in quotes as well, just in case there are spaces in FLUTTER_ROOT.
bin/flutter.bat
Outdated
@@ -93,6 +93,7 @@ GOTO :after_subroutine | |||
) | |||
|
|||
:do_snapshot | |||
IF EXIST "%FLUTTER_ROOT%\version" DEL %FLUTTER_ROOT%\version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quotes around the argument to DEL, just in case there a spaces in the FLUTTER_ROOT path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
edb0c6e
to
24bc52b
Compare
@tvolkert PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo one comment
|
||
switch (level) { | ||
case kX: | ||
parts[0] += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also set parts[1] and parts[2] to 0? You want to go from 1.3.6 to 2.0.0 and not 2.3.6, right?
dev/tools/lib/roll_dev.dart
Outdated
parts[0] += 1; | ||
break; | ||
case kY: | ||
parts[1] += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, but only for parts[2].
Generate the "version" file from git tags. Remove the old VERSION file and mentions of versions in pubspec.yaml files. Replace the old update_versions.dart script with a new roll_dev.dart script. Update "flutter channel". Update "flutter upgrade", including making it transition from alpha to dev. Update "flutter --version" and "flutter doctor".
Updated as suggested. Good catch! Thanks. Will land on green and roll dev shortly thereafter. |
|
||
print('Your tree is ready to publish Flutter $version to the "dev" channel.'); | ||
stdout.write('Are you? [yes/no] '); | ||
if (stdin.readLineSync() != 'yes') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to respect 'y' as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two extra characters per person every few months is probably a cost worth paying for the extra level of confidence in the confirmation. :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Generate the "version" file from git tags. Remove the old VERSION file and mentions of versions in pubspec.yaml files. Replace the old update_versions.dart script with a new roll_dev.dart script. Update "flutter channel". Update "flutter upgrade", including making it transition from alpha to dev. Update "flutter --version" and "flutter doctor".
git@github.com:flutter/engine.git/compare/6c605f8a9624...8672e79 git log 6c605f8..8672e79 --first-parent --oneline 2019-11-28 skia-flutter-autoroll@skia.org Roll src/third_party/skia c96f5108df28..73beaaa48fcc (2 commits) (#14065) 2019-11-28 hterkelsen@users.noreply.github.com Fallback to Roboto if no suitable font is found (#14061) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC cbracken@google.com on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md
Generate the "version" file from git tags.
Remove the old VERSION file and mentions of versions in pubspec.yaml files.
Replace the old update_versions.dart script with a new roll_dev.dart script.
Update "flutter channel".
Update "flutter upgrade", including making it transition from alpha to dev.
Update "flutter --version" and "flutter doctor".