refactor managed dependencies #3698

tomerd · 2021-08-31T02:46:22Z

motivation: ManagedDependencies are based on location instead of on identity, refactor them to be based on identity as part of the transition to identity based logic

changes:

transition ManagedDependencies underlying hashmap to be based on identity
update the ManagedDependencies edit state to include the "basedOn" property which is only relevant to that state
update calls ites to use package identity instead of location when reading/writing managed depedencies
refactor related call-sites to handle edited state more safely and correctly
add code comments to explain some of the more subtle business logic
adjust tests where needed

rdar://82954076

tomerd · 2021-08-31T02:46:49Z

needs #3696 first

tomerd · 2021-09-01T00:45:23Z

@swift-ci please smoke test

tomerd · 2021-09-01T00:50:05Z

@swift-ci please smoke test

tomerd · 2021-09-01T01:31:20Z

@swift-ci please smoke test

tomerd · 2021-09-01T01:34:56Z

Sources/Workspace/Workspace.swift

-                } else {
-                    currentDependency = nil
-                }
+            let currentDependency = self.state.dependencies[packageRef.identity]


👀 plz help make sure the logic here hasn't regressed

tomerd · 2021-09-01T04:32:35Z

@swift-ci please smoke test

tomerd · 2021-09-02T01:33:08Z

@swift-ci please test package compatibility

tomerd · 2021-09-02T16:57:56Z

@abertelrud @neonichu @shahmishal I looked at the package compatibility test and Kitura 5.1 is now failing due to an improved validation that we introduced in #3687 (which is already merged, so not this PR)

'Kitura-net' /private/tmp/Kitura/.build/checkouts/Kitura-net: error: 'kitura-net' dependency on 'https://github.com/Kitura/LoggerAPI.git' conflicts with dependency on 'https://github.com/IBM-Swift/LoggerAPI.git' which has the same identity 'loggerapi'
'KituraContracts' /private/tmp/Kitura/.build/checkouts/KituraContracts: error: 'kituracontracts' dependency on 'https://github.com/Kitura/LoggerAPI.git' conflicts with dependency on 'https://github.com/IBM-Swift/LoggerAPI.git' which has the same identity 'loggerapi'

the question is what the best strategy here, since the new validation find an issue with the existing code which used to "work" but was never valid and can lead to undefined behavior. should we remove the version of kitura from the compatibility suite until its fixed there, or do you have a better way?

tomerd · 2021-09-08T04:00:03Z

@swift-ci please test package compatibility

abertelrud · 2021-09-08T16:48:57Z

@abertelrud @neonichu @shahmishal I looked at the package compatibility test and Kitura 5.1 is now failing due to an improved validation that we introduced in #3687 (which is already merged, so not this PR)
'Kitura-net' /private/tmp/Kitura/.build/checkouts/Kitura-net: error: 'kitura-net' dependency on 'https://github.com/Kitura/LoggerAPI.git' conflicts with dependency on 'https://github.com/IBM-Swift/LoggerAPI.git' which has the same identity 'loggerapi'
'KituraContracts' /private/tmp/Kitura/.build/checkouts/KituraContracts: error: 'kituracontracts' dependency on 'https://github.com/Kitura/LoggerAPI.git' conflicts with dependency on 'https://github.com/IBM-Swift/LoggerAPI.git' which has the same identity 'loggerapi'
the question is what the best strategy here, since the new validation find an issue with the existing code which used to "work" but was never valid and can lead to undefined behavior. should we remove the version of kitura from the compatibility suite until its fixed there, or do you have a better way?

Seems similar to a marking a unit test as failing for external reason, except at a whole-package level. Could we let them know about the problem, disable Kitura, and then reenable it when they have a version that fixes the problem?

tomerd · 2021-09-08T17:03:24Z

Seems similar to a marking a unit test as failing for external reason, except at a whole-package level. Could we let them know about the problem, disable Kitura, and then reenable it when they have a version that fixes the problem?

I relaxed the error to warning for now which means the test suite passes. once we escalate to an error again (next major release) we will see if they fixed it already or we need to ask them to do so

Sources/Workspace/ManagedDependency.swift

tomerd · 2021-09-25T03:55:38Z

@swift-ci please smoke test

tomerd · 2021-09-25T03:56:13Z

@swift-ci please test package compatibility

tomerd · 2021-09-25T16:12:11Z

@mattt this should be in a bit of a better shape now. can you test to see if it works for you?

mattt · 2021-09-27T12:40:52Z

@swift-ci please smoke test

tomerd · 2021-09-27T18:27:45Z

@swift-ci please smoke test

tomerd · 2021-09-27T18:28:17Z

@mattt did you add commits here that I mistakingly override?

tomerd · 2021-09-27T18:28:46Z

@swift-ci please test package compatibility

mattt · 2021-09-27T20:05:54Z

@mattt did you add commits here that I mistakingly override?

I applied the typo fix suggestion from my original PR review earlier this morning (lbashedOn -> lbasedOn), which appears to have been lost with the force push. But that's no big deal. The commit ref is 4adf6f8 if you'd like to cherry pick that.

$ curl https://github.com/apple/swift-package-manager/commit/4adf6f87568bbd8cc5c80bfa29bd75ea1a1adc0c.patch | git am

tomerd · 2021-09-27T22:17:36Z

@abertelrud @neonichu ptal so we can merge this

Sources/PackageModel/PackageReference.swift

Sources/PackageGraph/PackageGraph+Loading.swift

Sources/PackageModel/PackageReference.swift

abertelrud · 2021-09-28T19:34:50Z

Sources/Workspace/ManagedDependency.swift

