Skip to content

Webdev: Look for pubspec.yaml next to .dart_tool/package_config.json instead of only current dir #2498

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 26, 2024
Merged

Webdev: Look for pubspec.yaml next to .dart_tool/package_config.json instead of only current dir #2498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b360e1
Look for pubspec.yaml next to .dart_tool/package_config.json instead …
sigurdm Sep 25, 2024
67334e2
Don't require build-runner to be a direct dep
sigurdm Sep 25, 2024
bc4bcdd
CHANGELOG.md
sigurdm Sep 25, 2024
5ae9b51
Oops - wrong changelog
sigurdm Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions webdev/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## 3.6.0-wip

- Support running from inside a pub workspace [#2498](https://github.com/dart-lang/webdev/pull/2498).

## 3.5.0

- Update `dwds` constraint to `24.0.0`.
Expand Down
5 changes: 2 additions & 3 deletions webdev/lib/src/command/build_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,8 @@ class BuildCommand extends Command<int> {

List<String> arguments;
try {
var pubspecLock = await readPubspecLock(configuration);
arguments = buildRunnerArgs(pubspecLock, configuration)
..addAll(validExtraArgs);
await validatePubspecLock(configuration);
arguments = buildRunnerArgs(configuration)..addAll(validExtraArgs);
} on PackageException catch (e) {
logWriter(logging.Level.SEVERE, 'Pubspec errors: ',
error: '${e.details}');
Expand Down
5 changes: 2 additions & 3 deletions webdev/lib/src/command/daemon_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,8 @@ class DaemonCommand extends Command<int> {
launchInChrome: true, debug: true, autoRun: false, release: false));
configureLogWriter(configuration.verbose);
// Validate the pubspec first to ensure we are in a Dart project.
PubspecLock? pubspecLock;
try {
pubspecLock = await readPubspecLock(configuration);
await validatePubspecLock(configuration);
} on PackageException catch (e) {
logWriter(Level.SEVERE, 'Pubspec errors: ', error: '${e.details}');
rethrow;
Expand Down Expand Up @@ -104,7 +103,7 @@ class DaemonCommand extends Command<int> {
}
});
daemon.registerDomain(daemonDomain);
var buildOptions = buildRunnerArgs(pubspecLock, configuration);
var buildOptions = buildRunnerArgs(configuration);
var extraArgs = argResults?.rest ?? [];
var directoryArgs =
extraArgs.where((arg) => !arg.startsWith('-')).toList();
Expand Down
5 changes: 2 additions & 3 deletions webdev/lib/src/command/serve_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,16 +95,15 @@ refresh: Performs a full page refresh.
Configuration configuration;
configuration = Configuration.fromArgs(argResults);
configureLogWriter(configuration.verbose);
PubspecLock? pubspecLock;
try {
pubspecLock = await readPubspecLock(configuration);
await validatePubspecLock(configuration);
} on PackageException catch (e) {
logWriter(Level.SEVERE, 'Pubspec errors: ', error: '${e.details}');
rethrow;
}
// Forward remaining arguments as Build Options to the Daemon.
// This isn't documented. Should it be advertised?
var buildOptions = buildRunnerArgs(pubspecLock, configuration)
var buildOptions = buildRunnerArgs(configuration)
..addAll(argResults!.rest.where((arg) => arg.startsWith('-')).toList());
var extraArgs = argResults?.rest ?? [];
var directoryArgs = extraArgs.where((arg) => !arg.startsWith('-')).toList();
Expand Down
6 changes: 2 additions & 4 deletions webdev/lib/src/command/shared.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ void addSharedArgs(ArgParser argParser,

/// Parses the provided [Configuration] to return a list of
/// `package:build_runner` appropriate arguments.
List<String> buildRunnerArgs(
PubspecLock pubspecLock, Configuration configuration) {
List<String> buildRunnerArgs(Configuration configuration) {
var arguments = <String>[];
if (configuration.release) {
arguments.add('--$releaseFlag');
Expand All @@ -103,11 +102,10 @@ List<String> buildRunnerArgs(
return arguments;
}

Future<PubspecLock> readPubspecLock(Configuration configuration) async {
Future<void> validatePubspecLock(Configuration configuration) async {
var pubspecLock = await PubspecLock.read();
await checkPubspecLock(pubspecLock,
requireBuildWebCompilers: configuration.requireBuildWebCompilers);
return pubspecLock;
}

/// Checks that the normalized form of [path] is a top level directory under
Expand Down
42 changes: 22 additions & 20 deletions webdev/lib/src/pubspec.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'dart:convert';
import 'dart:io';

import 'package:http/http.dart';
import 'package:path/path.dart' as p;
import 'package:pub_semver/pub_semver.dart';
import 'package:pubspec_parse/pubspec_parse.dart';
import 'package:yaml/yaml.dart';
Expand All @@ -31,12 +32,6 @@ class PackageExceptionDetails {
{this.description, bool missingDependency = false})
: _missingDependency = missingDependency;

static const noPubspecLock =
PackageExceptionDetails._('`pubspec.lock` does not exist.',
description: 'Run `$appName` in a Dart package directory. '
'Run `dart pub get` first.',
missingDependency: true);

static PackageExceptionDetails missingDep(
String pkgName, VersionConstraint constraint) =>
PackageExceptionDetails._(
Expand Down Expand Up @@ -75,17 +70,34 @@ class PubspecLock {

static Future<PubspecLock> read() async {
await _runPubDeps();
var dir = p.absolute(p.current);
while (true) {
final candidate = p.join(
dir,
'.dart_tool',
'package_config.json',
);
if (File(candidate).existsSync()) break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason not to use exists() here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really - I believe I once learnt that the async overhead is a lot larger than doing the stat itself - but I doubt it makes any difference.

Do you prefer the async method?

Copy link
Contributor Author

@sigurdm sigurdm Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This little program seems to confirm it:

import 'dart:io';

main() async {
  final s = Stopwatch()..start();
  for (var i = 0; i < 10000; i++) {
    await File('abc').exists();
  }
  print('async ${s.elapsed}');
  s.reset();
  for (var i = 0; i < 10000; i++) {
    File('abc').existsSync();
  }
  print(' sync ${s.elapsed}');
}
> dart --version
Dart SDK version: 3.6.0-271.0.dev (dev) (Mon Sep 23 21:07:16 2024 -0700) on "linux_x64"
> dart bin/exists.dart
async 0:00:00.233795
 sync 0:00:00.019898

So it seems the sync methods are ~10 times faster!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right, sync operations are definitely faster at the cost of blocking the thread from handling other asynchronous work. This is probably fine in this situation since I doubt we're doing any interesting asynchronous work while we're also looking for the package_config. Thanks for checking!

final next = p.dirname(dir);
if (next == dir) {
// Give up.
dir = p.current;
break;
}
dir = next;
}

var pubspecLock =
loadYaml(await File('pubspec.lock').readAsString()) as YamlMap;
var pubspecLock = loadYaml(
await File(p.relative(p.join(dir, 'pubspec.lock'))).readAsString())
as YamlMap;

var packages = pubspecLock['packages'] as YamlMap?;
return PubspecLock(packages);
}

List<PackageExceptionDetails> checkPackage(
String pkgName, VersionConstraint constraint,
{String? forArgument, bool requireDirect = true}) {
{String? forArgument}) {
var issues = <PackageExceptionDetails>[];
var missingDetails =
PackageExceptionDetails.missingDep(pkgName, constraint);
Expand All @@ -95,13 +107,6 @@ class PubspecLock {
if (pkgDataMap == null) {
issues.add(missingDetails);
} else {
var dependency = pkgDataMap['dependency'] as String?;
if (requireDirect &&
dependency != null &&
!dependency.startsWith('direct ')) {
issues.add(missingDetails);
}

var source = pkgDataMap['source'] as String?;
if (source == 'hosted') {
// NOTE: pkgDataMap['description'] should be:
Expand Down Expand Up @@ -133,7 +138,6 @@ Future<List<PackageExceptionDetails>> _validateBuildDaemonVersion(
var buildDaemonIssues = pubspecLock.checkPackage(
'build_daemon',
VersionConstraint.parse(buildDaemonConstraint),
requireDirect: false,
);

// Only warn of build_daemon issues if they have a dependency on the package.
Expand All @@ -146,8 +150,7 @@ Future<List<PackageExceptionDetails>> _validateBuildDaemonVersion(
// used by their application.
if (info.isNewer &&
pubspecLock
.checkPackage('build_daemon', info.buildDaemonConstraint,
requireDirect: false)
.checkPackage('build_daemon', info.buildDaemonConstraint)
.isEmpty) {
issues.add(PackageExceptionDetails._('$issuePreamble\n'
'A newer version of webdev is available which supports '
Expand All @@ -169,7 +172,6 @@ final buildWebCompilersConstraint = VersionConstraint.parse('^4.0.4');
Future<void> checkPubspecLock(PubspecLock pubspecLock,
{required bool requireBuildWebCompilers}) async {
var issues = <PackageExceptionDetails>[];

var buildRunnerIssues =
pubspecLock.checkPackage('build_runner', buildRunnerConstraint);

Expand Down
Loading