dart pub publish --dry-run analyzes all Dart files with regard to toplevel pubspec.yaml

[dcharkes]

Problem

We're analyzing all Dart files w.r.t. the toplevel pubspec.yaml, not w.r.t. the first pubspec.yaml found when walking folders up.

Actual behavior

The toplevel pubspec.yaml is used.

Package validation found the following potential issues:
* line 7, column 1 of test/data/native_add_add_source/build.dart: This package does not have native_toolchain_c in the `dependencies` or `dev_dependencies` section of `pubspec.yaml`.
    ╷
  7 │ import 'package:native_toolchain_c/native_toolchain_c.dart';
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    

This behavior is inconsistent with dart analyze.

Expected behavior

No errors, there is a pubspec.yaml inside test/data/native_add_add_source/ that contains a dependency on native_toolchain_c.

I expect the behavior to be similar to dart analyze respecting other pubspec.yamls.

Related issues

This issue seems to be related to:

• dart pub get - in all sub folders? #3855

Both of these issues have to do with assuming that there is only a single pubspec.yaml.

[dcharkes]

As a workaround:

1. .pubignore test/ (Might have no bad consequences. However, it requires keeping the gitignore and pubignore in sync.)
2. analysis_options.yaml exclude test/data/ (Undesired, it stops analysis of the test projects in the IDE. But without this dart analyze fails due to not running pub get in test/data/ subfolders.)

[jonasfj]

There is no harm to doing .pubignore of test/. Just beware that you'll need to copy .gitignore into .pubignore when you create it, as .gitignore will be ignored when .pubignore exists.

[jonasfj]

Option (2) won't really work, this issue is from:

https://github.com/dart-lang/pub/blob/master/lib/src/validator/strict_dependencies.dart

[devoncarew]

Note that as of #4068 we no longer analyze test/ as part of publishing validation.

[dcharkes]

Note that as of #4068 we no longer analyze test/ as part of publishing validation.

I don't believe this is true.

$ pwd
/Users/dacoharkes/src/dart-lang/native/pkgs/native_assets_builder
$ dart pub publish --dry-run
[...]
Total compressed archive size: 21 KB.
Validating package... (1.1s)
Package validation found the following potential issues:
* line 7, column 1 of test/data/native_add_add_source/build.dart: This package does not have native_toolchain_c in the `dependencies` or `dev_dependencies` section of `pubspec.yaml`.
    ╷
  7 │ import 'package:native_toolchain_c/native_toolchain_c.dart';
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
[...]

[dcharkes]

It's a different validator StrictDependenciesValidator

  /// Validates that no Dart files in `benchmark/`, `test/` or
  /// `tool/` have dependencies that aren't in [deps] or [devDeps].
  void _validateBenchmarkTestTool(Set<String> deps, Set<String> devDeps) {
    var directories = ['benchmark', 'test', 'tool'];
    for (var usage in _usagesBeneath(directories)) {
      if (!deps.contains(usage.package) && !devDeps.contains(usage.package)) {
        warnings.add(usage.dependenciesMissingMessage());
      }
    }
  }

@sigurdm I don't believe it makes sense to check for test, since the test directory cannot be used when pulling a package in via pub. Neither for tool and benchmark. What is this check used for?

  /// Returns the [files] that are inside [dir] (relative to the package
  /// entrypoint).
  // TODO(sigurdm): Consider moving this to a more central location.
  List<String> filesBeneath(String dir, {required bool recursive}) {
    final base = p.canonicalize(p.join(entrypoint.rootDir, dir));
    return files
        .where(
          recursive
              ? (file) => p.canonicalize(file).startsWith(base)
              : (file) => p.canonicalize(p.dirname(file)) == base,
        )
        .toList();
  }

Maybe this should stop recursing if it finds a pubspec.yaml

It should also respect .pubignore.

Currently the workaround of adding things to .pubignore also doesn't work. Which blocks me autopublishing on PRs such as dart-lang/native#937.

[sigurdm]

I don't believe it makes sense to check for test, since the test directory cannot be used when pulling a package in via pub. Neither for tool and benchmark. What is this check used for?

The validation was made before my time. I think the general idea is that you want the package to be in a good shape before publishing. If you have errors in test it is probably something you want to fix before publishing. So while

It should also respect .pubignore.

The StrictDependenciesValidator should indeed follow .pubignores. (If it doesn't, that is a bug).

The AnalyzeValidator currently doesn't follow .pubignores because we haven't found a good way of passing a possibly huge list of files to the analyzer (command lines have limitations on the number of arguments). (#3696)

Maybe this should stop recursing if it finds a pubspec.yaml

That is a possibility. But it is hard to know if the pubspec.yaml is there as testdata or as a genuine project. Maybe the rule should be that we make a convention not to analyze things within test/testdata