Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

francescomikulis · 2024-07-01T16:57:34Z

Motivation:

Resolving local package dependencies often outputs a warning:
is not valid git repository for '', will fetch again.

Modifications:

The underlying issue is that updating packages with a local SCM path dependency always fail, as the shell-based repository URL has a file:// scheme, while the absolute path string does not.

Result:

After this change we only compare the canonical URLs, which will resolve the inconsistency of comparing strings without considering the scheme and / or path extension.

Validating the origin of checked-out repositories no longer ensures that the path extension (often .git) matches the repository's specifier, as different git clients inconsistently preserve the path extension of the remote. Moving to the CanonicalPackageURL also ensures that absolute paths are treated as urls with a file:// scheme, matching git's behavior.

…l representation, correctly accepting results where repositories only differ by '.git'.

rauhul · 2024-07-01T17:05:33Z

Im gonna pre-flight what I think @MaxDesiatov will say: "Could you add some tests for this?"

xedin · 2024-07-01T18:15:35Z

Yeah, we definitely need tests for this before merging.

neonichu · 2024-07-01T22:58:16Z

The motivation for local dependencies makes sense to me, but I don't think it's correct to ignore schemes and path extensions here, the whole purpose of the validation was to make sure that we have the "correct" repo. If we're now not doing that anymore, it might make more sense to remove the entire validation logic.

bnbarham · 2024-07-01T23:13:48Z

If we're now not doing that anymore, it might make more sense to remove the entire validation logic.

TBH I'd be fine with this - the repository cache seems like the correct layer to validate this. Otherwise we're just going to run into issues where the cache says the repositories match, but this validation disagrees).

EDIT: Though thinking about it more, this does check that the directory is at least a valid git repository + that the underlying origin maps (in our canonicalization sense). Which is better than not doing that at all.

Sources/SourceControl/GitRepository.swift

bnbarham · 2024-07-02T02:55:26Z

Sources/SourceControl/GitRepository.swift

+            return CanonicalPackageURL(remoteURL) == CanonicalPackageURL(url.absoluteString)
+        case .path(let absolutePath):
+            // Compare the canonical representation, which will drop any suffix
+            return CanonicalPackageLocation(remoteURL) == CanonicalPackageLocation(absolutePath.pathString)


I doubt this matters in practice, but the caching logic always uses URL and it's probably better to do that here as well. That wouldn't fix the case mentioned in the description, but to fix that you could update computeCanonicalLocation to return a file scheme if one wasn't given (which seems somewhat reasonable given we're already inferring ssh if the user is git). Does that seem fine to you @neonichu?

MaxDesiatov · 2024-07-03T14:38:34Z

@swift-ci test

…ider. Clarified the API surface and reduced the change to only ignore path extensions for local (path-based) dependencies.

Sources/SourceControl/GitRepository.swift

…eintroduced the 'RepositorySpecifier' and moved to using 'CanonicalPackageURL', where paths now have a scheme of 'file'.

bnbarham · 2024-07-11T23:55:29Z

Sources/SourceControl/Repository.swift

-    public var url: String {
+    public var url: CanonicalPackageURL {
        switch self.location {
-        case .path(let path): return path.pathString
-        case .url(let url): return url.absoluteString
+        case .path(let path): return CanonicalPackageURL(path.pathString)
+        case .url(let url): return CanonicalPackageURL(url.absoluteString)


Why this change 🤔?

I removed this, it wasn't necessary.

francescomikulis · 2024-07-15T17:42:43Z

Tests/WorkspaceTests/SourceControlPackageContainerTests.swift

@@ -89,74 +89,6 @@ private class MockRepository: Repository {
    }
 }

-private class MockRepositories: RepositoryProvider {


This was unused.

bnbarham · 2024-07-15T21:34:00Z

@swift-ci please test

MaxDesiatov

IMO the change seems noteworthy enough to be included in CHANGELOG.md?

robinkunde · 2024-07-16T23:11:17Z

I was preparing my own PRs for this issue, but this one is much further along. Great work @francescomikulis!
BTW: The reason I was looking at this issue is that we have a complex dependency graph with several large repos, and discarding the workspace repo and copying it from the cache was happening several times during dependency resolution. This should be a noticeable performance improvement for large projects.

One question I had was: Would it make sense to canonicalize the origin URL before populating the cache? As it stands now, whether the cached repo uses https://github.com/swiftlang/swift-package-manager.git or https://github.com/swiftlang/swift-package-manager for example depends on which version of the URL was encountered first in a dependency graph. This PR fixes the lookup logic, but I think the logic for populating the cache should be as predictable as possible too.

…ension." This reverts commit 8e2691e.

MaxDesiatov · 2024-07-19T09:06:56Z

@swift-ci test

MaxDesiatov · 2024-07-19T09:07:46Z

@swift-ci test windows

francescomikulis · 2024-07-19T19:47:36Z

I was preparing my own PRs for this issue, but this one is much further along. Great work @francescomikulis! BTW: The reason I was looking at this issue is that we have a complex dependency graph with several large repos, and discarding the workspace repo and copying it from the cache was happening several times during dependency resolution. This should be a noticeable performance improvement for large projects.

One question I had was: Would it make sense to canonicalize the origin URL before populating the cache? As it stands now, whether the cached repo uses https://github.com/swiftlang/swift-package-manager.git or https://github.com/swiftlang/swift-package-manager for example depends on which version of the URL was encountered first in a dependency graph. This PR fixes the lookup logic, but I think the logic for populating the cache should be as predictable as possible too.

I agree that we should try and ensure that the underlying cache is populated with expected values, matching the new validation logic in the GitRepositoryProvider.
From some research, an example of this is in the RepositoryManager, where it keeps around maps such as var pendingLookups = [RepositorySpecifier: DispatchGroup](). To fully move towards a more canonical model we'd probably have to change RepositorySpecifier to wrap a Location that was canonicalized.
I believe such a change is out-of-scope for fixing these cache / performance issues, but I agree that this could be an advantageous improvement.

…sitories that aren't yet checked out, even though the InMemoryFileSystem claims the directories exist.

bnbarham · 2024-07-19T21:11:01Z

@swift-ci please test

bnbarham · 2024-07-22T17:55:51Z

@swift-ci please test Windows platform

…l representation, correctly accepting results where repositories only differ by '.git' (#7741) Resolving local package dependencies often outputs a warning: <path> is not valid git repository for '<repo>', will fetch again. The underlying issue is that updating packages with a local SCM path dependency always fail, as the shell-based repository URL has a file:// scheme, while the absolute path string does not. After this change we only compare the canonical URLs, which will resolve the inconsistency of comparing strings without considering the scheme and / or path extension. Validating the origin of checked-out repositories no longer ensures that the path extension (often .git) matches the repository's specifier, as different git clients inconsistently preserve the path extension of the remote. Moving to the CanonicalPackageURL also ensures that absolute paths are treated as urls with a file:// scheme, matching git's behavior.

Move the 'GitRepositoryProvider' to compare urls using their canonica…

76008c3

…l representation, correctly accepting results where repositories only differ by '.git'.

francescomikulis requested review from bnbarham, MaxDesiatov, jakepetroules and xedin as code owners July 1, 2024 16:57

francescomikulis requested a review from pmattos July 1, 2024 16:57

xedin approved these changes Jul 1, 2024

View reviewed changes

bnbarham reviewed Jul 2, 2024

View reviewed changes

MaxDesiatov added needs tests This change needs test coverage source control Changes to SCM-related code labels Jul 2, 2024

Added tests for the 'isValidDirectory' function of the RepositoryProv…

27a47a2

…ider. Clarified the API surface and reduced the change to only ignore path extensions for local (path-based) dependencies.

bnbarham reviewed Jul 11, 2024

View reviewed changes

Sources/SourceControl/GitRepository.swift Show resolved Hide resolved

Removed 'repositoryExists(at:)' in favor of 'isValidDirectory(_:)'. R…

096755c

…eintroduced the 'RepositorySpecifier' and moved to using 'CanonicalPackageURL', where paths now have a scheme of 'file'.

bnbarham reviewed Jul 11, 2024

View reviewed changes

bnbarham mentioned this pull request Jul 12, 2024

Add AbsolutePath and SourceControlURL parsing to swift package add-dependency #7769

Closed

Removed some unnecessary changes, restoring the original implementation.

f588e25

francescomikulis commented Jul 15, 2024

View reviewed changes

francescomikulis added 4 commits July 15, 2024 12:46

Removed more unnecessary changes.

b3bdde3

Clarified and expanded the tests.

363272c

Removed more unnecessary changes.

f5a8a65

Added tests for relative-path based origin.

51592d7

bnbarham approved these changes Jul 15, 2024

View reviewed changes

Feedback - added back the validation on the matching path extension.

8e2691e

MaxDesiatov added the add to change log label Jul 16, 2024

MaxDesiatov reviewed Jul 16, 2024

View reviewed changes

Hesham-atta approved these changes Jul 16, 2024

View reviewed changes

Revert "Feedback - added back the validation on the matching path ext…

0ead74d

…ension." This reverts commit 8e2691e.

MaxDesiatov removed the needs tests This change needs test coverage label Jul 19, 2024

Updated the test InMemoryRepositoryProvider to explicitly handle repo…

e1c678f

…sitories that aren't yet checked out, even though the InMemoryFileSystem claims the directories exist.

francescomikulis merged commit bc9a9e3 into main Jul 22, 2024
5 checks passed

francescomikulis deleted the repository-provider-compare-urls-with-canonical-representation branch July 22, 2024 22:21

francescomikulis mentioned this pull request Jul 22, 2024

[6.0] Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7809

Merged

robinkunde mentioned this pull request Apr 21, 2025

[SR-14393] SPM package repo URL conflicts fail resolution #4441

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

francescomikulis commented Jul 1, 2024 •

edited

Loading

rauhul commented Jul 1, 2024 •

edited

Loading

xedin commented Jul 1, 2024

neonichu commented Jul 1, 2024

bnbarham commented Jul 1, 2024 •

edited

Loading

bnbarham Jul 2, 2024

MaxDesiatov commented Jul 3, 2024

bnbarham Jul 11, 2024 •

edited

Loading

francescomikulis Jul 15, 2024

francescomikulis Jul 15, 2024

bnbarham commented Jul 15, 2024

MaxDesiatov left a comment

robinkunde commented Jul 16, 2024 •

edited

Loading

MaxDesiatov commented Jul 19, 2024

MaxDesiatov commented Jul 19, 2024

francescomikulis commented Jul 19, 2024

bnbarham commented Jul 19, 2024

bnbarham commented Jul 22, 2024

Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' #7741

Conversation

francescomikulis commented Jul 1, 2024 • edited Loading

Motivation:

Modifications:

Result:

rauhul commented Jul 1, 2024 • edited Loading

xedin commented Jul 1, 2024

neonichu commented Jul 1, 2024

bnbarham commented Jul 1, 2024 • edited Loading

bnbarham Jul 2, 2024

Choose a reason for hiding this comment

MaxDesiatov commented Jul 3, 2024

bnbarham Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

francescomikulis Jul 15, 2024

Choose a reason for hiding this comment

francescomikulis Jul 15, 2024

Choose a reason for hiding this comment

bnbarham commented Jul 15, 2024

MaxDesiatov left a comment

Choose a reason for hiding this comment

robinkunde commented Jul 16, 2024 • edited Loading

MaxDesiatov commented Jul 19, 2024

MaxDesiatov commented Jul 19, 2024

francescomikulis commented Jul 19, 2024

bnbarham commented Jul 19, 2024

bnbarham commented Jul 22, 2024

francescomikulis commented Jul 1, 2024 •

edited

Loading

rauhul commented Jul 1, 2024 •

edited

Loading

bnbarham commented Jul 1, 2024 •

edited

Loading

bnbarham Jul 11, 2024 •

edited

Loading

robinkunde commented Jul 16, 2024 •

edited

Loading