+            return ManagedDependency(
+                packageRef: packageRef,
+                state: .local,
+                // FIXME: This is just a fake entry, we should fix it.


Is that for a later PR, or a FIXME for a future update of this one?

this FIXME was there before, so not a new thing

I do think we should fix this, but it was not in scope for this PR

abertelrud · 2021-09-28T19:42:41Z

Sources/Workspace/ManagedDependency.swift

+            /// If the path is non-nil, the dependency is managed by a user and is
+            /// located at the path. In other words, this dependency is being used
+            /// for top of the tree style development.
+            case edited(basedOn: ManagedDependency?, unmanagedPath: AbsolutePath?)


Is it an error state for the basedOn to be nil here? Just asking for my understanding...

big picture, the whole "edited" feature needs to go away.

specifically, the basedOn can be nil in some situation unfortunately - tired to make it required but the logic still requires it

Yah, I think once we have explicit overrides, we can get rid of edit mode and therefore the entirety of managed dependencies.

motivation: ManagedDependencies are based on location instead of on identity, refactor them to be based on identity as part of the transition to identity based logic changes: * transition ManagedDependencies underlying hashmap to be based on identity * update the ManagedDependencies edit state to include the "basedOn" property which is only relevant to that state * update calls ites to use package identity instead of location when reading/writing managed depedencies * refactor related call-sites to handle edited state more safely and correctly * add code comments to explain some of the more subtle business logic * adjust tests where needed

tomerd · 2021-09-29T01:09:42Z

@swift-ci please smoke test

tomerd · 2021-09-29T01:23:07Z

@swift-ci please test package compatibility

tomerd requested review from abertelrud, friedbunny and neonichu as code owners August 31, 2021 02:46

tomerd mentioned this pull request Aug 31, 2021

refactor managed artifacts #3699

Merged

tomerd force-pushed the refactor/managed-depedencies branch from 615bd43 to f1c22db Compare September 1, 2021 00:45

tomerd force-pushed the refactor/managed-depedencies branch from f1c22db to 99d23a6 Compare September 1, 2021 00:49

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Sep 1, 2021

tomerd force-pushed the refactor/managed-depedencies branch from 99d23a6 to 53e43db Compare September 1, 2021 01:31

tomerd commented Sep 1, 2021

View reviewed changes

tomerd force-pushed the refactor/managed-depedencies branch 5 times, most recently from 3386076 to adb61b3 Compare September 1, 2021 04:32

abertelrud approved these changes Sep 2, 2021

View reviewed changes

tomerd added the DO NOT MERGE label Sep 9, 2021

tomerd self-assigned this Sep 9, 2021

tomerd removed the ready Author believes the PR is ready to be merged & any feedback has been addressed label Sep 10, 2021

tomerd mentioned this pull request Sep 10, 2021

Implement dependency resolution through package registry #3640

Closed

4 tasks

mattt reviewed Sep 13, 2021

View reviewed changes

Sources/Workspace/ManagedDependency.swift Outdated Show resolved Hide resolved

mattt reviewed Sep 13, 2021

View reviewed changes

Sources/Workspace/ManagedDependency.swift Show resolved Hide resolved

Sources/Workspace/ManagedDependency.swift Show resolved Hide resolved

mattt mentioned this pull request Sep 16, 2021

Additional refactoring of ManagedDependencies #3750

Merged

tomerd force-pushed the refactor/managed-depedencies branch from adb61b3 to 0f377e0 Compare September 25, 2021 03:54

tomerd mentioned this pull request Sep 26, 2021

refactor location abstractions #3764

Merged

tomerd added ready Author believes the PR is ready to be merged & any feedback has been addressed and removed DO NOT MERGE labels Sep 26, 2021

mattt approved these changes Sep 27, 2021

View reviewed changes

tomerd force-pushed the refactor/managed-depedencies branch from 4adf6f8 to de10175 Compare September 27, 2021 18:27

neonichu reviewed Sep 28, 2021

View reviewed changes

Sources/PackageModel/PackageReference.swift Outdated Show resolved Hide resolved

neonichu reviewed Sep 28, 2021

View reviewed changes

Sources/PackageModel/PackageReference.swift Outdated Show resolved Hide resolved

neonichu approved these changes Sep 28, 2021

View reviewed changes

abertelrud reviewed Sep 28, 2021

View reviewed changes

Sources/PackageGraph/PackageGraph+Loading.swift Outdated Show resolved Hide resolved

abertelrud reviewed Sep 28, 2021

View reviewed changes

Sources/PackageModel/PackageReference.swift Outdated Show resolved Hide resolved

abertelrud reviewed Sep 28, 2021

View reviewed changes

tomerd force-pushed the refactor/managed-depedencies branch from de10175 to 3e343ce Compare September 29, 2021 01:05

tomerd mentioned this pull request Sep 29, 2021

refactor workspace repository handling #3766

Merged

tomerd merged commit 3d48f94 into swiftlang:main Sep 29, 2021

refactor managed dependencies #3698

refactor managed dependencies #3698

Uh oh!

Conversation

tomerd commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Aug 31, 2021

Uh oh!

tomerd commented Sep 1, 2021

Uh oh!

tomerd commented Sep 1, 2021

Uh oh!

tomerd commented Sep 1, 2021

Uh oh!

tomerd Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Sep 1, 2021

Uh oh!

tomerd commented Sep 2, 2021

Uh oh!

tomerd commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Sep 8, 2021

Uh oh!

abertelrud commented Sep 8, 2021

Uh oh!

tomerd commented Sep 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomerd commented Sep 25, 2021

Uh oh!

tomerd commented Sep 25, 2021

Uh oh!

tomerd commented Sep 25, 2021

Uh oh!

mattt commented Sep 27, 2021

Uh oh!

tomerd commented Sep 27, 2021

Uh oh!

tomerd commented Sep 27, 2021

Uh oh!

tomerd commented Sep 27, 2021

Uh oh!

mattt commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Sep 27, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abertelrud Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

tomerd Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

tomerd Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

tomerd Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonichu Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

tomerd commented Sep 29, 2021

Uh oh!

tomerd commented Sep 29, 2021

tomerd commented Aug 31, 2021 •

edited

Loading

tomerd Sep 1, 2021 •

edited

Loading

tomerd commented Sep 2, 2021 •

edited

Loading

mattt commented Sep 27, 2021 •

edited

Loading

tomerd Sep 28, 2021 •

edited

Loading