Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
5 changes: 5 additions & 0 deletions script/tool/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## NEXT

- Add `against-pub` flag for version-check.
- Add `log-status` flag for publish-check.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated.

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


## 0.1.0+1

- Re-add the bin/ directory.
Expand Down
92 changes: 92 additions & 0 deletions script/tool/lib/src/common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:args/command_runner.dart';
import 'package:colorize/colorize.dart';
import 'package:file/file.dart';
import 'package:git/git.dart';
import 'package:http/http.dart' as http;
import 'package:meta/meta.dart';
import 'package:path/path.dart' as p;
import 'package:pub_semver/pub_semver.dart';
Expand Down Expand Up @@ -549,6 +550,97 @@ class ProcessRunner {
}
}

/// Finding version of [package] that is published on pub.
///
/// Note: you should manually close the [httpClient] when done using the finder.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems worth noting on the constructor rather than the class.

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

class PubVersionFinder {

/// Constructor.
PubVersionFinder({@required this.package, this.pubHost = defaultPubHost, @required this.httpClient});

/// The default pub host to use.
static const String defaultPubHost = 'https://pub.dev';

/// The package name.
final String package;

/// The pub host url, defaults to `https://pub.dev`.
final String pubHost;

/// The http client, can override for testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's required, then it's not really an override for testing.

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

///
/// You should manually close this client when done using this finder.
final http.Client httpClient;

/// Get the package version on pub.
Future<PubVersionFinderResponse> getPackageVersion() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the class would make more sense as a class if package were an argument here rather than on the constructor. I.e., you configure it for a given server/network setup, then you can query packages.

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!

final Uri pubHostUri = Uri.parse(pubHost);
final Uri url = pubHostUri.replace(path: '/packages/$package.json');
final http.Response response = await httpClient.get(url);

if (response.statusCode == 404) {
return PubVersionFinderResponse(
versions: null,
result: PubVersionFinderResult.noPackageFound,
httpResponse: response);
} else if (response.statusCode != 200) {
return PubVersionFinderResponse(
versions: null,
result: PubVersionFinderResult.fail,
httpResponse: response);
}
final List<Version> versions =
(json.decode(response.body)['versions'] as List<dynamic>)
.map<Version>(
(final dynamic versionString) => Version.parse(versionString as String))
.toList();

versions.sort((Version a, Version b) {
/// TODO(cyanglaz): Think about how to handle pre-release version with [Version.prioritize].
return b.compareTo(a);
});

return PubVersionFinderResponse(
versions: versions,
result: PubVersionFinderResult.success,
httpResponse: response);
}
}

/// Represents a response for [PubVersionFinder].
class PubVersionFinderResponse {
/// Constructor.
const PubVersionFinderResponse({this.versions, this.result, this.httpResponse});

/// The versions found in [PubVersionFinder].
///
/// This is sorted by largest to smallest, so the first element in the list is the largest version.
/// Might be `null` if the [result] is not [PubVersionFinderResult.success].
final List<Version> versions;

/// The result of the version finder.
final PubVersionFinderResult result;

/// The response object of the http request.
final http.Response httpResponse;
}

/// An enum represents the result of [PubVersionFinder].
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: representing

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

enum PubVersionFinderResult {
/// The version finder successfully found a version.
success,

/// The version finder failed to find a valid version.
///
/// This might due to http connection errors or user errors.
fail,

/// The version finder failed to locate the package.
///
/// This is usually OK when the package is new.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "usually"? When would it not be okay for a new package not to be found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworded, this was not what I meant.

noPackageFound,
}

/// Finding diffs based on `baseGitDir` and `baseSha`.
class GitVersionFinder {
/// Constructor
Expand Down
138 changes: 123 additions & 15 deletions script/tool/lib/src/publish_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:convert';
import 'dart:io' as io;

import 'package:colorize/colorize.dart';
import 'package:file/file.dart';
import 'package:http/http.dart' as http;
import 'package:pub_semver/pub_semver.dart';
import 'package:pubspec_parse/pubspec_parse.dart';

import 'common.dart';
Expand All @@ -18,6 +21,7 @@ class PublishCheckCommand extends PluginCommand {
Directory packagesDir,
FileSystem fileSystem, {
ProcessRunner processRunner = const ProcessRunner(),
this.httpClient,
}) : super(packagesDir, fileSystem, processRunner: processRunner) {
argParser.addFlag(
_allowPrereleaseFlag,
Expand All @@ -26,9 +30,27 @@ class PublishCheckCommand extends PluginCommand {
'the SDK constraint is a pre-release version, is ignored.',
defaultsTo: false,
);
argParser.addFlag(_machineFlag,
help: 'Only prints a final status as a defined string.\n'
'The possible values are:\n'
' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks.\n'
' $_resultNoPublish: There are no packages needs to be published. Either no pubspec change detected or all versions have already been published.\n'
' $_resultError: Some error has occurred.',
defaultsTo: false,
negatable: true);
}

static const String _allowPrereleaseFlag = 'allow-pre-release';
static const String _machineFlag = 'machine';
static const String _resultNeedsPublish = 'needs-publish';
static const String _resultNoPublish = 'no-publish';
static const String _resultError = 'error';

final List<String> _validStatus = <String>[
_resultNeedsPublish,
_resultNoPublish,
_resultError
];

@override
final String name = 'publish-check';
Expand All @@ -37,13 +59,41 @@ class PublishCheckCommand extends PluginCommand {
final String description =
'Checks to make sure that a plugin *could* be published.';

/// The custom http client used to query versions on pub.
final http.Client httpClient;

@override
Future<void> run() async {
final ZoneSpecification logSwitchSpecification = ZoneSpecification(
print: (Zone self, ZoneDelegate parent, Zone zone, String message) {
final bool logMachineMessage = argResults[_machineFlag] as bool;
final bool isMachineMessage = _validStatus.contains(message);
if (logMachineMessage == isMachineMessage) {
parent.print(zone, message);
}
});

await runZoned(_runCommand, zoneSpecification: logSwitchSpecification);
}

Future<void> _runCommand() async {
final List<Directory> failedPackages = <Directory>[];

String resultToLog = _resultNoPublish;
await for (final Directory plugin in getPlugins()) {
if (!(await _passesPublishCheck(plugin))) {
failedPackages.add(plugin);
final _PublishCheckResult result = await _passesPublishCheck(plugin);
switch (result) {
case _PublishCheckResult._needsPublish:
if (failedPackages.isEmpty) {
resultToLog = _resultNeedsPublish;
}
break;
case _PublishCheckResult._noPublish:
break;
case _PublishCheckResult.error:
failedPackages.add(plugin);
resultToLog = _resultError;
break;
}
}

Expand All @@ -56,12 +106,19 @@ class PublishCheckCommand extends PluginCommand {
final Colorize colorizedError = Colorize('$error\n$joinedFailedPackages')
..red();
print(colorizedError);
throw ToolExit(1);
} else {
final Colorize passedMessage =
Colorize('All packages passed publish check!')..green();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the new tests, it's very strange to have terminal color commands in the --machine output. I think we should replace all the Colorize calls with a utility method like:

_printImportantStatusMessage(String message, {@required bool isError}) {
  if (argResults[_machineFlag] as bool) {
    print('${isError ? 'ERROR' : 'SUCCESS'}: $message');
  } else {
    final Colorize colorizedMessage = Colorize(message);
    if (isError) {
      colorizedMessage.red();
    } else {
      colorizedMessage.green();
    }
    print(colorizedMessage);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated so that both machine mesasge and human message include the error and success prefix.

print(passedMessage);
}

final Colorize passedMessage =
Colorize('All packages passed publish check!')..green();
print(passedMessage);
// This has to be last output of this command because it is promised in the help section.
Copy link
Contributor

Choose a reason for hiding this comment

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

Obsolete.

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

if (argResults[_machineFlag] as bool) {
print(resultToLog);
}
if (failedPackages.isNotEmpty) {
throw ToolExit(1);
}
}

Pubspec _tryParsePubspec(Directory package) {
Expand Down Expand Up @@ -89,17 +146,23 @@ class PublishCheckCommand extends PluginCommand {
final Completer<void> stdOutCompleter = Completer<void>();
process.stdout.listen(
(List<int> event) {
io.stdout.add(event);
outputBuffer.write(String.fromCharCodes(event));
final String output = String.fromCharCodes(event);
if (output.isNotEmpty) {
print(output);
outputBuffer.write(output);
}
},
onDone: () => stdOutCompleter.complete(),
);

final Completer<void> stdInCompleter = Completer<void>();
process.stderr.listen(
(List<int> event) {
io.stderr.add(event);
outputBuffer.write(String.fromCharCodes(event));
final String output = String.fromCharCodes(event);
if (output.isNotEmpty) {
print(Colorize(output)..red());
outputBuffer.write(output);
}
},
onDone: () => stdInCompleter.complete(),
);
Expand All @@ -121,24 +184,69 @@ class PublishCheckCommand extends PluginCommand {
'Packages with an SDK constraint on a pre-release of the Dart SDK should themselves be published as a pre-release version.');
}

Future<bool> _passesPublishCheck(Directory package) async {
Future<_PublishCheckResult> _passesPublishCheck(Directory package) async {
final String packageName = package.basename;
print('Checking that $packageName can be published.');

final Pubspec pubspec = _tryParsePubspec(package);
if (pubspec == null) {
return false;
print('no pubspec');
return _PublishCheckResult.error;
} else if (pubspec.publishTo == 'none') {
print('Package $packageName is marked as unpublishable. Skipping.');
return true;
return _PublishCheckResult._noPublish;
}

final Version version = pubspec.version;
final bool alreadyPublished = await _checkIfAlreadyPublished(
packageName: packageName, version: version);
if (alreadyPublished) {
print(
'Package $packageName version: $version has already be published on pub.');
return _PublishCheckResult._noPublish;
}

if (await _hasValidPublishCheckRun(package)) {
print('Package $packageName is able to be published.');
return true;
return _PublishCheckResult._needsPublish;
} else {
print('Unable to publish $packageName');
return false;
return _PublishCheckResult.error;
}
}

// Check if `packageName` already has `version` published on pub.
Future<bool> _checkIfAlreadyPublished(
{String packageName, Version version}) async {
final PubVersionFinder pubVersionFinder = PubVersionFinder(
package: packageName, httpClient: httpClient ?? http.Client());
final PubVersionFinderResponse pubVersionFinderResponse =
await pubVersionFinder.getPackageVersion();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want a try/catch here to deal with issues reaching pub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need try-catch, but we should check the status of pubVersionFinderResponse. Done.

bool published;
switch (pubVersionFinderResponse.result) {
case PubVersionFinderResult.success:
published = pubVersionFinderResponse.versions.contains(version);
break;
case PubVersionFinderResult.fail:
printErrorAndExit(errorMessage: '''
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this break the --machine case, because the final message will never print?

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking this should return something, rather than bailing completely. Is the reason for this that we assume that all the other checks will fail the same way because it's likely a network issue?

If we do want to keep this construction, this line (and the line above) need comments explaining why they are doing what they are doing, since it's non-obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok, I updated to print the message and handled the error later

Error fetching version on pub for $packageName.
HTTP Status ${pubVersionFinderResponse.httpResponse.statusCode}
HTTP response: ${pubVersionFinderResponse.httpResponse.body}
''');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm realizing that failures here are going to be almost impossible to debug, because we won't have any information in the log.

Perhaps we should consider doing something like a JSON dictionary of output, with status and detailed log messages as separate keys? That would mean we'd need to consume the result with something like a Dart script that can parse the JSON, rather than just a one-liner in the GitHub Action. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good Idea, updated the code to do exactly that

break;
case PubVersionFinderResult.noPackageFound:
published = false;
break;
}
pubVersionFinder.httpClient.close();
return published;
}
}

enum _PublishCheckResult {
_needsPublish,

_noPublish,

error,
}
Loading