Skip to content
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.

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

Do not use canonical location for checkouts #7001

Merged
merged 1 commit into from Oct 16, 2023

Conversation

neonichu
Copy link
Member

In #6780, we switched to using the canonical location of a package for the checkouts paths, but that can lead to mixing up checkouts storage when there has been a legitimate change to locations that isn't reflected in the canonical location (most prominently switching from SSH to HTTPS or vice versa). Because of that, we should undo that change.

rdar://116534183

@neonichu neonichu self-assigned this Oct 13, 2023
@neonichu
Copy link
Member Author

@swift-ci please test

@neonichu
Copy link
Member Author

I missed this test, need to update:

[1614/1614] Testing CommandsTests.TestToolTests/testList
Test Suite 'Selected tests' started at 2023-10-13 20:08:41.286Test Suite 'RepositoryManagerTests' started at 2023-10-13 20:08:41.288Test Case 'RepositoryManagerTests.testCanonicalLocation' started at 2023-10-13 20:08:41.288/home/build-user/swiftpm/Tests/SourceControlTests/RepositoryManagerTests.swift:336: error: RepositoryManagerTests.testCanonicalLocation : XCTAssertEqual failed: ("foo-b1347716") is not equal to ("foo-926b93f1") -/home/build-user/swiftpm/Tests/SourceControlTests/RepositoryManagerTests.swift:336: error: RepositoryManagerTests.testCanonicalLocation : XCTAssertEqual failed: ("foo-4171dede") is not equal to ("foo-926b93f1") -/home/build-user/swiftpm/Tests/SourceControlTests/RepositoryManagerTests.swift:336: error: RepositoryManagerTests.testCanonicalLocation : XCTAssertEqual failed: ("foo-9e3a7536") is not equal to ("foo-926b93f1") -Test Case 'RepositoryManagerTests.testCanonicalLocation' failed (0.004 seconds)Test Suite 'RepositoryManagerTests' failed at 2023-10-13 20:08:41.292Executed 1 test, with 3 failures (0 unexpected) in 0.004 (0.004) secondsTest Suite 'Selected tests' failed at 2023-10-13 20:08:41.292Executed 1 test, with 3 failures (0 unexpected) in 0.004 (0.004) seconds

@neonichu
Copy link
Member Author

Hm, I think the only way to handle this is to delete the test, but that doesn't seem entirely right to me since we do want to preserve that "http://scm.com/org/foo" and "http://scm.com/org/foo.git" would get the same storage location, but right now with the changes from this PR, they don't.

@tomerd
Copy link
Member

tomerd commented Oct 13, 2023

Hm, I think the only way to handle this is to delete the test, but that doesn't seem entirely right to me since we do want to preserve that "http://scm.com/org/foo" and "http://scm.com/org/foo.git" would get the same storage location, but right now with the changes from this PR, they don't.

that would require using something similar but different from CanonicalLocation, eg something that removes the .git suffix

@neonichu neonichu force-pushed the do-not-use-canonical-location-for-checkouts branch from 14ae248 to 844acbb Compare October 13, 2023 23:05
@neonichu
Copy link
Member Author

@swift-ci please test

@neonichu
Copy link
Member Author

@swift-ci please test windows

@neonichu
Copy link
Member Author

WorkspaceTests.testDeterministicURLPreference expects that http and https locations are considered the same, I am not sure that is a reasonable expectation?

@neonichu
Copy link
Member Author

Ah the issue here is that I am changing what equality of CanonicalPackageLocation does without realizing it. In the common case, we don't want the scheme to affect equality.

In #6780, we switched to using the canonical location of a package for the checkouts paths, but that can lead to mixing up checkouts storage when there has been a legitimate change to locations that isn't reflected in the canonical location (most prominently switching from SSH to HTTPS or vice versa). Because of that, we should undo that change.

rdar://116534183
@neonichu neonichu force-pushed the do-not-use-canonical-location-for-checkouts branch from 844acbb to 480858b Compare October 16, 2023 20:06
@neonichu
Copy link
Member Author

@swift-ci please test

@neonichu
Copy link
Member Author

@swift-ci please test windows

@neonichu neonichu merged commit 15e77b0 into main Oct 16, 2023
5 checks passed
@neonichu neonichu deleted the do-not-use-canonical-location-for-checkouts branch October 16, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants