Allow relative path-dependencies from git dependencies #4060

sigurdm · 2023-11-28T12:36:38Z

The paths are interpreted as paths inside the same git repo (at the same ref).

Internally, and in the lock-file they are treated as git-dependencies.

We do this by always interpreting dependencies in the context of the Description of the pubspec that contains it. This mechanism is now also used for passing the path relative path-dependencies are relative against. Much more streamlined!

We have to change how RootDescription is behaving, because the description now has to be created before we parse the pubspec it cannot know the Package object it belongs to, but just the path. (necessitating a bit of caching for everything to line up).

This fixes #449

sigurdm · 2023-12-01T10:58:47Z

lib/src/package.dart

      sources,
      expectedName: name,
      allowOverridesFile: withPubspecOverrides,
+      containingDescription: RootDescription(dir),


Check if this is right

Updated this to take an explicit description

jonasfj

I think this looks sound, but I'm not 100% sure.

Could we try to split it into smaller PRs. Some of the refactoring, renaming, etc, could be done in separate PRs.
Even the changes in behavior that we're doing to RootSource could be done in a separate PR.
We could possibly even introduce containingDescription in a PR without modifying PathSource to return GitDescription objects just yet.

I see how it makes sense to get everything working. But it would be a LOT faster to review one aspect of the changes at the time.
And we'd be more confident we're not missing something.

I'm a little confused about the root/entrypoint names.
The PackageGraph stuff looks like a possible footgun in the future (though it might already be one).
The changes in RootSource I don't know about (maybe it's a minor concern).
All the places we put in the wrong containingDescription because we don't care, scares me a bit... Should we make an UnknownDescription we can put in? Just an idea 🤣
Should we have a test cases where we have a diamond dependency. Ie. two git-dependencies from the same repo that both have a the same path-dependency.

jonasfj · 2023-12-06T09:17:16Z

lib/src/source.dart

    String name,
    Object? description, {
-    String? containingDir,
+    Description? containingDescription,


Update documentation and explain in excruciating details what this is :D

Also I'm a bit curious, why is it not the referencing pubspec?
Is the the snippet of the pubspec that references this dependency?

Description is very overloaded and hard to understand.

Yeah - maybe we need to think a bit more here.

A "description" is the abstract form of what you find in a pubspec on the right-hand side of a dependency-colon.

dependencies: retry: # here comes the description

The containingDescription is the description of the pubspec that referenced the one you are parsing now.

Eg. if your root pubspec, it is described by a RootDescription, because it is not a reference from another pubspec, has:

dependencies: retry: git: https://github.com/abc/retry

Then you would parse the retry pubspec.yaml with a RootDescription as the containingDescription.

I'll try to make a better documentation...

jonasfj · 2023-12-06T09:32:55Z

lib/src/package.dart

+  /// Loads the package whose root directory is [packageDir] as a root package.
+  factory Package.entrypoint(
+    String dir,
+    SourceRegistry sources,
+  ) {


In workspaces should we call the workspaces packages for entrypoints and then have isEntrypoint instead of isRoot all over the solver?

Other places I see PackageRef.root(..., so should this be Package.root instead?

jonasfj · 2023-12-06T09:34:18Z

lib/src/source/hosted.dart

-            packages.add(Package.load(null, entry, cache.sources));
+            // We are loading as an entrypoint because we don't look at the
+            // dependencies, so they do not have to be interpreted relative to
+            // anything.
+            packages.add(Package.entrypoint(entry, cache.sources));


The move away from Package.load(null, ... ) is orthogonal and could have been done in a separate PR is that correct?

jonasfj · 2023-12-06T09:38:27Z

lib/src/command/add.dart

            cache.sources,
            // Resolve relative paths relative to current, not where the pubspec.yaml is.
            location: p.toUri(p.join(p.current, 'descriptor')),
+            containingDescription: RootDescription('.'),


Why not RootDescription(entrypoint.rootDir), just asking? I'm guessing it doesn't matter at all?

if it doesn't matter then why not null ?

jonasfj · 2023-12-06T09:39:41Z

lib/src/entrypoint.dart


  Package? _root;

  /// The root package this entrypoint is associated with.


Should documentation say "entrypoint package"

Do we want to reuse this word, instead of root. If so maybe this should be a separate PR.

I think for now "root" is good enough. With workspaces we might really have to change stuff

jonasfj · 2023-12-06T09:57:55Z

lib/src/source/root.dart

+    // final description = id.description.description;
+    // if (description is! RootDescription) {
+    //   throw ArgumentError('Wrong source');
+    // }
+    // return description.package.pubspec;


Intentional or not?

Isn't this either an UnsupportedError or an AssertionError?

This should never happen, right? so AssertionError?

The commented out code was not intentional

jonasfj · 2023-12-06T09:58:52Z

lib/src/source/root.dart

+    throw ArgumentError('Trying to get versions of the root package');
+
+    // final description = ref.description;
+    // if (description is! RootDescription) {
+    //   throw ArgumentError('Wrong source');
+    // }
+    // return [PackageId.root(description.package)];


Isn't this either an UnsupportedError or an AssertionError?

This should never happen, right? so AssertionError?

Also ArgumentErrors should never happen... I don't really care. In my small universe AssertionErrors are reserved for assertions (that are disabled when not running with assertions)

Changed to UnsupportedError

jonasfj · 2023-12-06T10:03:50Z

lib/src/package.dart

+
+  RootDescription get description => RootDescription(_dir);
+  ResolvedRootDescription get resolvedDescription =>
+      ResolvedRootDescription(description);


What is this? Documentation?

This is indeed nonsense

jonasfj · 2023-12-06T10:06:41Z

lib/src/solver/package_lister.dart

+    if (id.isRoot) {
+      pubspec = _rootPubspec!;
+    } else {


Why do we need this now?

Is it because we changed what RootSource can do? Is this wise?

Should we consider moving some of these refactorings into separate PRs.

jonasfj · 2023-12-06T10:08:23Z

lib/src/source/git.dart

 }

-class GitResolvedDescription extends ResolvedDescription {
+class ResolvedGitDescription extends ResolvedDescription {


This rename is completely orthogonal to the rest of the PR, right?

Should we consider doing it in a separate PR upfront, so there is less to review here?

(the rename seems like something that would be easy to review and land)

Aye - this is now #4138

sigurdm

This change should be a lot more manageable now.

PTAL

sigurdm closed this Nov 28, 2023

sigurdm reopened this Nov 28, 2023

sigurdm requested review from jonasfj and szakarias November 28, 2023 13:10

sigurdm commented Dec 1, 2023

View reviewed changes

jonasfj reviewed Dec 6, 2023

View reviewed changes

sigurdm mentioned this pull request Feb 15, 2024

Make a RootDescription not depend on the root Package #4140

Merged

sigurdm mentioned this pull request Feb 26, 2024

Delay error reporting for dependencies #4143

Closed

Merge

840510b

sigurdm force-pushed the path_dependency_in_git branch from 1f79a48 to 840510b Compare March 12, 2024 14:48

sigurdm commented Mar 12, 2024

View reviewed changes

jonasfj approved these changes Mar 14, 2024

View reviewed changes

sigurdm merged commit c60b1a0 into dart-lang:master Mar 14, 2024


		Package? _root;

		/// The root package this entrypoint is associated with.

Allow relative path-dependencies from git dependencies #4060

Allow relative path-dependencies from git dependencies #4060

Uh oh!

Conversation

sigurdm commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasfj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigurdm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants