[swift-build-dev] [swift-evolution] Proposal Discussion Thread: SwiftPM: Locking and Overriding Dependencies

[Daniel Dunbar]

> On Mar 21, 2016, at 4:05 PM, Max Howell <max.howell at apple.com> wrote:
> 
>> 2. I like VersionLocks.json well enough, but would like to see a discussion about possible alternatives. My personal proposal (in line with #1) is to use "PackageVersions.json" which has a nice agreement with Package.swift and would mean two common metadata files show up adjacent. I don't really want to bike shed on the name, but I suspect whatever we pick first will last for a while so I would at least like to review the various alternatives. I also will throw out that my personal opinion is we don't need to pick a name that bears much resemblance with existing terminology, whatever we pick will eventually become "the standard" for the SwiftPM ecosystem so I would prefer to pick the most-descriptive-possible name up front, not one that alludes to the same concept in other systems.
> 
> I like PackageVersions.json

Cool. There are still some instances of `VersionLocks.json` in the doc, btw.

>> 3. I like the terminology section here, I almost feel like we should adopt that as official terminology in our documentation (which I don't think we have yet, correct me if I am wrong).
> 
> We don’t, but I agree, we should aim to pick some names and use them consistently.
> 
>> 4. I would like it if the lock file recorded the exact SHA it received, and validate that when retrieving. This helps protect users against MITM attacks or unexpected changes if an upstream modifies a tag. It also can be used as part of safety checks when migrating to an alternate repository host which is expected to have the same content.
> 
> Good point, this should be there.
> 
>> 5. The "workflow - build" sections #2,3,4 are rather complicated. Is this because the proposal is trying to work with existing Packages layouts, or because the proposal is trying to handle the various variations of what the user may have checked in inside the Packages subdirectory?
> 
> The latter, if we are to support checking in the `Packages` directory, we should handle it when it is so. Is there a simpler way you can see?

I think the minimal feature here is that swiftpm grows the ability to read a specification file which declares how the package dependency tree is resolved. These seems like it should be primitive, and straightforward, so it is worrisome that the proposal is so complex.

What is your mental model for what checked in Packages should be? I can see several ways to interpret them:
A. The checked in copy is just a replica of the dependency repository. It exists to ensure the product can be built in a self contained manner.
B. The checked in copy *is* the dependency, it is basically a "convenient fork at a pinned version". SwiftPM recognizes the use case so that it can provide convenient features for the use case (like easily updating the fork).
C. Something in between. The checked in copy exists to ensure self containment, but it is also not treated like a fork.

I feel that too many problems are being combined into one proposal, and I think it makes it hard to work through and discuss the ramifications of all the changes here. There are at least five problems discussed in the current proposal:
1. The feature for pinning of package versions.
2. The workflow for interacting with the pinning feature (i.e., --lock, etc.).
3. Interactions between checked in Package trees and package versions.
4. Swift toolchain versioning issues.
5. Local diffs.
#1 and #2 obviously make sense together, and I can see how #1 and #3 might have to be in the same proposal (since implementing #1 may require solving #3 to not break things). I would like to see the rest be teased apart just so it is easier to understand all the implications.

A priori, I don't see how that minimal feature should require significant discussion w.r.t. checked in Packages. For example, consider the three models I gave above:
- If the expected model is (A), then the behavior I get with checked in Packages should always be exactly the same as if I blew it away (assuming the remote hasn't had content deleted). Therefore, the only new behaviors that need come with version pinning are probably a few cases of error detection (when updating versus a remote which has deleted content).
 - If the expected model is (B), then I would expect the behavior to be that there *must* be a PackageVersions.json, and the Packages *must* match those in that file. This may be what the existing rules are trying to codify, if so I think it would be most clear to simply specify the intent.
	o I can see how there are workflow issues around the user editing their "local fork" and needing to update both the PackageVersions.json, but it seems like they follow from the basic behavior of "the PackageVersions *must* match the local fork".
 - If the expected model is (C), then I think it is important to clarify exactly what a checked in Packages subdirectory is for first. I have trouble seeing what that is when the implementation uses it *and* the user may modify it.

>> 6. I wonder if we should be defining, as Eloy alludes to, two different things:
>>  - The version lock file, which defines the expected versions for the package manager to use when it is doing package resolution.
>>  - The package state file (in Packages.swift), which is used by the package manager to track information on the Packages/ subdir state in order to provide useful features primarily focused at the scenarios when the user is modifying those files.
>> Currently it seems like a lot of the behaviors in the proposal are focused at the latter case, but they feel like they should be decoupled problems to me.
> 
> I’m not sure we need a second file, since the versions of the “installed dependencies” are recorded in the directory names as well as that we also do full clones, so that information is part of the clone & checkout.

Good point. I thought it might help to have all the data combined in a file we could easily read, but I guess the only thing we can't infer is if the user makes a modification to a tag or something where we wouldn't have the previous hash.

 - Daniel

> 
>